From 2230bd17f2fdeba49182cfa0ae2b1a6cc90f6248 Mon Sep 17 00:00:00 2001 From: ghm Date: Thu, 28 Nov 2019 02:31:36 -0800 Subject: [PATCH] Add a flag to GuardedByChecker to allow matching ERRORs & member selects on methods. Plumb a flag through so that future improvements are possible. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=282908895 --- .../AbstractLockMethodChecker.java | 3 +- .../threadsafety/GuardedByBinder.java | 24 ++++--- .../threadsafety/GuardedByChecker.java | 20 ++++-- .../threadsafety/GuardedByFlags.java | 34 ++++++++++ .../threadsafety/GuardedByUtils.java | 10 +-- .../threadsafety/HeldLockAnalyzer.java | 68 ++++++++++++------- .../threadsafety/LockMethodChecker.java | 6 +- .../threadsafety/UnlockMethodChecker.java | 6 +- .../threadsafety/GuardedByBinderTest.java | 3 +- .../threadsafety/GuardedByCheckerTest.java | 25 ++++++- .../threadsafety/HeldLockAnalyzerTest.java | 5 ++ 11 files changed, 154 insertions(+), 50 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByFlags.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java index 6637a85aa6c..5782a45f596 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java @@ -103,7 +103,8 @@ private static Optional> parseLockExpressions( ImmutableSet.Builder builder = ImmutableSet.builder(); for (String lockExpression : lockExpressions) { Optional guard = - GuardedByBinder.bindString(lockExpression, GuardedBySymbolResolver.from(tree, state)); + GuardedByBinder.bindString( + lockExpression, GuardedBySymbolResolver.from(tree, state), GuardedByFlags.allOn()); if (!guard.isPresent()) { return Optional.empty(); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java index a101ca9bc0f..c797908c0f8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java @@ -48,7 +48,7 @@ public class GuardedByBinder { * Optional.empty()} if the AST node doesn't correspond to a 'simple' lock expression. */ public static Optional bindExpression( - JCTree.JCExpression exp, VisitorState visitorState) { + JCTree.JCExpression exp, VisitorState visitorState, GuardedByFlags flags) { try { return Optional.of( bind( @@ -57,14 +57,16 @@ public static Optional bindExpression( ALREADY_BOUND_RESOLVER, ASTHelpers.getSymbol(visitorState.findEnclosing(ClassTree.class)), visitorState.getTypes(), - visitorState.getNames()))); + visitorState.getNames(), + flags))); } catch (IllegalGuardedBy expected) { return Optional.empty(); } } /** Creates a {@link GuardedByExpression} from a string, given the resolution context. */ - static Optional bindString(String string, GuardedBySymbolResolver resolver) { + static Optional bindString( + String string, GuardedBySymbolResolver resolver, GuardedByFlags flags) { try { return Optional.of( bind( @@ -73,7 +75,8 @@ static Optional bindString(String string, GuardedBySymbolRe resolver, resolver.enclosingClass(), resolver.visitorState().getTypes(), - resolver.visitorState().getNames()))); + resolver.visitorState().getNames(), + flags))); } catch (IllegalGuardedBy expected) { return Optional.empty(); } @@ -84,17 +87,20 @@ private static class BinderContext { final ClassSymbol thisClass; final Types types; final Names names; + final GuardedByFlags flags; - public BinderContext(Resolver resolver, ClassSymbol thisClass, Types types, Names names) { + public BinderContext( + Resolver resolver, ClassSymbol thisClass, Types types, Names names, GuardedByFlags flags) { this.resolver = resolver; this.thisClass = thisClass; this.types = types; this.names = names; + this.flags = flags; } public static BinderContext of( - Resolver resolver, ClassSymbol thisClass, Types types, Names names) { - return new BinderContext(resolver, thisClass, types, names); + Resolver resolver, ClassSymbol thisClass, Types types, Names names, GuardedByFlags flags) { + return new BinderContext(resolver, thisClass, types, names, flags); } } @@ -221,9 +227,9 @@ public GuardedByExpression visitMemberSelect(MemberSelectTree node, BinderContex checkGuardedBy(base != null, "Bad expression: %s", node.getExpression()); Symbol sym = context.resolver.resolveSelect(base, node); checkGuardedBy(sym != null, "Could not resolve: %s", node); - // TODO(cushon): allow MethodSymbol here once clean-up is done + // TODO(b/144776758): remove flag once clean-up is done checkGuardedBy( - sym instanceof Symbol.VarSymbol /* || sym instanceof Symbol.MethodSymbol*/, + sym instanceof Symbol.VarSymbol || sym instanceof Symbol.MethodSymbol, "Bad member symbol: %s", sym.getClass()); return bindSelect(normalizeBase(context, sym, base), sym); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java index 0f6973668a6..f130defec6e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java @@ -21,6 +21,7 @@ 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; @@ -51,6 +52,12 @@ public class GuardedByChecker extends BugChecker private static final String JUC_READ_WRITE_LOCK = "java.util.concurrent.locks.ReadWriteLock"; + private final GuardedByFlags flags; + + public GuardedByChecker(ErrorProneFlags flags) { + this.flags = GuardedByFlags.of(flags.getBoolean("GuardedByChecker:MatchOnErrors").orElse(true)); + } + @Override public Description matchMethod(MethodTree tree, final VisitorState state) { // Constructors (and field initializers, instance initalizers, and class initalizers) are free @@ -69,12 +76,13 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState return NO_MATCH; } - private void analyze(final VisitorState state) { + private void analyze(VisitorState state) { HeldLockAnalyzer.analyze( state, (ExpressionTree tree, GuardedByExpression guard, HeldLockSet live) -> report(GuardedByChecker.this.checkGuardedAccess(tree, guard, live, state), state), - this::isSuppressed); + this::isSuppressed, + flags); } @Override @@ -108,8 +116,8 @@ protected Description checkGuardedAccess( return NO_MATCH; } - // TODO(cushon): re-enable once the clean-up is done - if (guard.kind() == GuardedByExpression.Kind.ERROR) { + // TODO(b/144776758): remove flag once the clean-up is done + if (!flags.matchMethodSymbols() && guard.kind() == GuardedByExpression.Kind.ERROR) { return NO_MATCH; } @@ -212,8 +220,8 @@ private void report(Description description, VisitorState state) { } /** Validates that {@code @GuardedBy} strings can be resolved. */ - Description validate(Tree tree, VisitorState state) { - GuardedByValidationResult result = GuardedByUtils.isGuardedByValid(tree, state); + private Description validate(Tree tree, VisitorState state) { + GuardedByValidationResult result = GuardedByUtils.isGuardedByValid(tree, state, flags); if (result.isValid()) { return Description.NO_MATCH; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByFlags.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByFlags.java new file mode 100644 index 00000000000..cb3b75777dd --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByFlags.java @@ -0,0 +1,34 @@ +/* + * Copyright 2019 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.errorprone.bugpatterns.threadsafety; + +import com.google.auto.value.AutoValue; + +/** + * Flags that control the behavior of threadsafety utils to facilitate rolling out new + * functionality. + */ +@AutoValue +public abstract class GuardedByFlags { + public abstract boolean matchMethodSymbols(); + + public static GuardedByFlags of(boolean matchMethodSymbols) { + return new AutoValue_GuardedByFlags(matchMethodSymbols); + } + + public static GuardedByFlags allOn() { + return of(true); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java index 1f32d6013a3..57a64c3f285 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java @@ -100,7 +100,8 @@ static GuardedByValidationResult ok() { } } - public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState state) { + public static GuardedByValidationResult isGuardedByValid( + Tree tree, VisitorState state, GuardedByFlags flags) { ImmutableSet guards = GuardedByUtils.getGuardValues(tree, state); if (guards.isEmpty()) { return GuardedByValidationResult.ok(); @@ -109,7 +110,7 @@ public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState List boundGuards = new ArrayList<>(); for (String guard : guards) { Optional boundGuard = - GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, state)); + GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, state), flags); if (!boundGuard.isPresent()) { return GuardedByValidationResult.invalid("could not resolve guard"); } @@ -134,9 +135,10 @@ public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState return GuardedByValidationResult.ok(); } - public static Symbol bindGuardedByString(Tree tree, String guard, VisitorState visitorState) { + public static Symbol bindGuardedByString( + Tree tree, String guard, VisitorState visitorState, GuardedByFlags flags) { Optional bound = - GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState)); + GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState), flags); if (!bound.isPresent()) { return null; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java index 2f6b45b4961..67876949e1d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java @@ -81,17 +81,21 @@ public interface LockEventListener { * members. */ public static void analyze( - VisitorState state, LockEventListener listener, Predicate isSuppressed) { + VisitorState state, + LockEventListener listener, + Predicate isSuppressed, + GuardedByFlags flags) { HeldLockSet locks = HeldLockSet.empty(); - locks = handleMonitorGuards(state, locks); - new LockScanner(state, listener, isSuppressed).scan(state.getPath(), locks); + locks = handleMonitorGuards(state, locks, flags); + new LockScanner(state, listener, isSuppressed, flags).scan(state.getPath(), locks); } // Don't use Class#getName() for inner classes, we don't want `Monitor$Guard` private static final String MONITOR_GUARD_CLASS = "com.google.common.util.concurrent.Monitor.Guard"; - private static HeldLockSet handleMonitorGuards(VisitorState state, HeldLockSet locks) { + private static HeldLockSet handleMonitorGuards( + VisitorState state, HeldLockSet locks, GuardedByFlags flags) { JCNewClass newClassTree = ASTHelpers.findEnclosingNode(state.getPath(), JCNewClass.class); if (newClassTree == null) { return locks; @@ -105,7 +109,7 @@ private static HeldLockSet handleMonitorGuards(VisitorState state, HeldLockSet l } Optional lockExpression = GuardedByBinder.bindExpression( - Iterables.getOnlyElement(newClassTree.getArguments()), state); + Iterables.getOnlyElement(newClassTree.getArguments()), state, flags); if (!lockExpression.isPresent()) { return locks; } @@ -117,14 +121,19 @@ private static class LockScanner extends TreePathScanner { private final VisitorState visitorState; private final LockEventListener listener; private final Predicate isSuppressed; + private final GuardedByFlags flags; private static final GuardedByExpression.Factory F = new GuardedByExpression.Factory(); private LockScanner( - VisitorState visitorState, LockEventListener listener, Predicate isSuppressed) { + VisitorState visitorState, + LockEventListener listener, + Predicate isSuppressed, + GuardedByFlags flags) { this.visitorState = visitorState; this.listener = listener; this.isSuppressed = isSuppressed; + this.flags = flags; } @Override @@ -143,7 +152,8 @@ public Void visitMethod(MethodTree tree, HeldLockSet locks) { // for invocations. for (String guard : GuardedByUtils.getGuardValues(tree, visitorState)) { Optional bound = - GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState)); + GuardedByBinder.bindString( + guard, GuardedBySymbolResolver.from(tree, visitorState), flags); if (bound.isPresent()) { locks = locks.plus(bound.get()); } @@ -162,7 +172,7 @@ public Void visitTry(TryTree tree, HeldLockSet locks) { // Cheesy try/finally heuristic: assume that all locks released in the finally // are held for the entirety of the try and catch statements. Collection releasedLocks = - ReleasedLockFinder.find(tree.getFinallyBlock(), visitorState); + ReleasedLockFinder.find(tree.getFinallyBlock(), visitorState, flags); if (resources.isEmpty()) { scan(tree.getBlock(), locks.plusAll(releasedLocks)); } else { @@ -179,7 +189,7 @@ public Void visitTry(TryTree tree, HeldLockSet locks) { public Void visitSynchronized(SynchronizedTree tree, HeldLockSet locks) { // The synchronized expression is held in the body of the synchronized statement: Optional lockExpression = - GuardedByBinder.bindExpression((JCExpression) tree.getExpression(), visitorState); + GuardedByBinder.bindExpression((JCExpression) tree.getExpression(), visitorState, flags); scan(tree.getBlock(), lockExpression.isPresent() ? locks.plus(lockExpression.get()) : locks); return null; } @@ -224,11 +234,13 @@ public Void visitVariable(VariableTree node, HeldLockSet locks) { private void checkMatch(ExpressionTree tree, HeldLockSet locks) { for (String guardString : GuardedByUtils.getGuardValues(tree, visitorState)) { - GuardedByBinder.bindString(guardString, GuardedBySymbolResolver.from(tree, visitorState)) + GuardedByBinder.bindString( + guardString, GuardedBySymbolResolver.from(tree, visitorState), flags) .ifPresent( guard -> { Optional boundGuard = - ExpectedLockCalculator.from((JCTree.JCExpression) tree, guard, visitorState); + ExpectedLockCalculator.from( + (JCTree.JCExpression) tree, guard, visitorState, flags); 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. @@ -278,11 +290,14 @@ static LockResource create(String className, String lockMethod, String unlockMet private static class LockOperationFinder extends TreeScanner { static Collection find( - Tree tree, VisitorState state, Matcher lockOperationMatcher) { + Tree tree, + VisitorState state, + Matcher lockOperationMatcher, + GuardedByFlags flags) { if (tree == null) { return Collections.emptyList(); } - LockOperationFinder finder = new LockOperationFinder(state, lockOperationMatcher); + LockOperationFinder finder = new LockOperationFinder(state, lockOperationMatcher, flags); tree.accept(finder, null); return finder.locks; } @@ -290,6 +305,7 @@ static Collection find( private static final String READ_WRITE_LOCK_CLASS = "java.util.concurrent.locks.ReadWriteLock"; private final Matcher lockOperationMatcher; + private final GuardedByFlags flags; /** Matcher for ReadWriteLock lock accessors. */ private static final Matcher READ_WRITE_ACCESSOR_MATCHER = @@ -300,9 +316,11 @@ static Collection find( private final VisitorState state; private final Set locks = new HashSet<>(); - private LockOperationFinder(VisitorState state, Matcher lockOperationMatcher) { + private LockOperationFinder( + VisitorState state, Matcher lockOperationMatcher, GuardedByFlags flags) { this.state = state; this.lockOperationMatcher = lockOperationMatcher; + this.flags = flags; } @Override @@ -323,7 +341,7 @@ private void handleReleasedLocks(MethodInvocationTree tree) { return; } Optional node = - GuardedByBinder.bindExpression((JCExpression) tree, state); + GuardedByBinder.bindExpression((JCExpression) tree, state, flags); if (node.isPresent()) { GuardedByExpression receiver = ((GuardedByExpression.Select) node.get()).base(); locks.add(receiver); @@ -350,11 +368,12 @@ private void handleUnlockAnnotatedMethods(MethodInvocationTree tree) { } for (String lockString : annotation.value()) { Optional guard = - GuardedByBinder.bindString(lockString, GuardedBySymbolResolver.from(tree, state)); + GuardedByBinder.bindString( + lockString, GuardedBySymbolResolver.from(tree, state), flags); // TODO(cushon): http://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#ifPresent if (guard.isPresent()) { Optional lock = - ExpectedLockCalculator.from((JCExpression) tree, guard.get(), state); + ExpectedLockCalculator.from((JCExpression) tree, guard.get(), state, flags); if (lock.isPresent()) { locks.add(lock.get()); } @@ -377,8 +396,9 @@ private static Iterable> unlockMatchers() { return Iterables.transform(LOCK_RESOURCES, LockResource::createUnlockMatcher); } - static Collection find(Tree tree, VisitorState state) { - return LockOperationFinder.find(tree, state, UNLOCK_MATCHER); + static Collection find( + Tree tree, VisitorState state, GuardedByFlags flags) { + return LockOperationFinder.find(tree, state, UNLOCK_MATCHER, flags); } } @@ -396,8 +416,9 @@ private static Iterable> unlockMatchers() { return Iterables.transform(LOCK_RESOURCES, LockResource::createLockMatcher); } - static Collection find(Tree tree, VisitorState state) { - return LockOperationFinder.find(tree, state, LOCK_MATCHER); + static Collection find( + Tree tree, VisitorState state, GuardedByFlags flags) { + return LockOperationFinder.find(tree, state, LOCK_MATCHER, flags); } } @@ -430,14 +451,15 @@ static class ExpectedLockCalculator { static Optional from( JCTree.JCExpression guardedMemberExpression, GuardedByExpression guard, - VisitorState state) { + VisitorState state, + GuardedByFlags flags) { if (isGuardReferenceAbsolute(guard)) { return Optional.of(guard); } Optional guardedMember = - GuardedByBinder.bindExpression(guardedMemberExpression, state); + GuardedByBinder.bindExpression(guardedMemberExpression, state, flags); if (!guardedMember.isPresent()) { return Optional.empty(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/LockMethodChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/LockMethodChecker.java index b084ac05c1a..63bec524164 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/LockMethodChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/LockMethodChecker.java @@ -45,12 +45,14 @@ protected ImmutableList getLockExpressions(MethodTree tree) { @Override protected Set getActual(MethodTree tree, VisitorState state) { - return ImmutableSet.copyOf(HeldLockAnalyzer.AcquiredLockFinder.find(tree.getBody(), state)); + return ImmutableSet.copyOf( + HeldLockAnalyzer.AcquiredLockFinder.find(tree.getBody(), state, GuardedByFlags.allOn())); } @Override protected Set getUnwanted(MethodTree tree, VisitorState state) { - return ImmutableSet.copyOf(HeldLockAnalyzer.ReleasedLockFinder.find(tree.getBody(), state)); + return ImmutableSet.copyOf( + HeldLockAnalyzer.ReleasedLockFinder.find(tree.getBody(), state, GuardedByFlags.allOn())); } @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/UnlockMethodChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/UnlockMethodChecker.java index 700c530f0b6..10591a51309 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/UnlockMethodChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/UnlockMethodChecker.java @@ -45,12 +45,14 @@ protected ImmutableList getLockExpressions(MethodTree tree) { @Override protected Set getActual(MethodTree tree, VisitorState state) { - return ImmutableSet.copyOf(HeldLockAnalyzer.ReleasedLockFinder.find(tree.getBody(), state)); + return ImmutableSet.copyOf( + HeldLockAnalyzer.ReleasedLockFinder.find(tree.getBody(), state, GuardedByFlags.allOn())); } @Override protected Set getUnwanted(MethodTree tree, VisitorState state) { - return ImmutableSet.copyOf(HeldLockAnalyzer.AcquiredLockFinder.find(tree.getBody(), state)); + return ImmutableSet.copyOf( + HeldLockAnalyzer.AcquiredLockFinder.find(tree.getBody(), state, GuardedByFlags.allOn())); } @Override diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java index 612272fe288..7e4ac029e74 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java @@ -520,7 +520,8 @@ private String bind(String className, String exprString, JavaFileObject fileObje compilationUnit, task.getContext(), null, - VisitorState.createForUtilityPurposes(task.getContext()))); + VisitorState.createForUtilityPurposes(task.getContext())), + GuardedByFlags.allOn()); if (!guardExpression.isPresent()) { throw new IllegalGuardedBy(exprString); } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByCheckerTest.java index 016257112ac..ef24b7cd40d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByCheckerTest.java @@ -330,6 +330,28 @@ public void i541() { .doTest(); } + @Test + public void methodQualifiedWithThis() { + compilationHelper + .addSourceLines( + "threadsafety/Test.java", + "package threadsafety;", + "import java.util.List;", + "import javax.annotation.concurrent.GuardedBy;", + "class Itself {", + " @GuardedBy(\"this\")", + " void f() {};", + " void g() {", + " // BUG: Diagnostic contains:", + " // should be guarded by 'this'", + " this.f();", + " synchronized (this) { f(); }", + " synchronized (this) { this.f(); }", + " }", + "}") + .doTest(); + } + @Test public void testCtor() { compilationHelper @@ -1523,7 +1545,6 @@ public void regression_b27686620() { .doTest(); } - @Ignore // TODO(cushon): clean up existing instances and re-enable @Test public void qualifiedMethod() { compilationHelper @@ -1678,7 +1699,7 @@ public void newClassBase() { " f.x = 10;", " }", " void bar () {", - " // TODO(b/112275411): handle new class expressions", + " // BUG: Diagnostic contains: should be guarded by 'mu'", " new Foo().x = 11;", " }", "}") diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzerTest.java index 050dc08444e..034992edbae 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzerTest.java @@ -20,6 +20,7 @@ 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; @@ -252,6 +253,10 @@ public void testLockMethodEnclosingAccess() { @BugPattern(name = "GuardedByLockSet", summary = "", explanation = "", severity = ERROR) public static class GuardedByLockSetAnalyzer extends GuardedByChecker { + public GuardedByLockSetAnalyzer() { + super(ErrorProneFlags.empty()); + } + @Override protected Description checkGuardedAccess( Tree tree, GuardedByExpression guard, HeldLockSet live, VisitorState state) {