Skip to content

Commit

Permalink
Support @GuardedBy annotations with multiple locks
Browse files Browse the repository at this point in the history 
e.g. `@GuardedBy({"this", "lock"}) int x;`

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=184066319
  • Loading branch information
@cushon @eaftan
cushon authored and eaftan committed Feb 1, 2018
1 parent 354a6c8 commit 2a6f04b
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 43 deletions.
63 changes: 41 additions & 22 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java
View file
Expand Up @@ -16,9 +16,12 @@

package com.google.errorprone.bugpatterns.threadsafety;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.errorprone.util.ASTHelpers.getSymbol;

import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.VisitorState;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Attribute;
Expand All @@ -27,35 +30,45 @@
import com.sun.tools.javac.parser.ParserFactory;
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.util.Context;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
import javax.lang.model.element.AnnotationValue;
import javax.lang.model.util.SimpleAnnotationValueVisitor8;

/** @author cushon@google.com (Liam Miller-Cushon) */
public class GuardedByUtils {
static String getGuardValue(Tree tree, VisitorState state) {
static ImmutableSet<String> getGuardValues(Tree tree, VisitorState state) {
Symbol sym = getSymbol(tree);
if (sym == null) {
return null;
}
return sym.getRawAttributes()
.stream()
.filter(a -> a.getAnnotationType().asElement().getSimpleName().contentEquals("GuardedBy"))
.findFirst()
.flatMap(
a -> Optional.ofNullable(a.member(state.getName("value"))).flatMap(v -> asString(v)))
.orElse(null);
.map(a -> a.member(state.getName("value")))
.filter(v -> v != null)
.flatMap(a -> asStrings(a))
.collect(toImmutableSet());
}

private static Optional<String> asString(Attribute v) {
return Optional.ofNullable(
private static Stream<String> asStrings(Attribute v) {
return MoreObjects.firstNonNull(
v.accept(
new SimpleAnnotationValueVisitor8<String, Void>() {
new SimpleAnnotationValueVisitor8<Stream<String>, Void>() {
@Override
public String visitString(String s, Void aVoid) {
return s;
public Stream<String> visitString(String s, Void aVoid) {
return Stream.of(s);
}

@Override
public Stream<String> visitArray(List<? extends AnnotationValue> list, Void aVoid) {
return list.stream().flatMap(a -> a.accept(this, null)).filter(x -> x != null);
}
},
null));
null),
Stream.empty());
}

public static JCTree.JCExpression parseString(String guardedByString, Context context) {
Expand Down Expand Up @@ -95,15 +108,19 @@ static GuardedByValidationResult ok() {
}

public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState state) {
String guard = GuardedByUtils.getGuardValue(tree, state);
if (guard == null) {
ImmutableSet<String> guards = GuardedByUtils.getGuardValues(tree, state);
if (guards.isEmpty()) {
return GuardedByValidationResult.ok();
}

Optional<GuardedByExpression> boundGuard =
GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, state));
if (!boundGuard.isPresent()) {
return GuardedByValidationResult.invalid("could not resolve guard");
List<GuardedByExpression> boundGuards = new ArrayList<>();
for (String guard : guards) {
Optional<GuardedByExpression> boundGuard =
GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, state));
if (!boundGuard.isPresent()) {
return GuardedByValidationResult.invalid("could not resolve guard");
}
boundGuards.add(boundGuard.get());
}

Symbol treeSym = getSymbol(tree);
Expand All @@ -112,11 +129,13 @@ public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState
return GuardedByValidationResult.ok();
}

boolean staticGuard =
boundGuard.get().kind() == GuardedByExpression.Kind.CLASS_LITERAL
|| (boundGuard.get().sym() != null && boundGuard.get().sym().isStatic());
if (treeSym.isStatic() && !staticGuard) {
return GuardedByValidationResult.invalid("static member guarded by instance");
for (GuardedByExpression boundGuard : boundGuards) {
boolean staticGuard =
boundGuard.kind() == GuardedByExpression.Kind.CLASS_LITERAL
|| (boundGuard.sym() != null && boundGuard.sym().isStatic());
if (treeSym.isStatic() && !staticGuard) {
return GuardedByValidationResult.invalid("static member guarded by instance");
}
}

return GuardedByValidationResult.ok();
Expand Down
37 changes: 16 additions & 21 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java
View file
Expand Up @@ -142,8 +142,7 @@ public Void visitMethod(MethodTree tree, HeldLockSet locks) {

// @GuardedBy annotations on methods are trusted for declarations, and checked
// for invocations.
String guard = GuardedByUtils.getGuardValue(tree, visitorState);
if (guard != null) {
for (String guard : GuardedByUtils.getGuardValues(tree, visitorState)) {
Optional<GuardedByExpression> bound =
GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState));
if (bound.isPresent()) {
Expand Down Expand Up @@ -221,26 +220,22 @@ public Void visitVariable(VariableTree node, HeldLockSet locks) {
}

private void checkMatch(ExpressionTree tree, HeldLockSet locks) {
String guardString = GuardedByUtils.getGuardValue(tree, visitorState);
if (guardString == null) {
return;
}

Optional<GuardedByExpression> guard =
GuardedByBinder.bindString(guardString, GuardedBySymbolResolver.from(tree, visitorState));
if (!guard.isPresent()) {
return;
}
Optional<GuardedByExpression> boundGuard =
ExpectedLockCalculator.from((JCTree.JCExpression) tree, guard.get(), visitorState);
if (!boundGuard.isPresent()) {
// We couldn't resolve a guarded by expression in the current scope, so we can't
// guarantee the access is protected and must report an error to be safe.
listener.handleGuardedAccess(
tree, new GuardedByExpression.Factory().error(guardString), locks);
return;
for (String guardString : GuardedByUtils.getGuardValues(tree, visitorState)) {
GuardedByBinder.bindString(guardString, GuardedBySymbolResolver.from(tree, visitorState))
.ifPresent(
guard -> {
Optional<GuardedByExpression> boundGuard =
ExpectedLockCalculator.from((JCTree.JCExpression) tree, guard, visitorState);
if (!boundGuard.isPresent()) {
// We couldn't resolve a guarded by expression in the current scope, so we can't
// guarantee the access is protected and must report an error to be safe.
listener.handleGuardedAccess(
tree, new GuardedByExpression.Factory().error(guardString), locks);
return;
}
listener.handleGuardedAccess(tree, boundGuard.get(), locks);
});
}
listener.handleGuardedAccess(tree, boundGuard.get(), locks);
}
}

Expand Down
32 changes: 32 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByCheckerTest.java
View file
Expand Up @@ -1556,4 +1556,36 @@ public void lambda() throws Exception {
"}")
.doTest();
}

@Test
public void multipleLocks() throws Exception {
compilationHelper
.addSourceLines(
"GuardedBy.java", //
"@interface GuardedBy {",
" String[] value() default {};",
"}")
.addSourceLines(
"Test.java",
"public class Test {",
" private final Object mu = new Object();",
" @GuardedBy({\"this\", \"mu\"}) int x;",
" void f() {",
" synchronized (this) {",
" synchronized (mu) {",
" x++;",
" }",
" }",
" synchronized (this) {",
" // BUG: Diagnostic contains: should be guarded by 'this.mu'",
" x++;",
" }",
" synchronized (mu) {",
" // BUG: Diagnostic contains: should be guarded by 'this'",
" x++;",
" }",
" }",
"}")
.doTest();
}
}

0 comments on commit 2a6f04b

Please sign in to comment.