Don't suggest ImmutableSet if ImmutableList is unused.

PiperOrigin-RevId: 624898303
google · Apr 15, 2024 · 53d787c · 53d787c
1 parent 23547ac
commit 53d787c
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 7 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ImmutableSetForContains.java b/core/src/main/java/com/google/errorprone/bugpatterns/ImmutableSetForContains.java
@@ -19,6 +19,9 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+import static com.google.errorprone.bugpatterns.ImmutableSetForContains.UsageState.ALLOWED_USAGE;
+import static com.google.errorprone.bugpatterns.ImmutableSetForContains.UsageState.DISALLOWED_USAGE;
+import static com.google.errorprone.bugpatterns.ImmutableSetForContains.UsageState.NEVER_USED;
 import static com.google.errorprone.matchers.Matchers.allOf;
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.hasAnnotationWithSimpleName;
@@ -122,7 +125,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
       if (isSuppressed(var, state)) {
         continue;
       }
-      if (!usageScanner.disallowedVarUsages.get(getSymbol(var))) {
+      if (usageScanner.varUsages.get(getSymbol(var)) == UsageState.ALLOWED_USAGE) {
         firstReplacement = Optional.of(var);
         fix.merge(convertListToSetInit(var, state));
       }
@@ -195,13 +198,13 @@ private static final class ImmutableVarUsageScanner extends TreeScanner<Void, Vi
 
     // For each ImmutableList usage var, we will keep a map indicating if the var has been used in a
     // way that would prevent us from making it a set.
-    private final Map<Symbol, Boolean> disallowedVarUsages;
+    private final Map<Symbol, UsageState> varUsages;
 
     private ImmutableVarUsageScanner(ImmutableSet<VariableTree> immutableListVar) {
-      this.disallowedVarUsages =
+      this.varUsages =
           immutableListVar.stream()
               .map(ASTHelpers::getSymbol)
-              .collect(toMap(x -> x, x -> Boolean.FALSE));
+              .collect(toMap(x -> x, x -> NEVER_USED));
     }
 
     @Override
@@ -220,11 +223,15 @@ private boolean allowedFuncOnImmutableVar(MethodInvocationTree methodTree, Visit
       if (receiver == null) {
         return false;
       }
-      if (!disallowedVarUsages.containsKey(getSymbol(receiver))) {
+      if (!varUsages.containsKey(getSymbol(receiver))) {
         // Not a function invocation on any of the candidate immutable list vars.
         return false;
       }
-      return ALLOWED_FUNCTIONS_ON_LIST.matches(methodTree, state);
+      boolean allowed = ALLOWED_FUNCTIONS_ON_LIST.matches(methodTree, state);
+      if (allowed && (varUsages.get(getSymbol(receiver)) == NEVER_USED)) {
+        varUsages.put(getSymbol(receiver), ALLOWED_USAGE);
+      }
+      return allowed;
     }
 
     @Override
@@ -240,7 +247,13 @@ public Void visitMemberSelect(MemberSelectTree memberSelectTree, VisitorState vi
     }
 
     private void recordDisallowedUsage(Symbol symbol) {
-      disallowedVarUsages.computeIfPresent(symbol, (sym, oldVal) -> Boolean.TRUE);
+      varUsages.computeIfPresent(symbol, (sym, oldVal) -> DISALLOWED_USAGE);
     }
   }
+
+  enum UsageState {
+    NEVER_USED,
+    ALLOWED_USAGE,
+    DISALLOWED_USAGE
+  }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ImmutableSetForContainsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ImmutableSetForContainsTest.java
@@ -401,4 +401,53 @@ public void suppressionOnVariableTree_noFinding() {
         .expectUnchanged()
         .doTest();
   }
+
+  @Test
+  public void bothGetDisallowedAndContainsAllowed_noFinding() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import com.google.common.collect.ImmutableList;",
+            "class Test {",
+            "  private static final ImmutableList<String> MY_LIST_1 =",
+            "           ImmutableList.of(\"hello\");",
+            "  private void myFunc() {",
+            "    String myString = MY_LIST_1.get(0);",
+            "    boolean myBool1 = MY_LIST_1.contains(\"he\");",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void bothContainsAllowedAndGetDisallowed_noFinding() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import com.google.common.collect.ImmutableList;",
+            "class Test {",
+            "  private static final ImmutableList<String> MY_LIST_1 =",
+            "           ImmutableList.of(\"hello\");",
+            "  private void myFunc() {",
+            "    boolean myBool1 = MY_LIST_1.contains(\"he\");",
+            "    String myString = MY_LIST_1.get(0);",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void unusedVariable_noFinding() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import com.google.common.collect.ImmutableList;",
+            "class Test {",
+            "  private static final ImmutableList<String> MY_LIST = ImmutableList.of(\"hello\");",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
 }