Skip to content

Add KnownImmediateMethods flag to avoid @GuardedBy false positives on immediate invocations of guarded access #5940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EamonTracey wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add KnownImmediateMethods flag to avoid @GuardedBy false positives on immediate invocations of guarded access #5940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd76b5a
add @RunsImmediately
EamonTracey Jul 1, 2026
9a0845d
self nits
EamonTracey Jul 1, 2026
8583c76
self nits again
EamonTracey Jul 1, 2026
655ddb5
empty
EamonTracey Jul 15, 2026
a9ea5b4
delete annotation add flag
EamonTracey Aug 1, 2026
a2e6ae5
limpia
EamonTracey Aug 1, 2026
77967b1
finalize beautiful
EamonTracey Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<ExpressionTree> invokesLambdasImmediately;

@Inject
GuardedByChecker() {}
GuardedByChecker(ErrorProneFlags flags) {
this.invokesLambdasImmediately = HeldLockAnalyzer.invokesLambdasImmediately(flags);
}

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
Expand All @@ -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())));
Expand All @@ -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);
Expand All @@ -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
Expand Down
59 changes: 54 additions & 5 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<ExpressionTree> INVOKES_LAMBDAS_IMMEDIATELY =
private static final Matcher<ExpressionTree> WELL_KNOWN_IMMEDIATE_METHODS =
anyOf(
instanceMethod()
.onExactClass("java.util.Optional")
Expand All @@ -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<ExpressionTree> invokesLambdasImmediately(ErrorProneFlags flags) {
ImmutableList<String> configured = flags.getListOrEmpty(KNOWN_IMMEDIATE_METHODS_FLAG);
if (configured.isEmpty()) {
return WELL_KNOWN_IMMEDIATE_METHODS;
}
return anyOf(
ImmutableList.<Matcher<ExpressionTree>>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<ExpressionTree> parseKnownImmediateMethod(String spec) {
List<String> 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 <fully.qualified.ClassName>#<methodName>",
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 {

Expand All @@ -106,10 +146,14 @@ public interface LockEventListener {
* members.
*/
public static void analyze(
VisitorState state, LockEventListener listener, Predicate<Tree> isSuppressed) {
VisitorState state,
LockEventListener listener,
Predicate<Tree> isSuppressed,
Matcher<ExpressionTree> 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`
Expand Down Expand Up @@ -138,14 +182,19 @@ private static final class LockScanner extends TreePathScanner<Void, HeldLockSet
private final VisitorState visitorState;
private final LockEventListener listener;
private final Predicate<Tree> isSuppressed;
private final Matcher<ExpressionTree> invokesLambdasImmediately;

private static final GuardedByExpression.Factory F = new GuardedByExpression.Factory();

private LockScanner(
VisitorState visitorState, LockEventListener listener, Predicate<Tree> isSuppressed) {
VisitorState visitorState,
LockEventListener listener,
Predicate<Tree> isSuppressed,
Matcher<ExpressionTree> invokesLambdasImmediately) {
this.visitorState = visitorState;
this.listener = listener;
this.isSuppressed = isSuppressed;
this.invokesLambdasImmediately = invokesLambdasImmediately;
}

@Override
Expand Down Expand Up @@ -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.
Expand Down
Loading