RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Aug 27 14:32:31 UTC 2018
Sensible fixes - generally looks good.
The change to analyzeParens is subtle - that is, if you see something
like (x) -> ..., that logic would reasonably conclude that the
parenthesis corresponds to an implicit lambda decl - so parser would
attempt to go there, which at first seems sensible. But in reality, when
inside a 'case' you really want the compiler to process a (x) followed
by an arrow (->), where the former is the (parenthesized) constant
expression and the latter is the CASE arrow.
But, one followup question is: what happens with this?
case (x, y) -> 42
I believe your patch now classify this as a parenthesized expression,
which probably generates several downstream errors?
Maurizio
On 27/08/18 15:06, Jan Lahoda wrote:
> 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