RFR: JDK-8223443: Calling Trees.getScope early changes names of local/anonymous classes
maurizio.cimadamore at oracle.com
Tue Jul 2 17:02:28 UTC 2019
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.
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