This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch GROOVY-10156 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 063d1ca5c9dd66aee06fcdd25f7a5afa430dc6dc Author: Daniel Sun <[email protected]> AuthorDate: Sun May 17 13:32:12 2026 +0900 GROOVY-10156: Unreachable bytecode in switch statement --- .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') + } + } } /**
