RFR: JDK-8223305: Compiler support for Switch Expressions

[Maurizio Cimadamore]

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