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

[Jan Lahoda]

Hi Maurizio,

Thanks for comments, please see responses inline:

On 02. 07. 19 19:02, Maurizio Cimadamore wrote:
> 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.)

I've tried to improve the patch based on these comments, updated patch:
http://cr.openjdk.java.net/~jlahoda/8223443/webrev.01/

And a delta between the last version and this one:
http://cr.openjdk.java.net/~jlahoda/8223443/webrev.delta.00.01/

> 
> 
> 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?).

Speculatively attributing the whole tree (or the whole top-level class) 
is I think troublesome for two reasons. One is performance (as 
attributing whole class takes more time than attributing a single 
method). The other is that we cannot re-enter or attribute again 
(top-level or nested) classes. JavacTrees.getAttrContext is finding the 
first part of the AST we can reattribute.

I tried to tweak the attribStatToTree in JavacTrees so that it would 
delegate to attribSpeculative, but so far without too much success.

Thanks,
     Jan

> 
> 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