[IfChainToSwitch] improve detection of duplicate constant values and invalid switch expression types

PiperOrigin-RevId: 938260967

Author: markhbrady

core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java

@@ -90,6 +90,7 @@
 import java.util.stream.Stream;
 import javax.inject.Inject;
 import javax.lang.model.element.ElementKind;
+import javax.lang.model.type.TypeKind;
 import org.jspecify.annotations.Nullable;
 
 /** Checks for chains of if statements that may be converted to a switch. */
@@ -113,6 +114,9 @@ public final class IfChainToSwitch extends BugChecker implements IfTreeMatcher {
           "java.lang.Integer",
           "java.lang.String");
 
+  // Non-reference types that are allowed for switch subjects, as specified in JLS 21 §14.11.
+  private static final ImmutableSet<TypeKind> ALLOWED_SWITCH_SUBJECT_NON_REFERENCE_TYPES =
+      ImmutableSet.of(TypeKind.CHAR, TypeKind.BYTE, TypeKind.SHORT, TypeKind.INT);
   private final boolean enableMain;
   private final boolean enableSafe;
   private final int maxChainLength;
@@ -729,6 +733,18 @@ private static Optional<List<CaseIr>> maybeDetectDuplicateConstants(List<CaseIr>
           // Check for compile-time constants
           Object constant = constValue(expression);
           if (constant != null) {
+            // Canonicialize constants (by widening) to detect identical values across various
+            // (boxed) primitive types
+            constant =
+                switch (constant) {
+                  case Float f -> (double) f;
+                  case Double d -> d;
+                  case Number n -> n.longValue(); // primitive integer type
+                  case Boolean bool -> bool;
+                  case Character c -> (long) c;
+                  default -> constant;
+                };
+
             if (seenConstants.contains(constant)) {
               return Optional.empty();
             }
@@ -1412,6 +1428,20 @@ private Optional<ExpressionTree> validateCompileTimeConstantForSubject(
       return Optional.empty();
     }
 
+    // The variable being switched on must also be one of the following types; this is
+    // an outer bound (relaxed in preview features, which this checker does not yet support)
+    if (subject.isPresent()) {
+      var switchExpressionType = getType(subject.get());
+      boolean validSwitchExpressionType =
+          switchExpressionType != null // Should never be null
+              && (switchExpressionType.isReference()
+                  || ALLOWED_SWITCH_SUBJECT_NON_REFERENCE_TYPES.contains(
+                      switchExpressionType.getKind()));
+      if (!validSwitchExpressionType) {
+        return Optional.empty();
+      }
+    }
+
     boolean addDefault = hasElse && !hasElseIf;
     int previousCaseEndPosition =
         cases.isEmpty()
@@ -1425,7 +1455,7 @@ private Optional<ExpressionTree> validateCompileTimeConstantForSubject(
             /* guardOptional= */ Optional.empty(),
             /* expressionsOptional= */ Optional.of(ImmutableList.of(compileTimeConstant)),
             /* arrowRhsOptional= */ arrowRhsOptional,
-            /* caseSourceCodeRange= */ Range.openClosed(previousCaseEndPosition, caseEndPosition)));
+            /* caseSourceCodeRange= */ Range.closedOpen(previousCaseEndPosition, caseEndPosition)));
     if (addDefault) {
       previousCaseEndPosition =
           cases.isEmpty()
@@ -1439,7 +1469,7 @@ private Optional<ExpressionTree> validateCompileTimeConstantForSubject(
               /* guardOptional= */ Optional.empty(),
               /* expressionsOptional= */ Optional.empty(),
               /* arrowRhsOptional= */ elseOptional,
-              /* caseSourceCodeRange= */ Range.openClosed(
+              /* caseSourceCodeRange= */ Range.closedOpen(
                   previousCaseEndPosition,
                   elseOptional.isPresent()
                       ? getStartPosition(elseOptional.get())

core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java

@@ -314,6 +314,32 @@ public void foo(Suit s) {
         .doTest();
   }
 
+  @Test
+  public void ifChain_longSwitchExpression_noError() {
+    // Similar to ifChain_longSwitch_noError, but constants are ints instead of longs
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            import java.lang.Number;
+
+            class Test {
+              public void foo(Suit s) {
+                long l = s == null ? 1 : 2;
+                if (l == 23) {
+                  System.out.println("It's 23");
+                } else if (l == 45) {
+                  System.out.println("It's 45");
+                } else if (l == 67) {
+                  System.out.println("It's 67");
+                }
+              }
+            }
+            """)
+        .setArgs(ENABLE_MAIN, MIN_CHAIN_LENGTH_3)
+        .doTest();
+  }
+
   @Test
   public void ifChain_intSwitch_error() {
     // Switch on int is supported
@@ -4440,6 +4466,54 @@ public void foo(Object o) {
         .doTest();
   }
 
+  @Test
+  public void ifChain_intDuplicateConstantDifferentTypes_fails() {
+    // Because 'a' is encoded as 97 (in ASCII), they are both the same (when cast to int)
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              public void foo(int x) {
+                if (x == 'a') {
+                  System.out.println("a");
+                } else if (x == 97) {
+                  System.out.println("97");
+                } else if (x == 98) {
+                  System.out.println("98");
+                }
+              }
+            }
+            """)
+        .expectUnchanged()
+        .setArgs(ENABLE_MAIN, MIN_CHAIN_LENGTH_3)
+        .doTest();
+  }
+
+  @Test
+  public void ifChain_shortDuplicateConstantDifferentTypes_fails() {
+    // Because 'a' is encoded as 97 (in ASCII), they are both the same (when cast to short)
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              public void foo(short x) {
+                if (x == 'a') {
+                  System.out.println("a");
+                } else if (x == 97) {
+                  System.out.println("97");
+                } else if (x == 98) {
+                  System.out.println("98");
+                }
+              }
+            }
+            """)
+        .expectUnchanged()
+        .setArgs(ENABLE_MAIN, MIN_CHAIN_LENGTH_3)
+        .doTest();
+  }
+
   /** Substitute underscore for {@code unused} variables, if supported. */
   private static String maybeChangeToUnnamedVariable(String s) {
     if (Runtime.version().feature() >= 22) {