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

[Maurizio Cimadamore]

Looks great.

One minor nit that I had not notice yesterday on the error messages; I 
think this message:

switch expression completes without providing a value\n\
+ (switch expressions must either provide a value or throw for all 
possible input values)


Works better than this

switch rule completes normally\n\
+ (switch rules in switch expressions must either provide a value or throw)


The reason being that "switch rule completes normally", the first line 
of the message, is kind of vague, and does not give many indications of 
what's wrong.

I'd prefer something like this:

switch rule completes without providing a value\n\
+ (switch rules in switch expressions must either provide a value or throw)


There is a bit of repetition, yes, but I think it's a better message.

I'll leave this to your consideration. No need for another review.

Thanks!
Maurizio

On 21/11/2018 10:31, Jan Lahoda wrote:
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20181121/83a9d768/attachment.html>