Migrating from Bug 578856 - [18] ECJ rejects valid code with switch pattern and conditional expression
The compiler reports a syntax error with the following code:
public void bar(Object o) {
int i = switch(o) {
case String a && o != null ? true : false -> {
yield 0;
}
default -> 1;
};
}
// Syntax error on token "a", delete this token
If I wrap the guard in (), like below, then the code compiles:
case String a && (o != null ? true : false) ->
Discussion Details:
@jarthana : I believe that the ecj behaviour is correct. Here’s the reasoning..
From https://docs.oracle.com/javase/tutorial/java/nutsandbolts/operators.html we see that Conditional-And Operator “&&” has higher operator precedence than the Conditional Operator “?:” . From https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.23, we see that “The conditional-and operator is syntactically left-associative (it groups left-to-right).”
Applying the above to the case statement,
int i = switch(o) {
case String a && o != null ? true : false -> {
yield 0;
}
I would NOT expect the result to be:
case ((String a) && ((o != null) ? true : false))
rather I would expect the result to be:
case ((String a) && (o != null)) ? true : false
which is indeed a syntactical error as flagged by ecj.
By the same argument, since the Conditional-or has a lower precedence than conditional-And, the following code
case String a && b || c ->
would translate to case ((String a) && b) || c ->
and would give error.
However, in case of !=, there is no issue, since != has a higher precedence than &&
case String a && o != null
and hence this would translate to (o!= null)
Also, I don't see any mention of the precedence changes in spec 420 [latest at https://cr.openjdk.java.net/~gbierman/jep420/latest]
By the above, I believe the flagging of syntax error is the right thing to do for code in comment 0
@stephan-herrmann : Can you also please have a look and let know of comments, if any? thanks!
I was going back and forth here:
In this example the '&&' token does not indicate a ConditionalAndExpression (since the lhs is not an expression), so the above reasoning about precedence '&&' vs. '||' falls short.
Secondly, reporting syntax error does not follow from the grammar, but it is caused by the VanguardParser failing to detect goal.hasBeenReached() during disambiguateCasePattern.
This happens, because after "case String a" the follow set doesn't include the observed token '&&'. I guess this should just be added to PatternCaseLabelFollow.
When doing so, we are able to parse more but now GuardedPattern is prematurely reduced with only "String a && o != null" leaving everything from '?' unrecognized.
This syntax error, however, seems justified, because ConditionalExpression is NOT in ConditionalAndExpression as required for GuardedPattern:
GuardedPattern:
PrimaryPattern && ConditionalAndExpression
If this reasoning is correct, then ecj should indeed report syntax error, just a little later than where it does today.
See that also this is illegal:
case String a && (o != null) || false -> {
whereas this is legal:
case String a && (o != null) | false -> {
Reason: also ConditionalOrExpression is not in ConditionalAndExpression, but InclusiveOrExpression is.
(In reply to Jay Arthanareeswaran from comment #0)
If I wrap the guard in (), like below, then the code compiles:
case String a && (o != null ? true : false) ->
Also this is correct, since PrimaryNoNewArray (which can produce 'PushLPAREN Expression_NotName PushRPAREN') is in ConditionalAndExpression (via many indirections :) ).
[tag] [reply] [−]Comment 4
(In reply to Stephan Herrmann from comment #2)
Thanks Stephan for taking a look. Meanwhile there is a reponse from Gavin that has come in a few hours back in the thread - https://mail.openjdk.java.net/pipermail/amber-spec-experts/2022-March/003307.html
Looks like a conflict in the first glance - need to look into this further as well - am yet to go through your comments also
(In reply to Manoj Palat from comment #4)
Looks like a conflict in the first glance [...]
Conflict between what?
(In reply to Stephan Herrmann from comment #2)
I was going back and forth here:
Thanks Stephan for pitching in. Went through the comments, did another round of analysis and debugging of ecj to see where we are and here are my thoughts
In this example the '&&' token does not indicate a ConditionalAndExpression
(since the lhs is not an expression), so the above reasoning about
precedence '&&' vs. '||' falls short.
Agree - I went down through an erroneous path here - precedence doesn't have any relevance here.
Secondly, reporting syntax error does not follow from the grammar, but it is
caused by the VanguardParser failing to detect goal.hasBeenReached() during
disambiguateCasePattern.
+1
This happens, because after "case String a" the follow set doesn't include
the observed token '&&'. I guess this should just be added to
PatternCaseLabelFollow.
I had some comments about this earlier - in bug 578241 comment 4 to be precise - ie "PatternCaseLabelFollow represents the FollowSet of a CaseLabelElement for a pattern - sorry for the bad naming - So, TokenNameAND_AND is a misfit here since it cannot come in a follow set of a pattern case label. "
Can rethink about this but as mentioned did not put && since its not a FOLLOW of the case label, strictly speaking, that is.
When doing so, we are able to parse more but now GuardedPattern is
prematurely reduced with only "String a && o != null" leaving everything
from '?' unrecognized.
This syntax error, however, seems justified, because ConditionalExpression
is NOT in ConditionalAndExpression as required for GuardedPattern:
As an answer to comment 5, the conflict mentioned in comment 4 is between the response from EG group[mail thread from comment 4] where it is mentioned that javac is right in accepting for ecj this is a syntax error. I went down the path given by you in the comment and see that it should be a syntax error indeed
GuardedPattern:
PrimaryPattern && ConditionalAndExpression
.
Reason: also ConditionalOrExpression is not in ConditionalAndExpression, but
InclusiveOrExpression is.
So without parenthesis, ie the code in description :
case String a && o != null ? true : false ->
follows the path of:
666 AssignmentExpression -> ConditionalExpression
683 Expression ::= AssignmentExpression
688 ConstantExpression -> Expression
And as you mentioned this cannot be reduced to a ConditionalAndExpression and hence agree that the error is justified
Now moving to with parenthesis,
case String a && (o != null ? true : false) ->
I agree, as usual :) with your conclusion again in comment 3
Looked through the grammar in https://docs.oracle.com/javase/specs/jls/se17/html/jls-19.html and additionally the preview part (for GuardedPattern), I see that this indeed should be accepted. The ecj version given below for reference
875 ConditionalExpression_NotName ::= ConditionalOrExpression_NotName
QUESTION Expression COLON
ConditionalExpression
877 AssignmentExpression_NotName -> ConditionalExpression_NotName
879 Expression_NotName -> AssignmentExpression_NotName
This is in fact equivalent to the rules
666 AssignmentExpression -> ConditionalExpression
683 Expression ::= AssignmentExpression
And then on consumption of RParen
510 PushRPAREN ::= RPAREN
Gets to the next rule:
516 PrimaryNoNewArray ::= ‘(‘ Expression_NotName ‘)’
ie and so on..
516 PrimaryNoNewArray ::= PushLPAREN Expression_NotName PushRPAREN
511 Primary -> PrimaryNoNewArray
602 PostfixExpression -> Primary
616 UnaryExpressionNotPlusMinus -> PostfixExpression
613 UnaryExpression -> UnaryExpressionNotPlusMinus
635 MultiplicativeExpression -> UnaryExpression
639 AdditiveExpression -> MultiplicativeExpression
642 ShiftExpression -> AdditiveExpression
646 RelationalExpression -> ShiftExpression
352 InstanceofExpression -> RelationalExpression
*651 EqualityExpression -> InstanceofExpression
654 AndExpression -> EqualityExpression
656 ExclusiveOrExpression -> AndExpression
658 InclusiveOrExpression -> ExclusiveOrExpression
660 ConditionalAndExpression -> InclusiveOrExpression
366 GuardedPattern ::= PrimaryPattern AND_AND ConditionalAndExpression
[Dev Note - the numeric represents the internal state - act in Parser.consumeRule() - of ecj for the given java.g at this point in time - They may not have a direct control flow while debugging - to mandatorily (for debugging) have this flow through the states, change -> to ::= with a consume**() method - else many of these states may get optimized away]
So in conclusion, agree with your analysis:
case String a && o != null ? true : false -> // Reject
case String a && (o != null ? true : false) -> // Accept
[tag] [reply] [−]Comment 7Stephan Herrmann CLA Friend 2022-03-18 12:28:50 EDT
(In reply to Manoj Palat from comment #6)
(In reply to Stephan Herrmann from comment #2)
This happens, because after "case String a" the follow set doesn't include
the observed token '&&'. I guess this should just be added to
PatternCaseLabelFollow.
I had some comments about this earlier - in bug 578241 comment 4 to be
precise - ie "PatternCaseLabelFollow represents the FollowSet of a
CaseLabelElement for a pattern - sorry for the bad naming - So,
TokenNameAND_AND is a misfit here since it cannot come in a follow set of a
pattern case label. "
Can rethink about this but as mentioned did not put && since its not a
FOLLOW of the case label, strictly speaking, that is.
I think the confusion here stems from the fact that the grammar is not left-factored (i.e., two grammar rules PrimaryPattern and GuardedPattern have the same prefix).
The vanguard parser tries to match this goal:
BeginCaseElement Pattern
Once it has seen "case String a" it assumes matching is done and proceeds to checking the next token against the follow set.
Perhaps the following equivalent rule would produce a different result (just guessing):
Pattern:
PrimaryPattern GuardOpt
But since all the vanguard parsing is outside spec-land, I would be comfortable with opportunistically adding '&&' to the follow set, saying: when you recognized a (minimal) Pattern, the text can legally continue with '&&'. If that would lead to prematurely accepting smth that is not a GuardedPattern after all, I would be hard pressed to imagine that a different disambiguation would have produced a legal interpretation of the input. Can you think of an input that would cause this kind of trouble?
[tag] [reply] [−]Comment 8
(In reply to Stephan Herrmann from comment #7)
But since all the vanguard parsing is outside spec-land, I would be
comfortable with opportunistically adding '&&' to the follow set, saying:
when you recognized a (minimal) Pattern, the text can legally continue with
'&&'.
Yes, since vanguard is outside spec-land, we can implement this change - moving this to the 4.24 time-frame
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent