RFR: JDK-8223305: Compiler support for Switch Expressions

Jan Lahoda jan.lahoda at oracle.com
Thu May 30 14:50:09 UTC 2019 

On 30. 05. 19 16:15, Maurizio Cimadamore wrote:
> Looks great! You only missed one rename in Resolve:
> 
>       private Symbol checkVarType(Symbol bestSoFar, Name name) {
> 
> 
> I guess that should be checkRestrictedType, or something like it?

Yes, thanks for noticing. Will fix.

Jan

> 
> Maurizio
> 
> On 30/05/2019 14:58, Jan Lahoda wrote:
>> Hi,
>>
>> Thanks for all the comments so far. I've uploaded a new version of the 
>> patch here:
>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.01/
>> delta webrev from the previous state:
>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.delta.00.01/
>>
>> The changes are:
>> -adjustment to the updated spec, where yield is a restricted 
>> identifier (this required generalization of current error messages for 
>> var)
>> -placing yield-related elements at the end for changes in 
>> com.sun.source, to better reflect alphabet order
>> -simplification of error code from 
>> compiler.err.break.complex.value.no.switch.expression  to 
>> compiler.err.no.switch.expression.
>>
>> This patch does not include unification of the 
>> AttrContext.returnResult/yieldResult, but I've done that here:
>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.unified.attrcontext.result/ 
>>
>>
>> There is a small issue in return handling, as return needs to look and 
>> the enclosing Envs to see if there is a switch expression or not. But 
>> if this looks OK, I can fold it into the main patch.
>>
>> Further feedback is welcome!
>>
>> Thanks,
>>     Jan
>>
>> On 28. 05. 19 12:09, Maurizio Cimadamore wrote:
>>> Hi
>>> I thought a bit more about the code (the Attr part) and I have few 
>>> more comments:
>>>
>>> * I don't immediately see the need for having another field in 
>>> AttrContext. After all we could rename/repurpose the existing 
>>> resultInfo.
>>>
>>> * of course, to do that, we need a way to detect whether we're inside 
>>> a switch expression, but that should be possible by looking at the 
>>> env? Or you could even exploit the check context, after all, the 
>>> check context for a case arm is always created in the 
>>> switchExpressionContext method.
>>>
>>> * it seems like when you are in visitYield, you should first check if 
>>> there's a target for the yield - and if there's not you log an error. 
>>> That will also remove some dependencies from yieldResult.
>>>
>>> Of course you can also leave the code as is. It just occurred that 
>>> having a separate ResultInfo specifically for yield (or break with 
>>> value, as the code was also there before) seemed a bit redundant. But 
>>> I can also believe that the current approach leads to more sensible 
>>> code.
>>>
>>>
>>> Also, one small comment inline below.
>>>
>>>
>>> On 28/05/2019 10:37, Jan Lahoda wrote:
>>>> On 28. 05. 19 11:11, Maurizio Cimadamore wrote:
>>>>> Looks good. Just few comments/questions:
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> * I think the error keys in compiler.properties could use some 
>>>>> renaming. e.g.
>>>>>
>>>>> compiler.err.break.complex.value.no.switch.expression -> 
>>>>> compiler.err.no.switch.expression
>>>>> compiler.err.break.complex.value.no.switch.expression.qualify -> 
>>>>> compiler.err.no.switch.expression.qualify
>>>>
>>>> Sure, will do.
>>>>
>>>>>
>>>>> * what is the new Log.hasErrorOn - and why has Flow been changed to 
>>>>> use it?
>>>>
>>>> Consider code like this:
>>>> ---
>>>> public class Y2 {
>>>>     private void t() {
>>>>         break;
>>>>     }
>>>> }
>>>> ---
>>>>
>>>> When compiled like this:
>>>> javac -XDdev -XDshould-stop.at=FLOW Y2.java
>>>
>>> ah ok - it's the failover logic. I was trying to think of an example 
>>> w/o shouldStopAt and could not think of much.
>>>
>>> Thanks
>>> Maurizio
>>>
>>>> It will crash:
>>>> ---
>>>> Y2.java:4: error: break outside switch or loop
>>>>              break;
>>>>              ^
>>>> 1 error
>>>> An exception has occurred in the compiler (11.0.3). Please file a 
>>>> bug against the Java compiler via the Java bug reporting page 
>>>> (http://bugreport.java.com) after checking the Bug Database 
>>>> (http://bugs.java.com) for duplicates. Include your program and the 
>>>> following diagnostic in your report. Thank you.
>>>> java.lang.AssertionError
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.util.Assert.check(Assert.java:46)
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.visitMethodDef(Flow.java:518) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:866) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$BaseAnalyzer.scan(Flow.java:398) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.visitClassDef(Flow.java:488) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:774) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$BaseAnalyzer.scan(Flow.java:398) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.analyzeTree(Flow.java:759) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.analyzeTree(Flow.java:751) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.comp.Flow.analyzeTree(Flow.java:216)
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1401) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1375) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:973) 
>>>>
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:311)
>>>>         at 
>>>> jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:170)
>>>>         at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:57)
>>>>         at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:43)
>>>> ---
>>>>
>>>> The reason is that javac asserts that it has properly processed the 
>>>> exits - but here the original code is broken, and an error has 
>>>> already been reported and this given spot, so it seems safe to not 
>>>> crash javac here.
>>>>
>>>> Thanks,
>>>>     Jan
>>>>
>>>>>
>>>>> Thanks
>>>>> Maurizio
>>>>>
>>>>>
>>>>> It seems like a whitespace got remove here?
>>>>>
>>>>> On 24/05/2019 15:48, Jan Lahoda wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I'd like to ask for a review of changes to update javac to follow 
>>>>>> the current spec for switch expressions, in particular the break 
>>>>>> -> yield change:
>>>>>> http://cr.openjdk.java.net/~gbierman/jep354-jls-20190524.html
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.00/
>>>>>>
>>>>>> JBS:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8223305
>>>>>>
>>>>>> Feedback is welcome!
>>>>>>
>>>>>> Thanks,
>>>>>>    Jan

More information about the compiler-dev mailing list