Report an error for uses of @GuardedBy members with unresolvable locks

these errors are usually also reported at the declaration site, but this
change ensures coverage if the declaration site diagnostics are suppressed,
or if the string references a private field that isn't visible in the interface
jar.

PiperOrigin-RevId: 385251336

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;
@@ -53,6 +54,13 @@ public class GuardedByChecker extends BugChecker
 
   private final GuardedByFlags flags = GuardedByFlags.allOn();
 
+  private final boolean reportMissingGuards;
+
+  public GuardedByChecker(ErrorProneFlags errorProneFlags) {
+    reportMissingGuards =
+        errorProneFlags.getBoolean("GuardedByChecker:reportMissingGuards").orElse(false);
+  }
+
   @Override
   public Description matchMethod(MethodTree tree, final VisitorState state) {
     // Constructors (and field initializers, instance initalizers, and class initalizers) are free
@@ -77,7 +85,8 @@ private void analyze(VisitorState state) {
         (ExpressionTree tree, GuardedByExpression guard, HeldLockSet live) ->
             report(GuardedByChecker.this.checkGuardedAccess(tree, guard, live, state), state),
         this::isSuppressed,
-        flags);
+        flags,
+        reportMissingGuards);
   }
 
   @Override

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java

@@ -85,10 +85,12 @@ public static void analyze(
       VisitorState state,
       LockEventListener listener,
       Predicate<Tree> isSuppressed,
-      GuardedByFlags flags) {
+      GuardedByFlags flags,
+      boolean reportMissingGuards) {
     HeldLockSet locks = HeldLockSet.empty();
     locks = handleMonitorGuards(state, locks, flags);
-    new LockScanner(state, listener, isSuppressed, flags).scan(state.getPath(), locks);
+    new LockScanner(state, listener, isSuppressed, flags, reportMissingGuards)
+        .scan(state.getPath(), locks);
   }
 
   // Don't use Class#getName() for inner classes, we don't want `Monitor$Guard`
@@ -123,18 +125,21 @@ private static class LockScanner extends TreePathScanner<Void, HeldLockSet> {
     private final LockEventListener listener;
     private final Predicate<Tree> isSuppressed;
     private final GuardedByFlags flags;
+    private final boolean reportMissingGuards;
 
     private static final GuardedByExpression.Factory F = new GuardedByExpression.Factory();
 
     private LockScanner(
         VisitorState visitorState,
         LockEventListener listener,
         Predicate<Tree> isSuppressed,
-        GuardedByFlags flags) {
+        GuardedByFlags flags,
+        boolean reportMissingGuards) {
       this.visitorState = visitorState;
       this.listener = listener;
       this.isSuppressed = isSuppressed;
       this.flags = flags;
+      this.reportMissingGuards = reportMissingGuards;
     }
 
     @Override
@@ -239,24 +244,32 @@ public Void visitClass(ClassTree node, HeldLockSet locks) {
 
     private void checkMatch(ExpressionTree tree, HeldLockSet locks) {
       for (String guardString : GuardedByUtils.getGuardValues(tree, visitorState)) {
-        GuardedByBinder.bindString(
-                guardString, GuardedBySymbolResolver.from(tree, visitorState), flags)
-            .ifPresent(
-                guard -> {
-                  Optional<GuardedByExpression> boundGuard =
-                      ExpectedLockCalculator.from(
-                          (JCTree.JCExpression) tree, guard, visitorState, flags);
-                  if (!boundGuard.isPresent()) {
-                    // We couldn't resolve a guarded by expression in the current scope, so we can't
-                    // guarantee the access is protected and must report an error to be safe.
-                    listener.handleGuardedAccess(
-                        tree, new GuardedByExpression.Factory().error(guardString), locks);
-                    return;
-                  }
-                  listener.handleGuardedAccess(tree, boundGuard.get(), locks);
-                });
+        Optional<GuardedByExpression> guard =
+            GuardedByBinder.bindString(
+                guardString, GuardedBySymbolResolver.from(tree, visitorState), flags);
+        if (!guard.isPresent()) {
+          if (reportMissingGuards) {
+            invalidLock(tree, locks, guardString);
+          }
+          continue;
+        }
+        Optional<GuardedByExpression> boundGuard =
+            ExpectedLockCalculator.from(
+                (JCTree.JCExpression) tree, guard.get(), visitorState, flags);
+        if (!boundGuard.isPresent()) {
+          // We couldn't resolve a guarded by expression in the current scope, so we can't
+          // guarantee the access is protected and must report an error to be safe.
+          invalidLock(tree, locks, guardString);
+          continue;
+        }
+        listener.handleGuardedAccess(tree, boundGuard.get(), locks);
       }
     }
+
+    private void invalidLock(ExpressionTree tree, HeldLockSet locks, String guardString) {
+      listener.handleGuardedAccess(
+          tree, new GuardedByExpression.Factory().error(guardString), locks);
+    }
   }
 
   /** An abstraction over the lock classes we understand. */

core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByCheckerTest.java

@@ -1670,10 +1670,12 @@ public void testStaticMemberClass_enclosingInstanceLock() {
             "  }",
             "  public void m(Baz b) {",
             "    synchronized (mu) {",
+            "      // BUG: Diagnostic contains: 'mu', which could not be resolved",
             "      b.x++;",
             "    }",
             "  }",
             "}")
+        .setArgs("-XepOpt:GuardedByChecker:reportMissingGuards=true")
         .doTest();
   }
 
@@ -1748,4 +1750,29 @@ public void suppressionOnMethod() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void testMissingGuard() {
+    compilationHelper
+        .addSourceLines(
+            "threadsafety/Lib.java",
+            "package threadsafety;",
+            "import javax.annotation.concurrent.GuardedBy;",
+            "@SuppressWarnings(\"GuardedBy\")",
+            "class Lib {",
+            "  @GuardedBy(\"lock\")",
+            "  public void doSomething() {}",
+            "}")
+        .addSourceLines(
+            "threadsafety/Test.java",
+            "package threadsafety;",
+            "class Test {",
+            "  void m(Lib lib) {",
+            "    // BUG: Diagnostic contains: 'lock', which could not be resolved",
+            "    lib.doSomething();",
+            "  }",
+            "}")
+        .setArgs("-XepOpt:GuardedByChecker:reportMissingGuards=true")
+        .doTest();
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzerTest.java

@@ -20,6 +20,7 @@
 
 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;
@@ -245,6 +246,11 @@ public void testLockMethodEnclosingAccess() {
   /** A customized {@link GuardedByChecker} that prints more test-friendly diagnostics. */
   @BugPattern(name = "GuardedByLockSet", summary = "", explanation = "", severity = ERROR)
   public static class GuardedByLockSetAnalyzer extends GuardedByChecker {
+
+    public GuardedByLockSetAnalyzer(ErrorProneFlags errorProneFlags) {
+      super(errorProneFlags);
+    }
+
     @Override
     protected Description checkGuardedAccess(
         Tree tree, GuardedByExpression guard, HeldLockSet live, VisitorState state) {