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 01f4cd74556..b2494e83506 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 @@ -17,11 +17,11 @@ package com.google.errorprone.bugpatterns.threadsafety; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; -import static com.google.errorprone.bugpatterns.threadsafety.HeldLockAnalyzer.INVOKES_LAMBDAS_IMMEDIATELY; import static com.google.errorprone.matchers.Description.NO_MATCH; 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; @@ -32,8 +32,10 @@ import com.google.errorprone.bugpatterns.threadsafety.GuardedByExpression.Select; import com.google.errorprone.bugpatterns.threadsafety.GuardedByUtils.GuardedByValidationResult; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.MemberReferenceTree; import com.sun.source.tree.MethodInvocationTree; @@ -60,8 +62,12 @@ public class GuardedByChecker extends BugChecker private static final String JUC_READ_WRITE_LOCK = "java.util.concurrent.locks.ReadWriteLock"; + private final Matcher invokesLambdasImmediately; + @Inject - GuardedByChecker() {} + GuardedByChecker(ErrorProneFlags flags) { + this.invokesLambdasImmediately = HeldLockAnalyzer.invokesLambdasImmediately(flags); + } @Override public Description matchMethod(MethodTree tree, VisitorState state) { @@ -79,7 +85,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { var parent = state.getPath().getParentPath().getLeaf(); if (parent instanceof MethodInvocationTree methodInvocationTree - && INVOKES_LAMBDAS_IMMEDIATELY.matches(methodInvocationTree, state)) { + && invokesLambdasImmediately.matches(methodInvocationTree, state)) { return NO_MATCH; } analyze(state.withPath(new TreePath(state.getPath(), tree.getBody()))); @@ -90,7 +96,7 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { var parent = state.getPath().getParentPath().getLeaf(); if (parent instanceof MethodInvocationTree methodInvocationTree - && INVOKES_LAMBDAS_IMMEDIATELY.matches(methodInvocationTree, state)) { + && invokesLambdasImmediately.matches(methodInvocationTree, state)) { return NO_MATCH; } analyze(state); @@ -101,7 +107,8 @@ private void analyze(VisitorState state) { HeldLockAnalyzer.analyze( state, (tree, guard, live) -> report(checkGuardedAccess(tree, guard, live, state), state), - tree -> isSuppressed(tree, state)); + tree -> isSuppressed(tree, state), + invokesLambdasImmediately); } @Override 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 29c79c4b8ed..5d409146e96 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 @@ -16,12 +16,16 @@ package com.google.errorprone.bugpatterns.threadsafety; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.anyMethod; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.threadsafety.GuardedByExpression.Kind; import com.google.errorprone.bugpatterns.threadsafety.GuardedByExpression.Select; @@ -64,8 +68,14 @@ * @author cushon@google.com (Liam Miller-Cushon) */ public final class HeldLockAnalyzer { + /** + * The flag used to extend {@link #WELL_KNOWN_IMMEDIATE_METHODS} with additional methods, e.g. + * {@code -XepOpt:GuardedBy:KnownImmediateMethods=com.example.Transaction#doSomething}. + */ + private static final String KNOWN_IMMEDIATE_METHODS_FLAG = "GuardedBy:KnownImmediateMethods"; + /** Methods which invoke lambdas on the same thread. */ - static final Matcher INVOKES_LAMBDAS_IMMEDIATELY = + private static final Matcher WELL_KNOWN_IMMEDIATE_METHODS = anyOf( instanceMethod() .onExactClass("java.util.Optional") @@ -88,6 +98,36 @@ public final class HeldLockAnalyzer { .onClass("com.google.common.collect.Iterables") .namedAnyOf("tryFind", "any", "all", "indexOf")); + private static final Splitter HASH_SPLITTER = Splitter.on('#'); + + /** + * Returns a matcher for invocations of methods which invoke their functional interface arguments + * on the calling thread before returning: the well known JDK and Guava methods above, plus any + * methods listed in {@code -XepOpt:GuardedBy:KnownImmediateMethods}. + */ + public static Matcher invokesLambdasImmediately(ErrorProneFlags flags) { + ImmutableList configured = flags.getListOrEmpty(KNOWN_IMMEDIATE_METHODS_FLAG); + if (configured.isEmpty()) { + return WELL_KNOWN_IMMEDIATE_METHODS; + } + return anyOf( + ImmutableList.>builder() + .add(WELL_KNOWN_IMMEDIATE_METHODS) + .addAll(configured.stream().map(HeldLockAnalyzer::parseKnownImmediateMethod).iterator()) + .build()); + } + + /** Parses a single {@code fully.qualified.ClassName#methodName} entry into a matcher. */ + private static Matcher parseKnownImmediateMethod(String spec) { + List parts = HASH_SPLITTER.splitToList(spec.trim()); + checkArgument( + parts.size() == 2 && !parts.get(0).isEmpty() && !parts.get(1).isEmpty(), + "Malformed value \"%s\" for -XepOpt:%s; expected #", + spec, + KNOWN_IMMEDIATE_METHODS_FLAG); + return anyMethod().onDescendantOf(parts.get(0)).named(parts.get(1)); + } + /** Listener interface for accesses to guarded members. */ public interface LockEventListener { @@ -106,10 +146,14 @@ public interface LockEventListener { * members. */ public static void analyze( - VisitorState state, LockEventListener listener, Predicate isSuppressed) { + VisitorState state, + LockEventListener listener, + Predicate isSuppressed, + Matcher invokesLambdasImmediately) { HeldLockSet locks = HeldLockSet.empty(); locks = handleMonitorGuards(state, locks); - new LockScanner(state, listener, isSuppressed).scan(state.getPath(), locks); + new LockScanner(state, listener, isSuppressed, invokesLambdasImmediately) + .scan(state.getPath(), locks); } // Don't use Class#getName() for inner classes, we don't want `Monitor$Guard` @@ -138,14 +182,19 @@ private static final class LockScanner extends TreePathScanner isSuppressed; + private final Matcher invokesLambdasImmediately; private static final GuardedByExpression.Factory F = new GuardedByExpression.Factory(); private LockScanner( - VisitorState visitorState, LockEventListener listener, Predicate isSuppressed) { + VisitorState visitorState, + LockEventListener listener, + Predicate isSuppressed, + Matcher invokesLambdasImmediately) { this.visitorState = visitorState; this.listener = listener; this.isSuppressed = isSuppressed; + this.invokesLambdasImmediately = invokesLambdasImmediately; } @Override @@ -231,7 +280,7 @@ public Void visitNewClass(NewClassTree tree, HeldLockSet locks) { public Void visitLambdaExpression(LambdaExpressionTree node, HeldLockSet heldLockSet) { var parent = getCurrentPath().getParentPath().getLeaf(); if (parent instanceof MethodInvocationTree methodInvocationTree - && INVOKES_LAMBDAS_IMMEDIATELY.matches(methodInvocationTree, visitorState)) { + && invokesLambdasImmediately.matches(methodInvocationTree, visitorState)) { return super.visitLambdaExpression(node, heldLockSet); } // Don't descend into lambdas; they will be analyzed separately. 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 0332986d4a8..3eb3082d968 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 @@ -2424,6 +2424,298 @@ public synchronized void add(Optional x) { .doTest(); } + @Test + public void knownImmediateMethods_lambda() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + import java.util.ArrayList; + import java.util.List; + + class Test { + @GuardedBy("this") + private final List xs = new ArrayList<>(); + + public synchronized void f() { + runNow(() -> xs.clear()); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_methodReference() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + import java.util.ArrayList; + import java.util.List; + + class Test { + @GuardedBy("this") + private final List xs = new ArrayList<>(); + + public synchronized void f() { + runNow(xs::clear); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_flagNotSet_shouldBeFlagged() { + compilationHelper + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + import java.util.ArrayList; + import java.util.List; + + class Test { + @GuardedBy("this") + private final List xs = new ArrayList<>(); + + public synchronized void f() { + // BUG: Diagnostic contains: should be guarded by 'this' + runNow(() -> xs.clear()); + // BUG: Diagnostic contains: should be guarded by 'this' + runNow(xs::clear); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_wrongGuard_shouldBeFlagged() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + import java.util.ArrayList; + import java.util.List; + + class Test { + final Object mu = new Object(); + + @GuardedBy("mu") + private final List xs = new ArrayList<>(); + + public synchronized void f() { + // BUG: Diagnostic contains: should be guarded by 'this.mu' + runNow(() -> xs.clear()); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_multipleAccessesInLambdaBody() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + final Object mu = new Object(); + + @GuardedBy("this") + int x; + + @GuardedBy("mu") + int y; + + public synchronized void f() { + runNow( + () -> { + x++; + // BUG: Diagnostic contains: should be guarded by 'this.mu' + y++; + }); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_synchronizedBlock() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + final Object mu = new Object(); + + @GuardedBy("mu") + int x; + + public void f() { + synchronized (mu) { + runNow(() -> x++); + } + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_explicitLock() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + import java.util.concurrent.locks.Lock; + + class Test { + final Lock lock = null; + + @GuardedBy("lock") + int x; + + public void f() { + lock.lock(); + try { + runNow(() -> x++); + } finally { + lock.unlock(); + } + // BUG: Diagnostic contains: should be guarded by 'this.lock' + runNow(() -> x++); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_multipleEntries() { + compilationHelperWithKnownImmediateMethods( + "com.example.NotOnClasspath#run, threadsafety.Test#runNow") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + @GuardedBy("this") + int x; + + public synchronized void f() { + runNow(() -> x++); + } + + private void runNow(Runnable r) { + r.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_descendantOfListedClass() { + compilationHelperWithKnownImmediateMethods("threadsafety.Runner#runNow") + .addSourceLines( + "threadsafety/Runner.java", + """ + package threadsafety; + + interface Runner { + void runNow(Runnable r); + } + """) + .addSourceLines( + "threadsafety/DirectRunner.java", + """ + package threadsafety; + + class DirectRunner implements Runner { + @Override + public void runNow(Runnable r) { + r.run(); + } + } + """) + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + private final DirectRunner runner = new DirectRunner(); + + @GuardedBy("this") + int x; + + public synchronized void f() { + runner.runNow(() -> x++); + } + } + """) + .doTest(); + } + @Test public void methodReferences_shouldBeFlagged() { compilationHelper @@ -2546,4 +2838,9 @@ public class IllegalStartOfExpression { """) .doTest(); } + + private CompilationTestHelper compilationHelperWithKnownImmediateMethods(String methods) { + return CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=" + methods); + } } 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 76f249b844d..f779d637a5f 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 @@ -17,9 +17,12 @@ package com.google.errorprone.bugpatterns.threadsafety; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static org.junit.Assert.assertThrows; +import com.google.common.collect.ImmutableList; 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; @@ -251,11 +254,32 @@ void m() { .doTest(); } + @Test + public void knownImmediateMethodsFlag_malformed_throws() { + for (String value : + ImmutableList.of( + "com.example.Foo", // no method name + "#bar", // no class name + "com.example.Foo#", // empty method name + "a#b#c", // too many separators + "", // empty entry, e.g. from a trailing comma + "com.example.Foo#run,")) { + ErrorProneFlags flags = + ErrorProneFlags.builder().putFlag("GuardedBy:KnownImmediateMethods", value).build(); + assertThrows( + "GuardedBy:KnownImmediateMethods=\"" + value + "\"", + IllegalArgumentException.class, + () -> HeldLockAnalyzer.invokesLambdasImmediately(flags)); + } + } + /** A customized {@link GuardedByChecker} that prints more test-friendly diagnostics. */ @BugPattern(name = "GuardedByLockSet", summary = "", explanation = "", severity = ERROR) public static class GuardedByLockSetAnalyzer extends GuardedByChecker { @Inject - GuardedByLockSetAnalyzer() {} + GuardedByLockSetAnalyzer(ErrorProneFlags flags) { + super(flags); + } @Override protected Description checkGuardedAccess( diff --git a/docs/bugpattern/GuardedBy.md b/docs/bugpattern/GuardedBy.md index 470d4db67e1..2fb9b275bd5 100644 --- a/docs/bugpattern/GuardedBy.md +++ b/docs/bugpattern/GuardedBy.md @@ -185,6 +185,38 @@ private void doSomething(Runnable r) { However, the check does special-case some method calls which are known to immediately call the provided lambda or method reference. +For your own methods, you can extend that list with the +`GuardedBy:KnownImmediateMethods` flag, which takes a comma-separated list of +methods in `fully.qualified.ClassName#methodName` form. For example, +`-XepOpt:GuardedBy:KnownImmediateMethods=com.example.Transaction#doSomething` +makes the check analyze lambdas and method references passed to +`Transaction.doSomething` in the caller's lock scope: + +```java +class Transaction { + @GuardedBy("this") + int x; + + public synchronized void handle() { + doSomething(() -> { + ++x; // OK: 'doSomething' is configured to run the lambda immediately. + }); + } + + private void doSomething(Runnable r) { + r.run(); + } +} +``` + +Methods declared on subtypes of the listed class are matched, too, and both +static and instance methods are supported. + +The contract is trusted but not verified: listing a method that actually defers +its argument to another thread can hide real concurrency bugs. Note also that +the flag applies to the method as a whole — every functional interface argument +passed to a listed method is treated as invoked immediately. + #### False negatives with aliasing ```java