Skip to content

Commit

Permalink
Make the logic of detecting at least one allowed usage more explicit.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
java-team-github-bot authored and Error Prone Team committed Apr 30, 2024
1 parent fd9b826 commit c8df502
Showing 1 changed file with 28 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
}
}

0 comments on commit c8df502

Please sign in to comment.