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 b4430a2751a..a66f73ce205 100644 --- 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 disallowedVarUsages; + private final Map varUsages; private ImmutableVarUsageScanner(ImmutableSet 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 index 9ee5ee7c9df..1174b99765a 100644 --- 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 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 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 MY_LIST = ImmutableList.of(\"hello\");", + "}") + .expectUnchanged() + .doTest(); + } }