From 5a29a1a41c8a5821e2456ac086e28dbf8c9d913a Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Fri, 16 Jul 2021 17:05:35 -0700 Subject: [PATCH] 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 --- .../threadsafety/GuardedByChecker.java | 11 +++- .../threadsafety/HeldLockAnalyzer.java | 51 ++++++++++++------- .../threadsafety/GuardedByCheckerTest.java | 27 ++++++++++ .../threadsafety/HeldLockAnalyzerTest.java | 6 +++ 4 files changed, 75 insertions(+), 20 deletions(-) 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 676eda0de1e..15e4d5a8813 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; @@ -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 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 47e038c547b..82c43b48685 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 @@ -85,10 +85,12 @@ public static void analyze( VisitorState state, LockEventListener listener, Predicate 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,6 +125,7 @@ private static class LockScanner extends TreePathScanner { private final LockEventListener listener; private final Predicate isSuppressed; private final GuardedByFlags flags; + private final boolean reportMissingGuards; private static final GuardedByExpression.Factory F = new GuardedByExpression.Factory(); @@ -130,11 +133,13 @@ private LockScanner( VisitorState visitorState, LockEventListener listener, Predicate 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 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 guard = + GuardedByBinder.bindString( + guardString, GuardedBySymbolResolver.from(tree, visitorState), flags); + if (!guard.isPresent()) { + if (reportMissingGuards) { + invalidLock(tree, locks, guardString); + } + continue; + } + Optional 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. */ 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 6edbe6fb3ab..ba816cccc5e 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 @@ -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(); + } } 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 ecf11d2c0f7..4eb7d33dd62 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 @@ -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) {