[IfChainToSwitch] add unit tests for handling of duplicated patterns

PiperOrigin-RevId: 875389833

Author: markhbrady

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

@@ -21,14 +21,17 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.bugpatterns.SwitchUtils.COMPILE_TIME_CONSTANT_MATCHER;
+import static com.google.errorprone.bugpatterns.SwitchUtils.getReferencedLocalVariablesInTree;
 import static com.google.errorprone.bugpatterns.SwitchUtils.isEnumValue;
 import static com.google.errorprone.bugpatterns.SwitchUtils.renderComments;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.constValue;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
 import static com.google.errorprone.util.ASTHelpers.isSubtype;
 import static com.google.errorprone.util.ASTHelpers.sameVariable;
+import static com.google.errorprone.util.ASTHelpers.stripParentheses;
 import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT;
 import static com.sun.source.tree.Tree.Kind.THROW;
 import static java.lang.Math.max;
@@ -416,6 +419,28 @@ private static boolean switchOnNullWouldImplicitlyThrow(
         && cases.stream().noneMatch(CaseIr::hasCaseNull);
   }
 
+  /**
+   * Determines whether the given case IR has an "unguarded" pattern. As defined in the JLS,
+   * "unguarded" means that either there is no guard or the guard is the boolean literal `true`.
+   */
+  private static boolean isUnguarded(CaseIr caseIr) {
+    // Not a pattern
+    if (caseIr.instanceOfOptional().isEmpty()) {
+      return false;
+    }
+
+    if (caseIr.guardOptional().isEmpty()) {
+      return true;
+    }
+
+    // Guard is present and is `true`
+    ExpressionTree guard = stripParentheses(caseIr.guardOptional().get());
+    return isBooleanLiteral(guard)
+        && guard instanceof LiteralTree literalTree
+        && literalTree.getValue() instanceof Boolean b
+        && b;
+  }
+
   /**
    * Analyzes the supplied case IRs for a switch statement for issues related default/unconditional
    * cases. If deemed necessary, this method injects a `default` and/or `case null` into the
@@ -450,7 +475,7 @@ private Optional<List<CaseIr>> maybeFixDefaultNullAndUnconditional(
             .filter(
                 caseIr ->
                     caseIr.instanceOfOptional().isPresent()
-                        && caseIr.guardOptional().isEmpty()
+                        && isUnguarded(caseIr)
                         && isSubtype(
                             getType(subject),
                             getType(caseIr.instanceOfOptional().get().type()),
@@ -674,12 +699,13 @@ && switchOnNullWouldImplicitlyThrow(
   }
 
   /**
-   * Analyzes the supplied case IRs for duplicate constants (either primitives or enum values). If
+   * Analyzes the supplied case IRs for duplicate constants (primitives, enum values, or `null`). If
    * any duplicates are found, returns {@code Optional.empty()}.
    */
   private static Optional<List<CaseIr>> maybeDetectDuplicateConstants(List<CaseIr> cases) {
 
     Set<Object> seenConstants = new HashSet<>();
+    boolean seenNull = false;
 
     for (CaseIr caseIr : cases) {
       if (caseIr.expressionsOptional().isPresent()) {
@@ -701,6 +727,13 @@ private static Optional<List<CaseIr>> maybeDetectDuplicateConstants(List<CaseIr>
             }
             seenConstants.add(sym);
           }
+
+          if (isNull(expression)) {
+            if (seenNull) {
+              return Optional.empty();
+            }
+            seenNull = true;
+          }
         }
       }
     }
@@ -987,6 +1020,12 @@ private Optional<ExpressionTree> validatePredicateForSubject(
               && !b) {
             return Optional.empty();
           }
+          // A guard cannot reference a local variable that is not final nor effectively final;
+          // see JLS 21 §14.11.1.
+          if (getReferencedLocalVariablesInTree(rightOperandNoParentheses).stream()
+              .anyMatch(varSymbol -> !isConsideredFinal(varSymbol))) {
+            return Optional.empty();
+          }
           // Update last case to attach the guard
           cases.set(
               currentCasesSize,
@@ -1657,7 +1696,7 @@ public static boolean isDominatedBy(
         boolean isPrimitive = getType(constantExpression).isPrimitive();
         if (isPrimitive) {
           // Guarded patterns cannot dominate primitives
-          if (lhs.guardOptional().isPresent()) {
+          if (!isUnguarded(lhs)) {
             continue;
           }
           if (lhs.instanceOfOptional().isPresent()) {
@@ -1689,7 +1728,7 @@ public static boolean isDominatedBy(
         }
         boolean isEnum = isEnumValue(constantExpression, state);
         if (isEnum) {
-          if (lhs.guardOptional().isPresent()) {
+          if (!isUnguarded(lhs)) {
             // Guarded patterns cannot dominate enum values
             continue;
           }
@@ -1710,7 +1749,7 @@ public static boolean isDominatedBy(
         // RHS must be a reference
         // The rhs-reference code would be needed to support e.g. String literals.  It is included
         // for completeness.
-        if (lhs.guardOptional().isPresent()) {
+        if (!isUnguarded(lhs)) {
           // Guarded patterns cannot dominate references
           continue;
         }
@@ -1741,7 +1780,7 @@ public static boolean isDominatedBy(
     }
 
     // RHS must be a pattern
-    if (lhs.guardOptional().isPresent()) {
+    if (!isUnguarded(lhs)) {
       // LHS has a guard, so cannot dominate RHS
       return false;
     }

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

@@ -21,6 +21,7 @@
 import static com.google.common.collect.Iterables.getLast;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.bugpatterns.SwitchUtils.getReferencedLocalVariablesInTree;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
@@ -557,28 +558,7 @@ private void handle(Tree tree) {
    * supplied {@code tree}.
    */
   private static boolean hasReadsOrWritesOfVariableInTree(VarSymbol symbol, Tree tree) {
-    Set<VarSymbol> referencedLocalVariables = new HashSet<>();
-    new TreeScanner<Void, Void>() {
-      @Override
-      public Void visitMemberSelect(MemberSelectTree memberSelect, Void unused) {
-        handle(memberSelect);
-        return super.visitMemberSelect(memberSelect, null);
-      }
-
-      @Override
-      public Void visitIdentifier(IdentifierTree identifier, Void unused) {
-        handle(identifier);
-        return super.visitIdentifier(identifier, null);
-      }
-
-      private void handle(Tree tree) {
-        var symbol = getSymbol(tree);
-        if (symbol instanceof VarSymbol varSymbol) {
-          referencedLocalVariables.add(varSymbol);
-        }
-      }
-    }.scan(tree, null);
-    return referencedLocalVariables.contains(symbol);
+    return getReferencedLocalVariablesInTree(tree).contains(symbol);
   }
 
   /**

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

@@ -442,5 +442,30 @@ static String renderNullDefaultKindPrefix(
     };
   }
 
+  public static Set<VarSymbol> getReferencedLocalVariablesInTree(Tree tree) {
+    Set<VarSymbol> referencedLocalVariables = new HashSet<>();
+    new TreeScanner<Void, Void>() {
+      @Override
+      public Void visitMemberSelect(MemberSelectTree memberSelect, Void unused) {
+        handle(memberSelect);
+        return super.visitMemberSelect(memberSelect, null);
+      }
+
+      @Override
+      public Void visitIdentifier(IdentifierTree identifier, Void unused) {
+        handle(identifier);
+        return super.visitIdentifier(identifier, null);
+      }
+
+      private void handle(Tree tree) {
+        var symbol = getSymbol(tree);
+        if (symbol instanceof VarSymbol varSymbol) {
+          referencedLocalVariables.add(varSymbol);
+        }
+      }
+    }.scan(tree, null);
+    return referencedLocalVariables;
+  }
+
   private SwitchUtils() {}
 }