Copilot commented on code in PR #2538:
URL: https://github.com/apache/groovy/pull/2538#discussion_r3252906019
##########
src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java:
##########
@@ -1286,4 +1289,128 @@ public static boolean maybeFallsThrough(final Statement
statement) {
return true;
}
+
+ /**
+ * Indicates whether switch-case code may fall through into the next case
body.
+ */
+ @Internal
+ public static boolean maybeFallsThroughToNextSwitchCase(final Statement
statement, final SwitchStatement switchStatement) {
+ return analyzeSwitchFlow(statement,
labelSet(switchStatement.getStatementLabels()), Set.of()).mayContinue;
+ }
+
+ private static boolean maybeFallsThroughSwitch(final SwitchStatement
statement) {
+ Set<String> switchLabels = labelSet(statement.getStatementLabels());
+ SwitchFlow flow = analyzeSwitchFlow(statement.getDefaultStatement(),
switchLabels, Set.of());
+ boolean nextPathMayFallThrough = flow.mayBreak || flow.mayContinue;
+ boolean switchMayFallThrough = nextPathMayFallThrough;
+
+ List<CaseStatement> caseStatements = statement.getCaseStatements();
+ for (int i = caseStatements.size() - 1; i >= 0; i -= 1) {
+ flow = analyzeSwitchFlow(caseStatements.get(i).getCode(),
switchLabels, Set.of());
+ nextPathMayFallThrough = flow.mayBreak || (flow.mayContinue &&
nextPathMayFallThrough);
+ switchMayFallThrough |= nextPathMayFallThrough;
+ }
+
+ return switchMayFallThrough;
+ }
+
+ private static SwitchFlow analyzeSwitchFlow(final Statement statement,
final Set<String> switchLabels, final Set<String> inheritedLabels) {
+ if (statement.isEmpty()) return SwitchFlow.FALL_THROUGH;
+
+ Set<String> localLabels = inheritedLabels;
+ List<String> labels = statement.getStatementLabels();
+ if (labels != null && !labels.isEmpty()) {
+ localLabels = mergeLabels(inheritedLabels, labels);
+ }
+
+ if (statement instanceof BreakStatement breakStatement) {
+ String label = breakStatement.getLabel();
+ if (label == null || switchLabels.contains(label)) {
+ return SwitchFlow.BREAK;
+ }
+ if (localLabels.contains(label)) {
+ return SwitchFlow.FALL_THROUGH;
+ }
+ return SwitchFlow.ABRUPT;
+ } else if (statement instanceof ContinueStatement || statement
instanceof ReturnStatement || statement instanceof ThrowStatement) {
+ return SwitchFlow.ABRUPT;
+ } else if (statement instanceof BlockStatement block) {
+ boolean mayContinue = true, mayBreak = false;
+ for (Statement subStatement : block.getStatements()) {
+ if (!mayContinue) break;
+
+ SwitchFlow flow = analyzeSwitchFlow(subStatement,
switchLabels, localLabels);
+ mayContinue = flow.mayContinue;
+ mayBreak |= flow.mayBreak;
+ }
+ return SwitchFlow.of(mayContinue, mayBreak);
+ } else if (statement instanceof IfStatement ifStatement) {
+ SwitchFlow thenFlow = analyzeSwitchFlow(ifStatement.getIfBlock(),
switchLabels, localLabels);
+ SwitchFlow elseFlow =
analyzeSwitchFlow(ifStatement.getElseBlock(), switchLabels, localLabels);
+ return SwitchFlow.of(thenFlow.mayContinue || elseFlow.mayContinue,
thenFlow.mayBreak || elseFlow.mayBreak);
+ } else if (statement instanceof TryCatchStatement tryCatch) {
+ SwitchFlow finallyFlow =
analyzeSwitchFlow(tryCatch.getFinallyStatement(), switchLabels, localLabels);
+
+ SwitchFlow bodyFlow =
analyzeSwitchFlow(tryCatch.getTryStatement(), switchLabels, localLabels);
+ for (CatchStatement catchStatement :
tryCatch.getCatchStatements()) {
+ SwitchFlow catchFlow =
analyzeSwitchFlow(catchStatement.getCode(), switchLabels, localLabels);
+ bodyFlow = SwitchFlow.of(bodyFlow.mayContinue ||
catchFlow.mayContinue, bodyFlow.mayBreak || catchFlow.mayBreak);
+ }
+
+ return SwitchFlow.of(finallyFlow.mayContinue &&
bodyFlow.mayContinue, finallyFlow.mayBreak || (finallyFlow.mayContinue &&
bodyFlow.mayBreak));
+ } else if (statement instanceof SynchronizedStatement
synchronizedStatement) {
+ return analyzeSwitchFlow(synchronizedStatement.getCode(),
switchLabels, localLabels);
+ } else if (statement instanceof SwitchStatement switchStatement) {
+ return SwitchFlow.of(maybeFallsThroughSwitch(switchStatement),
false);
+ }
+
+ return SwitchFlow.of(maybeFallsThrough(statement), false);
+ }
+
Review Comment:
This fallback collapses every unhandled statement to a plain may-continue
result and always discards `mayBreak`. That misses labeled breaks to the
enclosing switch from inside loops or nested switches, so
`maybeFallsThroughSwitch` can return false even though a `break outer` path
exits the switch; callers such as try/finally generation and method epilogue
generation can then omit the normal fall-through path and produce invalid or
misdirected bytecode.
##########
src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java:
##########
@@ -1286,4 +1289,128 @@ public static boolean maybeFallsThrough(final Statement
statement) {
return true;
}
+
+ /**
+ * Indicates whether switch-case code may fall through into the next case
body.
+ */
+ @Internal
+ public static boolean maybeFallsThroughToNextSwitchCase(final Statement
statement, final SwitchStatement switchStatement) {
+ return analyzeSwitchFlow(statement,
labelSet(switchStatement.getStatementLabels()), Set.of()).mayContinue;
+ }
+
+ private static boolean maybeFallsThroughSwitch(final SwitchStatement
statement) {
+ Set<String> switchLabels = labelSet(statement.getStatementLabels());
+ SwitchFlow flow = analyzeSwitchFlow(statement.getDefaultStatement(),
switchLabels, Set.of());
+ boolean nextPathMayFallThrough = flow.mayBreak || flow.mayContinue;
+ boolean switchMayFallThrough = nextPathMayFallThrough;
+
+ List<CaseStatement> caseStatements = statement.getCaseStatements();
+ for (int i = caseStatements.size() - 1; i >= 0; i -= 1) {
+ flow = analyzeSwitchFlow(caseStatements.get(i).getCode(),
switchLabels, Set.of());
+ nextPathMayFallThrough = flow.mayBreak || (flow.mayContinue &&
nextPathMayFallThrough);
+ switchMayFallThrough |= nextPathMayFallThrough;
+ }
+
+ return switchMayFallThrough;
+ }
+
+ private static SwitchFlow analyzeSwitchFlow(final Statement statement,
final Set<String> switchLabels, final Set<String> inheritedLabels) {
+ if (statement.isEmpty()) return SwitchFlow.FALL_THROUGH;
+
+ Set<String> localLabels = inheritedLabels;
+ List<String> labels = statement.getStatementLabels();
+ if (labels != null && !labels.isEmpty()) {
+ localLabels = mergeLabels(inheritedLabels, labels);
+ }
+
+ if (statement instanceof BreakStatement breakStatement) {
+ String label = breakStatement.getLabel();
+ if (label == null || switchLabels.contains(label)) {
+ return SwitchFlow.BREAK;
+ }
+ if (localLabels.contains(label)) {
+ return SwitchFlow.FALL_THROUGH;
Review Comment:
Treating a break to a label declared inside the analyzed statement as
ordinary continuation loses the fact that the break jumps to after that labeled
statement and skips any remaining statements in it. For example, a labeled
block in a case can `break` out of the block before a following `return`, so
the case still falls through to the next case; this logic would report no
fall-through and suppress the required jump to the next case.
##########
src/test/groovy/bugs/Groovy10156.groovy:
##########
@@ -0,0 +1,427 @@
+/*
+ * 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
+
+final class Groovy10156 extends AbstractBytecodeTestCase {
+
+ @Test
+ void testEnumSwitchWithBreakDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ 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
+ }
+ }
+ enum E {
+ ONE, TWO, THREE
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithReturnDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ return 1
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithThrowDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ 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')
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ ''')
+ }
+
+ @Test
+ void
testEnumSwitchWithNestedSwitchWhereAllPathsReturnDoesNotEmitSyntheticFallthroughJumps()
{
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ 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
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ ''')
+ }
+
+ @Test
+ void testEnumSwitchWithContinueDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ 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
+ }
+ }
+ enum E {
+ ONE, TWO, THREE
+ }
+ ''')
+ }
+
+ @Test
+ void
testEnumSwitchWithTryCatchFinallyWhereAllPathsReturnDoesNotEmitSyntheticFallthroughJumps()
{
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ try {
+ return 1
+ } catch (RuntimeException ignored) {
+ return 2
+ } finally {
+ }
+ default:
+ return 3
+ }
+ }
+ }
+ enum E {
+ ONE
+ }
+ ''')
+ }
+
+ @Test
+ void
testEnumSwitchWithSynchronizedReturnDoesNotEmitSyntheticFallthroughJumps() {
+ assertNoUnreachableSequenceInTestMethod '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ enum E {
+ ONE, TWO
+ }
+
+ @CompileStatic
+ class C {
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ synchronized (this) {
+ return 1
+ }
+ default:
+ return 2
+ }
+ }
+ }
+ '''
+ }
+
+ @Test
+ void testEnumSwitchWithEmptyCaseStillFallsThrough() {
+ assertNoUnreachableSequenceInTestMethod '''
+ @groovy.transform.CompileStatic
+ class C {
+ int test(E e) {
+ switch (e) {
+ case E.ONE:
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ '''
+ }
+
+ @Test
+ void testEnumSwitchWithMixedBreakAndFallThroughPreservesSemantics() {
+ assertNoUnreachableSequenceInTestMethod '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ enum E {
+ ONE, TWO, THREE
+ }
+
+ @CompileStatic
+ class C {
+ 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
+ }
+ }
+ '''
+ }
+
+ @Test
+ void testLabeledSwitchRemainsValid() {
+ assertNoUnreachableSequenceInTestMethod '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ enum E {
+ ONE, TWO
+ }
+
+ @CompileStatic
+ class C {
+ int test(E e) {
+ outer:
+ switch (e) {
+ case E.ONE:
+ return 1
+ default:
+ return 2
+ }
+ }
+ }
+ '''
+ }
+
+ @Test
+ void testNestedLabeledSwitchRemainsValid() {
+ assertNoUnreachableSequenceInTestMethod '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ enum E {
+ ONE, TWO
+ }
+
+ @CompileStatic
+ class C {
+ 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 testSwitchFlowAnalysisHandlesLabeledStatementsInCaseBodies() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ int test(E e, boolean flag) {
+ switch (e) {
+ case E.ONE:
+ inner:
+ if (flag) {
+ return 1
+ }
+ case E.TWO:
+ return 2
+ default:
+ return 3
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ ''')
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesTryCatchFinallyInCaseBodies() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ 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
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ ''')
+ }
+
+ @Test
+ void testSwitchFlowAnalysisHandlesSynchronizedBlocksInCaseBodies() {
+ assertNoUnreachableSequenceInTestMethod('''
+ @groovy.transform.CompileStatic
+ class C {
+ 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
+ }
+ }
+ }
+ enum E {
+ ONE, TWO
+ }
+ ''')
+ }
+
+ @Test
+ void testLabeledSwitchWithNamedBreakFromInsideLoopRemainsValid() {
+ assertNoUnreachableSequenceInTestMethod '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ enum E {
+ ONE, TWO
+ }
+
+ @CompileStatic
+ class C {
+ int test(E e) {
+ int result = 0
+ outer:
+ switch (e) {
+ case E.ONE:
+ for (int i = 0; i < 3; i++) {
+ if (i == 1) break outer
+ }
+ result = 99
+ break
+ default:
+ result = -1
+ }
+ result
+ }
+ }
+ '''
+ }
+
+ private void assertNoUnreachableSequenceInTestMethod(final String source) {
+ def bytecode = compile(classNamePattern: 'C', method: 'test', source)
+ assert !bytecode.hasStrictSequence(UNREACHABLE_SEQUENCE)
Review Comment:
The new regression tests only compile the snippets and assert that one
opcode sequence is absent; they never execute `C.test` or assert its return
values. Since this change alters switch fall-through and labeled-break code
generation, semantic regressions (for example falling into the default case
instead of the next case) can pass these tests as long as the bytecode no
longer contains this sequence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
