GROOVY-12289: Switch expressions with duplicate case labels compile under @TypeChecked but fail under @CompileStatic

Author: paulk-asert

src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesSwitchExpressionWriter.java

@@ -31,7 +31,6 @@
 import org.codehaus.groovy.classgen.asm.OperandStack;
 import org.codehaus.groovy.classgen.asm.SwitchExpressionWriter;
 import org.codehaus.groovy.classgen.asm.VariableSlotLoader;
-import org.codehaus.groovy.syntax.SyntaxException;
 import org.codehaus.groovy.transform.stc.StaticTypesMarker;
 import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
@@ -167,7 +166,6 @@ private boolean writeIntSwitch(final SwitchExpression expression,
         Label defaultTarget = new Label();
         ArmGroup<Integer> group = groupArms(expression.getCaseStatements(),
                 cs -> intConstant(cs.getExpression()), defaultTarget);
-        if (group.error) return true;
         if (group.keys == null) return false;
 
         OperandStack operandStack = controller.getOperandStack();
@@ -239,7 +237,6 @@ private boolean writeStringSwitch(final SwitchExpression expression,
         Label defaultTarget = new Label();
         ArmGroup<String> group = groupArms(expression.getCaseStatements(),
                 cs -> stringConstant(cs.getExpression()), defaultTarget);
-        if (group.error) return true;
         if (group.keys == null) return false;
 
         jumpIfNull(selectorIndex, selectorType, defaultTarget);
@@ -261,7 +258,6 @@ private boolean writeEnumSwitch(final SwitchExpression expression,
         Label defaultTarget = new Label();
         ArmGroup<String> group = groupArms(expression.getCaseStatements(),
                 cs -> enumConstantName(cs.getExpression(), enumType), defaultTarget);
-        if (group.error) return true;
         if (group.keys == null) return false;
 
         MethodVisitor mv = controller.getMethodVisitor();
@@ -339,6 +335,10 @@ private void emitStringHashDispatch(final int stringLocal, final List<String> ke
      * Forward pass extracts every constant key (or skips the optimizer).
      * Backward pass gives empty colon prefixes the following body's label,
      * or {@code defaultTarget} when the empty suffix falls into default.
+     * Duplicate keys also skip the optimizer: the type checker reports them
+     * as an error (GROOVY-12289), so reaching here with one means checking
+     * was bypassed ({@code TypeCheckingMode.SKIP} or an extension), where
+     * sequential first-match-wins dispatch preserves dynamic semantics.
      */
     private <K> ArmGroup<K> groupArms(final List<CaseStatement> caseStatements,
             final Function<CaseStatement, K> keyFn, final Label defaultTarget) {
@@ -349,11 +349,7 @@ private <K> ArmGroup<K> groupArms(final List<CaseStatement> caseStatements,
         Set<K> seen = new HashSet<>();
         for (CaseStatement caseStatement : caseStatements) {
             K key = keyFn.apply(caseStatement);
-            if (key == null) return ArmGroup.skip();
-            if (!seen.add(key)) {
-                addError("Duplicate case label: " + key, caseStatement);
-                return ArmGroup.error();
-            }
+            if (key == null || !seen.add(key)) return ArmGroup.skip();
             keys.add(key);
         }
 
@@ -396,35 +392,25 @@ private void jumpIfNull(final int selectorIndex, final ClassNode selectorType, f
         controller.getMethodVisitor().visitJumpInsn(IFNULL, target);
     }
 
-    private void addError(final String message, final CaseStatement caseStatement) {
-        controller.getSourceUnit().addError(new SyntaxException(message, caseStatement));
-    }
-
     /**
-     * {@code keys == null && !error} means "not a constant switch, try the next
-     * optimizer". {@code error} means a compile error was already reported.
+     * {@code keys == null} means "not an optimizable constant switch, try the
+     * next optimizer (or fall back to sequential dispatch)".
      */
     private static final class ArmGroup<K> {
         final List<K> keys;
         final List<Label> targets;
-        final boolean error;
 
-        private ArmGroup(final List<K> keys, final List<Label> targets, final boolean error) {
+        private ArmGroup(final List<K> keys, final List<Label> targets) {
             this.keys = keys;
             this.targets = targets;
-            this.error = error;
         }
 
         static <K> ArmGroup<K> skip() {
-            return new ArmGroup<>(null, null, false);
-        }
-
-        static <K> ArmGroup<K> error() {
-            return new ArmGroup<>(null, null, true);
+            return new ArmGroup<>(null, null);
         }
 
         static <K> ArmGroup<K> of(final List<K> keys, final List<Label> targets) {
-            return new ArmGroup<>(keys, targets, false);
+            return new ArmGroup<>(keys, targets);
         }
     }
 }

src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java

@@ -154,8 +154,12 @@
 
 import static org.apache.groovy.ast.tools.ClassNodeUtils.getNestHost;
 import static org.apache.groovy.ast.tools.MethodNodeUtils.withDefaultArgumentMethods;
+import static org.apache.groovy.ast.tools.SwitchExpressionUtils.enumConstantName;
+import static org.apache.groovy.ast.tools.SwitchExpressionUtils.intConstant;
 import static org.apache.groovy.ast.tools.SwitchExpressionUtils.isIntegralType;
 import static org.apache.groovy.ast.tools.SwitchExpressionUtils.isOptimizedIntSwitch;
+import static org.apache.groovy.ast.tools.SwitchExpressionUtils.stringConstant;
+import static org.apache.groovy.ast.tools.SwitchExpressionUtils.unwrapEnumType;
 import static org.apache.groovy.util.BeanUtils.capitalize;
 import static org.apache.groovy.util.BeanUtils.decapitalize;
 import static org.codehaus.groovy.ast.ClassHelper.AUTOCLOSEABLE_TYPE;
@@ -4928,6 +4932,7 @@ public void visitSwitchExpression(final SwitchExpression expression) {
             expression.setType(resultType);
 
             typeCheckSwitchExpressionIsCase(expression);
+            checkSwitchExpressionDuplicateLabels(expression);
             checkSwitchExpressionExhaustiveness(expression);
         } finally {
             typeCheckingContext.popTemporaryTypeInfo();
@@ -4981,6 +4986,38 @@ && isOptimizedIntSwitch(selectorType, expression.getCaseStatements())) {
         }
     }
 
+    /**
+     * Reports a repeated constant case label in a switch expression. Sequential
+     * {@code isCase} semantics make the second arm dead code, and the optimized
+     * {@code tableswitch}/{@code lookupswitch} forms cannot represent it at all,
+     * so it is rejected here, uniformly for type-checked and statically-compiled
+     * code (GROOVY-12289). Labels compared are the same ones the optimizers key
+     * on: int-family, String and enum constants; anything else (GStrings, calls,
+     * regex or collection labels) cannot be proven duplicated statically and is
+     * left to sequential first-match-wins dispatch.
+     *
+     * @since 6.0.0
+     */
+    private void checkSwitchExpressionDuplicateLabels(final SwitchExpression expression) {
+        ClassNode enumType = unwrapEnumType(getType(expression.getExpression()));
+        Set<Object> seen = new HashSet<>();
+        for (CaseStatement caseStatement : expression.getCaseStatements()) {
+            Expression label = caseStatement.getExpression();
+            Object key = null;
+            if (enumType != null && enumType.isEnum()) {
+                String name = enumConstantName(label, enumType);
+                // keyed by type and name so an enum constant never collides with
+                // a string label of the same spelling (a distinct, legal label)
+                if (name != null) key = Map.entry(enumType, name);
+            }
+            if (key == null) key = intConstant(label);
+            if (key == null) key = stringConstant(label);
+            if (key != null && !seen.add(key)) {
+                addStaticTypeError("Duplicate case label: " + label.getText(), label);
+            }
+        }
+    }
+
     /**
      * {@inheritDoc}
      */

src/test-resources/groovy/transform/stc/Groovy12289Extension.groovy

@@ -0,0 +1,26 @@
+/*
+ *  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.
+ */
+// GROOVY-12289: a DSL that wants dynamic first-match-wins semantics for
+// duplicate constant case labels can opt affected methods out of type
+// checking; they then compile and dispatch as dynamic Groovy.
+beforeVisitMethod { mn ->
+    if (mn.name.startsWith('dsl')) {
+        handled = true
+    }
+}

src/test/groovy/groovy/transform/stc/TypeCheckingExtensionsTest.groovy

@@ -656,4 +656,27 @@ final class TypeCheckingExtensionsTest extends StaticTypeCheckingTestCase {
             assert m(1) == 10
         '''
     }
+
+    @Test // GROOVY-12289
+    void testSwitchExpressionDuplicateLabelEscapeHatch() {
+        String dsl = '''
+            def dsl(int x) {
+                def r = switch (x) {
+                    case 1 -> 'a'
+                    case 1 -> 'b'
+                    default -> 'c'
+                }
+                r
+            }
+        '''
+        shouldFailWithMessages dsl, 'Duplicate case label: 1'
+
+        // an extension that opts DSL-style methods out of type checking
+        // restores dynamic first-match-wins semantics for them
+        extension = 'groovy/transform/stc/Groovy12289Extension.groovy'
+        assertScript dsl + '''
+            assert dsl(1) == 'a'
+            assert dsl(9) == 'c'
+        '''
+    }
 }

src/test/groovy/org/codehaus/groovy/classgen/Jep361SwitchExpressionTest.groovy

@@ -601,4 +601,85 @@ final class Jep361SwitchExpressionTest {
             }
         ''', 'cannot be used together'
     }
+
+    //--------------------------------------------------------------------------
+    // Duplicate case labels (GROOVY-12289)
+
+    @Test
+    void duplicateConstantLabelDynamicFirstMatchWins() {
+        assertScript '''
+            def m(x) { def r = switch (x) { case 1 -> 'a'; case 1 -> 'b'; default -> 'c' }; r }
+            assert m(1) == 'a'
+            assert m(9) == 'c'
+        '''
+    }
+
+    @Test
+    void duplicateConstantLabelIsTypeCheckingError() {
+        for (mode in ['@groovy.transform.TypeChecked', '@groovy.transform.CompileStatic']) {
+            for (dup in [
+                    [decl: 'int x',    arms: "case 1 -> 'a'; case 1 -> 'b'",         label: '1'],
+                    [decl: 'String x', arms: 'case "a" -> 1; case "a" -> 2',         label: 'a'],
+                    [decl: 'int x',    arms: "case 1 -> 'a'; case 1 -> 'b'; case f() -> 'd'", label: '1']]) {
+                def err = shouldFail MultipleCompilationErrorsException, """
+                    class C {
+                        def f() { 5 }
+                        $mode
+                        def m(${dup.decl}) { def r = switch (x) { ${dup.arms}; default -> 'z' }; r }
+                    }
+                """
+                assert err.message.contains('[Static type checking] - Duplicate case label: ' + dup.label)
+            }
+        }
+    }
+
+    @Test
+    void duplicateEnumLabelIsTypeCheckingError() {
+        for (mode in ['@groovy.transform.TypeChecked', '@groovy.transform.CompileStatic']) {
+            def err = shouldFail MultipleCompilationErrorsException, """
+                enum E { X, Y }
+                class C {
+                    $mode
+                    def m(E e) { def r = switch (e) { case E.X -> 1; case E.X -> 2; default -> 0 }; r }
+                }
+            """
+            assert err.message.contains('Duplicate case label: E.X')
+        }
+    }
+
+    @Test
+    void duplicateNonConstantLabelIsNotAnError() {
+        assertBoth '''
+            def m(String x, String y) {
+                def r = switch (x) { case "${y}" -> 1; case "${y}" -> 2; default -> 0 }
+                r
+            }
+            assert m('a', 'a') == 1
+            assert m('b', 'a') == 0
+        '''
+    }
+
+    // an enum constant and a string label with the same spelling are distinct
+    // labels, not duplicates (the string arm simply never matches an enum)
+    @Test
+    void enumAndStringLabelsWithSameNameAreNotDuplicates() {
+        assertBoth '''
+            enum E { X, Y }
+            def m(E e) {
+                def r = switch (e) { case E.X -> 1; case 'X' -> 2; default -> 0 }
+                r
+            }
+            assert m(E.X) == 1
+            assert m(E.Y) == 0
+        '''
+    }
+
+    @Test
+    void duplicateLabelInSwitchStatementIsNotAnError() {
+        assertBoth '''
+            def m(int x) { switch (x) { case 1: return 'a'; case 1: return 'b'; default: return 'c' } }
+            assert m(1) == 'a'
+            assert m(9) == 'c'
+        '''
+    }
 }

src/test/groovy/org/codehaus/groovy/classgen/asm/sc/SwitchExpressionStaticCompileTest.groovy

@@ -581,6 +581,30 @@ final class SwitchExpressionStaticCompileTest extends AbstractBytecodeTestCase {
                 }
             }
         '''
-        assert err.message.contains('Duplicate case label')
+        // GROOVY-12289: reported by the type checker, so @TypeChecked fails identically
+        assert err.message.contains('[Static type checking] - Duplicate case label: 1')
+    }
+
+    // GROOVY-12289: with type checking bypassed, the optimizers cannot represent
+    // the duplicate key, so the writer falls back to sequential dispatch with
+    // dynamic first-match-wins semantics instead of reporting a late error.
+    @Test
+    void staticSkipModeDuplicateCaseFallsBackToSequentialDispatch() {
+        assertScript '''
+            @groovy.transform.CompileStatic
+            class C {
+                @groovy.transform.CompileStatic(groovy.transform.TypeCheckingMode.SKIP)
+                def m(int x) {
+                    def r = switch (x) {
+                        case 1 -> 'a'
+                        case 1 -> 'b'
+                        default -> 'c'
+                    }
+                    r
+                }
+            }
+            assert new C().m(1) == 'a'
+            assert new C().m(9) == 'c'
+        '''
     }
 }