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

[Jan Lahoda]

Hi Maurizio,

Thanks for the comments. I've renamed NOT_ALIVE to DEAD, as suggested, 
and replace "alive.alive" with appropriate tests (usually either == 
Liveness.DEAD or != Liveness.DEAD). I was first afraid this would look 
too bad, but seems to be quite reasonable to me in the end.

Updated webrev is here:
http://cr.openjdk.java.net/~jlahoda/8212982/webrev.01/

Any further feedback is welcome!

Thanks,
     Jan

On 20.11.2018 19:17, Maurizio Cimadamore wrote:
> 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