Skip to content

Commit

Permalink
Streamline error handling for Switch Expressions (#3199)
Browse files Browse the repository at this point in the history 
* Deprecate unused IProblem constants, simplify messages, rename methods for readability, get rid of unused property strings, subsume unnecessarily distinct diagnostics, ensure error handling happens in appropriate places, remove dead code, simplify control flow ...
  • Loading branch information
@srikanth-sankaran
srikanth-sankaran authored Oct 30, 2024
1 parent 5963384 commit d0af8a4
Show file tree
Hide file tree
Showing 14 changed files with 135 additions and 217 deletions.
40 changes: 30 additions & 10 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2325,35 +2325,55 @@ public interface IProblem {
int SwitchExpressionsYieldNoResultExpression = Internal + 1702;
/** @since 3.21 */
int SwitchExpressionaYieldSwitchLabeledBlockCompletesNormally = Internal + 1703;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldLastStatementCompletesNormally = Internal + 1704;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldTrailingSwitchLabels = Internal + 1705;
/** @since 3.21 */
int SwitchPreviewMixedCase = Syntax + 1706;
/** @since 3.21 */
int SwitchExpressionsYieldMissingDefaultCase = Syntax + 1707;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldMissingValue = Syntax + 1708;
/** @since 3.21 */
int SwitchExpressionsYieldMissingEnumConstantCase = Syntax + 1709;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldIllegalLastStatement = Internal + 1710;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldBreakNotAllowed = Syntax + 1711;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldUnqualifiedMethodWarning = Syntax + 1712;
/** @since 3.21 */
int SwitchExpressionsYieldUnqualifiedMethodError = Syntax + 1713;
/** @since 3.21 */
int SwitchExpressionsYieldOutsideSwitchExpression = Syntax + 1714;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldRestrictedGeneralWarning = Internal + 1715;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldIllegalStatement = Internal + 1716;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldTypeDeclarationWarning = Internal + 1717;
/** @since 3.21 */
/** @since 3.21
* @deprecated no longer issued - will be removed
*/
int SwitchExpressionsYieldTypeDeclarationError = Internal + 1718;
/** @since 3.22 */
int MultiConstantCaseLabelsNotSupported = Syntax + 1719;
Expand Down
2 changes: 1 addition & 1 deletion ...pse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/BreakStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
}
return flowInfo; // pretend it did not break since no actual target
} else if (targetContext == FlowContext.NonLocalGotoThroughSwitchContext) { // JLS 13 14.15
currentScope.problemReporter().switchExpressionsBreakOutOfSwitchExpression(this);
currentScope.problemReporter().breakOutOfSwitchExpression(this);
return flowInfo; // pretend it did not break since no actual target
}

Expand Down
17 changes: 13 additions & 4 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 @@ -43,7 +43,7 @@ public class CaseStatement extends Statement {
public BranchLabel targetLabel;
public Expression[] constantExpressions; // case with multiple expressions - if you want a under-the-hood view, use peeledLabelExpressions()
public BranchLabel[] targetLabels; // for multiple expressions
public boolean isExpr = false;
public boolean isSwitchRule = false;

public SwitchStatement swich; // owning switch
public int typeSwitchIndex; // for the first pattern among this.constantExpressions
Expand Down Expand Up @@ -177,6 +177,16 @@ public String toString() {
*/
public ResolvedCase[] resolveCase(BlockScope scope, TypeBinding switchExpressionType, SwitchStatement switchStatement) {
this.swich = switchStatement;

if (this.isSwitchRule)
this.swich.switchBits |= SwitchStatement.LabeledRules;
else
this.swich.switchBits |= SwitchStatement.LabeledBlockStatementGroup;

if ((this.swich.switchBits & (SwitchStatement.LabeledRules | SwitchStatement.LabeledBlockStatementGroup)) == (SwitchStatement.LabeledRules | SwitchStatement.LabeledBlockStatementGroup)) {
scope.problemReporter().arrowColonMixup(this);
}

scope.enclosingCase = this; // record entering in a switch case block
if (this.constantExpressions == Expression.NO_EXPRESSIONS) {
checkDuplicateDefault(scope, switchStatement, this);
Expand Down Expand Up @@ -461,16 +471,15 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
public StringBuilder printStatement(int tab, StringBuilder output) {
printIndent(tab, output);
if (this.constantExpressions == Expression.NO_EXPRESSIONS) {
output.append("default "); //$NON-NLS-1$
output.append(this.isExpr ? "->" : ":"); //$NON-NLS-1$ //$NON-NLS-2$
output.append("default"); //$NON-NLS-1$
} else {
output.append("case "); //$NON-NLS-1$
for (int i = 0, l = this.constantExpressions.length; i < l; ++i) {
this.constantExpressions[i].printExpression(0, output);
if (i < l -1) output.append(',');
}
output.append(this.isExpr ? " ->" : " :"); //$NON-NLS-1$ //$NON-NLS-2$
}
output.append(this.isSwitchRule ? " ->" : " :"); //$NON-NLS-1$ //$NON-NLS-2$
return output;
}

Expand Down
2 changes: 1 addition & 1 deletion ....jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ContinueStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
}
return flowInfo; // pretend it did not continue since no actual target
} else if (targetContext == FlowContext.NonLocalGotoThroughSwitchContext) {
currentScope.problemReporter().switchExpressionsContinueOutOfSwitchExpression(this);
currentScope.problemReporter().continueOutOfSwitchExpression(this);
return flowInfo; // pretend it did not continue since no actual target
}

Expand Down
2 changes: 1 addition & 1 deletion ...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ private void yieldQualifiedCheck(BlockScope currentScope) {
return;
if (!CharOperation.equals(this.selector, TypeConstants.YIELD))
return;
currentScope.problemReporter().switchExpressionsYieldUnqualifiedMethodError(this);
currentScope.problemReporter().unqualifiedYieldMethod(this);
}
private void recordCallingClose(BlockScope currentScope, FlowContext flowContext, FlowInfo flowInfo, Expression closeTarget) {
if (closeTarget.isThis() || closeTarget.isSuper()) {
Expand Down
2 changes: 1 addition & 1 deletion ...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
currentScope.problemReporter().cannotReturnInInitializer(this);
return FlowInfo.DEAD_END;
} else if (traversedContext.associatedNode instanceof SwitchExpression) {
currentScope.problemReporter().switchExpressionsReturnWithinSwitchExpression(this);
currentScope.problemReporter().returnOutOfSwitchExpression(this);
return FlowInfo.DEAD_END;
}
} while ((traversedContext = traversedContext.getLocalParent()) != null);
Expand Down
62 changes: 15 additions & 47 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,53 +125,11 @@ private void computeNullStatus(FlowInfo flowInfo, FlowContext flowContext) {
this.nullStatus = status;
}

@Override
protected void completeNormallyCheck(BlockScope blockScope) {
int sz = this.statements != null ? this.statements.length : 0;
if (sz == 0) return;
/* JLS 12 15.28.1 Given a switch expression, if the switch block consists of switch labeled rules
* then it is a compile-time error if any switch labeled block can complete normally.
*/
if ((this.switchBits & LabeledRules) != 0) {
for (Statement stmt : this.statements) {
if (!(stmt instanceof Block))
continue;
if (stmt.canCompleteNormally())
blockScope.problemReporter().switchExpressionLastStatementCompletesNormally(stmt);
}
return;
}
/* JLS 12 15.28.1
* If, on the other hand, the switch block consists of switch labeled statement groups, then it is a
* compile-time error if either the last statement in the switch block can complete normally, or the
* switch block includes one or more switch labels at the end.
*/
Statement lastNonCaseStmt = null;
Statement firstTrailingCaseStmt = null;
for (int i = sz - 1; i >= 0; i--) {
Statement stmt = this.statements[sz - 1];
if (stmt instanceof CaseStatement)
firstTrailingCaseStmt = stmt;
else {
lastNonCaseStmt = stmt;
break;
}
}
if (lastNonCaseStmt != null) {
if (lastNonCaseStmt.canCompleteNormally())
blockScope.problemReporter().switchExpressionLastStatementCompletesNormally(lastNonCaseStmt);
else if (lastNonCaseStmt instanceof ContinueStatement || lastNonCaseStmt instanceof ReturnStatement) {
blockScope.problemReporter().switchExpressionIllegalLastStatement(lastNonCaseStmt);
}
}
if (firstTrailingCaseStmt != null) {
blockScope.problemReporter().switchExpressionTrailingSwitchLabels(firstTrailingCaseStmt);
}
}
@Override
protected boolean needToCheckFlowInAbsenceOfDefaultBranch() { // JLS 12 16.1.8
return (this.switchBits & LabeledRules) == 0;
}

@Override
public Expression[] getPolyExpressions() {
List<Expression> polys = new ArrayList<>();
Expand Down Expand Up @@ -379,7 +337,7 @@ public TypeBinding resolveType(BlockScope upperScope) {

resultExpressionsCount = this.resultExpressions.size();
if (resultExpressionsCount == 0) {
upperScope.problemReporter().switchExpressionNoResultExpressions(this);
upperScope.problemReporter().unyieldingSwitchExpression(this);
return this.resolvedType = null;
}

Expand All @@ -397,7 +355,6 @@ public TypeBinding resolveType(BlockScope upperScope) {
}
return this.resolvedType = computeConversions(this.scope, this.expectedType) ? this.expectedType : null;
}
// fall through
} else {
// re-resolving of poly expression:
resultExpressionsCount = this.resultExpressions.size();
Expand All @@ -418,7 +375,6 @@ public TypeBinding resolveType(BlockScope upperScope) {
if (yieldErrors)
return this.resolvedType = null;
this.resolvedType = computeConversions(this.scope, this.expectedType) ? this.expectedType : null;
// fall through
}

boolean uniformYield = true;
Expand Down Expand Up @@ -541,7 +497,7 @@ public TypeBinding resolveType(BlockScope upperScope) {
}
return this.resolvedType = commonType.capture(this.scope, this.sourceStart, this.sourceEnd);
}
this.scope.problemReporter().switchExpressionIncompatibleResultExpressions(this);
this.scope.problemReporter().incompatibleSwitchExpressionResults(this);
return null;
} finally {
if (this.scope != null) this.scope.enclosingCase = null; // no longer inside switch case block
Expand Down Expand Up @@ -569,6 +525,18 @@ private boolean areAllIntegerResultExpressionsConvertibleToTargetType(TypeBindin
@Override
public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, FlowInfo flowInfo) {
flowInfo = super.analyseCode(currentScope, flowContext, flowInfo);
// 15.28.1
if ((this.switchBits & LabeledRules) != 0) {
for (Statement stmt : this.statements) {
if (stmt instanceof Block && stmt.canCompleteNormally())
currentScope.problemReporter().switchExpressionBlockCompletesNormally(stmt);
}
} else {
Statement ultimateStmt = this.statements[this.statements.length - 1]; // length guaranteed > 0
if (ultimateStmt.canCompleteNormally())
currentScope.problemReporter().switchExpressionBlockCompletesNormally(ultimateStmt);
}

this.resultExpressionNullStatus = new ArrayList<>(0);
final CompilerOptions compilerOptions = currentScope.compilerOptions();
if (compilerOptions.enableSyntacticNullAnalysisForFields) {
Expand Down
27 changes: 2 additions & 25 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 @@ -105,6 +105,7 @@ public static record SingletonBootstrap(String id, char[] selector, char[] signa
public final static int TotalPattern = ASTNode.Bit3;
public final static int Exhaustive = ASTNode.Bit4;
public final static int QualifiedEnum = ASTNode.Bit5;
public final static int LabeledBlockStatementGroup = ASTNode.Bit6;

// for switch on strings
private static final char[] SecretStringVariableName = " switchDispatchString".toCharArray(); //$NON-NLS-1$
Expand Down Expand Up @@ -402,9 +403,6 @@ protected int getFallThroughState(Statement stmt, BlockScope blockScope) {
}
return FALLTHROUGH;
}
protected void completeNormallyCheck(BlockScope blockScope) {
// do nothing
}
protected boolean needToCheckFlowInAbsenceOfDefaultBranch() {
return !this.isExhaustive();
}
Expand Down Expand Up @@ -493,7 +491,6 @@ else if ((statement.bits & ASTNode.DocumentedFallthrough) == 0) { // the case is
}
}
}
completeNormallyCheck(currentScope);
}

final TypeBinding resolvedTypeBinding = this.expression.resolvedType;
Expand Down Expand Up @@ -1266,7 +1263,6 @@ && defaultFound && isExhaustive()) {
if (this.dispatchPatternCopy == null) {
addSecretPatternSwitchVariables(upperScope);
}
reportMixingCaseTypes();

complainIfNotExhaustiveSwitch(upperScope, expressionType, compilerOptions);

Expand Down Expand Up @@ -1501,26 +1497,7 @@ protected boolean ignoreMissingDefaultCase(CompilerOptions compilerOptions) {
public boolean isTrulyExpression() {
return false;
}
private void reportMixingCaseTypes() {
if (this.caseCount == 0) {
if (this.defaultCase != null && this.defaultCase.isExpr)
this.switchBits |= LabeledRules;
return;
}
if (this.cases[0] == null)
return;
boolean isExpr = this.cases[0].isExpr;
if (isExpr) this.switchBits |= LabeledRules;
for (int i = 1, l = this.caseCount; i < l; ++i) {
if (this.cases[i].isExpr != isExpr) {
this.scope.problemReporter().switchExpressionMixedCase(this.cases[i]);
return;
}
}
if (this.defaultCase != null && this.defaultCase.isExpr != isExpr) {
this.scope.problemReporter().switchExpressionMixedCase(this.defaultCase);
}
}

private void reportDuplicateCase(final Statement duplicate,
final Statement original,
int length) {
Expand Down
25 changes: 8 additions & 17 deletions ...pse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/YieldStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,27 +207,20 @@ public void resolve(BlockScope scope) {
this.switchExpression = scope.enclosingSwitchExpression();
if (this.switchExpression != null) {
this.switchExpression.resultExpressions.add(this.expression);
if (this.switchExpression.expressionContext == ASSIGNMENT_CONTEXT || this.switchExpression.expressionContext == INVOCATION_CONTEXT) { // poly switch expression
if (this.switchExpression.expressionContext == ASSIGNMENT_CONTEXT || this.switchExpression.expressionContext == INVOCATION_CONTEXT) { // When switch expression is poly ...
this.expression.setExpressionContext(this.switchExpression.expressionContext); // result expressions feature in same context ...
this.expression.setExpectedType(this.switchExpression.expectedType); // ... with the same target type
}
}
}

if (this.switchExpression != null || this.isImplicit) {
if (this.switchExpression == null && this.isImplicit && !this.expression.statementExpression()) {
if (scope.compilerOptions().sourceLevel >= ClassFileConstants.JDK14) {
/* JLS 13 14.11.2
Switch labeled rules in switch statements differ from those in switch expressions (15.28).
In switch statements they must be switch labeled statement expressions, ... */
scope.problemReporter().invalidExpressionAsStatement(this.expression);
return;
}
}
} else {
if (scope.compilerOptions().sourceLevel >= ClassFileConstants.JDK14) {
scope.problemReporter().switchExpressionsYieldOutsideSwitchExpression(this);
if (this.isImplicit) {
if (this.switchExpression == null && !this.expression.statementExpression()) {
scope.problemReporter().invalidExpressionAsStatement(this.expression);
return;
}
} else if (this.switchExpression == null) {
scope.problemReporter().yieldOutsideSwitchExpression(this);
}
TypeBinding type = this.expression.resolveType(scope);
if (this.switchExpression != null && type != null)
Expand All @@ -236,12 +229,10 @@ Switch labeled rules in switch statements differ from those in switch expression

@Override
public StringBuilder printStatement(int tab, StringBuilder output) {
if (!this.isImplicit)
printIndent(tab, output).append("yield"); //$NON-NLS-1$
if (this.isImplicit) {
this.expression.print(tab, output);
} else {
output.append(' ');
printIndent(tab, output).append("yield "); //$NON-NLS-1$
this.expression.printExpression(tab, output);
}
return output.append(';');
Expand Down
4 changes: 2 additions & 2 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9470,7 +9470,7 @@ protected void consumeCaseLabelExpr() {
if (!this.parsingJava14Plus) {
problemReporter().arrowInCaseStatementsNotSupported(caseStatement);
}
caseStatement.isExpr = true;
caseStatement.isSwitchRule = true;
}
protected void consumeDefaultLabelExpr() {
// SwitchLabelDefaultExpr ::= 'default' '->'
Expand All @@ -9479,7 +9479,7 @@ protected void consumeDefaultLabelExpr() {
if (!this.parsingJava14Plus) {
problemReporter().arrowInCaseStatementsNotSupported(defaultStatement);
}
defaultStatement.isExpr = true;
defaultStatement.isSwitchRule = true;
}
protected void consumeSwitchExpression() {
// SwitchExpression ::= 'switch' '(' Expression ')' OpenBlock SwitchExpressionBlock
Expand Down
Loading

0 comments on commit d0af8a4

Please sign in to comment.