From 18a12f0d400f50592a1f9d4bc293c2506135f33d Mon Sep 17 00:00:00 2001 From: David Kunzmann Date: Mon, 3 Aug 2026 13:38:04 +0200 Subject: [PATCH 1/2] USER-2406: Fix FN on empty condition loop --- .../checks/LoopExecutingAtMostOnceCheck.java | 78 ++++++++++++++++++- .../checks/LoopExecutingAtMostOnceCheck.java | 47 +++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/java-checks/src/main/java/org/sonar/java/checks/LoopExecutingAtMostOnceCheck.java b/java-checks/src/main/java/org/sonar/java/checks/LoopExecutingAtMostOnceCheck.java index 8ce88221c58..10f5158c830 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/LoopExecutingAtMostOnceCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/LoopExecutingAtMostOnceCheck.java @@ -25,20 +25,26 @@ import org.sonar.java.model.LiteralUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.BlockTree; import org.sonar.plugins.java.api.tree.BreakStatementTree; +import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.ContinueStatementTree; import org.sonar.plugins.java.api.tree.DoWhileStatementTree; import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.ForEachStatement; import org.sonar.plugins.java.api.tree.ForStatementTree; +import org.sonar.plugins.java.api.tree.IfStatementTree; import org.sonar.plugins.java.api.tree.LambdaExpressionTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.ReturnStatementTree; +import org.sonar.plugins.java.api.tree.SwitchExpressionTree; +import org.sonar.plugins.java.api.tree.SwitchStatementTree; import org.sonar.plugins.java.api.tree.SyntaxToken; import org.sonar.plugins.java.api.tree.ThrowStatementTree; import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.TryStatementTree; import org.sonar.plugins.java.api.tree.WhileStatementTree; @Rule(key = "S1751") @@ -97,12 +103,18 @@ public void visitNode(Tree tree) { * break; // last unconditional jump to exit the infinite loop * } * + * For {@code for(;;)} loops, the exemption only applies when the body contains a conditional + * control-flow structure (if, switch, loop, or try), distinguishing the goto-idiom from + * a degenerate {@code for(;;) { break; }} that always exits immediately. */ private static boolean isEmptyConditionLoop(Tree loopTree) { switch (loopTree.kind()) { case FOR_STATEMENT: ForStatementTree fst = (ForStatementTree) loopTree; - return fst.initializer().isEmpty() && fst.condition() == null && fst.update().isEmpty(); + return fst.initializer().isEmpty() + && fst.condition() == null + && fst.update().isEmpty() + && containsConditional(fst.statement()); case WHILE_STATEMENT: // 'while(false)' does not compile, unreachable code return isTrue(((WhileStatementTree) loopTree).condition()); @@ -116,6 +128,70 @@ private static boolean isEmptyConditionLoop(Tree loopTree) { } } + /** + * Returns true if {@code tree} contains at least one conditional, loop, or try statement + * at any depth, without crossing lambda or anonymous class boundaries. + */ + private static boolean containsConditional(Tree tree) { + ConditionalVisitor visitor = new ConditionalVisitor(); + tree.accept(visitor); + return visitor.found; + } + + private static class ConditionalVisitor extends BaseTreeVisitor { + boolean found = false; + + @Override + public void visitIfStatement(IfStatementTree tree) { + found = true; + } + + @Override + public void visitSwitchStatement(SwitchStatementTree tree) { + found = true; + } + + @Override + public void visitSwitchExpression(SwitchExpressionTree tree) { + found = true; + } + + @Override + public void visitForStatement(ForStatementTree tree) { + found = true; + } + + @Override + public void visitForEachStatement(ForEachStatement tree) { + found = true; + } + + @Override + public void visitWhileStatement(WhileStatementTree tree) { + found = true; + } + + @Override + public void visitDoWhileStatement(DoWhileStatementTree tree) { + found = true; + } + + @Override + public void visitTryStatement(TryStatementTree tree) { + found = true; + } + + @Override + public void visitLambdaExpression(LambdaExpressionTree tree) { + // scope boundary — do not descend into lambdas + } + + @Override + public void visitClass(ClassTree tree) { + // scope boundary — do not descend into anonymous/inner classes + } + } + private static boolean isTrue(ExpressionTree expressionTree) { ExpressionTree expr = ExpressionUtils.skipParentheses(expressionTree); return LiteralUtils.isTrue(expr); diff --git a/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java b/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java index 05baed39c45..73f63a54691 100644 --- a/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java +++ b/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java @@ -243,6 +243,10 @@ void m4() { } void m5() { + for (;;){ + break; // Noncompliant + } + for(;;) { if (isItTrue()) { // ... @@ -295,6 +299,49 @@ void m6() { } while ((((true)))); } + void m5_extra() { + // for(;;) with nested loop — goto-idiom → Compliant + for(;;) { + while (isItTrue()) { + break; // Noncompliant + } + break; // Compliant + } + + // for(;;) with switch — goto-idiom → Compliant + for(;;) { + switch (getInt()) { + case 1: break; + } + break; // Compliant + } + + // for(;;) with try — goto-idiom → Compliant + for(;;) { + try { + doSomething(); + } catch (Exception e) { + break; + } + break; // Compliant + } + + // for(;;) with only a method call (no conditional) → Noncompliant + for(;;) { + foo(); + break; // Noncompliant + } + + // for(;;) with conditional inside lambda — lambda is a scope boundary → Noncompliant + for(;;) { + Runnable r = () -> { if (isItTrue()) return; }; + break; // Noncompliant + } + } + + abstract int getInt(); + abstract void doSomething() throws Exception; + abstract void foo(); abstract boolean isItTrue(); } From 4a1dedeb4a5fbb97ea648b324923821e928c5862 Mon Sep 17 00:00:00 2001 From: David Kunzmann Date: Mon, 3 Aug 2026 14:13:43 +0200 Subject: [PATCH 2/2] Improved coverage --- .../checks/LoopExecutingAtMostOnceCheck.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java b/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java index 73f63a54691..78da4fd09d1 100644 --- a/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java +++ b/java-checks/src/test/files/checks/LoopExecutingAtMostOnceCheck.java @@ -337,9 +337,52 @@ void m5_extra() { Runnable r = () -> { if (isItTrue()) return; }; break; // Noncompliant } + + // for(;;) with switch expression — goto-idiom → Compliant + for(;;) { + int x = switch (getInt()) { + case 1 -> 10; + default -> 20; + }; + break; // Compliant + } + + // for(;;) with for-each — goto-idiom → Compliant + for(;;) { + for (Object o : getList()) { + foo(); + } + break; // Compliant + } + + // for(;;) with nested for — goto-idiom → Compliant + for(;;) { + for (int i = 0; i < 10; i++) { + foo(); + } + break; // Compliant + } + + // for(;;) with do-while — goto-idiom → Compliant + for(;;) { + do { + foo(); + } while (isItTrue()); + break; // Compliant + } + + // for(;;) with anonymous class — scope boundary does NOT make it goto-idiom → Noncompliant + for(;;) { + Runnable r = new Runnable() { + @Override + public void run() { if (isItTrue()) foo(); } + }; + break; // Noncompliant + } } abstract int getInt(); + abstract java.util.List getList(); abstract void doSomething() throws Exception; abstract void foo(); abstract boolean isItTrue();