From bd76b5af24e6d32eb6e4a622ef680ca344ffa109 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Wed, 1 Jul 2026 17:04:35 -0400 Subject: [PATCH 1/4] add @RunsImmediately --- .../concurrent/RunsImmediately.java | 33 +++ .../threadsafety/GuardedByChecker.java | 5 +- .../threadsafety/HeldLockAnalyzer.java | 31 ++- .../threadsafety/GuardedByCheckerTest.java | 239 ++++++++++++++++++ docs/bugpattern/GuardedBy.md | 27 ++ 5 files changed, 331 insertions(+), 4 deletions(-) create mode 100644 annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java 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..947d7e0317a --- /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 to analyze a lambda or method reference passed as + * the argument in the caller's lock scope. + */ +@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..2a58a90a3a8 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; @@ -88,6 +90,33 @@ 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; + } + // varargs: a trailing argument maps to the last parameter + 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 +260,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..8bcdafcae87 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,245 @@ public synchronized void add(Optional x) { .doTest(); } + @Test + public void runsImmediately_lambda_noError() { + 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_lambda_lockNotHeld_stillError() { + 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 void modifyState(int newState) { + // BUG: Diagnostic contains: should be guarded by 'this' + safeRun(() -> state = newState); + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void runsImmediately_lambda_wrongGuard_stillError() { + 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 withoutRunsImmediately_lambda_stillError() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + @GuardedBy("this") + private int state = 0; + + public synchronized void modifyState(int newState) { + // BUG: Diagnostic contains: should be guarded by 'this' + safeRun(() -> state = newState); + } + + private void safeRun(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 runsImmediately_methodReference_noError() { + 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_methodReference_lockNotHeld_stillError() { + 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 void modifyState() { + // BUG: Diagnostic contains: should be guarded by 'this' + safeRun(this::zeroStateInternal); + } + + @GuardedBy("this") + private void zeroStateInternal() { + state = 0; + } + + private void safeRun(@RunsImmediately Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void withoutRunsImmediately_methodReference_stillError() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + @GuardedBy("this") + private int state = 0; + + public synchronized void modifyState() { + // BUG: Diagnostic contains: should be guarded by 'this' + safeRun(this::zeroStateInternal); + } + + @GuardedBy("this") + private void zeroStateInternal() { + state = 0; + } + + private void safeRun(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..a5e38b29629 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, not verified: annotating a parameter whose value is +actually deferred to another thread can hide real concurrency bugs. + #### False negatives with aliasing ```java From 9a0845dcfea2743600ca04ddeb8ca14f5a919771 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Wed, 1 Jul 2026 19:00:57 -0400 Subject: [PATCH 2/4] self nits --- .../threadsafety/HeldLockAnalyzer.java | 3 +- .../threadsafety/GuardedByCheckerTest.java | 132 ++---------------- 2 files changed, 11 insertions(+), 124 deletions(-) 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 2a58a90a3a8..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 @@ -67,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") @@ -112,7 +112,6 @@ static boolean invokesArgumentImmediately( if (index < 0 || params.isEmpty()) { return false; } - // varargs: a trailing argument maps to the last parameter VarSymbol param = index < params.size() ? params.get(index) : params.getLast(); return ASTHelpers.hasAnnotation(param, RUNS_IMMEDIATELY, state); } 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 8bcdafcae87..413b206948f 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 @@ -2425,7 +2425,7 @@ public synchronized void add(Optional x) { } @Test - public void runsImmediately_lambda_noError() { + public void runsImmediately_lambda() { compilationHelper .addSourceLines( "Test.java", @@ -2455,7 +2455,7 @@ private void safeRun(@RunsImmediately Runnable runnable) { } @Test - public void runsImmediately_lambda_lockNotHeld_stillError() { + public void runsImmediately_methodReference() { compilationHelper .addSourceLines( "Test.java", @@ -2467,9 +2467,13 @@ class Test { @GuardedBy("this") private int state = 0; - public void modifyState(int newState) { - // BUG: Diagnostic contains: should be guarded by 'this' - safeRun(() -> state = newState); + public synchronized void modifyState() { + safeRun(this::zeroStateInternal); + } + + @GuardedBy("this") + private void zeroStateInternal() { + state = 0; } private void safeRun(@RunsImmediately Runnable runnable) { @@ -2481,7 +2485,7 @@ private void safeRun(@RunsImmediately Runnable runnable) { } @Test - public void runsImmediately_lambda_wrongGuard_stillError() { + public void runsImmediately_lambda_wrongGuard() { compilationHelper .addSourceLines( "Test.java", @@ -2508,31 +2512,6 @@ private void safeRun(@RunsImmediately Runnable runnable) { .doTest(); } - @Test - public void withoutRunsImmediately_lambda_stillError() { - compilationHelper - .addSourceLines( - "Test.java", - """ - import com.google.errorprone.annotations.concurrent.GuardedBy; - - class Test { - @GuardedBy("this") - private int state = 0; - - public synchronized void modifyState(int newState) { - // BUG: Diagnostic contains: should be guarded by 'this' - safeRun(() -> state = newState); - } - - private void safeRun(Runnable runnable) { - runnable.run(); - } - } - """) - .doTest(); - } - @Test public void runsImmediately_lambda_multipleGuardedAccesses() { compilationHelper @@ -2572,97 +2551,6 @@ private void safeRun(@RunsImmediately Runnable runnable) { .doTest(); } - @Test - public void runsImmediately_methodReference_noError() { - 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_methodReference_lockNotHeld_stillError() { - 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 void modifyState() { - // BUG: Diagnostic contains: should be guarded by 'this' - safeRun(this::zeroStateInternal); - } - - @GuardedBy("this") - private void zeroStateInternal() { - state = 0; - } - - private void safeRun(@RunsImmediately Runnable runnable) { - runnable.run(); - } - } - """) - .doTest(); - } - - @Test - public void withoutRunsImmediately_methodReference_stillError() { - compilationHelper - .addSourceLines( - "Test.java", - """ - import com.google.errorprone.annotations.concurrent.GuardedBy; - - class Test { - @GuardedBy("this") - private int state = 0; - - public synchronized void modifyState() { - // BUG: Diagnostic contains: should be guarded by 'this' - safeRun(this::zeroStateInternal); - } - - @GuardedBy("this") - private void zeroStateInternal() { - state = 0; - } - - private void safeRun(Runnable runnable) { - runnable.run(); - } - } - """) - .doTest(); - } - @Test public void methodReferences_shouldBeFlagged() { compilationHelper From 8583c76adda98c29cff872388c893f3ec4973591 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Wed, 1 Jul 2026 19:28:57 -0400 Subject: [PATCH 3/4] self nits again --- .../concurrent/RunsImmediately.java | 4 +-- .../threadsafety/GuardedByCheckerTest.java | 29 +++++++++++++++++++ docs/bugpattern/GuardedBy.md | 2 +- 3 files changed, 32 insertions(+), 3 deletions(-) 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 index 947d7e0317a..48d5d13fa90 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java @@ -25,8 +25,8 @@ * 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 to analyze a lambda or method reference passed as - * the argument in the caller's lock scope. + *

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) 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 413b206948f..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 @@ -2484,6 +2484,35 @@ private void safeRun(@RunsImmediately Runnable runnable) { .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 diff --git a/docs/bugpattern/GuardedBy.md b/docs/bugpattern/GuardedBy.md index a5e38b29629..6814a10b94e 100644 --- a/docs/bugpattern/GuardedBy.md +++ b/docs/bugpattern/GuardedBy.md @@ -209,7 +209,7 @@ class Transaction { } ``` -The contract is trusted, not verified: annotating a parameter whose value is +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 From 655ddb5900f8542a674d96a7773857ee75fde9c9 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Wed, 15 Jul 2026 10:49:05 -0400 Subject: [PATCH 4/4] empty