Skip to content

Commit

Permalink
Scanner and Parser support for Switch expressions 2.0 (#3264)
Browse files Browse the repository at this point in the history 
* Restructure switch expressions grammar aligning it with JLS terminology; Eliminate unnecessary calls outs from the automaton (-> instead of ::=); fold multiple consume rules into one by parameterizing; delete dangling non-terminals (ThrowExpression ...); deduplicate unnecessary distinct non-terminals (e.g `SwitchLabelExpr` vs `SwitchLabel` ...) 
* Replace the pair of tokens, first the synthetic `BeginCaseExpr` followed by '->' in switch rules with a single synthetic `CaseArrow`. The scanner will disambiguate whether a  token '->' is `TokenNameARROW` used in lambdas or is a `CaseArrow` used in switch rules by checking if the automaton will shift  `CaseArrow` or not. 
* DiagnoseParser's repair of a misclassified '->' will allow it to recover perfectly! Just suppressing the message will do.
* Get rid of code engaging `VanguardParser` in reconnaissance missions for `yield` and '->' disambiguation.
* Eliminate feedback from Parser to Scanner on co-ordinates of the case statement
* Eliminate feedforward from Parser to SwitchStatement on whether it contains patterns, nulls etc; Let Switch discover it's self by itself :)
* Declutter `SwitchStatement.resolve()` by simplifying control flow; streamline management of secrets; 
* Delete `SelectionParserTest12.java` as it is fully duplicated by `SelectionParserTest13.java`; Rename the latter to `SelectionParserTest14.java` and ensure it runs in the right compliance mode. Hook up this dangling junit to a parent
* Tweak a failing test case in `PrimitiveInPatternsTestSH.java` to work around a bug elsewhere that was masked so far. #3265 raised as follow up.
* Adjust a few tests that result in altered diagnostics due to grammar refactoring
* Adjust test in `FormatterRegressionTests.java`  and `ResolveTests12To15.java` to run at the right compliance levels
  • Loading branch information
@srikanth-sankaran
srikanth-sankaran authored Nov 8, 2024
1 parent dd9a963 commit b294075
Show file tree
Hide file tree
Showing 50 changed files with 1,087 additions and 2,000 deletions.
74 changes: 28 additions & 46 deletions org.eclipse.jdt.core.compiler.batch/grammar/java.g
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ $Terminals
ElidedSemicolonAndRightBrace
AT308
AT308DOTDOTDOT
BeginCaseExpr
CaseArrow
RestrictedIdentifierYield
RestrictedIdentifierrecord
RestrictedIdentifiersealed
Expand Down Expand Up @@ -226,9 +226,6 @@ Goal ::= '(' ParenthesizedCastNameAndBounds
Goal ::= '<' ReferenceExpressionTypeArgumentsAndTrunk
-- JSR 308 Reconnaissance mission.
Goal ::= '@' TypeAnnotations
-- JSR 354 Reconnaissance mission.
Goal ::= '->' YieldStatement
Goal ::= '->' SwitchLabelCaseLhs
-- JEP 409 Sealed types Reconnaissance mission.
Goal ::= RestrictedIdentifiersealed Modifiersopt
Goal ::= RestrictedIdentifierpermits PermittedTypes
Expand Down Expand Up @@ -1491,37 +1488,41 @@ IfThenElseStatementNoShortIf ::= 'if' '(' Expression ')' PostExpressionInIf Stat
/:$readableName IfStatement:/

SwitchStatement ::= 'switch' '(' Expression ')' PostExpressionInSwitchStatement OpenBlock SwitchBlock
/.$putCase consumeStatementSwitch() ; $break ./
/.$putCase consumeSwitchStatementOrExpression(true) ; $break ./
/:$readableName SwitchStatement:/

SwitchBlock ::= '{' '}'
/.$putCase consumeEmptySwitchBlock() ; $break ./
/.$putCase consumeSwitchBlock(false) ; $break ./

SwitchBlock ::= '{' SwitchBlockStatements '}'
SwitchBlock ::= '{' SwitchLabels '}'
SwitchBlock -> '{' SwitchBlockStatements '}'
SwitchBlock -> '{' SwitchLabels '}'
SwitchBlock ::= '{' SwitchBlockStatements SwitchLabels '}'
/.$putCase consumeSwitchBlock() ; $break ./
/.$putCase consumeSwitchBlock(true) ; $break ./
/:$readableName SwitchBlock:/

SwitchBlockStatements -> SwitchBlockStatement
SwitchBlockStatements ::= SwitchBlockStatements SwitchBlockStatement
/.$putCase consumeSwitchBlockStatements() ; $break ./
/:$readableName SwitchBlockStatements:/

SwitchBlockStatement -> SwitchLabeledRule
SwitchBlockStatement -> SwitchRule
SwitchBlockStatement ::= SwitchLabels BlockStatements
/.$putCase consumeSwitchBlockStatement() ; $break ./
/:$readableName SwitchBlockStatement:/

SwitchLabels -> SwitchLabel
SwitchLabels ::= SwitchLabels SwitchLabel
/.$putCase consumeSwitchLabels() ; $break ./
/:$readableName SwitchLabels:/
SwitchLabels ::= SwitchLabel ':'
/.$putCase consumeSwitchLabels(false, false) ; $break ./
SwitchLabels ::= SwitchLabels SwitchLabel ':'
/.$putCase consumeSwitchLabels(true, false) ; $break ./
/:$readableName SwitchLabel:/

PostCaseArrow ::= $empty
/.$putCase consumeSwitchLabels(false, true) ; $break ./

SwitchLabel ::= SwitchLabelCaseLhs ':'
/. $putCase consumeCaseLabel(); $break ./
SwitchLabel -> SwitchLabelCaseLhs
/:$readableName SwitchLabel:/

SwitchLabel ::= 'default' ':'
SwitchLabel ::= 'default'
/. $putCase consumeDefaultLabel(); $break ./
/:$readableName SwitchLabel:/

Expand All @@ -1531,34 +1532,20 @@ UnaryExpressionNotPlusMinus -> SwitchExpression
UnaryExpressionNotPlusMinus_NotName -> SwitchExpression

SwitchExpression ::= 'switch' '(' Expression ')' PostExpressionInSwitchExpression OpenBlock SwitchBlock
/.$putCase consumeSwitchExpression() ; $break ./
/.$putCase consumeSwitchStatementOrExpression(false) ; $break ./
/:$readableName SwitchExpression:/

SwitchLabeledRule ::= SwitchLabeledExpression
SwitchLabeledRule ::= SwitchLabeledBlock
SwitchLabeledRule ::= SwitchLabeledThrowStatement
/. $putCase consumeSwitchLabeledRule(); $break ./
/:$readableName SwitchLabeledRule:/

SwitchLabeledExpression ::= SwitchLabelExpr Expression ';'
/. $putCase consumeSwitchLabeledExpression(); $break ./
/:$readableName SwitchLabeledExpression:/
SwitchRule ::= SwitchLabel CaseArrow PostCaseArrow Expression ';'
/. $putCase consumeSwitchRule(SwitchRuleKind.EXPRESSION); $break ./
/:$readableName SwitchRule:/

SwitchLabeledBlock ::= SwitchLabelExpr Block
/. $putCase consumeSwitchLabeledBlock(); $break ./
/:$readableName SwitchLabeledBlock:/
SwitchRule ::= SwitchLabel CaseArrow PostCaseArrow Block
/. $putCase consumeSwitchRule(SwitchRuleKind.BLOCK); $break ./
/:$readableName SwitchRule:/

SwitchLabeledThrowStatement ::= SwitchLabelExpr ThrowExpression ';'
/. $putCase consumeSwitchLabeledThrowStatement(); $break ./
/:$readableName SwitchLabeledThrowStatement:/

SwitchLabelExpr ::= 'default' '->'
/. $putCase consumeDefaultLabelExpr(); $break ./
/:$readableName SwitchLabelDefaultExpr:/

SwitchLabelExpr ::= SwitchLabelCaseLhs BeginCaseExpr '->'
/. $putCase consumeCaseLabelExpr(); $break ./
/:$readableName SwitchLabelExpr:/
SwitchRule ::= SwitchLabel CaseArrow PostCaseArrow ThrowStatement
/. $putCase consumeSwitchRule(SwitchRuleKind.THROW); $break ./
/:$readableName SwitchRule:/

SwitchLabelCaseLhs ::= 'case' CaseLabelElements
/. $putCase consumeSwitchLabelCaseLhs(); $break ./
Expand Down Expand Up @@ -1669,10 +1656,6 @@ ThrowStatement ::= 'throw' Expression ';'
/.$putCase consumeStatementThrow(); $break ./
/:$readableName ThrowStatement:/

ThrowExpression ::= 'throw' Expression
/.$putCase consumeThrowExpression() ; $break ./
/:$readableName ThrowExpression:/

SynchronizedStatement ::= OnlySynchronized '(' Expression ')' Block
/.$putCase consumeStatementSynchronized(); $break ./
/:$readableName SynchronizedStatement:/
Expand Down Expand Up @@ -3191,4 +3174,3 @@ UNDERSCORE ::= '_'

$end
-- need a carriage return after the $end

22 changes: 10 additions & 12 deletions ...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/CaseStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,31 +202,30 @@ public ResolvedCase[] resolveCase(BlockScope scope, TypeBinding switchExpression
for (Expression e : this.constantExpressions) {
count++;
if (e instanceof FakeDefaultLiteral) {
switchStatement.containsPatterns = switchStatement.isNonTraditional = true;
checkDuplicateDefault(scope, switchStatement, this.constantExpressions.length > 1 ? e : this);
if (count != 2 || nullCaseCount < 1) {
if (count != 2 || nullCaseCount < 1)
scope.problemReporter().patternSwitchCaseDefaultOnlyAsSecond(e);
}
continue;
}
if (e instanceof NullLiteral) {
if (switchStatement.nullCase == null) {
switchStatement.containsNull = switchStatement.isNonTraditional = true;
if (switchStatement.nullCase == null)
switchStatement.nullCase = this;
}
nullCaseCount++;
if (count > 1 && nullCaseCount < 2) {
if (count > 1 && nullCaseCount < 2)
scope.problemReporter().patternSwitchNullOnlyOrFirstWithDefault(e);
}
}

// tag constant name with enum type for privileged access to its members
if (switchExpressionType != null && switchExpressionType.isEnum() && (e instanceof SingleNameReference)) {
if (switchExpressionType != null && switchExpressionType.isEnum() && (e instanceof SingleNameReference))
((SingleNameReference) e).setActualReceiverType((ReferenceBinding)switchExpressionType);
} else if (e instanceof FakeDefaultLiteral) {
continue; // already processed
}

e.setExpressionContext(ExpressionContext.TESTING_CONTEXT);
if (e instanceof Pattern p)
if (e instanceof Pattern p) {
switchStatement.containsPatterns = switchStatement.isNonTraditional = true;
p.setOuterExpressionType(switchExpressionType);
}

TypeBinding caseType = e.resolveType(scope);

Expand All @@ -235,7 +234,6 @@ public ResolvedCase[] resolveCase(BlockScope scope, TypeBinding switchExpression
continue;
}


if (caseType.isValidBinding()) {
if (e instanceof Pattern) {
for (Pattern p : ((Pattern) e).getAlternatives()) {
Expand Down
93 changes: 53 additions & 40 deletions ...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1085,37 +1085,29 @@ boolean isAllowedType(TypeBinding type) {
@Override
public void resolve(BlockScope upperScope) {
try {
boolean isStringSwitch = false;
TypeBinding expressionType = this.expression.resolveType(upperScope);
CompilerOptions compilerOptions = upperScope.compilerOptions();
if (expressionType != null) {
this.expression.computeConversion(upperScope, expressionType, expressionType);
checkType: {

if (!expressionType.isValidBinding()) {
expressionType = null; // fault-tolerance: ignore type mismatch from constants from hereon
break checkType;
} else if (expressionType.isBaseType()) {
if (JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(compilerOptions)) {
}

if (expressionType.isBaseType()) {
if (JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(compilerOptions))
this.isPrimitiveSwitch = true;
}
if (this.expression.isConstantValueOfTypeAssignableToType(expressionType, TypeBinding.INT))
break checkType;
if (expressionType.isCompatibleWith(TypeBinding.INT))
break checkType;
} else if (expressionType.isEnum()) {
break checkType;
} else if (!this.containsPatterns && !this.containsNull && upperScope.isBoxingCompatibleWith(expressionType, TypeBinding.INT)) {
this.expression.computeConversion(upperScope, TypeBinding.INT, expressionType);
break checkType;
} else if (expressionType.id == TypeIds.T_JavaLangString) {
if (this.containsPatterns || this.containsNull) {
isStringSwitch = !JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions);
this.isNonTraditional = true;
break checkType;
}
isStringSwitch = true;
break checkType;
}

if (expressionType.id == TypeIds.T_JavaLangString || expressionType.isEnum() || upperScope.isBoxingCompatibleWith(expressionType, TypeBinding.INT))
break checkType;

if (!JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions) || (expressionType.isBaseType() && expressionType.id != T_null && expressionType.id != T_void)) {
if (!this.isPrimitiveSwitch) { // when isPrimitiveSwitch is set it is approved above
upperScope.problemReporter().incorrectSwitchType(this.expression, expressionType);
Expand All @@ -1126,16 +1118,10 @@ public void resolve(BlockScope upperScope) {
}
}
}
if (isStringSwitch) {
// the secret variable should be created before iterating over the switch's statements that could
// create more locals. This must be done to prevent overlapping of locals
// See https://bugs.eclipse.org/bugs/show_bug.cgi?id=356002
this.dispatchStringCopy = new LocalVariableBinding(SecretStringVariableName, upperScope.getJavaLangString(), ClassFileConstants.AccDefault, false);
upperScope.addLocalVariable(this.dispatchStringCopy);
this.dispatchStringCopy.setConstant(Constant.NotAConstant);
this.dispatchStringCopy.useFlag = LocalVariableBinding.USED;
}
addSecretPatternSwitchVariables(upperScope);

if (expressionType != null)
reserveSecretVariablesSlots(upperScope);

if (this.statements != null) {
if (this.scope == null)
this.scope = new BlockScope(upperScope);
Expand Down Expand Up @@ -1258,12 +1244,22 @@ && defaultFound && isExhaustive()) {
upperScope.problemReporter().undocumentedEmptyBlock(this.blockStart, this.sourceEnd);
}
}
// Try it again in case we found any qualified enums.
if (this.dispatchPatternCopy == null) {
addSecretPatternSwitchVariables(upperScope);
}

complainIfNotExhaustiveSwitch(upperScope, expressionType, compilerOptions);
if (expressionType != null) {
if (!expressionType.isBaseType() && upperScope.isBoxingCompatibleWith(expressionType, TypeBinding.INT)) {
if (this.containsPatterns || this.containsNull) {
if (!JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions) || (expressionType.isBaseType() && expressionType.id != T_null && expressionType.id != T_void)) {
if (!this.isPrimitiveSwitch) { // when isPrimitiveSwitch is set it is approved above
upperScope.problemReporter().incorrectSwitchType(this.expression, expressionType);
expressionType = null; // fault-tolerance: ignore type mismatch from constants from hereon
}
}
} else
this.expression.computeConversion(upperScope, TypeBinding.INT, expressionType);
}
releaseUnusedSecretVariables(upperScope);
complainIfNotExhaustiveSwitch(upperScope, expressionType, compilerOptions);
}

} finally {
if (this.scope != null) this.scope.enclosingCase = null; // no longer inside switch case block
Expand Down Expand Up @@ -1473,19 +1469,36 @@ private boolean needPatternDispatchCopy() {
}
return !(eType.isPrimitiveOrBoxedPrimitiveType() || eType.isEnum() || eType.id == TypeIds.T_JavaLangString); // classic selectors
}
private void addSecretPatternSwitchVariables(BlockScope upperScope) {

private void reserveSecretVariablesSlots(BlockScope upperScope) { // may be released later if unused.

if (this.expression.resolvedType.id == T_JavaLangString) {
this.dispatchStringCopy = new LocalVariableBinding(SecretStringVariableName, upperScope.getJavaLangString(), ClassFileConstants.AccDefault, false);
upperScope.addLocalVariable(this.dispatchStringCopy);
this.dispatchStringCopy.setConstant(Constant.NotAConstant);
}

this.scope = new BlockScope(upperScope);

this.dispatchPatternCopy = new LocalVariableBinding(SecretPatternVariableName, this.expression.resolvedType, ClassFileConstants.AccDefault, false);
this.scope.addLocalVariable(this.dispatchPatternCopy);
this.dispatchPatternCopy.setConstant(Constant.NotAConstant);

this.restartIndexLocal = new LocalVariableBinding(SecretPatternRestartIndexName, TypeBinding.INT, ClassFileConstants.AccDefault, false);
this.scope.addLocalVariable(this.restartIndexLocal);
this.restartIndexLocal.setConstant(Constant.NotAConstant);
}

private void releaseUnusedSecretVariables(BlockScope upperScope) {
if (this.expression.resolvedType.id == T_JavaLangString && !this.isNonTraditional)
this.dispatchStringCopy.useFlag = LocalVariableBinding.USED;

if (needPatternDispatchCopy()) {
this.scope = new BlockScope(upperScope);
this.dispatchPatternCopy = new LocalVariableBinding(SecretPatternVariableName, this.expression.resolvedType, ClassFileConstants.AccDefault, false);
this.scope.addLocalVariable(this.dispatchPatternCopy);
this.dispatchPatternCopy.setConstant(Constant.NotAConstant);
this.dispatchPatternCopy.useFlag = LocalVariableBinding.USED;
this.restartIndexLocal = new LocalVariableBinding(SecretPatternRestartIndexName, TypeBinding.INT, ClassFileConstants.AccDefault, false);
this.scope.addLocalVariable(this.restartIndexLocal);
this.restartIndexLocal.setConstant(Constant.NotAConstant);
this.restartIndexLocal.useFlag = LocalVariableBinding.USED;
}
}

protected void reportMissingEnumConstantCase(BlockScope upperScope, FieldBinding enumConstant) {
upperScope.problemReporter().missingEnumConstantCase(this, enumConstant);
}
Expand Down
Loading

0 comments on commit b294075

Please sign in to comment.