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

[Jan Lahoda]

On 01. 11. 19 16:11, Maurizio Cimadamore wrote:
> Looks ok. I personally would rename attribIsolated to attribSpeculative 
> (e.g. add another overload) unless it clashes?

Thanks - I've renamed the method, and pushed the patch.

Jan

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