RFR: JDK-8223443: Calling Trees.getScope early changes names of local/anonymous classes
jan.lahoda at oracle.com
Thu Jul 4 15:34:52 UTC 2019
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:
And a delta between the last version and this one:
> 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.
> 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.
> On 09/05/2019 16:39, Jan Lahoda wrote:
>> 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
>> 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:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8223443
>> How does this look?
More information about the compiler-dev