From 245ddd2936ee85b0e3971449f3bd751493d0aa7a Mon Sep 17 00:00:00 2001 From: markbrady Date: Tue, 13 Oct 2026 12:05:39 -0700 Subject: [PATCH] [Reachability] count a labeled `break` only against the labeled statement PiperOrigin-RevId: 998800813 --- .../google/errorprone/util/Reachability.java | 8 ++-- .../errorprone/util/ReachabilityTest.java | 44 +++---------------- 2 files changed, 11 insertions(+), 41 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/util/Reachability.java b/check_api/src/main/java/com/google/errorprone/util/Reachability.java index 963a50381ef..0b6a54802b3 100644 --- a/check_api/src/main/java/com/google/errorprone/util/Reachability.java +++ b/check_api/src/main/java/com/google/errorprone/util/Reachability.java @@ -165,7 +165,8 @@ private boolean scan(Tree tree) { /* A break statement cannot complete normally. */ @Override public Boolean visitBreak(BreakTree tree, Void unused) { - breaks.add(skipLabel(requireNonNull(((JCTree.JCBreak) tree).target))); + // The target of a `break` with a label is the labeled statement + breaks.add(requireNonNull(((JCTree.JCBreak) tree).target)); return false; } @@ -218,9 +219,8 @@ public Boolean visitEmptyStatement(EmptyStatementTree tree, Void unused) { public Boolean visitLabeledStatement(LabeledStatementTree tree, Void unused) { // (1) boolean completes = scan(tree.getStatement()); - // (2) `visitBreak` records a `break` against the statement contained in its target (see - // `skipLabel`). - return completes || breaks.contains(tree.getStatement()); + // (2) + return completes || breaks.contains(tree); } /* An expression statement can complete normally iff it is reachable. */ diff --git a/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java b/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java index 6e4623774f4..02839776393 100644 --- a/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java +++ b/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java @@ -158,16 +158,6 @@ void f(int x, Object o, Shape shape, Paint paint, Color color) { */ private CompilationTestHelper containedStatementCanCompleteNormallyTest( boolean expected, String labeledStatement) { - return containedStatementCanCompleteNormallyTest(expected, expected, labeledStatement); - } - - /** - * Like {@link #containedStatementCanCompleteNormallyTest(boolean, String)}, but documents a bug - * where {@link Reachability} disagrees with javac: {@code reachability} is what {@link - * Reachability} currently answers, and {@code javac} is what javac answers (the ground truth). - */ - private CompilationTestHelper containedStatementCanCompleteNormallyTest( - boolean reachability, boolean javac, String labeledStatement) { String label = "l:\n"; checkArgument(labeledStatement.startsWith(label), labeledStatement); String statement = labeledStatement.substring(label.length()); @@ -193,10 +183,10 @@ void javacAgrees(int x, Object o) { } """ .formatted( - reachability, + expected, labeledStatement, statement, - javac ? "String s = \"\";" : "s.length();")); + expected ? "String s = \"\";" : "s.length();")); } @Test @@ -615,11 +605,7 @@ public void breakExitsLabeledStatement_while() { } """; canCompleteNormallyTest(/* expected= */ true, statement).doTest(); - // BUG: Reachability credits `break l` to the statement contained in `l`, so it disagrees with - // javac, which says that statement can't complete normally. - containedStatementCanCompleteNormallyTest( - /* reachability= */ true, /* javac= */ false, statement) - .doTest(); + containedStatementCanCompleteNormallyTest(/* expected= */ false, statement).doTest(); } /** @@ -636,11 +622,7 @@ public void breakExitsLabeledStatement_do() { } while (true); """; canCompleteNormallyTest(/* expected= */ true, statement).doTest(); - // BUG: Reachability credits `break l` to the statement contained in `l`, so it disagrees with - // javac, which says that statement can't complete normally. - containedStatementCanCompleteNormallyTest( - /* reachability= */ true, /* javac= */ false, statement) - .doTest(); + containedStatementCanCompleteNormallyTest(/* expected= */ false, statement).doTest(); } /** @@ -657,11 +639,7 @@ public void breakExitsLabeledStatement_for() { } """; canCompleteNormallyTest(/* expected= */ true, statement).doTest(); - // BUG: Reachability credits `break l` to the statement contained in `l`, so it disagrees with - // javac, which says that statement can't complete normally. - containedStatementCanCompleteNormallyTest( - /* reachability= */ true, /* javac= */ false, statement) - .doTest(); + containedStatementCanCompleteNormallyTest(/* expected= */ false, statement).doTest(); } /** @@ -681,11 +659,7 @@ public void breakExitsLabeledStatement_switch() { } """; canCompleteNormallyTest(/* expected= */ true, statement).doTest(); - // BUG: Reachability credits `break l` to the statement contained in `l`, so it disagrees with - // javac, which says that statement can't complete normally. - containedStatementCanCompleteNormallyTest( - /* reachability= */ true, /* javac= */ false, statement) - .doTest(); + containedStatementCanCompleteNormallyTest(/* expected= */ false, statement).doTest(); } /** Like {@link #breakExitsLabeledStatement_switch}, but with switch rules. */ @@ -703,11 +677,7 @@ public void breakExitsLabeledStatement_arrowSwitch() { } """; canCompleteNormallyTest(/* expected= */ true, statement).doTest(); - // BUG: Reachability credits `break l` to the statement contained in `l`, so it disagrees with - // javac, which says that statement can't complete normally. - containedStatementCanCompleteNormallyTest( - /* reachability= */ true, /* javac= */ false, statement) - .doTest(); + containedStatementCanCompleteNormallyTest(/* expected= */ false, statement).doTest(); } /**