RFR: JDK-8223443: Calling Trees.getScope early changes names of local/anonymous classes

[Maurizio Cimadamore]

Hi Jan,
the fix in itself is fine. And I'm ok going ahead with it modulo few nits:

* there seem to be commented lines in TestGetScopeBinaryNames
* I'm not a big fan of names like "tree2FlatName", "new2Old", 
"prepareTree2FlatName" - but I can understand that's subjective
* some extra doc would be helpful, especially in the visitClassDef 
inside fixLocalClassNames - e.g. it would be helpful if the comments 
called out the cases that you can encounter explicitly (e.g. we have 
seen an unattributed tree, so we have to make up a new name, vs. we have 
seen an already attributed tree, so we copy the name over, etc.)


More generally, I think that leaning on the deferred attribution 
machinery is the right move here, and in fact I wonder if we couldn't 
leverage that machinery more. For instance, if I look at 
JavacTrees::getAttrContext I can't help but thinking that that method is 
a poor man version of speculative attribution - so, rather than using 
that method and fix up the results so that they 'match' what javac would 
otherwise produce during Attr, wouldn't it be better to use javac's Attr 
directly (e.g. speculative re-attributing the entire class, and stopping 
at a given tree, them returning the speculative env?).

Maybe the approach I suggest is too complex, or it would lead to a poor 
performance model, but I just wanted to share this thought as it came up 
when looking at the JavacTrees code.

Cheers
Maurizio

On 09/05/2019 16:39, Jan Lahoda wrote:
> Hi,
>
> In the Trees API, there is the (com.sun.source.util.)Trees.getScope 
> method. It works by cloning the body of the enclosing method and 
> attributing the clone, capturing the appropriate Env.
>
> There are a few issues with this method:
> 1. if getScope is called on an unattributed (but already entered) AST, 
> while attributing the copy of the enclosing method, (some of) the 
> names of the local/anonymous classes are assigned to the copy, and 
> then not reused when actually attributing the real AST and generating 
> classfiles. So calling getScope "early" may lead to generating 
> different classfiles with different names for the local/anonymous 
> classes. This can be fixed by un-entering the classes (which is 
> already done in DeferredAttr).
> 2. the binary names of the TypeElements for local/anonymous classes in 
> the returned Scope do not match those from the real AST. This can be 
> fixed by assigning the correct flat/binary names to the returned 
> ClassSymbols.
> 3. if there are (compile-time) errors in the enclosing method, these 
> may be reported during this attribution for getScope. This can be 
> fixed by ignoring errors reported from the attribution for getScope.
>
> The proposed fix for these is here:
> http://cr.openjdk.java.net/~jlahoda/8223443/webrev.00/
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8223443
>
> How does this look?
>
> Thanks,
>     Jan