JDK-8212982: Rule cases in switch expression accepted even if complete normally

[Maurizio Cimadamore]

I like the tri-state fix; there is however something that leaves me 
perplexed. I see often idioms such as

if (alive.alive ...

Now, it seems like the Liveliness enum has a dual nature; on the one 
hand it's a tri-state (alive, dead, recovery). But there are cases in 
which you want to project it down to a two-state. The use of an hidden 
boolean here makes the code less readable, because a reader might ask - 
how is that different from alive == Liveliness.ALIVE?

I think we should try to think as to what is the condition that we want 
to capture with the alive.alive and then have a method on the enum which 
returns 'true' for certain constants and false for others.

I think what the code is doing is checking whether the liveliness *is 
not* dead.

So perhaps, conditions such as alive.alive might better be rewritten as 
alive != Liveliness.NOT_ALIVE

(btw, probably better to call NOT_ALIVE just DEAD and avoid all the 
double negations).

Maurizio


On 20/11/2018 17:14, Jan Lahoda wrote:
> Hi,
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8212982
>
> The issue here is that javac accepts switch expressions that complete 
> normally without providing a value, which is not correct. A (simpler) 
> fix is to enhance Flow with the necessary checks + enhancements to 
> place the errors at a good place.
>
> Webrev: http://cr.openjdk.java.net/~jlahoda/8212982/webrev.00/
>
> This patch has a problem in cases like:
> ---
> public class SE {
>      private String t(int i) {
>          return switch (i) {
>              default:
>                  break "";
>                  System.err.println(0);
>          };
>      }
> }
> ---
>
> This produces:
> ---
> $ javac --enable-preview --source 12 SE.java
> SE.java:6: error: unreachable statement
>                  System.err.println(0);
>                  ^
> SE.java:7: error: switch expression completes without providing a value
>          };
>          ^
>   (switch expressions must either provide a value or throw for all 
> possible input values)
> ---
>
> The second error is caused by the first one (Flow will reset "alive" 
> to "true" when reporting the "unreachable statement" error as an error 
> recovery). A patch that changes the "alive" field to be tri-state 
> (ALIVE, NOT_ALIVE, RECOVERY) so that it can suppress the second error 
> in case of this recovery is here:
>
> Webrev 2: http://cr.openjdk.java.net/~jlahoda/8212982/webrev.00b/
>
> (The only differences between the patches are in the Flow.java and 
> ExpressionSwitchUnreachable.out.)
>
> This is longer, but I think it provides better errors, so I'd prefer 
> this patch, but I am also fine with the first one.
>
> Any feedback is welcome!
>
> Thanks,
>     Jan