RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Jan Lahoda
jan.lahoda at oracle.com
Mon Aug 27 14:06:39 UTC 2018
Hello,
Turned out there were some bugs in the last patch:
-when parsing constant in rule cases ("case <constant> -> ..."), lambda
expression parsing is disabled, but the flag that disables lambda
expression parsing is cleared for more complicated expressions like
"case a + a -> ...". The proposed patch tweaks the JavacParser to
preserve the flag when parsing mode changes. The test (RuleParsingTest)
intentionally tests also expressions that are not constants, to ensure
error recovery can handle them.
-when the expression in a value break is a lambda, it is not processed
by LambdaToMethod properly. Fixed by a tweak to TreeTranslator to also
translate value breaks
A patch that fixes these problems, and also adds tests for variable
scopes in switches with rule cases is here:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.05/
Delta webrev from previous version:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.04.05/
Any feedback is welcome!
Thanks,
Jan
On 9.8.2018 14:53, Maurizio Cimadamore wrote:
> Looks good.
>
> Maurizio
>
>
> On 09/08/18 12:59, Jan Lahoda wrote:
>> One more tiny change to tests to fix a failure on Windows (I apologize
>> for not folding that into the previous update):
>> webrev:
>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.04/
>> delta from previous (webrev.03):
>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.03.04/
>>
>> Thanks,
>> Jan
>>
>> On 9.8.2018 13:39, Jan Lahoda wrote:
>>> On 3.8.2018 22:39, Maurizio Cimadamore wrote:
>>>> Looks good - when I eyeballed compiler.properties earlier I missed
>>>> this:
>>>>
>>>> "value break not supported in 'enhanced for' "
>>>>
>>>> I understand why you get 'enhanced for', not sure that adds value
>>>> w.r.t.
>>>> just 'for'. I don't have a strong opinion.
>>>
>>> Sounds reasonable, I've updated the compiler.properties, and added one
>>> more test for SimpleTreeVisitor.
>>>
>>> An updated patch is here:
>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.03/
>>>
>>> delta from the previous revision:
>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.02.03/
>>>
>>> Diagnostics:
>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.03/diags.html
>>>
>>> Thanks,
>>> Jan
>>>
>>>>
>>>> Maurizio
>>>>
>>>>
>>>> On 03/08/18 21:26, Jan Lahoda wrote:
>>>>> On 3.8.2018 16:22, Jan Lahoda wrote:
>>>>>> On 3.8.2018 15:47, Maurizio Cimadamore wrote:
>>>>>>> I like the diagnostic changes, and the API change - I think it's
>>>>>>> more
>>>>>>> consistent now - thanks for taking a second look.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> I've forgot to include the generated examples for this update, so
>>>>>> I've
>>>>>> placed it here:
>>>>>> file:///usr/local/home/lahvac/src/jdk/cr.openjdk.java.net/8192963/webrev.02/diags.html
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> I apologize for a wrong link, the correct one is:
>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.02/diags.html
>>>>>
>>>>> Jan
>>>>>
>>>>>>
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>>>
>>>>>>> Thumbs up.
>>>>>>>
>>>>>>> Maurizio
>>>>>>>
>>>>>>>
>>>>>>> On 03/08/18 14:35, Jan Lahoda wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Thanks for all the comments. An updated patch is here:
>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.02/
>>>>>>>> Changes against the previous revision:
>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.01.02/
>>>>>>>>
>>>>>>>> Updated specdiff:
>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/api.diff.02/overview-summary.html
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Any feedback is welcome.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jan
>>>>>>>>
>>>>>>>> On 1.8.2018 18:39, Maurizio Cimadamore wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 01/08/18 16:43, Jan Lahoda wrote:
>>>>>>>>>> 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".
>>>>>>>>>>
>>>>>>>>> Fine, but do we need two distinct errors? Or do we need maybe the
>>>>>>>>> same
>>>>>>>>> error and an extra detail string at the bottom for the 'you are
>>>>>>>>> inside a
>>>>>>>>> switch expr, but there's some other breaky context in between'
>>>>>>>>> condition?
>>>>>>>>>>>
>>>>>>>>>>> - "break is missing a value to return from switch expression"
>>>>>>>>>>
>>>>>>>>>> How about
>>>>>>>>>> "missing break value"?
>>>>>>>>>>
>>>>>>>>>> (the break is present, but the value is missing.)
>>>>>>>>> I'm fine with that!
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> - 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.
>>>>>>>>> I see, I'm not a big fan of the conditional expression nesting too
>>>>>>>>> - I
>>>>>>>>> think the important part is 'A != B' and it should be put front
>>>>>>>>> and
>>>>>>>>> center. That said, this is not a blocking issue, and I'm ok with a
>>>>>>>>> followup.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>> Maurizio
>>>>>>>>>>
>>>>>>>>>> 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