RFR: JDK-8230847: Trees.getScope may crash when invoked for statement inside switch.

[Maurizio Cimadamore]

Looks ok. I personally would rename attribIsolated to attribSpeculative 
(e.g. add another overload) unless it clashes?

Maurizio

On 12/09/2019 10:30, Jan Lahoda wrote:
> Hi,
>
> There are some cases where the Trees.getScope may misbehave:
> -consider code like this:
> switch (i) {
>      case 0:
>          String.valueOf(0); //calling getScope on this place
>          int i;
>          break;
> }
>
> javac will create a copy the tree, attribute it, and stop the 
> attribution at the specified place by throwing BreakAttr. But in the 
> case above, Attr.handleSwitch will invoke Attr.addVars (in a finally), 
> which will try to add the "int i;"'s symbol into a Scope, which fails, 
> as that variable is not attributed. The proposed solution is to simply 
> not call addVars if an exception is thrown, by moving the call out of 
> the current finally block.
>
> -the attribution when a scope is being computed is not running with a 
> ArgumentAttr.withLocalCacheContext(), which may lead to the cache 
> pollution, and trouble for further getScope (or other attribution) 
> invocations (see the testMemberRefs() test). The proposal is to run 
> with the withLocalCacheContext(), but as a more general approach, I 
> tried to reuse as much as possible from the speculative attribution 
> for Scope computation, as was proposed some time back.
>
> -there is a corner case, where if the BreakAttr is thrown while 
> attribution an annotation's type, Annotate.flush() may not finish, and 
> there may be remaining tasks in the Annotate queues, which may fail on 
> subsequent flushes (possibly due to subsequent invocations of 
> getScope()). The proposed solution here is to save and restore the 
> queues for the isolated/speculative attributions. In the current 
> patch, this is being done for all speculative attributions, but if 
> that would be too dangerous, we could move that to getScope 
> computation only. (See tests testAnnotations() and 
> testAnnotationsLazy().)
>
> -there is a corner case, where calling getScope inside an initializer 
> of a class that depends on itself (i.e. has a cyclic dependency) may 
> crash, as the initializer is not properly recognized as an 
> initializer. The proposed solution is to recognize initializers in 
> erroneous classes as intializers.
>
>
> Proposed webrev:
> http://cr.openjdk.java.net/~jlahoda/8230847/webrev.00/
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8230847
>
> How does this look?
>
> Thanks,
>     Jan