Skip to content

Commit

Permalink
Rip out GuardedBy flag.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 716286798
  • Loading branch information
graememorgan authored and Error Prone Team committed Jan 16, 2025
1 parent 7431230 commit 42a3701
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

import com.google.common.base.Joiner;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
Expand Down Expand Up @@ -64,8 +63,8 @@ public class GuardedByChecker extends BugChecker
private final GuardedByFlags flags;

@Inject
GuardedByChecker(ErrorProneFlags flags) {
this.flags = GuardedByFlags.fromFlags(flags);
GuardedByChecker() {
this.flags = GuardedByFlags.allOn();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,17 @@
package com.google.errorprone.bugpatterns.threadsafety;

import com.google.auto.value.AutoValue;
import com.google.errorprone.ErrorProneFlags;

/**
* Flags that control the behavior of threadsafety utils to facilitate rolling out new
* functionality.
*
* <p>This has no flags for now, but is still plumbed through to make it easier to flag guard
* changes to {@link GuardedByChecker} in the future. Otherwise, it's rather difficult.
* <p>This may have no flags, but is still plumbed through to make it easier to flag guard changes
* to {@link GuardedByChecker} in the future. Otherwise, it's rather difficult.
*/
@AutoValue
public abstract class GuardedByFlags {
public abstract boolean restrictToErrorProneGuardedBy();

public static GuardedByFlags allOn() {
return new AutoValue_GuardedByFlags(true);
}

public static GuardedByFlags fromFlags(ErrorProneFlags flags) {
return new AutoValue_GuardedByFlags(
flags.getBoolean("GuardedBy:RestrictToErrorProneGuardedBy").orElse(true));
return new AutoValue_GuardedByFlags();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,13 @@
* @author [email protected] (Liam Miller-Cushon)
*/
public final class GuardedByUtils {
public static ImmutableSet<String> getGuardValues(Symbol sym, GuardedByFlags flags) {
public static ImmutableSet<String> getGuardValues(Symbol sym) {
List<Attribute.Compound> rawAttributes = sym.getRawAttributes();
if (rawAttributes.isEmpty()) {
return ImmutableSet.of();
}
return rawAttributes.stream()
.filter(
a ->
flags.restrictToErrorProneGuardedBy()
? a.type.tsym.flatName().contentEquals(GUARDED_BY)
: a.getAnnotationType().asElement().getSimpleName().contentEquals("GuardedBy"))
.filter(a -> a.type.tsym.flatName().contentEquals(GUARDED_BY))
.flatMap(
a ->
MoreAnnotations.getValue(a, "value")
Expand All @@ -62,9 +58,9 @@ public static ImmutableSet<String> getGuardValues(Symbol sym, GuardedByFlags fla
.collect(toImmutableSet());
}

static ImmutableSet<String> getGuardValues(Tree tree, GuardedByFlags flags) {
static ImmutableSet<String> getGuardValues(Tree tree) {
Symbol sym = getSymbol(tree);
return sym == null ? ImmutableSet.of() : getGuardValues(sym, flags);
return sym == null ? ImmutableSet.of() : getGuardValues(sym);
}

private static final String GUARDED_BY = "com.google.errorprone.annotations.concurrent.GuardedBy";
Expand Down Expand Up @@ -107,7 +103,7 @@ static GuardedByValidationResult ok() {

public static GuardedByValidationResult isGuardedByValid(
Tree tree, VisitorState state, GuardedByFlags flags) {
ImmutableSet<String> guards = GuardedByUtils.getGuardValues(tree, flags);
ImmutableSet<String> guards = GuardedByUtils.getGuardValues(tree);
if (guards.isEmpty()) {
return GuardedByValidationResult.ok();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public Void visitMethod(MethodTree tree, HeldLockSet locks) {

// @GuardedBy annotations on methods are trusted for declarations, and checked
// for invocations.
for (String guard : GuardedByUtils.getGuardValues(tree, flags)) {
for (String guard : GuardedByUtils.getGuardValues(tree)) {
Optional<GuardedByExpression> bound =
GuardedByBinder.bindString(
guard, GuardedBySymbolResolver.from(tree, visitorState), flags);
Expand Down Expand Up @@ -270,7 +270,7 @@ public Void visitClass(ClassTree node, HeldLockSet locks) {
}

private void checkMatch(ExpressionTree tree, HeldLockSet locks) {
for (String guardString : GuardedByUtils.getGuardValues(tree, flags)) {
for (String guardString : GuardedByUtils.getGuardValues(tree)) {
Optional<GuardedByExpression> guard =
GuardedByBinder.bindString(
guardString,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,12 @@ public class ThreadSafeAnalysis {
private final VisitorState state;
private final WellKnownThreadSafety wellKnownThreadSafety;
private final ThreadSafety threadSafety;
private final GuardedByFlags flags;

public ThreadSafeAnalysis(
BugChecker bugChecker,
VisitorState state,
WellKnownThreadSafety wellKnownThreadSafety,
GuardedByFlags flags) {
BugChecker bugChecker, VisitorState state, WellKnownThreadSafety wellKnownThreadSafety) {
this.bugChecker = bugChecker;
this.state = state;
this.wellKnownThreadSafety = wellKnownThreadSafety;
this.flags = flags;

this.threadSafety = ThreadSafety.threadSafeBuilder(wellKnownThreadSafety).build(state);
}
Expand Down Expand Up @@ -227,7 +222,7 @@ private Violation isFieldThreadSafe(
if (var.getModifiers().contains(Modifier.STATIC)) {
return Violation.absent();
}
if (!GuardedByUtils.getGuardValues(var, flags).isEmpty()) {
if (!GuardedByUtils.getGuardValues(var).isEmpty()) {
return Violation.absent();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
Expand Down Expand Up @@ -77,12 +76,10 @@ public class ThreadSafeChecker extends BugChecker
MemberReferenceTreeMatcher {

private final WellKnownThreadSafety wellKnownThreadSafety;
private final GuardedByFlags flags;

@Inject
ThreadSafeChecker(WellKnownThreadSafety wellKnownThreadSafety, ErrorProneFlags flags) {
ThreadSafeChecker(WellKnownThreadSafety wellKnownThreadSafety) {
this.wellKnownThreadSafety = wellKnownThreadSafety;
this.flags = GuardedByFlags.fromFlags(flags);
}

// check instantiations of `@ThreadSafe`s in method references
Expand All @@ -105,7 +102,7 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
checkInvocation(
tree, ((JCNewClass) tree).constructorType, state, ((JCNewClass) tree).constructor);
// check instantiations of `@ThreadSafeTypeParameter`s in class constructor invocations
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety, flags);
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety);
Violation info =
analysis.checkInstantiation(
getSymbol(tree.getIdentifier()).getTypeParameters(), getType(tree).getTypeArguments());
Expand All @@ -116,7 +113,7 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
}

private void checkInvocation(Tree tree, Type methodType, VisitorState state, Symbol symbol) {
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety, flags);
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety);
Violation info = analysis.checkInvocation(methodType, symbol);
if (info.isPresent()) {
state.reportMatch(buildDescription(tree).setMessage(info.message()).build());
Expand All @@ -135,7 +132,7 @@ public Description matchTypeParameter(TypeParameterTree tree, VisitorState state
}
default -> {}
}
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety, flags);
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety);
if (analysis.hasThreadSafeTypeParameterAnnotation((TypeVariableSymbol) sym)) {
if (analysis.getThreadSafeAnnotation(sym.owner, state) == null) {
return buildDescription(tree)
Expand All @@ -155,7 +152,7 @@ public Description matchTypeParameter(TypeParameterTree tree, VisitorState state

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety, flags);
ThreadSafeAnalysis analysis = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety);
if (tree.getSimpleName().length() == 0) {
// anonymous classes have empty names
// TODO(cushon): once Java 8 happens, require @ThreadSafe on anonymous classes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import com.google.errorprone.BugPattern;
import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.Tree;
Expand Down Expand Up @@ -256,9 +255,7 @@ void m() {
@BugPattern(name = "GuardedByLockSet", summary = "", explanation = "", severity = ERROR)
public static class GuardedByLockSetAnalyzer extends GuardedByChecker {
@Inject
GuardedByLockSetAnalyzer(ErrorProneFlags flags) {
super(flags);
}
GuardedByLockSetAnalyzer() {}

@Override
protected Description checkGuardedAccess(
Expand Down

0 comments on commit 42a3701

Please sign in to comment.