JDK-8212982: Rule cases in switch expression accepted even if complete normally (original) (raw)

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Wed Nov 21 11:02:54 UTC 2018

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\

Works better than this

switch rule completes normally\n\

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\

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 NOTALIVE 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.NOTALIVE (btw, probably better to call NOTALIVE 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, NOTALIVE, 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>

More information about the compiler-dev mailing list