This is an automated email from the ASF dual-hosted git repository.
daniellansun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 8c33592c59 GROOVY-10156: Unreachable bytecode in switch statement
(#2538)
8c33592c59 is described below
commit 8c33592c59edfe095699d2e6716deea81737a844
Author: Daniel Sun <[email protected]>
AuthorDate: Sun May 17 21:25:24 2026 +0800
GROOVY-10156: Unreachable bytecode in switch statement (#2538)
---
.github/workflows/groovy-build-coverage.yml | 10 +-
.../codehaus/groovy/ast/tools/GeneralUtils.java | 369 ++++++++++-
.../codehaus/groovy/classgen/asm/CompileStack.java | 48 +-
.../groovy/classgen/asm/StatementWriter.java | 169 ++---
.../asm/sc/StaticTypesStatementWriter.java | 17 +-
src/test/groovy/bugs/Groovy10156.groovy | 703 +++++++++++++++++++++
src/test/groovy/groovy/ForLoopTest.groovy | 27 +
.../classgen/asm/AbstractBytecodeTestCase.groovy | 65 +-
8 files changed, 1276 insertions(+), 132 deletions(-)
diff --git a/.github/workflows/groovy-build-coverage.yml
b/.github/workflows/groovy-build-coverage.yml
index 09d6fbe356..3d5cccd46b 100644
--- a/.github/workflows/groovy-build-coverage.yml
+++ b/.github/workflows/groovy-build-coverage.yml
@@ -85,13 +85,13 @@ jobs:
- name: Test with Gradle
run: ./gradlew -Pgroovy.grape.bridge-cache=true -Pcoverage=true
jacocoAllReport
timeout-minutes: 60
- - name: SonarCloud analysis (with coverage)
- env:
- SONAR_TOKEN: ${{ secrets.SONARCLOUD_TOKEN }}
- run: ./gradlew -Pcoverage=true sonar
- timeout-minutes: 20
- name: Upload coverage to Codecov
uses: codecov/codecov-action@57e3a136b779b570ffcdbf80b3bdc90e7fab3de2
# v6.0.0
with:
token: ${{ secrets.CODECOV_TOKEN }}
verbose: true
+ - name: SonarCloud analysis (with coverage)
+ env:
+ SONAR_TOKEN: ${{ secrets.SONARCLOUD_TOKEN }}
+ run: ./gradlew -Pcoverage=true sonar
+ timeout-minutes: 20
diff --git a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
index 05086ea028..62e2e6ecd1 100644
--- a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
+++ b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
@@ -19,6 +19,7 @@
package org.codehaus.groovy.ast.tools;
import groovy.lang.MetaProperty;
+import groovy.transform.Internal;
import org.codehaus.groovy.ast.ASTNode;
import org.codehaus.groovy.ast.AnnotatedNode;
import org.codehaus.groovy.ast.AnnotationNode;
@@ -38,10 +39,12 @@ import org.codehaus.groovy.ast.expr.BooleanExpression;
import org.codehaus.groovy.ast.expr.CastExpression;
import org.codehaus.groovy.ast.expr.ClassExpression;
import org.codehaus.groovy.ast.expr.ClosureExpression;
+import org.codehaus.groovy.ast.expr.ClosureListExpression;
import org.codehaus.groovy.ast.expr.ConstantExpression;
import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
import org.codehaus.groovy.ast.expr.DeclarationExpression;
import org.codehaus.groovy.ast.expr.ElvisOperatorExpression;
+import org.codehaus.groovy.ast.expr.EmptyExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.FieldExpression;
import org.codehaus.groovy.ast.expr.LambdaExpression;
@@ -56,18 +59,23 @@ import
org.codehaus.groovy.ast.expr.StaticMethodCallExpression;
import org.codehaus.groovy.ast.expr.TernaryExpression;
import org.codehaus.groovy.ast.expr.VariableExpression;
import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.BreakStatement;
import org.codehaus.groovy.ast.stmt.CaseStatement;
import org.codehaus.groovy.ast.stmt.CatchStatement;
+import org.codehaus.groovy.ast.stmt.ContinueStatement;
+import org.codehaus.groovy.ast.stmt.DoWhileStatement;
import org.codehaus.groovy.ast.stmt.EmptyStatement;
import org.codehaus.groovy.ast.stmt.ExpressionStatement;
import org.codehaus.groovy.ast.stmt.ForStatement;
import org.codehaus.groovy.ast.stmt.IfStatement;
+import org.codehaus.groovy.ast.stmt.LoopingStatement;
import org.codehaus.groovy.ast.stmt.ReturnStatement;
import org.codehaus.groovy.ast.stmt.Statement;
import org.codehaus.groovy.ast.stmt.SwitchStatement;
import org.codehaus.groovy.ast.stmt.SynchronizedStatement;
import org.codehaus.groovy.ast.stmt.ThrowStatement;
import org.codehaus.groovy.ast.stmt.TryCatchStatement;
+import org.codehaus.groovy.ast.stmt.WhileStatement;
import org.codehaus.groovy.classgen.BytecodeExpression;
import org.codehaus.groovy.control.io.ReaderSource;
import org.codehaus.groovy.runtime.GeneratedClosure;
@@ -464,7 +472,7 @@ public class GeneralUtils {
/**
* Returns a constant expression with the default value for the given type
- * (i.e., {@code false} for boolean, {@code 0} for numbers or {@code null}.
+ * (i.e., {@code false} for boolean, {@code 0} for numbers or {@code
null}).
*
* @since 4.0.0
*/
@@ -1264,26 +1272,359 @@ public class GeneralUtils {
return false;
} else if (statement instanceof ThrowStatement) {
return false;
- } else if (statement instanceof BlockStatement) {
- List<Statement> list = ((BlockStatement)
statement).getStatements();
+ } else if (statement instanceof BlockStatement blockStmt) {
+ List<Statement> list = blockStmt.getStatements();
final int last = list.size() - 1;
if (!maybeFallsThrough(list.get(last))) return false;
for (int i = 0; i < last; i += 1)
if (!maybeFallsThrough(list.get(i))) return false;
- } else if (statement instanceof IfStatement) {
- return maybeFallsThrough(((IfStatement) statement).getElseBlock())
- || maybeFallsThrough(((IfStatement) statement).getIfBlock());
- } else if (statement instanceof SwitchStatement) {
- // TODO
- } else if (statement instanceof TryCatchStatement tryCatch) {
- if (!maybeFallsThrough(tryCatch.getFinallyStatement())) return
false;
- for (CatchStatement cs : tryCatch.getCatchStatements())
+ } else if (statement instanceof IfStatement ifStmt) {
+ return maybeFallsThrough(ifStmt.getElseBlock())
+ || maybeFallsThrough(ifStmt.getIfBlock());
+ } else if (statement instanceof LoopingStatement loopingStmt) {
+ return maybeFallsThroughLoop(loopingStmt);
+ } else if (statement instanceof SwitchStatement switchStmt) {
+ return maybeFallsThroughSwitch(switchStmt);
+ } else if (statement instanceof TryCatchStatement tryCatchStmt) {
+ if (!maybeFallsThrough(tryCatchStmt.getFinallyStatement())) return
false;
+ for (CatchStatement cs : tryCatchStmt.getCatchStatements())
if (maybeFallsThrough(cs.getCode())) return true;
- return maybeFallsThrough(tryCatch.getTryStatement());
- } else if (statement instanceof SynchronizedStatement) {
- return maybeFallsThrough(((SynchronizedStatement)
statement).getCode());
+ return maybeFallsThrough(tryCatchStmt.getTryStatement());
+ } else if (statement instanceof SynchronizedStatement syncStmt) {
+ return maybeFallsThrough(syncStmt.getCode());
}
return true;
}
+
+ /**
+ * Returns {@code true} if the given statement is effectively empty — i.e.
it is {@code null},
+ * an {@link EmptyStatement}, or a {@link BlockStatement} whose every
nested statement is itself
+ * empty (recursively). This is subtly different from {@link
Statement#isEmpty()}, which only
+ * checks whether a {@link BlockStatement} has zero entries, not whether
those entries are all
+ * empty.
+ */
+ @Internal
+ public static boolean isEmptyStatement(final Statement statement) {
+ if (statement == null || statement.isEmpty()) return true;
+ if (statement instanceof BlockStatement blockStatement) {
+ for (Statement subStatement : blockStatement.getStatements()) {
+ if (!isEmptyStatement(subStatement)) return false;
+ }
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Returns the compile-time constant boolean value of a loop condition
expression.
+ * Handles nested {@link BooleanExpression} / {@link NotExpression}
wrappers and
+ * an absent (empty) condition, which is always {@code true} (e.g. {@code
for(;;)}).
+ * Returns {@link ConditionValue#UNKNOWN} when the value cannot be
determined statically.
+ */
+ @Internal
+ public static ConditionValue constantBooleanValue(Expression expression) {
+ if (expression instanceof EmptyExpression) {
+ return ConditionValue.ALWAYS_TRUE; // empty for-loop condition is
always true (e.g. for(;;))
+ }
+ boolean reverse = false;
+ while (expression instanceof BooleanExpression boolExpr) {
+ if (boolExpr instanceof NotExpression) reverse = !reverse;
+ expression = boolExpr.getExpression();
+ }
+ if (expression instanceof ConstantExpression ce && ce.getValue()
instanceof Boolean) {
+ return ((Boolean) ce.getValue()) != reverse ?
ConditionValue.ALWAYS_TRUE : ConditionValue.ALWAYS_FALSE;
+ }
+ return ConditionValue.UNKNOWN;
+ }
+
+ /**
+ * Indicates whether switch-case code may fall through into the next case
body.
+ */
+ @Internal
+ public static boolean maybeFallsThroughToNextSwitchCase(final Statement
statement, final SwitchStatement switchStatement) {
+ Objects.requireNonNull(switchStatement);
+ return analyzeStatementFlow(statement).mayContinue;
+ }
+
+ private static boolean maybeFallsThroughSwitch(final SwitchStatement
statement) {
+ return analyzeStatementFlow(statement).mayContinue;
+ }
+
+ private static boolean maybeFallsThroughLoop(final LoopingStatement
statement) {
+ return analyzeLoopFlow(statement).mayContinue;
+ }
+
+ /**
+ * Indicates whether execution of a loop body may reach the loop's
condition/update step.
+ */
+ @Internal
+ public static boolean mayReachLoopCondition(final LoopingStatement
statement) {
+ StatementFlow flow = analyzeStatementFlow(statement.getLoopBlock());
+ Set<String> loopLabels = labelSet(((Statement)
statement).getStatementLabels());
+ return mayReachLoopCondition(flow, loopLabels);
+ }
+
+ private static boolean mayReachLoopCondition(final StatementFlow flow,
final Set<String> loopLabels) {
+ return flow.mayContinue || flow.mayContinueUnlabeled ||
flow.continuesToAny(loopLabels);
+ }
+
+ private static boolean mayBreakTo(final StatementFlow flow, final
Set<String> labels) {
+ return flow.mayBreakUnlabeled || flow.breaksToAny(labels);
+ }
+
+ private static StatementFlow analyzeStatementFlow(final Statement
statement) {
+ if (statement.isEmpty()) return StatementFlow.FALL_THROUGH;
+
+ if (statement instanceof BreakStatement breakStatement) {
+ return StatementFlow.breakFlow(breakStatement.getLabel());
+ } else if (statement instanceof ContinueStatement continueStatement) {
+ return StatementFlow.continueFlow(continueStatement.getLabel());
+ } else if (statement instanceof ReturnStatement || statement
instanceof ThrowStatement) {
+ return StatementFlow.ABRUPT;
+ } else if (statement instanceof BlockStatement block) {
+ StatementFlow flow = StatementFlow.FALL_THROUGH;
+ for (Statement subStatement : block.getStatements()) {
+ flow = flow.then(analyzeStatementFlow(subStatement));
+ if (!flow.mayContinue) break;
+ }
+ return flow.consumeLabels(labelSet(block.getStatementLabels()));
+ } else if (statement instanceof IfStatement ifStatement) {
+ StatementFlow thenFlow =
analyzeStatementFlow(ifStatement.getIfBlock());
+ StatementFlow elseFlow =
analyzeStatementFlow(ifStatement.getElseBlock());
+ return thenFlow.or(elseFlow);
+ } else if (statement instanceof TryCatchStatement tryCatch) {
+ StatementFlow finallyFlow =
analyzeStatementFlow(tryCatch.getFinallyStatement());
+ StatementFlow bodyFlow =
analyzeStatementFlow(tryCatch.getTryStatement());
+ for (CatchStatement catchStatement :
tryCatch.getCatchStatements()) {
+ bodyFlow =
bodyFlow.or(analyzeStatementFlow(catchStatement.getCode()));
+ }
+ return bodyFlow.thenFinally(finallyFlow);
+ } else if (statement instanceof SynchronizedStatement
synchronizedStatement) {
+ return analyzeStatementFlow(synchronizedStatement.getCode());
+ } else if (statement instanceof LoopingStatement loopingStatement) {
+ return analyzeLoopFlow(loopingStatement);
+ } else if (statement instanceof SwitchStatement switchStatement) {
+ return analyzeNestedSwitchFlow(switchStatement);
+ }
+
+ return StatementFlow.of(maybeFallsThrough(statement), Set.of(),
Set.of());
+ }
+
+ private static StatementFlow analyzeLoopFlow(final LoopingStatement
statement) {
+ StatementFlow flow = analyzeStatementFlow(statement.getLoopBlock());
+ ConditionValue condition = loopCondition(statement);
+ Set<String> loopLabels = labelSet(((Statement)
statement).getStatementLabels());
+ boolean mayBreakOutOfLoop = mayBreakTo(flow, loopLabels);
+ boolean mayReachLoopCondition = mayReachLoopCondition(flow,
loopLabels);
+ boolean mayExitViaCondition = mayExitLoopViaCondition(statement,
condition, mayReachLoopCondition);
+ boolean mayRunLoopBlock = mayRunLoopBlock(statement, condition);
+ Set<String> breakLabels = mayRunLoopBlock ?
withoutLabels(flow.breakLabels, loopLabels) : Set.of();
+ Set<String> continueLabels = mayRunLoopBlock ?
withoutLabels(flow.continueLabels, loopLabels) : Set.of();
+ return StatementFlow.of(mayBreakOutOfLoop || mayExitViaCondition,
breakLabels, continueLabels);
+ }
+
+ private static boolean mayRunLoopBlock(final LoopingStatement statement,
final ConditionValue condition) {
+ return !(statement instanceof WhileStatement ||
isClassicForLoop(statement))
+ || condition != ConditionValue.ALWAYS_FALSE;
+ }
+
+ private static boolean mayExitLoopViaCondition(final LoopingStatement
statement, final ConditionValue condition, final boolean bodyMayContinue) {
+ if (statement instanceof DoWhileStatement) {
+ return bodyMayContinue && condition != ConditionValue.ALWAYS_TRUE;
+ }
+ if (statement instanceof ForStatement forStatement &&
!isClassicForLoop(forStatement)) {
+ return true;
+ }
+ return condition != ConditionValue.ALWAYS_TRUE;
+ }
+
+ private static boolean isClassicForLoop(final LoopingStatement statement) {
+ return statement instanceof ForStatement
+ && ((ForStatement) statement).getCollectionExpression() instanceof
ClosureListExpression;
+ }
+
+ private static ConditionValue loopCondition(final LoopingStatement
statement) {
+ if (statement instanceof WhileStatement whileStatement) {
+ return constantBooleanValue(whileStatement.getBooleanExpression());
+ }
+ if (statement instanceof DoWhileStatement doWhileStatement) {
+ return
constantBooleanValue(doWhileStatement.getBooleanExpression());
+ }
+ if (statement instanceof ForStatement forStatement
+ && forStatement.getCollectionExpression() instanceof
ClosureListExpression closureListExpression) {
+ List<Expression> expressions =
closureListExpression.getExpressions();
+ if (!expressions.isEmpty()) {
+ return
constantBooleanValue(expressions.get((expressions.size() - 1) / 2));
+ }
+ }
+ return ConditionValue.UNKNOWN;
+ }
+
+ private static Set<String> labelSet(final List<String> labels) {
+ if (labels == null || labels.isEmpty()) return Set.of();
+ return new LinkedHashSet<>(labels);
+ }
+
+ private static StatementFlow analyzeNestedSwitchFlow(final SwitchStatement
statement) {
+ Set<String> switchLabels = labelSet(statement.getStatementLabels());
+ StatementFlow nextFlow =
flowAfterSwitch(statement.getDefaultStatement(), switchLabels);
+ StatementFlow switchFlow = nextFlow;
+
+ List<CaseStatement> caseStatements = statement.getCaseStatements();
+ for (int i = caseStatements.size() - 1; i >= 0; i -= 1) {
+ StatementFlow flow =
analyzeStatementFlow(caseStatements.get(i).getCode());
+ boolean mayBreakSwitch = mayBreakTo(flow, switchLabels);
+ Set<String> breakLabels = withoutLabels(flow.breakLabels,
switchLabels);
+ nextFlow = StatementFlow.of(
+ mayBreakSwitch || (flow.mayContinue && nextFlow.mayContinue),
+ union(breakLabels, flow.mayContinue ? nextFlow.breakLabels :
Set.of()),
+ union(flow.continueLabels, flow.mayContinue ?
nextFlow.continueLabels : Set.of())
+ );
+ switchFlow = nextFlow.or(switchFlow);
+ }
+
+ return switchFlow;
+ }
+
+ private static StatementFlow flowAfterSwitch(final Statement statement,
final Set<String> switchLabels) {
+ StatementFlow flow = analyzeStatementFlow(statement);
+ return StatementFlow.of(
+ mayBreakTo(flow, switchLabels) || flow.mayContinue,
+ withoutLabels(flow.breakLabels, switchLabels),
+ flow.continueLabels
+ );
+ }
+
+ private static Set<String> union(final Set<String> left, final Set<String>
right) {
+ if (left.isEmpty()) return right;
+ if (right.isEmpty()) return left;
+
+ Set<String> labels = new LinkedHashSet<>(left);
+ labels.addAll(right);
+ return labels;
+ }
+
+ private static Set<String> withoutLabels(final Set<String> labels, final
Set<String> removedLabels) {
+ if (labels.isEmpty() || removedLabels.isEmpty()) return labels;
+
+ Set<String> remainingLabels = new LinkedHashSet<>(labels);
+ remainingLabels.removeAll(removedLabels);
+ return remainingLabels;
+ }
+
+ /**
+ * Represents the flow of control through a switch case or loop body,
tracking whether execution may continue past the end of the block
+ * @param mayContinue whether execution may continue past the end of the
block (i.e. not all paths return, throw, or break out of the block)
+ * @param mayBreakUnlabeled whether the block may be exited via an
unlabeled break statement
+ * @param breakLabels the set of labels of break statements that may exit
the block (if any)
+ */
+ private record StatementFlow(boolean mayContinue, boolean
mayBreakUnlabeled, Set<String> breakLabels,
+ boolean mayContinueUnlabeled, Set<String>
continueLabels) {
+ /**
+ * Code always terminates abruptly via {@code return}, {@code throw},
or {@code continue}.
+ */
+ static final StatementFlow ABRUPT = new StatementFlow(false, false,
Set.of(), false, Set.of());
+ /**
+ * Code may continue after this statement and does not leave
unresolved breaks behind.
+ */
+ static final StatementFlow FALL_THROUGH = new StatementFlow(true,
false, Set.of(), false, Set.of());
+
+ static StatementFlow breakFlow(final String label) {
+ if (label == null) return flow(false, true, Set.of(), false,
Set.of());
+ return flow(false, false, Set.of(label), false, Set.of());
+ }
+
+ static StatementFlow continueFlow(final String label) {
+ if (label == null) return flow(false, false, Set.of(), true,
Set.of());
+ return flow(false, false, Set.of(), false, Set.of(label));
+ }
+
+ static StatementFlow of(final boolean mayContinue, final Set<String>
breakLabels, final Set<String> continueLabels) {
+ return flow(mayContinue, false, breakLabels, false,
continueLabels);
+ }
+
+ private static StatementFlow flow(final boolean mayContinue, final
boolean mayBreakUnlabeled, final Set<String> breakLabels,
+ final boolean mayContinueUnlabeled,
final Set<String> continueLabels) {
+ if (!mayContinue && !mayBreakUnlabeled && breakLabels.isEmpty() &&
!mayContinueUnlabeled && continueLabels.isEmpty()) {
+ return ABRUPT;
+ }
+ if (mayContinue && !mayBreakUnlabeled && breakLabels.isEmpty() &&
!mayContinueUnlabeled && continueLabels.isEmpty()) {
+ return FALL_THROUGH;
+ }
+ return new StatementFlow(mayContinue, mayBreakUnlabeled,
breakLabels, mayContinueUnlabeled, continueLabels);
+ }
+
+ StatementFlow then(final StatementFlow next) {
+ if (!mayContinue) return this;
+ if (!mayBreakUnlabeled && breakLabels.isEmpty() &&
!mayContinueUnlabeled && continueLabels.isEmpty()) return next;
+ return flow(
+ next.mayContinue,
+ mayBreakUnlabeled || next.mayBreakUnlabeled,
+ union(breakLabels, next.breakLabels),
+ mayContinueUnlabeled || next.mayContinueUnlabeled,
+ union(continueLabels, next.continueLabels)
+ );
+ }
+
+ StatementFlow or(final StatementFlow other) {
+ return flow(
+ mayContinue || other.mayContinue,
+ mayBreakUnlabeled || other.mayBreakUnlabeled,
+ union(breakLabels, other.breakLabels),
+ mayContinueUnlabeled || other.mayContinueUnlabeled,
+ union(continueLabels, other.continueLabels)
+ );
+ }
+
+ StatementFlow thenFinally(final StatementFlow finallyFlow) {
+ return flow(
+ finallyFlow.mayContinue && mayContinue,
+ finallyFlow.mayBreakUnlabeled || (finallyFlow.mayContinue &&
mayBreakUnlabeled),
+ union(finallyFlow.breakLabels, finallyFlow.mayContinue ?
breakLabels : Set.of()),
+ finallyFlow.mayContinueUnlabeled || (finallyFlow.mayContinue
&& mayContinueUnlabeled),
+ union(finallyFlow.continueLabels, finallyFlow.mayContinue ?
continueLabels : Set.of())
+ );
+ }
+
+ StatementFlow consumeLabels(final Set<String> labels) {
+ if (labels.isEmpty() || breakLabels.isEmpty()) return this;
+
+ Set<String> remainingBreakLabels = withoutLabels(breakLabels,
labels);
+ boolean consumedBreak = remainingBreakLabels.size() !=
breakLabels.size();
+ return flow(mayContinue || consumedBreak, mayBreakUnlabeled,
remainingBreakLabels, mayContinueUnlabeled, continueLabels);
+ }
+
+ boolean breaksToAny(final Set<String> labels) {
+ if (labels.isEmpty() || breakLabels.isEmpty()) return false;
+
+ for (String label : labels) {
+ if (breakLabels.contains(label)) return true;
+ }
+ return false;
+ }
+
+ boolean continuesToAny(final Set<String> labels) {
+ if (labels.isEmpty() || continueLabels.isEmpty()) return false;
+
+ for (String label : labels) {
+ if (continueLabels.contains(label)) return true;
+ }
+ return false;
+ }
+ }
+
+ /**
+ * Represents the compile-time constant value of a boolean loop-condition
or branch expression.
+ * Used by flow analysis to avoid emitting unreachable bytecode.
+ */
+ @Internal
+ public enum ConditionValue {
+ /** The boolean expression always evaluates to {@code false}. */
+ ALWAYS_FALSE,
+ /** The boolean expression always evaluates to {@code true}. */
+ ALWAYS_TRUE,
+ /** The value cannot be determined at compile time. */
+ UNKNOWN
+ }
}
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
index 314e7ef91e..737f5a3851 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
@@ -554,13 +554,7 @@ public class CompileStack {
pushVariableScope(scope);
continueLabel = new Label();
breakLabel = new Label();
- if (labelNames != null) {
- for (String labelName: labelNames) {
- if (labelName == null) continue;
- namedLoopBreakLabel.put(labelName, breakLabel);
- namedLoopContinueLabel.put(labelName, continueLabel);
- }
- }
+ registerNamedLabels(labelNames, breakLabel, continueLabel);
}
/**
@@ -582,13 +576,7 @@ public class CompileStack {
pushState();
continueLabel = new Label();
breakLabel = new Label();
- if (labelNames != null) {
- for (String labelName: labelNames) {
- if (labelName == null) continue;
- namedLoopBreakLabel.put(labelName, breakLabel);
- namedLoopContinueLabel.put(labelName, continueLabel);
- }
- }
+ registerNamedLabels(labelNames, breakLabel, continueLabel);
}
/**
@@ -607,8 +595,23 @@ public class CompileStack {
* @return the break label
*/
public Label pushSwitch() {
+ return pushSwitch(null);
+ }
+
+ /**
+ * Creates a new break label, registers it as the named break target for
each
+ * label name, and pushes an element onto the state stack so that a later
call
+ * to {@link #pop()} restores the previous state.
+ *
+ * @param labelNames the statement labels on the switch (may be {@code
null})
+ * @return the break label
+ *
+ * @since 6.0.0
+ */
+ public Label pushSwitch(final List<String> labelNames) {
pushState();
breakLabel = new Label();
+ registerNamedLabels(labelNames, breakLabel, null);
return breakLabel;
}
@@ -623,13 +626,22 @@ public class CompileStack {
public Label pushBreakable(final List<String> labelNames) {
pushState();
Label namedBreakLabel = new Label();
- if (labelNames != null) {
- for (String labelName: labelNames) {
- if (labelName == null) continue;
+ registerNamedLabels(labelNames, namedBreakLabel, null);
+ return namedBreakLabel;
+ }
+
+ private void registerNamedLabels(final List<String> labelNames, final
Label namedBreakLabel, final Label namedContinueLabel) {
+ if (labelNames == null) return;
+
+ for (String labelName : labelNames) {
+ if (labelName == null) continue;
+ if (namedBreakLabel != null) {
namedLoopBreakLabel.put(labelName, namedBreakLabel);
}
+ if (namedContinueLabel != null) {
+ namedLoopContinueLabel.put(labelName, namedContinueLabel);
+ }
}
- return namedBreakLabel;
}
/**
diff --git
a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
index 9faca8a624..d3d4708822 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
@@ -22,14 +22,11 @@ import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.VariableScope;
import org.codehaus.groovy.ast.expr.BinaryExpression;
-import org.codehaus.groovy.ast.expr.BooleanExpression;
import org.codehaus.groovy.ast.expr.ClosureListExpression;
-import org.codehaus.groovy.ast.expr.ConstantExpression;
import org.codehaus.groovy.ast.expr.EmptyExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.MethodCall;
import org.codehaus.groovy.ast.expr.MethodCallExpression;
-import org.codehaus.groovy.ast.expr.NotExpression;
import org.codehaus.groovy.ast.stmt.AssertStatement;
import org.codehaus.groovy.ast.stmt.BlockStatement;
import org.codehaus.groovy.ast.stmt.BreakStatement;
@@ -37,7 +34,6 @@ import org.codehaus.groovy.ast.stmt.CaseStatement;
import org.codehaus.groovy.ast.stmt.CatchStatement;
import org.codehaus.groovy.ast.stmt.ContinueStatement;
import org.codehaus.groovy.ast.stmt.DoWhileStatement;
-import org.codehaus.groovy.ast.stmt.EmptyStatement;
import org.codehaus.groovy.ast.stmt.ExpressionStatement;
import org.codehaus.groovy.ast.stmt.ForStatement;
import org.codehaus.groovy.ast.stmt.IfStatement;
@@ -59,8 +55,28 @@ import java.util.Optional;
import java.util.function.Consumer;
import static org.apache.groovy.ast.tools.ExpressionUtils.isNullConstant;
-import static org.codehaus.groovy.ast.tools.GeneralUtils.*;
-import static org.objectweb.asm.Opcodes.*;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.ConditionValue;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.castX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.constantBooleanValue;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.isEmptyStatement;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.mayReachLoopCondition;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.maybeFallsThrough;
+import static
org.codehaus.groovy.ast.tools.GeneralUtils.maybeFallsThroughToNextSwitchCase;
+import static org.objectweb.asm.Opcodes.ALOAD;
+import static org.objectweb.asm.Opcodes.ATHROW;
+import static org.objectweb.asm.Opcodes.CHECKCAST;
+import static org.objectweb.asm.Opcodes.GOTO;
+import static org.objectweb.asm.Opcodes.IADD;
+import static org.objectweb.asm.Opcodes.ICONST_1;
+import static org.objectweb.asm.Opcodes.ICONST_M1;
+import static org.objectweb.asm.Opcodes.IFEQ;
+import static org.objectweb.asm.Opcodes.IFNULL;
+import static org.objectweb.asm.Opcodes.INVOKEVIRTUAL;
+import static org.objectweb.asm.Opcodes.MONITORENTER;
+import static org.objectweb.asm.Opcodes.MONITOREXIT;
+import static org.objectweb.asm.Opcodes.NOP;
+import static org.objectweb.asm.Opcodes.RETURN;
/**
* Generates bytecode for Groovy statements by visiting AST statement nodes
@@ -187,15 +203,13 @@ public class StatementWriter {
OperandStack operandStack = controller.getOperandStack();
// declare the loop index and value variables
- BytecodeVariable indexVariable =
Optional.ofNullable(statement.getIndexVariable()).map(iv -> {
- mv.visitInsn(ICONST_M1); // initialize to -1 so increment can pair
with next()
- return compileStack.defineVariable(iv, true);
- }).orElse(null);
+ BytecodeVariable indexVariable = defineLoopIndexVariable(statement);
BytecodeVariable valueVariable =
compileStack.defineVariable(statement.getValueVariable(), false);
// get the iterator and generate the loop control
int iterator = compileStack.defineTemporaryVariable("iterator",
ClassHelper.Iterator_TYPE, true);
Label breakLabel = compileStack.getBreakLabel(), continueLabel =
compileStack.getContinueLabel();
+ boolean bodyMayReachContinue = mayReachLoopCondition(statement);
mv.visitVarInsn(ALOAD, iterator);
mv.visitJumpInsn(IFNULL, breakLabel);
@@ -227,12 +241,26 @@ public class StatementWriter {
// generate the loop body
statement.getLoopBlock().visit(controller.getAcg());
- mv.visitJumpInsn(GOTO, continueLabel);
+ writeLoopBackEdge(continueLabel, bodyMayReachContinue);
mv.visitLabel(breakLabel);
compileStack.removeVar(iterator);
}
+ protected final BytecodeVariable defineLoopIndexVariable(final
ForStatement statement) {
+ CompileStack compileStack = controller.getCompileStack();
+ return Optional.ofNullable(statement.getIndexVariable()).map(iv -> {
+ controller.getMethodVisitor().visitInsn(ICONST_M1); // initialize
to -1 so increment can pair with next()
+ return compileStack.defineVariable(iv, true);
+ }).orElse(null);
+ }
+
+ protected final void writeLoopBackEdge(final Label continueLabel, final
boolean bodyMayReachContinue) {
+ if (bodyMayReachContinue) {
+ controller.getMethodVisitor().visitJumpInsn(GOTO, continueLabel);
+ }
+ }
+
/**
* Emits the {@link java.util.Iterator#hasNext()} call via the given
visitor.
* Overrideable so subclasses can substitute a specialized or inlined
variant.
@@ -287,31 +315,26 @@ public class StatementWriter {
Label cond = new Label();
mv.visitLabel(cond);
- // visit condition leave boolean on stack
- {
- int mark = controller.getOperandStack().getStackLength();
- Expression condExpr = expressions.get(condIndex);
- condExpr.visit(controller.getAcg());
- controller.getOperandStack().castToBool(mark, true);
- }
- // jump if we don't want to continue
- // note: ifeq tests for ==0, a boolean is 0 if it is false
- controller.getOperandStack().jump(IFEQ, breakLabel);
-
- // Generate the loop body
- statement.getLoopBlock().visit(controller.getAcg());
-
- // visit increment
- mv.visitLabel(continueLabel);
- // fix for being on the wrong line when debugging for loop
- controller.getAcg().onLineNumber(statement, "increment condition");
- for (int i = condIndex + 1; i < size; i += 1) {
- visitExpressionOfLoopStatement(expressions.get(i));
+ ConditionValue conditionValue =
visitConditionOfLoopStatement(expressions.get(condIndex), breakLabel, mv);
+ boolean bodyMayReachContinue = mayReachLoopCondition(statement);
+ if (conditionValue != ConditionValue.ALWAYS_FALSE) {
+ // Generate the loop body
+ statement.getLoopBlock().visit(controller.getAcg());
+
+ if (bodyMayReachContinue) {
+ // visit increment
+ mv.visitLabel(continueLabel);
+ // fix for being on the wrong line when debugging for loop
+ controller.getAcg().onLineNumber(statement, "increment
condition");
+ for (int i = condIndex + 1; i < size; i += 1) {
+ visitExpressionOfLoopStatement(expressions.get(i));
+ }
+
+ // jump to test the condition again
+ writeLoopBackEdge(cond, true);
+ }
}
- // jump to test the condition again
- mv.visitJumpInsn(GOTO, cond);
-
// loop end
mv.visitLabel(breakLabel);
@@ -334,27 +357,21 @@ public class StatementWriter {
}
}
- private void visitConditionOfLoopingStatement(final BooleanExpression
expression, final Label breakLabel, final MethodVisitor mv) {
- Expression expr = expression;
- boolean reverse = false;
- do { // undo arbitrary nesting of (Boolean|Not)Expressions
- if (expr instanceof NotExpression) reverse = !reverse;
- expr = ((BooleanExpression) expr).getExpression();
- } while (expr instanceof BooleanExpression);
-
- // optimize constant boolean condition
- if (expr instanceof ConstantExpression && ((ConstantExpression)
expr).getValue() instanceof Boolean) {
- if (((ConstantExpression) expr).isFalseExpression() && !reverse) {
- mv.visitJumpInsn(GOTO, breakLabel); // unconditional exit
- return;
- } else {
- // unconditional loop
- return;
- }
+ private ConditionValue visitConditionOfLoopStatement(final Expression
expression, final Label breakLabel, final MethodVisitor mv) {
+ ConditionValue conditionValue = constantBooleanValue(expression);
+ if (conditionValue == ConditionValue.ALWAYS_FALSE) {
+ mv.visitJumpInsn(GOTO, breakLabel); // unconditional exit
+ return conditionValue;
+ }
+ if (conditionValue == ConditionValue.ALWAYS_TRUE) {
+ return conditionValue; // unconditional loop
}
+ int mark = controller.getOperandStack().getStackLength();
expression.visit(controller.getAcg());
+ controller.getOperandStack().castToBool(mark, true);
controller.getOperandStack().jump(IFEQ, breakLabel);
+ return ConditionValue.UNKNOWN;
}
/**
@@ -370,14 +387,15 @@ public class StatementWriter {
compileStack.pushLoop(statement.getStatementLabels());
Label continueLabel = compileStack.getContinueLabel();
Label breakLabel = compileStack.getBreakLabel();
+ boolean bodyMayReachContinue = mayReachLoopCondition(statement);
MethodVisitor mv = controller.getMethodVisitor();
mv.visitLabel(continueLabel);
- visitConditionOfLoopingStatement(statement.getBooleanExpression(),
breakLabel, mv);
- statement.getLoopBlock().visit(controller.getAcg());
-
- mv.visitJumpInsn(GOTO, continueLabel);
+ if (visitConditionOfLoopStatement(statement.getBooleanExpression(),
breakLabel, mv) != ConditionValue.ALWAYS_FALSE) {
+ statement.getLoopBlock().visit(controller.getAcg());
+ writeLoopBackEdge(continueLabel, bodyMayReachContinue);
+ }
mv.visitLabel(breakLabel);
compileStack.pop();
@@ -401,10 +419,12 @@ public class StatementWriter {
mv.visitLabel(blockLabel);
statement.getLoopBlock().visit(controller.getAcg());
- mv.visitLabel(continueLabel); // GROOVY-11739: continue jumps to
condition
- visitConditionOfLoopingStatement(statement.getBooleanExpression(),
breakLabel, mv);
-
- mv.visitJumpInsn(GOTO, blockLabel);
+ if (mayReachLoopCondition(statement)) {
+ mv.visitLabel(continueLabel); // GROOVY-11739: continue jumps to
condition
+ if
(visitConditionOfLoopStatement(statement.getBooleanExpression(), breakLabel,
mv) != ConditionValue.ALWAYS_FALSE) {
+ mv.visitJumpInsn(GOTO, blockLabel);
+ }
+ }
mv.visitLabel(breakLabel);
compileStack.pop();
@@ -534,7 +554,7 @@ public class StatementWriter {
final CompileStack compileStack = controller.getCompileStack();
recorder.excludedStatement = () -> {
- if (finallyStatement == null || finallyStatement instanceof
EmptyStatement) return;
+ if (isEmptyStatement(finallyStatement)) return;
final VariableScope originalScope = compileStack.getScope();
compileStack.pop();
@@ -578,7 +598,7 @@ public class StatementWriter {
// switch does not have a continue label; use enclosing continue label
CompileStack compileStack = controller.getCompileStack();
- Label breakLabel = compileStack.pushSwitch();
+ Label breakLabel =
compileStack.pushSwitch(statement.getStatementLabels());
int switchVariableIndex =
compileStack.defineTemporaryVariable("switch", exprType, true);
@@ -591,24 +611,26 @@ public class StatementWriter {
int i = 0;
for (Iterator<CaseStatement> iter = caseStatements.iterator();
iter.hasNext(); i += 1) {
- writeCaseStatement(iter.next(), switchVariableIndex, labels[i],
labels[i + 1]);
+ writeCaseStatement(iter.next(), statement, switchVariableIndex,
labels[i], labels[i + 1]);
}
statement.getDefaultStatement().visit(controller.getAcg());
- controller.getMethodVisitor().visitLabel(breakLabel);
+ if (maybeFallsThrough(statement) || statement.getStatementLabels() !=
null) {
+ controller.getMethodVisitor().visitLabel(breakLabel);
+ }
compileStack.removeVar(switchVariableIndex);
compileStack.pop();
}
- private void writeCaseStatement(final CaseStatement statement, final int
switchVariableIndex, final Label thisLabel, final Label nextLabel) {
- controller.getAcg().onLineNumber(statement, "visitCaseStatement");
+ private void writeCaseStatement(final CaseStatement caseStatement, final
SwitchStatement switchStatement, final int switchVariableIndex, final Label
thisLabel, final Label nextLabel) {
+ controller.getAcg().onLineNumber(caseStatement, "visitCaseStatement");
MethodVisitor mv = controller.getMethodVisitor();
OperandStack operandStack = controller.getOperandStack();
mv.visitVarInsn(ALOAD, switchVariableIndex);
- statement.getExpression().visit(controller.getAcg());
+ caseStatement.getExpression().visit(controller.getAcg());
operandStack.box();
controller.getBinaryExpressionHelper().getIsCaseMethod().call(mv);
operandStack.replace(ClassHelper.boolean_TYPE);
@@ -617,10 +639,10 @@ public class StatementWriter {
mv.visitLabel(thisLabel);
- statement.getCode().visit(controller.getAcg());
+ caseStatement.getCode().visit(controller.getAcg());
// now if we don't finish with a break we need to jump past the next
comparison
- if (nextLabel != null) {
+ if (nextLabel != null &&
maybeFallsThroughToNextSwitchCase(caseStatement.getCode(), switchStatement)) {
mv.visitJumpInsn(GOTO, nextLabel);
}
@@ -699,13 +721,18 @@ public class StatementWriter {
compileStack.writeExceptionTable(fb, catchAll, null);
compileStack.pop(); //pop fb
- finallyPart.run();
- mv.visitJumpInsn(GOTO, synchronizedEnd);
+ boolean fallthrough = maybeFallsThrough(statement.getCode());
+ if (fallthrough) {
+ finallyPart.run();
+ mv.visitJumpInsn(GOTO, synchronizedEnd);
+ }
mv.visitLabel(catchAll);
finallyPart.run();
mv.visitInsn(ATHROW);
- mv.visitLabel(synchronizedEnd);
+ if (fallthrough) {
+ mv.visitLabel(synchronizedEnd);
+ }
compileStack.removeVar(index);
}
diff --git
a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesStatementWriter.java
b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesStatementWriter.java
index 573ff1616b..01e1dea81d 100644
---
a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesStatementWriter.java
+++
b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesStatementWriter.java
@@ -38,7 +38,6 @@ import org.objectweb.asm.MethodVisitor;
import java.util.Enumeration;
import java.util.Objects;
-import java.util.Optional;
import static org.objectweb.asm.Opcodes.AALOAD;
import static org.objectweb.asm.Opcodes.ALOAD;
@@ -48,10 +47,8 @@ import static org.objectweb.asm.Opcodes.CALOAD;
import static org.objectweb.asm.Opcodes.DALOAD;
import static org.objectweb.asm.Opcodes.DUP;
import static org.objectweb.asm.Opcodes.FALOAD;
-import static org.objectweb.asm.Opcodes.GOTO;
import static org.objectweb.asm.Opcodes.IALOAD;
import static org.objectweb.asm.Opcodes.ICONST_0;
-import static org.objectweb.asm.Opcodes.ICONST_M1;
import static org.objectweb.asm.Opcodes.IFEQ;
import static org.objectweb.asm.Opcodes.IFNULL;
import static org.objectweb.asm.Opcodes.IF_ICMPGE;
@@ -119,12 +116,11 @@ public class StaticTypesStatementWriter extends
StatementWriter {
MethodVisitor mv = controller.getMethodVisitor();
AsmClassGenerator acg = controller.getAcg();
- BytecodeVariable indexVariable =
Optional.ofNullable(loop.getIndexVariable()).map(iv -> {
- mv.visitInsn(ICONST_M1); return compileStack.defineVariable(iv,
true);
- }).orElse(null);
+ BytecodeVariable indexVariable = defineLoopIndexVariable(loop);
BytecodeVariable valueVariable =
compileStack.defineVariable(loop.getValueVariable(),
arrayType.getComponentType(), false);
Label continueLabel = compileStack.getContinueLabel();
Label breakLabel = compileStack.getBreakLabel();
+ boolean bodyMayReachContinue =
GeneralUtils.mayReachLoopCondition(loop);
// load array on stack
arrayExpression.visit(acg);
@@ -161,7 +157,7 @@ public class StaticTypesStatementWriter extends
StatementWriter {
// loop body
loop.getLoopBlock().visit(acg);
- mv.visitJumpInsn(GOTO, continueLabel);
+ writeLoopBackEdge(continueLabel, bodyMayReachContinue);
mv.visitLabel(breakLabel);
@@ -202,12 +198,11 @@ public class StaticTypesStatementWriter extends
StatementWriter {
OperandStack operandStack = controller.getOperandStack();
MethodVisitor mv = controller.getMethodVisitor();
- BytecodeVariable indexVariable =
Optional.ofNullable(loop.getIndexVariable()).map(iv -> {
- mv.visitInsn(ICONST_M1); return compileStack.defineVariable(iv,
true);
- }).orElse(null);
+ BytecodeVariable indexVariable = defineLoopIndexVariable(loop);
BytecodeVariable valueVariable =
compileStack.defineVariable(loop.getValueVariable(), false);
Label continueLabel = compileStack.getContinueLabel();
Label breakLabel = compileStack.getBreakLabel();
+ boolean bodyMayReachContinue =
GeneralUtils.mayReachLoopCondition(loop);
collectionExpression.visit(controller.getAcg());
@@ -231,7 +226,7 @@ public class StaticTypesStatementWriter extends
StatementWriter {
}
loop.getLoopBlock().visit(controller.getAcg());
- mv.visitJumpInsn(GOTO, continueLabel);
+ writeLoopBackEdge(continueLabel, bodyMayReachContinue);
mv.visitLabel(breakLabel);
}
diff --git a/src/test/groovy/bugs/Groovy10156.groovy
b/src/test/groovy/bugs/Groovy10156.groovy
new file mode 100644
index 0000000000..158753bd45
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy10156.groovy
@@ -0,0 +1,703 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package bugs
+
+import org.codehaus.groovy.classgen.asm.AbstractBytecodeTestCase
+import org.junit.jupiter.api.Test
+
+import java.lang.reflect.Array
+
+final class Groovy10156 extends AbstractBytecodeTestCase {
+
+ @Test
+ void testEnumSwitchWithBreakDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ int result = 0
+ switch (e) {
+ case E.ONE:
+ result = 1
+ break
+ case E.TWO:
+ result = 2
+ break
+ case E.THREE:
+ result = 3
+ break
+ default:
+ result = 4
+ }
+ result
+ }
+ ''', 'ONE, TWO, THREE')
+ }
+
+ @Test
+ void testEnumSwitchWithReturnDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ return 1
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithThrowDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ throw new IllegalStateException('one')
+ case E.TWO:
+ throw new IllegalStateException('two')
+ default:
+ throw new IllegalStateException('default')
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void
testEnumSwitchWithNestedSwitchWhereAllPathsReturnDoesNotEmitSyntheticFallthroughJumps()
{
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e, boolean flag) {
+ switch (e) {
+ case E.ONE:
+ switch (flag) {
+ case true:
+ return 1
+ default:
+ return 2
+ }
+ case E.TWO:
+ return 3
+ default:
+ return 4
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithContinueDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E[] values) {
+ int result = 0
+ outer:
+ for (E e : values) {
+ switch (e) {
+ case E.ONE:
+ result += 1
+ continue outer
+ case E.TWO:
+ result += 2
+ break
+ default:
+ result += 4
+ }
+ result += 8
+ }
+ result
+ }
+ ''', 'ONE, TWO, THREE')
+ }
+
+ @Test
+ void
testEnumSwitchWithTryCatchFinallyWhereAllPathsReturnDoesNotEmitSyntheticFallthroughJumps()
{
+ assertNoUnexpectedUnreachableRethrowInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ try {
+ return 1
+ } catch (RuntimeException ignored) {
+ return 2
+ } finally {
+ }
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE')
+ }
+
+ @Test
+ void
testEnumSwitchWithSynchronizedReturnDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ synchronized (this) {
+ return 1
+ }
+ default:
+ return 2
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithInfiniteWhileDoesNotEmitSyntheticFallthroughJumps()
{
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ while (true) {
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void
testEnumSwitchWithInfiniteDoWhileDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ do {
+ } while (true)
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithConstantFalseWhileStillFallsThrough() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ while (false) {
+ return 99
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 2
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ assert c.test(enumValue(loader, 'THREE')) == 3
+ }
+ }
+
+ @Test
+ void testEnumSwitchWithClassicForConstantFalseStillFallsThrough() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ for (; false ;) {
+ return 99
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 2
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ assert c.test(enumValue(loader, 'THREE')) == 3
+ }
+ }
+
+ @Test
+ void testEnumSwitchWithForInLoopStillFallsThrough() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ for (value in [] as List<Integer>) {
+ return 99
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 2
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ assert c.test(enumValue(loader, 'THREE')) == 3
+ }
+ }
+
+ @Test
+ void
testEnumSwitchWithInfiniteClassicForDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ for (;;) {
+ return 1
+ }
+ default:
+ return 2
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithEmptyCaseStillFallsThrough() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithMixedBreakAndFallThroughPreservesSemantics() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e, boolean stopEarly) {
+ int result = 0
+ switch (e) {
+ case E.ONE:
+ if (stopEarly) break
+ result = 1
+ case E.TWO:
+ return result + 2
+ default:
+ return 4
+ }
+ return result + 8
+ }
+ ''', 'ONE, TWO, THREE')
+ }
+
+ @Test
+ void testLabeledSwitchRemainsValid() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e) {
+ outer:
+ switch (e) {
+ case E.ONE:
+ return 1
+ default:
+ return 2
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testNestedLabeledSwitchRemainsValid() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e, boolean flag) {
+ switch (e) {
+ case E.ONE:
+ inner:
+ switch (flag) {
+ case true:
+ return 2
+ default:
+ return 1
+ }
+ default:
+ return 3
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesLabeledBlockBreaksInCaseBodies() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e, boolean stopEarly) {
+ switch (e) {
+ case E.ONE:
+ inner: {
+ if (stopEarly) {
+ break inner
+ }
+ return 1
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE'), false) == 1
+ assert c.test(enumValue(loader, 'ONE'), true) == 2
+ assert c.test(enumValue(loader, 'TWO'), false) == 2
+ assert c.test(enumValue(loader, 'THREE'), false) == 3
+ }
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesTryCatchFinallyInCaseBodies() {
+ assertNoUnexpectedUnreachableRethrowInEnumSwitchMethod('''
+ int test(E e, boolean flag) {
+ switch (e) {
+ case E.ONE:
+ try {
+ if (flag) return 1
+ } catch (RuntimeException ignored) {
+ return 2
+ } finally {
+ }
+ case E.TWO:
+ return 3
+ default:
+ return 4
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesTryFinallyWithoutCatch() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ try {
+ return 1
+ } finally {
+ }
+ default:
+ return 2
+ }
+ }
+ ''') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 1
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ }
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesTryCatchFallthroughInCaseBodies() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ try {
+ throw new RuntimeException('boom')
+ } catch (RuntimeException ignored) {
+ } finally {
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 2
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ }
+ }
+
+ @Test
+ void
testSwitchFlowAnalysisHandlesTryCatchWithoutFinallyAndLocalTypeMerges() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ Object test(E e) {
+ switch (e) {
+ case E.ONE:
+ Object value
+ try {
+ value = System.nanoTime()
+ value = ''
+ } catch (RuntimeException ignored) {
+ }
+ return value
+ default:
+ return null
+ }
+ }
+ ''') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == ''
+ assert c.test(enumValue(loader, 'TWO')) == null
+ }
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesSynchronizedBlocksInCaseBodies() {
+ assertNoUnreachableBytecodeInEnumSwitchMethod('''
+ int test(E e, boolean flag) {
+ switch (e) {
+ case E.ONE:
+ synchronized (this) {
+ if (flag) {
+ return 1
+ }
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''')
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesInfiniteWhileWithBreakInCaseBodies() {
+ assertRunEnumSwitchAssertions('''
+ int test(E e, boolean stopEarly) {
+ switch (e) {
+ case E.ONE:
+ while (true) {
+ if (stopEarly) break
+ return 1
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE'), false) == 1
+ assert c.test(enumValue(loader, 'ONE'), true) == 2
+ assert c.test(enumValue(loader, 'TWO'), false) == 2
+ assert c.test(enumValue(loader, 'THREE'), false) == 3
+ }
+ }
+
+ @Test
+ void testLabeledBreakToSwitchInsideLoopStillAllowsNormalCompletion() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ int result = 0
+ synchronized (this) {
+ outer:
+ switch (e) {
+ case E.ONE:
+ for (int i = 0; i < 3; i++) {
+ if (i == 1) break outer
+ }
+ return 99
+ default:
+ return -1
+ }
+ result = 7
+ }
+ return result
+ }
+ ''') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 7
+ assert c.test(enumValue(loader, 'TWO')) == -1
+ }
+ }
+
+ @Test
+ void testEnumSwitchWithDoWhileContinueStillFallsThrough() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ do {
+ continue
+ } while (false)
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 2
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ assert c.test(enumValue(loader, 'THREE')) == 3
+ }
+ }
+
+ @Test
+ void testEnumSwitchWithLabeledDoWhileContinueStillFallsThrough() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ loop:
+ do {
+ continue loop
+ } while (false)
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ ''', 'ONE, TWO, THREE') { loader ->
+ def c = newTestSubject(loader)
+ assert c.test(enumValue(loader, 'ONE')) == 2
+ assert c.test(enumValue(loader, 'TWO')) == 2
+ assert c.test(enumValue(loader, 'THREE')) == 3
+ }
+ }
+
+ @Test
+ void testSwitchFlowAnalysisKeepsOuterLabeledContinueAbrupt() {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions('''
+ int test(E[] values) {
+ int result = 0
+ outer:
+ for (E e : values) {
+ switch (e) {
+ case E.ONE:
+ do {
+ result += 1
+ continue outer
+ } while (false)
+ default:
+ result += 10
+ }
+ result += 100
+ }
+ return result
+ }
+ ''') { loader ->
+ def c = newTestSubject(loader)
+ def enumType = loadEnumType(loader)
+ def pair = Array.newInstance(enumType, 2)
+ Array.set(pair, 0, enumValue(loader, 'ONE'))
+ Array.set(pair, 1, enumValue(loader, 'TWO'))
+ def single = Array.newInstance(enumType, 1)
+ Array.set(single, 0, enumValue(loader, 'ONE'))
+ assert c.test(pair) == 111
+ assert c.test(single) == 1
+ }
+ }
+
+ private void assertNoUnreachableBytecodeInTestMethod(final String source) {
+ def unreachable =
compileAndFindUnreachableInstructions(classNamePattern: 'C', method: 'test',
source)
+ assert unreachable.isEmpty(): "Unexpected unreachable instructions:
${unreachable}\n${sequence}"
+ }
+
+ private void assertNoUnexpectedUnreachableRethrowInTestMethod(final String
source) {
+ def unexpected =
compileAndFindUnreachableInstructions(classNamePattern: 'C', method: 'test',
source)
+ .findAll { !it.endsWith(':ATHROW') }
+ assert unexpected.isEmpty(): "Unexpected unreachable instructions:
${unexpected}\n${sequence}"
+ }
+
+ private void assertNoUnreachableBytecodeAndRunAssertions(final String
source, final Closure<?> assertions) {
+ assertNoUnreachableBytecodeInTestMethod(source)
+ assertRunAssertions(source, assertions)
+ }
+
+ private void assertNoUnreachableBytecodeInEnumSwitchMethod(final String
methodSource, final String enumConstants = 'ONE, TWO') {
+ assertNoUnreachableBytecodeInTestMethod(enumSwitchScript(methodSource,
enumConstants))
+ }
+
+ private void assertNoUnexpectedUnreachableRethrowInEnumSwitchMethod(final
String methodSource, final String enumConstants = 'ONE, TWO') {
+
assertNoUnexpectedUnreachableRethrowInTestMethod(enumSwitchScript(methodSource,
enumConstants))
+ }
+
+ private void assertNoUnreachableBytecodeAndRunEnumSwitchAssertions(final
String methodSource, final Closure<?> assertions) {
+ assertNoUnreachableBytecodeAndRunEnumSwitchAssertions(methodSource,
'ONE, TWO', assertions)
+ }
+
+ private void assertNoUnreachableBytecodeAndRunEnumSwitchAssertions(final
String methodSource, final String enumConstants, final Closure<?> assertions) {
+
assertNoUnreachableBytecodeAndRunAssertions(enumSwitchScript(methodSource,
enumConstants), assertions)
+ }
+
+ private void assertRunEnumSwitchAssertions(final String methodSource,
final Closure<?> assertions) {
+ assertRunEnumSwitchAssertions(methodSource, 'ONE, TWO', assertions)
+ }
+
+ private void assertRunEnumSwitchAssertions(final String methodSource,
final String enumConstants, final Closure<?> assertions) {
+ assertRunAssertions(enumSwitchScript(methodSource, enumConstants),
assertions)
+ }
+
+ private void assertRunAssertions(final String source, final Closure<?>
assertions) {
+ def loader = new GroovyClassLoader(this.class.classLoader)
+ try {
+ loader.parseClass(source, 'Groovy10156Runtime.groovy')
+ assertions.call(loader)
+ } finally {
+ loader.close()
+ }
+ }
+
+ private static String enumSwitchScript(final String methodSource, final
String enumConstants = 'ONE, TWO') {
+ """
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ enum E {
+ ${enumConstants}
+ }
+
+ @CompileStatic
+ class C {
+ ${indent(methodSource.stripIndent().trim())}
+ }
+ """.stripIndent()
+ }
+
+ private static String indent(final String source, final int spaces = 4) {
+ def prefix = ' ' * spaces
+ source.readLines().collect { line -> line ? prefix + line : line
}.join('\n')
+ }
+
+ private static Object newTestSubject(final GroovyClassLoader loader) {
+ loader.loadClass('C').getDeclaredConstructor().newInstance()
+ }
+
+ private static Class<?> loadEnumType(final GroovyClassLoader loader) {
+ loader.loadClass('E')
+ }
+
+ private static Object enumValue(final GroovyClassLoader loader, final
String constantName) {
+ Enum.valueOf(loadEnumType(loader), constantName)
+ }
+
+}
diff --git a/src/test/groovy/groovy/ForLoopTest.groovy
b/src/test/groovy/groovy/ForLoopTest.groovy
index 05fb76a253..865c40a58c 100644
--- a/src/test/groovy/groovy/ForLoopTest.groovy
+++ b/src/test/groovy/groovy/ForLoopTest.groovy
@@ -253,4 +253,31 @@ final class ForLoopTest {
}
assert counter == 10, "The body of the for loop wasn't executed, it
should have looped 10 times."
}
+
+ @Test
+ void testRuntimeCompiledForLoopsCoverOptimizedWriters() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ void runStaticLoops() {
+ for (int i, int v in new int[]{0, 1, 2}) {
+ assert i == v
+ continue
+ }
+ for (int i, Integer v in Collections.enumeration([0, 1, 2])) {
+ assert i == v
+ continue
+ }
+ }
+
+ runStaticLoops()
+
+ int total = 0
+ for (int i = 0, j = 2; i < 3; i += 1, j -= 1) {
+ total += i + j
+ }
+ assert total == 6
+ '''
+ }
}
diff --git
a/src/test/groovy/org/codehaus/groovy/classgen/asm/AbstractBytecodeTestCase.groovy
b/src/test/groovy/org/codehaus/groovy/classgen/asm/AbstractBytecodeTestCase.groovy
index 482b59db87..e2ee450506 100644
---
a/src/test/groovy/org/codehaus/groovy/classgen/asm/AbstractBytecodeTestCase.groovy
+++
b/src/test/groovy/org/codehaus/groovy/classgen/asm/AbstractBytecodeTestCase.groovy
@@ -27,6 +27,10 @@ import org.objectweb.asm.ClassReader
import org.objectweb.asm.ClassVisitor
import org.objectweb.asm.FieldVisitor
import org.objectweb.asm.MethodVisitor
+import org.objectweb.asm.tree.ClassNode
+import org.objectweb.asm.tree.analysis.Analyzer
+import org.objectweb.asm.tree.analysis.BasicInterpreter
+import org.objectweb.asm.util.Printer
import org.objectweb.asm.util.TraceClassVisitor
import java.security.CodeSource
@@ -37,11 +41,13 @@ import java.security.CodeSource
abstract class AbstractBytecodeTestCase {
Class clazz
+ byte[] classBytes
Map extractionOptions
InstructionSequence sequence
@BeforeEach
void setUp() {
+ classBytes = null
extractionOptions = [method: 'run']
}
@@ -59,7 +65,7 @@ abstract class AbstractBytecodeTestCase {
if (unit != null) {
try {
def firstClass = unit.classes.find { it.name ==
unit.firstClassNode.name }
- sequence = extractSequence(firstClass.bytes,
extractionOptions)
+ captureClassBytesAndSequence(firstClass, extractionOptions)
if (extractionOptions.print) println(sequence)
} catch (e) {
// probably an error in the script
@@ -78,6 +84,7 @@ abstract class AbstractBytecodeTestCase {
options = [method: 'run', classNamePattern: '.*script', *: options]
sequence = null
clazz = null
+ classBytes = null
def cu = new CompilationUnit()
def su = cu.addSource('script', scriptText)
cu.compile(Phases.CONVERSION)
@@ -88,12 +95,12 @@ abstract class AbstractBytecodeTestCase {
for (gc in cu.classes) {
if (gc.name ==~ options.classNamePattern) {
- sequence = extractSequence(gc.bytes, options)
+ captureClassBytesAndSequence(gc, options)
}
}
if (sequence == null && cu.classes) {
def gc = cu.classes.find { it.name == cu.firstClassNode.name }
- sequence = extractSequence(gc.bytes, options)
+ captureClassBytesAndSequence(gc, options)
}
for (gc in cu.classes) {
try {
@@ -117,24 +124,16 @@ abstract class AbstractBytecodeTestCase {
@Override
MethodVisitor visitMethod(final int access, final String name,
final String desc, final String signature, final String... exceptions) {
if (options.method == name) {
- // last in "tcv.p.text" is a list that will be filled by
"super.visit"
- tcv.p.text.add(tcv.p.text.size() - 2, '--BEGIN--\n')
- try {
+ return withSelectionMarkers(tcv) {
super.visitMethod(access, name, desc, signature,
exceptions)
- } finally {
- tcv.p.text.add('--END--\n')
}
}
}
@Override
FieldVisitor visitField(final int access, final String name, final
String desc, final String signature, final Object value) {
if (options.field == name) {
- // last in "tcv.p.text" is a list that will be filled by
"super.visit"
- tcv.p.text.add(tcv.p.text.size() - 2, '--BEGIN--\n')
- try {
+ return withSelectionMarkers(tcv) {
super.visitField(access, name, desc, signature, value)
- } finally {
- tcv.p.text.add('--END--\n')
}
}
}
@@ -143,6 +142,46 @@ abstract class AbstractBytecodeTestCase {
new ClassReader(bytes).accept(tcv, 0)
new InstructionSequence(instructions:
out.toString().split('\n')*.trim())
}
+
+ protected List<String> compileAndFindUnreachableInstructions(Map options =
[:], final String scriptText) {
+ options = [method: 'run', *: options]
+ compile(options, scriptText)
+ assert classBytes != null: 'No class bytes were captured during
compilation'
+ findUnreachableInstructions(classBytes, options.method)
+ }
+
+ protected List<String> findUnreachableInstructions(final byte[] bytes,
final String methodName) {
+ def classNode = new ClassNode()
+ new ClassReader(bytes).accept(classNode, ClassReader.EXPAND_FRAMES)
+
+ def methodNode = classNode.methods.find { it.name == methodName }
+ assert methodNode != null: "Method '${methodName}' was not found in
${classNode.name}"
+
+ def frames = new Analyzer(new
BasicInterpreter()).analyze(classNode.name, methodNode)
+ def unreachable = []
+ for (int i = 0; i < methodNode.instructions.size(); i += 1) {
+ def instruction = methodNode.instructions.get(i)
+ if (instruction.opcode >= 0 && frames[i] == null) {
+ unreachable << "${i}:${Printer.OPCODES[instruction.opcode]}"
+ }
+ }
+ unreachable
+ }
+
+ private void captureClassBytesAndSequence(final gc, final Map options) {
+ classBytes = gc.bytes
+ sequence = extractSequence(gc.bytes, options)
+ }
+
+ private static <T> T withSelectionMarkers(final TraceClassVisitor
traceClassVisitor, final Closure<T> visitAction) {
+ // The penultimate entry is the method/field body container populated
by super.visit*.
+ traceClassVisitor.p.text.add(traceClassVisitor.p.text.size() - 2,
'--BEGIN--\n')
+ try {
+ visitAction.call()
+ } finally {
+ traceClassVisitor.p.text.add('--END--\n')
+ }
+ }
}
/**
