diff --git a/annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java b/annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java new file mode 100644 index 00000000000..48d5d13fa90 --- /dev/null +++ b/annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java @@ -0,0 +1,33 @@ +/* + * Copyright 2026 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.annotations.concurrent; + +import static java.lang.annotation.ElementType.PARAMETER; +import static java.lang.annotation.RetentionPolicy.CLASS; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +/** + * Indicates that the annotated functional-interface parameter is run synchronously on the calling + * thread before the method returns, and is not stored or passed to another thread. + * + *

The {@link GuardedBy} check relies on this: a lambda or method reference passed as this + * argument is analyzed as if the caller's locks are held. + */ +@Target(PARAMETER) +@Retention(CLASS) +public @interface RunsImmediately {} 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..050ed395d3a 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,7 +17,6 @@ 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; @@ -79,7 +78,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)) { + && HeldLockAnalyzer.invokesArgumentImmediately(methodInvocationTree, tree, state)) { return NO_MATCH; } analyze(state.withPath(new TreePath(state.getPath(), tree.getBody()))); @@ -90,7 +89,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)) { + && HeldLockAnalyzer.invokesArgumentImmediately(methodInvocationTree, tree, state)) { return NO_MATCH; } analyze(state); 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..bca4a625db2 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 @@ -45,6 +45,8 @@ import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCExpression; import com.sun.tools.javac.tree.JCTree.JCNewClass; @@ -65,7 +67,7 @@ */ public final class HeldLockAnalyzer { /** Methods which invoke lambdas on the same thread. */ - static final Matcher INVOKES_LAMBDAS_IMMEDIATELY = + private static final Matcher INVOKES_LAMBDAS_IMMEDIATELY = anyOf( instanceMethod() .onExactClass("java.util.Optional") @@ -88,6 +90,32 @@ public final class HeldLockAnalyzer { .onClass("com.google.common.collect.Iterables") .namedAnyOf("tryFind", "any", "all", "indexOf")); + private static final String RUNS_IMMEDIATELY = + "com.google.errorprone.annotations.concurrent.RunsImmediately"; + + /** + * Returns true if {@code functionalArgument} (a lambda or method reference) passed to {@code + * invocation} is run on the same thread, i.e. the method is one of {@link + * #INVOKES_LAMBDAS_IMMEDIATELY} or the matching parameter is {@code @RunsImmediately}. + */ + static boolean invokesArgumentImmediately( + MethodInvocationTree invocation, ExpressionTree functionalArgument, VisitorState state) { + if (INVOKES_LAMBDAS_IMMEDIATELY.matches(invocation, state)) { + return true; + } + MethodSymbol sym = ASTHelpers.getSymbol(invocation); + if (sym == null) { + return false; + } + List params = sym.getParameters(); + int index = invocation.getArguments().indexOf(functionalArgument); + if (index < 0 || params.isEmpty()) { + return false; + } + VarSymbol param = index < params.size() ? params.get(index) : params.getLast(); + return ASTHelpers.hasAnnotation(param, RUNS_IMMEDIATELY, state); + } + /** Listener interface for accesses to guarded members. */ public interface LockEventListener { @@ -231,7 +259,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)) { + && invokesArgumentImmediately(methodInvocationTree, node, 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..3d1265b3081 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,162 @@ public synchronized void add(Optional x) { .doTest(); } + @Test + public void runsImmediately_lambda() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + import com.google.errorprone.annotations.concurrent.RunsImmediately; + + class Test { + @GuardedBy("this") + private int state = 0; + + public synchronized void modifyState(int newState) { + safeRun(() -> modifyStateInternal(newState)); + } + + @GuardedBy("this") + private void modifyStateInternal(int newState) { + state = newState; + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void runsImmediately_methodReference() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + import com.google.errorprone.annotations.concurrent.RunsImmediately; + + class Test { + @GuardedBy("this") + private int state = 0; + + public synchronized void modifyState() { + safeRun(this::zeroStateInternal); + } + + @GuardedBy("this") + private void zeroStateInternal() { + state = 0; + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void runsImmediately_lambda_synchronizedBlock() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + import com.google.errorprone.annotations.concurrent.RunsImmediately; + + class Test { + private final Object lock = new Object(); + + @GuardedBy("lock") + private int state = 0; + + public void modifyState(int newState) { + synchronized (lock) { + safeRun(() -> state = newState); + } + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void runsImmediately_lambda_wrongGuard() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + import com.google.errorprone.annotations.concurrent.RunsImmediately; + + class Test { + private final Object lock = new Object(); + + @GuardedBy("lock") + private int state = 0; + + public synchronized void modifyState(int newState) { + // BUG: Diagnostic contains: should be guarded by 'this.lock' + safeRun(() -> state = newState); + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void runsImmediately_lambda_multipleGuardedAccesses() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + import com.google.errorprone.annotations.concurrent.RunsImmediately; + + class Test { + @GuardedBy("this") + private int a = 0; + + @GuardedBy("this") + private int b = 0; + + private final Object other = new Object(); + + @GuardedBy("other") + private int c = 0; + + public synchronized void modify() { + safeRun( + () -> { + a = 1; + b = 2; + // BUG: Diagnostic contains: should be guarded by 'this.other' + c = 3; + }); + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + @Test public void methodReferences_shouldBeFlagged() { compilationHelper diff --git a/docs/bugpattern/GuardedBy.md b/docs/bugpattern/GuardedBy.md index 470d4db67e1..6814a10b94e 100644 --- a/docs/bugpattern/GuardedBy.md +++ b/docs/bugpattern/GuardedBy.md @@ -185,6 +185,33 @@ 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 opt in to the same behavior by annotating the +functional-interface parameter with +`com.google.errorprone.annotations.concurrent.RunsImmediately`. This documents +that the argument, if it is invoked at all, is invoked synchronously on the +calling thread before the method returns, so a lambda or method reference passed +there is analyzed in the caller's lock scope: + +```java +class Transaction { + @GuardedBy("this") + int x; + + public synchronized void handle() { + doSomething(() -> { + x++; // OK: 'doSomething' runs the lambda immediately, while 'this' is held. + }); + } + + private void doSomething(@RunsImmediately Runnable r) { + r.run(); + } +} +``` + +The contract is trusted but not verified: annotating a parameter whose value is +actually deferred to another thread can hide real concurrency bugs. + #### False negatives with aliasing ```java