RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)

Jan Lahoda jan.lahoda at oracle.com
Wed Aug 1 15:43:23 UTC 2018 

Hi Maurizio,

Thanks for the comments - responses are inlined.

On 1.8.2018 16:04, Maurizio Cimadamore wrote:
> Hi Jan,
> great work, some comments below.
>
> * Diagnostics. I see that some diagnostics seem to be inconsistent with
> what we do for 'return' - e.g. javac diagnostics for 'return' are quite
> succint:
>
> - "return outside method"
>
> - "missing return statement"
>
> - "unexpected return value"
>
> - "error: incompatible types: String cannot be converted to int"
>
> The analogous conditions for break in switch expression seem to be more
> verbose:
>
> - "expression break not immediately enclosed by a switch expression"

I think the closest error to "return outside method" is "value break 
outside of switch expression". The "expression break not immediately 
enclosed by a switch expression" is used when the break is inside 
another breakable statement inside the switch expression. I guess this 
should be rephrased at least to "value break not immediately enclosed by 
switch expression".

>
> - "break is missing a value to return from switch expression"

How about
"missing break value"?

(the break is present, but the value is missing.)

>
> - There's no equivalent for the third above - probably the closest
> condition would be to 'break expr' from a switch statement, in which
> case I bet you get a 'missing label' error - should we rectify that too?

There are several conditions, and the error differ, consider:
---
public class SwitchExpr {
       private void breakTest(int e) {
           break ;
           break x;
           break (e);
           while (true) {
                break x;
                break (e);
           }
       }
}
---

This gives:
---
SwitchExpr.java:3: error: break outside switch or loop
           break ;
           ^
SwitchExpr.java:4: error: undefined label: x
           break x;
           ^
SwitchExpr.java:5: error: value break outside of switch expression
           break (e);
           ^
SwitchExpr.java:7: error: undefined label: x
                break x;
                ^
SwitchExpr.java:8: error: value break outside of switch expression
                break (e);
                ^
5 errors
---

So when it is clear the break is a value break, a value break related 
diagnostic is reported, otherwise, the errors fall back to the existing 
diagnostics of cannot find labels.

>
> - "incompatible types: bad type in switch expression"
> ...
> (A cannot be converted to B)

I think this was modelled based on conditional expressions:
SwitchExpr.java:10: error: incompatible types: bad type in conditional 
expression
           B b = e == 0 ? (A) null : (B) null;
                          ^
     A cannot be converted to B

I'll look if this can be changed.

>
> Not sure whether we should try to maintain the same style across
> break/return, but it seemed worth at least pointing out.
>
> Other diagnostic comments:
>
> - "error: j is ambiguously both a valid expression and a valid label" -
> this seems convoluted, I would personally rephrase as something like:
>
> ambiguous reference to 'j'
> ...
> ('j' is both a label and an expression)

Will done.

>
> - "break is jumping outside of the enclosing switch expression" ->
> "break outside of enclosing switch expression"

Will do.

>
> - "continue is jumping outside of the enclosing switch expression" ->
> "continue outside of enclosing switch expression"

Will do.

>
> * Attr - The refactoring of switchExpr into switch + handleSwitch is
> very sweet - I like it!
>
> * Flow - a bit odd that we have to make up a new tree to call recordExit
> - but I see why you're doing it :-)
>
> * Lower - it would be nice to have a comment on SwitchExpression that
> shows what the generated code shape looks like; I think we do that for
> other non-trivial translations (e.g. for each)

Will do.

Thanks,
     Jan

>
> * ExpSwitchNestingTest - I see that this test is not using the newer
> combo framework. Actually, I'm not even sure this one needs any
> templating logic at all, as it seems the test is doing its own
> replacements. But I was going to note that this test is not using the
> shared context enhancements, which I guess it's still ok given the
> number of combinations checked is low. Maybe worth keeping an eye on
> this in the future, should we keep adding new cases to it.
>
> Maurizio
>
> On 01/08/18 07:28, Jan Lahoda wrote:
>> Hi Jon,
>>
>> Thanks for the comments!
>>
>> An updated webrev that reflects the comments is here:
>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.01/
>>
>> A delta webrev between this webrev and the webrev.00 is here:
>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.00.01/
>>
>> Does this look better?
>>
>> Thanks!
>>
>> Jan
>>
>> On 28.7.2018 01:12, Jonathan Gibbons wrote:
>>> CaseTree.java:52: spelling, "lables"
>>>
>>> CaseTree.java:84:  "return" should be "returns".
>>>
>>> Lots of @Deprecated annotations without corresponding @deprecated
>>> javadoc tag.
>>> Should the @apiNote really be @deprecated.  I also think that using
>>> @Deprecated
>>> like this is "a bit smelly", and had a discussion with Stuart "Dr
>>> Deprecator" regarding
>>> my concerns. If nothing else, assuming the feature is a success, we are
>>> setting up
>>> a precedent of marking an API as deprecated-for-removal, and then in the
>>> next
>>> release, simply removing the annotation.
>>>
>>> Attr.java:1482: ugly use of @SuppressWarnings. I understand why it is
>>> there,
>>> but it exemplifies why @Deprecated on the declaration is a bad idea.
>>> It'll be too easy to not remove these annotations in JDK 13.
>>>
>>> Attr:1825: why XXX
>>>
>>> LambdaToMethod: unnecessary new import, CaseKind
>>> Flow: unnecessary new import, CaseKind
>>>
>>> JavacParser: order of imports
>>>
>>> messages: generally, I think the text of the messages could be improved
>>> (related but not new) we should somehow identify language keywords from
>>> English text (i.e. words like `break, `case`, `continue`, `return`)
>>> 204: saying the name is both a valid label and a valid expression hints
>>> at the cause but not the error (i.e. the name is ambiguous)
>>>
>>> JCTree: inconsistent layout of annotations; inconsistent use of
>>> @Deprecated and @Deprecated(forRemoval=true)
>>>
>>> (tests)
>>> Not reviewed in detail yet.
>>> New test descriptions should use "/*" for jtreg comment block; not
>>> "/**". These are not javadoc documentation comments.
>>> New test descriptions should have @summary
>>> Presumably, we will have to change the -old.out files if the preview
>>> feature becomes standard in JDK 13.
>>>
>>>
>>> Overall,
>>>
>>> The logic is nice, but the use of @Deprecated(forRemoval=true) and
>>> resulting @SuppressWarnings really suck.
>>> I don't think the current practice is serving us well.
>>>
>>> -- Jon
>>>
>>>
>>> On 07/17/2018 11:00 AM, Jan Lahoda wrote:
>>>> Hi,
>>>>
>>>> As JEP 325 is in the proposed to target state, I thought it might be a
>>>> good idea to start a review process for the code.
>>>>
>>>> The code here strives to implement the current specification for the
>>>> Switch Expressions:
>>>> http://cr.openjdk.java.net/~gbierman/switch-expressions.html
>>>>
>>>> The current webrev is here:
>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.00/
>>>>
>>>> (includes a list of new errors.)
>>>>
>>>> JBS:
>>>> https://bugs.openjdk.java.net/browse/JDK-8192963
>>>> https://bugs.openjdk.java.net/browse/JDK-8206986
>>>>
>>>> CSRs:
>>>> https://bugs.openjdk.java.net/browse/JDK-8207241
>>>> https://bugs.openjdk.java.net/browse/JDK-8207406
>>>>
>>>> Any feedback is welcome!
>>>>
>>>> Thanks,
>>>>     Jan
>>>
>

More information about the compiler-dev mailing list