Incorrect control flow analysis causes statement subsequent to a switch

statement to be flagged unreachable under some circumstances * Fixes eclipse-jdt#3376
srikanth-sankaran · Dec 2, 2024 · ee1438a · ee1438a
1 parent 2528b4a
commit ee1438a
Show file tree

Hide file tree

Showing 5 changed files with 122 additions and 30 deletions.
diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ASTNode.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ASTNode.java
@@ -224,6 +224,7 @@ public abstract class ASTNode implements TypeConstants, TypeIds {
 	public static final int IsUsefulEmptyStatement = Bit1;
 
 	// for block and method declaration
+	public static final int SwitchRuleBlock = Bit1;
 	public static final int UndocumentedEmptyBlock = Bit4;
 	public static final int OverridingMethodWithSupercall = Bit5;
 	public static final int CanBeStatic = Bit9;   // used to flag a method that can be declared static

diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Block.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Block.java
@@ -42,36 +42,42 @@ public Block(int explicitDeclarations) {
 
 @Override
 public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, FlowInfo flowInfo) {
-	// empty block
-	if (this.statements == null)	return flowInfo;
-	int complaintLevel = (flowInfo.reachMode() & FlowInfo.UNREACHABLE) != 0 ? Statement.COMPLAINED_FAKE_REACHABLE : Statement.NOT_COMPLAINED;
-	CompilerOptions compilerOptions = currentScope.compilerOptions();
-	boolean enableSyntacticNullAnalysisForFields = compilerOptions.enableSyntacticNullAnalysisForFields;
-	for (Statement stat : this.statements) {
-		if ((complaintLevel = stat.complainIfUnreachable(flowInfo, this.scope, complaintLevel, true)) < Statement.COMPLAINED_UNREACHABLE) {
-			flowInfo = stat.analyseCode(this.scope, flowContext, flowInfo);
+	if (this.statements != null) {
+		int complaintLevel = (flowInfo.reachMode() & FlowInfo.UNREACHABLE) != 0 ? Statement.COMPLAINED_FAKE_REACHABLE : Statement.NOT_COMPLAINED;
+		CompilerOptions compilerOptions = currentScope.compilerOptions();
+		boolean enableSyntacticNullAnalysisForFields = compilerOptions.enableSyntacticNullAnalysisForFields;
+		for (Statement stat : this.statements) {
+			if ((complaintLevel = stat.complainIfUnreachable(flowInfo, this.scope, complaintLevel, true)) < Statement.COMPLAINED_UNREACHABLE) {
+				flowInfo = stat.analyseCode(this.scope, flowContext, flowInfo);
+			}
+			// record the effect of stat on the finally block of an enclosing try-finally, if any:
+			flowContext.mergeFinallyNullInfo(flowInfo);
+			if (enableSyntacticNullAnalysisForFields) {
+				flowContext.expireNullCheckedFieldInfo();
+			}
+			if (compilerOptions.analyseResourceLeaks) {
+				FakedTrackingVariable.cleanUpUnassigned(this.scope, stat, flowInfo, false);
+			}
 		}
-		// record the effect of stat on the finally block of an enclosing try-finally, if any:
-		flowContext.mergeFinallyNullInfo(flowInfo);
-		if (enableSyntacticNullAnalysisForFields) {
-			flowContext.expireNullCheckedFieldInfo();
+		if (this.scope != currentScope) {
+			// if block is tracking any resources other than the enclosing 'currentScope', analyse them now:
+			this.scope.checkUnclosedCloseables(flowInfo, flowContext, null, null);
 		}
-		if (compilerOptions.analyseResourceLeaks) {
-			FakedTrackingVariable.cleanUpUnassigned(this.scope, stat, flowInfo, false);
+		if (this.explicitDeclarations > 0) {
+			// cleanup assignment info for locals that are scoped to this block:
+			LocalVariableBinding[] locals = this.scope.locals;
+			if (locals != null) {
+				int numLocals = this.scope.localIndex;
+				for (int i = 0; i < numLocals; i++) {
+					flowInfo.resetAssignmentInfo(locals[i]);
+				}
+			}
 		}
 	}
-	if (this.scope != currentScope) {
-		// if block is tracking any resources other than the enclosing 'currentScope', analyse them now:
-		this.scope.checkUnclosedCloseables(flowInfo, flowContext, null, null);
-	}
-	if (this.explicitDeclarations > 0) {
-		// cleanup assignment info for locals that are scoped to this block:
-		LocalVariableBinding[] locals = this.scope.locals;
-		if (locals != null) {
-			int numLocals = this.scope.localIndex;
-			for (int i = 0; i < numLocals; i++) {
-				flowInfo.resetAssignmentInfo(locals[i]);
-			}
+	if ((this.bits & ASTNode.SwitchRuleBlock) != 0) { // switch rule blocks don't fall through
+		if (flowInfo != FlowInfo.DEAD_END) {
+			flowContext.recordBreakFrom(flowInfo);
+			return FlowInfo.DEAD_END;
 		}
 	}
 	return flowInfo;

diff --git a/...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java b/...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java
@@ -97,8 +97,7 @@ public static record SingletonBootstrap(String id, char[] selector, char[] signa
 	// fallthrough
 	public final static int CASE = 0;
 	public final static int FALLTHROUGH = 1;
-	public final static int ESCAPING = 2;
-	public final static int BREAKING  = 3;
+	public final static int BREAKING  = 2;
 
 	// Other bits
 	public final static int LabeledRules = ASTNode.Bit1;
@@ -432,8 +431,6 @@ else if ((statement.bits & ASTNode.DocumentedFallthrough) == 0) // the case is n
 					}
 					if ((complaintLevel = statement.complainIfUnreachable(caseInits, this.scope, complaintLevel, true)) < Statement.COMPLAINED_UNREACHABLE) {
 						caseInits = statement.analyseCode(this.scope, switchContext, caseInits);
-						if (caseInits == FlowInfo.DEAD_END)
-							fallThroughState = ESCAPING;
 						if (compilerOptions.enableSyntacticNullAnalysisForFields)
 							switchContext.expireNullCheckedFieldInfo();
 						if (compilerOptions.analyseResourceLeaks)

diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
@@ -9441,6 +9441,9 @@ protected void consumeSwitchRule(SwitchRuleKind kind) {
 		expr.bits &= ~ASTNode.InsideExpressionStatement;
 		YieldStatement yieldStatement = new YieldStatement(expr, true, expr.sourceStart, this.endStatementPosition);
 		this.astStack[this.astPtr] = yieldStatement;
+	} else if (kind == SwitchRuleKind.BLOCK) {
+		Block block = (Block) this.astStack[this.astPtr];
+		block.bits |= ASTNode.SwitchRuleBlock;
 	}
 	concatNodeLists();
 }

diff --git a/...dt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchTest.java b/...dt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchTest.java
@@ -3539,6 +3539,91 @@ public static void main(String[] args) {
 		"----------\n");
 }
 
+// https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3376
+// Incorrect control flow analysis causes statement subsequent to a switch statement to be flagged unreachable under some circumstances
+public void testIssue3376() throws Exception {
+	if (this.complianceLevel < ClassFileConstants.JDK14)
+		return;
+
+	this.runConformTest(new String[] {
+		"SwitchTest.java",
+		"""
+		public class SwitchTest {
+		    String unreachableCode(String arg) {
+		        String result;
+		        switch (arg) {
+		            case "a" -> {
+		                result = "A";
+		            }
+		            default -> throw new RuntimeException(arg);
+		        }
+		        return result;      // <- ecj reports "Unreachable code"
+		    }
+
+		    String unreachableCode2(String arg) {
+		        String result;
+		        switch (arg) {
+		            case "a" -> result = "A";
+		            default -> throw new RuntimeException(arg);
+		        }
+		        return result;
+		    }
+
+		     String unreachableCode3(String arg) {
+		        String result;
+		        switch (arg) {
+		            case "a" -> {
+		                result = "A";
+		                break;       // <- this makes ecj happy
+		            }
+		            default -> throw new RuntimeException(arg);
+		        }
+		        return result;
+		    }
+
+		    public static void main(String[] args) {
+				System.out.println(new SwitchTest().unreachableCode("a"));
+				System.out.println(new SwitchTest().unreachableCode2("a"));
+				System.out.println(new SwitchTest().unreachableCode3("a"));
+			}
+
+		}
+		""",
+	},
+	"A\nA\nA");
+}
+
+// https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3376
+// Incorrect control flow analysis causes statement subsequent to a switch statement to be flagged unreachable under some circumstances
+public void testIssue3376_2() throws Exception {
+	if (this.complianceLevel < ClassFileConstants.JDK14)
+		return;
+
+	this.runNegativeTest(new String[] {
+		"SwitchTest.java",
+		"""
+		public class SwitchTest {
+		    String unreachableCode(String arg) {
+		        String result;
+		        switch (arg) {
+		            case "a" : {
+		                result = "A";
+		            }
+		            default : throw new RuntimeException(arg);
+		        }
+		        return result;
+		    }
+		}
+		""",
+	},
+	"----------\n" +
+	"1. ERROR in SwitchTest.java (at line 10)\n" +
+	"	return result;\n" +
+	"	^^^^^^^^^^^^^^\n" +
+	"Unreachable code\n" +
+	"----------\n");
+}
+
 public static Class testClass() {
 	return SwitchTest.class;
 }