Report error for @nullable synchronized block expression (#1106)

uber · Dec 22, 2024 · 2754c45 · 2754c45
1 parent d0502e8
commit 2754c45
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 1 deletion.
diff --git a/jmh/src/test/java/com/uber/nullaway/jmh/BenchmarkCompilationTest.java b/jmh/src/test/java/com/uber/nullaway/jmh/BenchmarkCompilationTest.java
@@ -3,6 +3,7 @@
 import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
+import org.junit.Ignore;
 import org.junit.Test;
 
 /** Tests that all our JMH benchmarks compile successfully */
@@ -14,6 +15,7 @@ public void testAutodispose() throws IOException {
   }
 
   @Test
+  @Ignore("Need to update caffeine version; https://github.com/uber/NullAway/issues/1108")
   public void testCaffeine() throws IOException {
     assertTrue(new CaffeineCompiler().compile());
   }

diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -82,6 +82,7 @@
 import com.sun.source.tree.ReturnTree;
 import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.SwitchTree;
+import com.sun.source.tree.SynchronizedTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.TryTree;
 import com.sun.source.tree.TypeCastTree;
@@ -186,7 +187,8 @@ public class NullAway extends BugChecker
         BugChecker.CompoundAssignmentTreeMatcher,
         BugChecker.SwitchTreeMatcher,
         BugChecker.TypeCastTreeMatcher,
-        BugChecker.ParameterizedTypeTreeMatcher {
+        BugChecker.ParameterizedTypeTreeMatcher,
+        BugChecker.SynchronizedTreeMatcher {
 
   static final String INITIALIZATION_CHECK_NAME = "NullAway.Init";
   static final String OPTIONAL_CHECK_NAME = "NullAway.Optional";
@@ -1768,6 +1770,27 @@ public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState s
     return Description.NO_MATCH;
   }
 
+  @Override
+  public Description matchSynchronized(SynchronizedTree tree, VisitorState state) {
+    ExpressionTree lockExpr = tree.getExpression();
+    // For a synchronized block `synchronized (e) { ... }`, javac returns `(e)` as the expression.
+    // We strip the outermost parentheses for a nicer-looking error message.
+    if (lockExpr instanceof ParenthesizedTree) {
+      lockExpr = ((ParenthesizedTree) lockExpr).getExpression();
+    }
+    if (mayBeNullExpr(state, lockExpr)) {
+      ErrorMessage errorMessage =
+          new ErrorMessage(
+              MessageTypes.DEREFERENCE_NULLABLE,
+              "synchronized block expression \""
+                  + state.getSourceForNode(lockExpr)
+                  + "\" is @Nullable");
+      return errorBuilder.createErrorDescription(
+          errorMessage, buildDescription(lockExpr), state, null);
+    }
+    return Description.NO_MATCH;
+  }
+
   /**
    * Checks that all given expressions cannot be null, and for those that are {@code @Nullable},
    * reports an unboxing error.

diff --git a/nullaway/src/test/java/com/uber/nullaway/CoreTests.java b/nullaway/src/test/java/com/uber/nullaway/CoreTests.java
@@ -1071,4 +1071,23 @@ public void ternaryBothCasesNull() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void synchronizedDeref() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "TestCase.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "public class TestCase {",
+            "  public static void testPositive(@Nullable Object lock) {",
+            "    // BUG: Diagnostic contains: synchronized block expression \"lock\" is @Nullable",
+            "    synchronized (lock) {}",
+            "  }",
+            "  public static void testNegative(Object lock) {",
+            "    synchronized (lock) {}",
+            "  }",
+            "}")
+        .doTest();
+  }
 }