From c8df502ab7cc8ce16b1a2e53533e7c247eba4a85 Mon Sep 17 00:00:00 2001
From: Error Prone Team <java-team-github-bot@google.com>
Date: Tue, 30 Apr 2024 02:08:32 -0700
Subject: [PATCH] Make the logic of detecting at least one allowed usage more
 explicit.

The condition `== UsageState.ALLOWED_USAGE` doesn't convey the intention to trigger the finding for allowed usage only if at the same time there is no disallowed usage.
I'm renaming the constant to `ONLY_ALLOWED_USAGE` and adding the state transition methods to make the logic more explicit.

PiperOrigin-RevId: 629340469
---
 .../bugpatterns/ImmutableSetForContains.java  | 35 +++++++++++++++----
 1 file changed, 28 insertions(+), 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
index a66f73ce205..86373e1ac72 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ImmutableSetForContains.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ImmutableSetForContains.java
@@ -19,9 +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.bugpatterns.ImmutableSetForContains.UsageState.ONLY_ALLOWED_USAGE;
 import static com.google.errorprone.matchers.Matchers.allOf;
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.hasAnnotationWithSimpleName;
@@ -125,7 +125,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
       if (isSuppressed(var, state)) {
         continue;
       }
-      if (usageScanner.varUsages.get(getSymbol(var)) == UsageState.ALLOWED_USAGE) {
+      if (usageScanner.varUsages.get(getSymbol(var)).shouldReport()) {
         firstReplacement = Optional.of(var);
         fix.merge(convertListToSetInit(var, state));
       }
@@ -228,8 +228,9 @@ private boolean allowedFuncOnImmutableVar(MethodInvocationTree methodTree, Visit
         return false;
       }
       boolean allowed = ALLOWED_FUNCTIONS_ON_LIST.matches(methodTree, state);
-      if (allowed && (varUsages.get(getSymbol(receiver)) == NEVER_USED)) {
-        varUsages.put(getSymbol(receiver), ALLOWED_USAGE);
+      if (allowed) {
+        varUsages.computeIfPresent(
+            getSymbol(receiver), (sym, oldVal) -> oldVal.markAllowedUsageDetected());
       }
       return allowed;
     }
@@ -247,13 +248,33 @@ public Void visitMemberSelect(MemberSelectTree memberSelectTree, VisitorState vi
     }
 
     private void recordDisallowedUsage(Symbol symbol) {
-      varUsages.computeIfPresent(symbol, (sym, oldVal) -> DISALLOWED_USAGE);
+      varUsages.computeIfPresent(symbol, (sym, oldVal) -> oldVal.markDisallowedUsageDetected());
     }
   }
 
   enum UsageState {
+    /** No usage detected. */
     NEVER_USED,
-    ALLOWED_USAGE,
-    DISALLOWED_USAGE
+
+    /** At least one allowed usage detected. No disallowed usage detected. */
+    ONLY_ALLOWED_USAGE,
+
+    /** Disallowed usage detected. Allowed usage is ignored. */
+    DISALLOWED_USAGE;
+
+    UsageState markAllowedUsageDetected() {
+      if (this == DISALLOWED_USAGE) {
+        return DISALLOWED_USAGE;
+      }
+      return ONLY_ALLOWED_USAGE;
+    }
+
+    UsageState markDisallowedUsageDetected() {
+      return DISALLOWED_USAGE;
+    }
+
+    boolean shouldReport() {
+      return this == ONLY_ALLOWED_USAGE;
+    }
   }
 }