From bd76b5af24e6d32eb6e4a622ef680ca344ffa109 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Wed, 1 Jul 2026 17:04:35 -0400 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] empty From a9ea5b452a291b6392f767373ad23a7e379fb8f3 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Sat, 1 Aug 2026 14:43:39 -0400 Subject: [PATCH 5/7] delete annotation add flag --- .../concurrent/RunsImmediately.java | 33 ---- .../threadsafety/GuardedByChecker.java | 16 +- .../threadsafety/HeldLockAnalyzer.java | 75 +++++--- .../threadsafety/GuardedByCheckerTest.java | 168 +++++++++++++++--- .../threadsafety/HeldLockAnalyzerTest.java | 24 ++- docs/bugpattern/GuardedBy.md | 25 +-- 6 files changed, 242 insertions(+), 99 deletions(-) delete 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 deleted file mode 100644 index 48d5d13fa90..00000000000 --- a/annotations/src/main/java/com/google/errorprone/annotations/concurrent/RunsImmediately.java +++ /dev/null @@ -1,33 +0,0 @@ -/* - * 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 050ed395d3a..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 @@ -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; @@ -31,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; @@ -59,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) { @@ -78,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 - && HeldLockAnalyzer.invokesArgumentImmediately(methodInvocationTree, tree, state)) { + && invokesLambdasImmediately.matches(methodInvocationTree, state)) { return NO_MATCH; } analyze(state.withPath(new TreePath(state.getPath(), tree.getBody()))); @@ -89,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 - && HeldLockAnalyzer.invokesArgumentImmediately(methodInvocationTree, tree, state)) { + && invokesLambdasImmediately.matches(methodInvocationTree, state)) { return NO_MATCH; } analyze(state); @@ -100,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 bca4a625db2..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; @@ -45,8 +49,6 @@ 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; @@ -66,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. */ - private static final Matcher INVOKES_LAMBDAS_IMMEDIATELY = + private static final Matcher WELL_KNOWN_IMMEDIATE_METHODS = anyOf( instanceMethod() .onExactClass("java.util.Optional") @@ -90,30 +98,34 @@ 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"; + private static final Splitter HASH_SPLITTER = Splitter.on('#'); /** - * 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}. + * 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}. */ - 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; + public static Matcher invokesLambdasImmediately(ErrorProneFlags flags) { + ImmutableList configured = flags.getListOrEmpty(KNOWN_IMMEDIATE_METHODS_FLAG); + if (configured.isEmpty()) { + return WELL_KNOWN_IMMEDIATE_METHODS; } - 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); + 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. */ @@ -134,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` @@ -166,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 @@ -259,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 - && invokesArgumentImmediately(methodInvocationTree, node, 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 3d1265b3081..9e0b7e12b24 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,13 +2425,15 @@ public synchronized void add(Optional x) { } @Test - public void runsImmediately_lambda() { - compilationHelper + public void knownImmediateMethods_lambda() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") .addSourceLines( - "Test.java", + "threadsafety/Test.java", """ + package threadsafety; + import com.google.errorprone.annotations.concurrent.GuardedBy; - import com.google.errorprone.annotations.concurrent.RunsImmediately; class Test { @GuardedBy("this") @@ -2446,7 +2448,7 @@ private void modifyStateInternal(int newState) { state = newState; } - private void safeRun(@RunsImmediately Runnable runnable) { + private void safeRun(Runnable runnable) { runnable.run(); } } @@ -2455,13 +2457,47 @@ private void safeRun(@RunsImmediately Runnable runnable) { } @Test - public void runsImmediately_methodReference() { + public void knownImmediateMethods_flagNotSet_isFlagged() { compilationHelper .addSourceLines( - "Test.java", + "threadsafety/Test.java", + """ + package threadsafety; + + 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(() -> modifyStateInternal(newState)); + } + + @GuardedBy("this") + private void modifyStateInternal(int newState) { + state = newState; + } + + private void safeRun(Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_methodReference() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") + .addSourceLines( + "threadsafety/Test.java", """ + package threadsafety; + import com.google.errorprone.annotations.concurrent.GuardedBy; - import com.google.errorprone.annotations.concurrent.RunsImmediately; class Test { @GuardedBy("this") @@ -2476,7 +2512,7 @@ private void zeroStateInternal() { state = 0; } - private void safeRun(@RunsImmediately Runnable runnable) { + private void safeRun(Runnable runnable) { runnable.run(); } } @@ -2485,13 +2521,15 @@ private void safeRun(@RunsImmediately Runnable runnable) { } @Test - public void runsImmediately_lambda_synchronizedBlock() { - compilationHelper + public void knownImmediateMethods_lambda_synchronizedBlock() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") .addSourceLines( - "Test.java", + "threadsafety/Test.java", """ + package threadsafety; + import com.google.errorprone.annotations.concurrent.GuardedBy; - import com.google.errorprone.annotations.concurrent.RunsImmediately; class Test { private final Object lock = new Object(); @@ -2505,7 +2543,7 @@ public void modifyState(int newState) { } } - private void safeRun(@RunsImmediately Runnable runnable) { + private void safeRun(Runnable runnable) { runnable.run(); } } @@ -2514,13 +2552,15 @@ private void safeRun(@RunsImmediately Runnable runnable) { } @Test - public void runsImmediately_lambda_wrongGuard() { - compilationHelper + public void knownImmediateMethods_lambda_wrongGuard() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") .addSourceLines( - "Test.java", + "threadsafety/Test.java", """ + package threadsafety; + import com.google.errorprone.annotations.concurrent.GuardedBy; - import com.google.errorprone.annotations.concurrent.RunsImmediately; class Test { private final Object lock = new Object(); @@ -2533,7 +2573,7 @@ public synchronized void modifyState(int newState) { safeRun(() -> state = newState); } - private void safeRun(@RunsImmediately Runnable runnable) { + private void safeRun(Runnable runnable) { runnable.run(); } } @@ -2542,13 +2582,15 @@ private void safeRun(@RunsImmediately Runnable runnable) { } @Test - public void runsImmediately_lambda_multipleGuardedAccesses() { - compilationHelper + public void knownImmediateMethods_lambda_multipleGuardedAccesses() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") .addSourceLines( - "Test.java", + "threadsafety/Test.java", """ + package threadsafety; + import com.google.errorprone.annotations.concurrent.GuardedBy; - import com.google.errorprone.annotations.concurrent.RunsImmediately; class Test { @GuardedBy("this") @@ -2572,7 +2614,7 @@ public synchronized void modify() { }); } - private void safeRun(@RunsImmediately Runnable runnable) { + private void safeRun(Runnable runnable) { runnable.run(); } } @@ -2580,6 +2622,84 @@ private void safeRun(@RunsImmediately Runnable runnable) { .doTest(); } + @Test + public void knownImmediateMethods_multipleEntries() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + // Entries are comma-separated and trimmed, and an entry naming a class which isn't on the + // compilation classpath is simply never matched. + .setArgs( + "-XepOpt:GuardedBy:KnownImmediateMethods=com.example.NotOnClasspath#run," + + " threadsafety.Test#safeRun") + .addSourceLines( + "threadsafety/Test.java", + """ + package threadsafety; + + import com.google.errorprone.annotations.concurrent.GuardedBy; + + class Test { + @GuardedBy("this") + private int state = 0; + + public synchronized void modifyState(int newState) { + safeRun(() -> state = newState); + } + + private void safeRun(Runnable runnable) { + runnable.run(); + } + } + """) + .doTest(); + } + + @Test + public void knownImmediateMethods_matchesDescendantsOfListedClass() { + CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) + .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=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 { + // The flag names the supertype, but the receiver's static type is the subtype. + private final DirectRunner runner = new DirectRunner(); + + @GuardedBy("this") + private int state = 0; + + public synchronized void modifyState(int newState) { + runner.runNow(() -> state = newState); + } + } + """) + .doTest(); + } + @Test public void methodReferences_shouldBeFlagged() { compilationHelper 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..018528b6fb6 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,30 @@ 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( + 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 6814a10b94e..3a8fc6d5eff 100644 --- a/docs/bugpattern/GuardedBy.md +++ b/docs/bugpattern/GuardedBy.md @@ -185,12 +185,12 @@ 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: +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 { @@ -199,18 +199,23 @@ class Transaction { public synchronized void handle() { doSomething(() -> { - x++; // OK: 'doSomething' runs the lambda immediately, while 'this' is held. + x++; // OK: 'doSomething' is configured to run the lambda immediately. }); } - private void doSomething(@RunsImmediately Runnable r) { + private void doSomething(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. +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 From a2e6ae5e77905e575588b2f92b4c64ad28950fb9 Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Sat, 1 Aug 2026 15:15:21 -0400 Subject: [PATCH 6/7] limpia --- .../threadsafety/GuardedByCheckerTest.java | 191 +++++++++--------- .../threadsafety/HeldLockAnalyzerTest.java | 4 +- docs/bugpattern/GuardedBy.md | 4 +- 3 files changed, 95 insertions(+), 104 deletions(-) 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 9e0b7e12b24..871b9789fb4 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 @@ -2426,30 +2426,26 @@ public synchronized void add(Optional x) { @Test public void knownImmediateMethods_lambda() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") + 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 int state = 0; - - public synchronized void modifyState(int newState) { - safeRun(() -> modifyStateInternal(newState)); - } + private final List xs = new ArrayList<>(); - @GuardedBy("this") - private void modifyStateInternal(int newState) { - state = newState; + public synchronized void f() { + runNow(() -> xs.clear()); } - private void safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) @@ -2457,31 +2453,27 @@ private void safeRun(Runnable runnable) { } @Test - public void knownImmediateMethods_flagNotSet_isFlagged() { - compilationHelper + 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 int state = 0; - - public synchronized void modifyState(int newState) { - // BUG: Diagnostic contains: should be guarded by 'this' - safeRun(() -> modifyStateInternal(newState)); - } + private final List xs = new ArrayList<>(); - @GuardedBy("this") - private void modifyStateInternal(int newState) { - state = newState; + public synchronized void f() { + runNow(xs::clear); } - private void safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) @@ -2489,31 +2481,30 @@ private void safeRun(Runnable runnable) { } @Test - public void knownImmediateMethods_methodReference() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") + 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 int state = 0; - - public synchronized void modifyState() { - safeRun(this::zeroStateInternal); - } + private final List xs = new ArrayList<>(); - @GuardedBy("this") - private void zeroStateInternal() { - state = 0; + 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 safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) @@ -2521,30 +2512,30 @@ private void safeRun(Runnable runnable) { } @Test - public void knownImmediateMethods_lambda_synchronizedBlock() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") + 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 { - private final Object lock = new Object(); + final Object mu = new Object(); - @GuardedBy("lock") - private int state = 0; + @GuardedBy("mu") + private final List xs = new ArrayList<>(); - public void modifyState(int newState) { - synchronized (lock) { - safeRun(() -> state = newState); - } + public synchronized void f() { + // BUG: Diagnostic contains: should be guarded by 'this.mu' + runNow(() -> xs.clear()); } - private void safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) @@ -2552,9 +2543,8 @@ private void safeRun(Runnable runnable) { } @Test - public void knownImmediateMethods_lambda_wrongGuard() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") + public void knownImmediateMethods_multipleAccessesInLambdaBody() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") .addSourceLines( "threadsafety/Test.java", """ @@ -2563,18 +2553,25 @@ public void knownImmediateMethods_lambda_wrongGuard() { import com.google.errorprone.annotations.concurrent.GuardedBy; class Test { - private final Object lock = new Object(); + final Object mu = new Object(); - @GuardedBy("lock") - private int state = 0; + @GuardedBy("this") + int x; - public synchronized void modifyState(int newState) { - // BUG: Diagnostic contains: should be guarded by 'this.lock' - safeRun(() -> state = newState); + @GuardedBy("mu") + int y; + + public synchronized void f() { + runNow( + () -> { + x++; + // BUG: Diagnostic contains: should be guarded by 'this.mu' + y++; + }); } - private void safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) @@ -2582,9 +2579,8 @@ private void safeRun(Runnable runnable) { } @Test - public void knownImmediateMethods_lambda_multipleGuardedAccesses() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Test#safeRun") + public void knownImmediateMethods_synchronizedBlock() { + compilationHelperWithKnownImmediateMethods("threadsafety.Test#runNow") .addSourceLines( "threadsafety/Test.java", """ @@ -2593,43 +2589,31 @@ public void knownImmediateMethods_lambda_multipleGuardedAccesses() { import com.google.errorprone.annotations.concurrent.GuardedBy; class Test { - @GuardedBy("this") - private int a = 0; - - @GuardedBy("this") - private int b = 0; - - private final Object other = new Object(); + final Object mu = new Object(); - @GuardedBy("other") - private int c = 0; + @GuardedBy("mu") + int x; - public synchronized void modify() { - safeRun( - () -> { - a = 1; - b = 2; - // BUG: Diagnostic contains: should be guarded by 'this.other' - c = 3; - }); + public void f() { + synchronized (mu) { + runNow(() -> x++); + } } - private void safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) .doTest(); } + /** Entries are comma-separated, and surrounding whitespace is ignored. */ @Test public void knownImmediateMethods_multipleEntries() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - // Entries are comma-separated and trimmed, and an entry naming a class which isn't on the - // compilation classpath is simply never matched. - .setArgs( - "-XepOpt:GuardedBy:KnownImmediateMethods=com.example.NotOnClasspath#run," - + " threadsafety.Test#safeRun") + // The first entry names a class which isn't on the compilation classpath; it never matches. + compilationHelperWithKnownImmediateMethods( + "com.example.NotOnClasspath#run, threadsafety.Test#runNow") .addSourceLines( "threadsafety/Test.java", """ @@ -2639,24 +2623,24 @@ public void knownImmediateMethods_multipleEntries() { class Test { @GuardedBy("this") - private int state = 0; + int x; - public synchronized void modifyState(int newState) { - safeRun(() -> state = newState); + public synchronized void f() { + runNow(() -> x++); } - private void safeRun(Runnable runnable) { - runnable.run(); + private void runNow(Runnable r) { + r.run(); } } """) .doTest(); } + /** Methods declared on subtypes of the listed class are matched too. */ @Test - public void knownImmediateMethods_matchesDescendantsOfListedClass() { - CompilationTestHelper.newInstance(GuardedByChecker.class, getClass()) - .setArgs("-XepOpt:GuardedBy:KnownImmediateMethods=threadsafety.Runner#runNow") + public void knownImmediateMethods_descendantOfListedClass() { + compilationHelperWithKnownImmediateMethods("threadsafety.Runner#runNow") .addSourceLines( "threadsafety/Runner.java", """ @@ -2686,14 +2670,14 @@ public void runNow(Runnable r) { import com.google.errorprone.annotations.concurrent.GuardedBy; class Test { - // The flag names the supertype, but the receiver's static type is the subtype. + // The flag names Runner, but the receiver's static type is the subtype. private final DirectRunner runner = new DirectRunner(); @GuardedBy("this") - private int state = 0; + int x; - public synchronized void modifyState(int newState) { - runner.runNow(() -> state = newState); + public synchronized void f() { + runner.runNow(() -> x++); } } """) @@ -2822,4 +2806,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 018528b6fb6..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 @@ -267,7 +267,9 @@ public void knownImmediateMethodsFlag_malformed_throws() { ErrorProneFlags flags = ErrorProneFlags.builder().putFlag("GuardedBy:KnownImmediateMethods", value).build(); assertThrows( - IllegalArgumentException.class, () -> HeldLockAnalyzer.invokesLambdasImmediately(flags)); + "GuardedBy:KnownImmediateMethods=\"" + value + "\"", + IllegalArgumentException.class, + () -> HeldLockAnalyzer.invokesLambdasImmediately(flags)); } } diff --git a/docs/bugpattern/GuardedBy.md b/docs/bugpattern/GuardedBy.md index 3a8fc6d5eff..2fb9b275bd5 100644 --- a/docs/bugpattern/GuardedBy.md +++ b/docs/bugpattern/GuardedBy.md @@ -199,7 +199,7 @@ class Transaction { public synchronized void handle() { doSomething(() -> { - x++; // OK: 'doSomething' is configured to run the lambda immediately. + ++x; // OK: 'doSomething' is configured to run the lambda immediately. }); } @@ -209,7 +209,7 @@ class Transaction { } ``` -Methods declared on subtypes of the listed class are matched too, and both +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 From 77967b107458cabd972eff47dce60c0788b10e4d Mon Sep 17 00:00:00 2001 From: Eamon Tracey Date: Sat, 1 Aug 2026 15:35:46 -0400 Subject: [PATCH 7/7] finalize beautiful --- .../threadsafety/GuardedByCheckerTest.java | 40 +++++++++++++++++-- 1 file changed, 36 insertions(+), 4 deletions(-) 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 871b9789fb4..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 @@ -2608,10 +2608,44 @@ private void runNow(Runnable r) { .doTest(); } - /** Entries are comma-separated, and surrounding whitespace is ignored. */ + @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() { - // The first entry names a class which isn't on the compilation classpath; it never matches. compilationHelperWithKnownImmediateMethods( "com.example.NotOnClasspath#run, threadsafety.Test#runNow") .addSourceLines( @@ -2637,7 +2671,6 @@ private void runNow(Runnable r) { .doTest(); } - /** Methods declared on subtypes of the listed class are matched too. */ @Test public void knownImmediateMethods_descendantOfListedClass() { compilationHelperWithKnownImmediateMethods("threadsafety.Runner#runNow") @@ -2670,7 +2703,6 @@ public void runNow(Runnable r) { import com.google.errorprone.annotations.concurrent.GuardedBy; class Test { - // The flag names Runner, but the receiver's static type is the subtype. private final DirectRunner runner = new DirectRunner(); @GuardedBy("this")