[inspection] IDEA-257415 Warnings for value-based classes (Java 16)

This renames ValueBasedWarningsInspection to SynchronizeOnValueBasedClassInspection and alters the logic, it checks first the original type of the monitor and if it's not a value-based class then it employs DFA to infer the type more precisely and checks the inferred type if it differs from the type of the monitor. This solution is more robust, because DFA might fail sometimes.

This patch also adds the suppressId to SynchronizeOnValueBasedClassInspection in order to support the new javac warning category.

Signed-off-by: Nikita Eshkeev <nikita.eshkeev@jetbrains.com>

GitOrigin-RevId: f7c3520b84bf6f9080280dc2689ff4f63ac4be09

java/java-impl/src/META-INF/JavaPlugin.xml

@@ -1799,12 +1799,11 @@
                      groupKey="group.names.code.style.issues" groupBundle="messages.InspectionsBundle"
                      enabledByDefault="true" level="INFORMATION"
                      implementationClass="com.intellij.codeInspection.FillPermitsListInspection"/>
-
-    <localInspection groupPath="Java" language="JAVA" shortName="ValueBasedWarnings"
+    <localInspection groupPath="Java" language="JAVA" suppressId="synchronization" shortName="SynchronizeOnValueBasedClass"
                      key="inspection.value.based.warnings" bundle="messages.JavaBundle"
                      groupKey="group.names.compiler.issues" groupBundle="messages.InspectionsBundle"
                      enabledByDefault="true" level="WARNING"
-                     implementationClass="com.intellij.codeInspection.valuebased.ValueBasedWarningsInspection"/>
+                     implementationClass="com.intellij.codeInspection.valuebased.SynchronizeOnValueBasedClassInspection"/>
 
     <globalInspection groupPath="Java" language="JAVA" shortName="EmptyMethod" groupKey="group.names.declaration.redundancy" enabledByDefault="true" groupBundle="messages.InspectionsBundle"
                       level="WARNING" implementationClass="com.intellij.codeInspection.emptyMethod.EmptyMethodInspection"

java/java-impl/src/com/intellij/codeInspection/valuebased/SynchronizeOnValueBasedClassInspection.java

@@ -14,9 +14,14 @@ import org.jetbrains.annotations.Contract;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
 
-public final class ValueBasedWarningsInspection extends LocalInspectionTool {
+public final class SynchronizeOnValueBasedClassInspection extends LocalInspectionTool {
   private static final @NonNls String ANNOTATION_NAME = "jdk.internal.ValueBased";
 
+  @Override
+  public @NotNull String getID() {
+    return "synchronization";
+  }
+
   @Override
   public @NotNull PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder,
                                                  boolean isOnTheFly) {
@@ -30,24 +35,38 @@ public final class ValueBasedWarningsInspection extends LocalInspectionTool {
         final PsiExpression monitor = statement.getLockExpression();
         if (monitor == null) return;
 
-        final TypeConstraint constraint = TypeConstraint.fromDfType(CommonDataflow.getDfType(monitor));
+        final PsiType monitorType = monitor.getType();
+        if (monitorType == null) return;
 
-        final PsiType type = constraint.getPsiType(statement.getProject());
+        if (!isValueBasedClass(monitorType)) {
+          final TypeConstraint constraint = TypeConstraint.fromDfType(CommonDataflow.getDfType(monitor));
+          final PsiType inferredType = constraint.getPsiType(statement.getProject());
 
-        if (!extendsValueBasedClass(type)) return;
+          if (monitorType.equals(inferredType)) return;
+
+          if (!isValueBasedClass(inferredType)) return;
+        }
 
         holder.registerProblem(monitor, JavaBundle.message("inspection.value.based.warnings.synchronization"));
       }
     };
   }
 
+  /**
+   * A class is considered a value-based class when it is annotated with <code>jdk.internal.ValueBased</code>.
+   * Wherever the annotation is applied to an abstract class or interface, it is also applied to all subclasses in the JDK,
+   * so all such subclasses are considered value-based classes
+   *
+   * @param type type to decide if it's of a value-based class
+   * @return true when the argument is of a value-based class
+   */
   @Contract(value = "null -> false", pure = true)
-  private static boolean extendsValueBasedClass(PsiType type) {
+  private static boolean isValueBasedClass(PsiType type) {
     final PsiClass classType = PsiTypesUtil.getPsiClass(type);
     if (classType == null) return false;
 
     if (classType.hasAnnotation(ANNOTATION_NAME)) return true;
 
-    return ContainerUtil.or(type.getSuperTypes(), superType -> extendsValueBasedClass(superType));
+    return ContainerUtil.or(type.getSuperTypes(), superType -> isValueBasedClass(superType));
   }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/ComplexValueBasedHierarchy.java

@@ -25,4 +25,9 @@ class Main {
   void g(Object vb) {
     synchronized (vb) {}
   }
+
+  @SuppressWarnings("synchronization")
+  void h(ComplexVBHierarchy vb) {
+    synchronized (vb) {}
+  }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/ExtendValueBasedClass.java

@@ -11,6 +11,8 @@ class AC extends AbstractValueBased {
     synchronized (<warning descr="Attempt to synchronize on an instance of a value-based class">localAc</warning>) {}
     synchronized (AC.class) {}
 
+    synchronized (new Object()) {}
+
     f(ac);
     g(ac);
   }
@@ -22,4 +24,9 @@ class AC extends AbstractValueBased {
   void g(Object ac) {
     synchronized (ac) {}
   }
+
+  @SuppressWarnings("synchronization")
+  void h(AC ac) {
+    synchronized (ac) {}
+  }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/ImplementValueBasedInterface.java

@@ -22,4 +22,9 @@ class IVB implements IValueBased {
   void g(Object vb) {
     synchronized (vb) {}
   }
+
+  @SuppressWarnings("synchronization")
+  void h(IVB vb) {
+    synchronized (vb) {}
+  }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/UseValueBasedInstanceAsMonitor.java

@@ -21,4 +21,9 @@ class Main {
   void g(Object vb) {
     synchronized (vb) {}
   }
+
+  @SuppressWarnings("synchronization")
+  void h(OpenValueBased vb) {
+    synchronized (vb) {}
+  }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/classes/AbstractValueBased.java

@@ -1,4 +1,4 @@
 package valuebased.classes;
-package valuebased.classes;
+
 @jdk.internal.ValueBased
 public abstract static class AbstractValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/classes/IValueBased.java

@@ -1,3 +1,4 @@
 package valuebased.classes;
+
 @jdk.internal.ValueBased
 public interface IValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingClass.java

@@ -0,0 +1,14 @@
+// "Suppress for class" "true"
+
+@SuppressWarnings("synchronization")
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  {
+    synchronized(vb){ }
+  }
+}
+
+@jdk.internal.ValueBased
+class OpenValueBased {}
+

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingClassFromAnonymousClass.java

@@ -0,0 +1,17 @@
+// "Suppress for class" "true"
+
+@SuppressWarnings("synchronization")
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      {
+        synchronized (vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingClassFromLocalVariable.java

@@ -0,0 +1,17 @@
+// "Suppress for class" "true"
+
+@SuppressWarnings("synchronization")
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    var l = ((new OpenValueBased() {
+      {
+        synchronized (vb){ }
+      }
+    }));
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingClassFromMethod.java

@@ -0,0 +1,14 @@
+// "Suppress for class" "true"
+
+@SuppressWarnings("synchronization")
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f{
+    synchronized(vb){ }
+  }
+}
+
+@jdk.internal.ValueBased
+class OpenValueBased {}
+

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingLocalVariable.java

@@ -0,0 +1,17 @@
+// "Suppress for statement" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    var l = ((new OpenValueBased() {
+      {
+          //noinspection synchronization
+          synchronized (vb){ }
+      }
+    }));
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingLocalVariableFromAnonymousClass.java

@@ -0,0 +1,17 @@
+// "Suppress for statement" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      {
+          //noinspection synchronization
+          synchronized (vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingMethod.java

@@ -0,0 +1,14 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  @SuppressWarnings("synchronization")
+  void f(){
+    synchronized(vb){ }
+  }
+}
+
+@jdk.internal.ValueBased
+class OpenValueBased {}
+

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingMethodFromAnonymousClass.java

@@ -0,0 +1,17 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  @SuppressWarnings("synchronization")
+  void f(){
+    new OpenValueBased() {
+      {
+        synchronized (vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingMethodFromLocalVariable.java

@@ -0,0 +1,17 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  @SuppressWarnings("synchronization")
+  void f(){
+    var l = ((new OpenValueBased() {
+      {
+        synchronized (vb){ }
+      }
+    }));
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/afterSuppressEnclosingMethodInsideAnonymousClass.java

@@ -0,0 +1,20 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      @SuppressWarnings("synchronization")
+      @Override
+      void g() {
+        synchronized (vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased {
+  void g();
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingClass.java

@@ -0,0 +1,13 @@
+// "Suppress for class" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  {
+    synchronized(<caret>vb){ }
+  }
+}
+
+@jdk.internal.ValueBased
+class OpenValueBased {}
+

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingClassFromAnonymousClass.java

@@ -0,0 +1,16 @@
+// "Suppress for class" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      {
+        synchronized (<caret>vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingClassFromLocalVariable.java

@@ -0,0 +1,16 @@
+// "Suppress for class" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    var l = ((new OpenValueBased() {
+      {
+        synchronized (<caret>vb){ }
+      }
+    }));
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingClassFromMethod.java

@@ -0,0 +1,13 @@
+// "Suppress for class" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f{
+    synchronized(<caret>vb){ }
+  }
+}
+
+@jdk.internal.ValueBased
+class OpenValueBased {}
+

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingLocalVariable.java

@@ -0,0 +1,16 @@
+// "Suppress for statement" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    var l = ((new OpenValueBased() {
+      {
+        synchronized (<caret>vb){ }
+      }
+    }));
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingLocalVariableFromAnonymousClass.java

@@ -0,0 +1,16 @@
+// "Suppress for statement" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      {
+        synchronized (<caret>vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingMethod.java

@@ -0,0 +1,13 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    synchronized(<caret>vb){ }
+  }
+}
+
+@jdk.internal.ValueBased
+class OpenValueBased {}
+

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingMethodFromAnonymousClass.java

@@ -0,0 +1,16 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      {
+        synchronized (<caret>vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingMethodFromLocalVariable.java

@@ -0,0 +1,16 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    var l = ((new OpenValueBased() {
+      {
+        synchronized (<caret>vb){ }
+      }
+    }));
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased { }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/valuebased/quickfix/beforeSuppressEnclosingMethodInsideAnonymousClass.java

@@ -0,0 +1,19 @@
+// "Suppress for method" "true"
+
+class Main {
+  final OpenValueBased vb = new OpenValueBased();
+
+  void f(){
+    new OpenValueBased() {
+      @Override
+      void g() {
+        synchronized (<caret>vb){ }
+      }
+    };
+  }
+}
+
+@jdk.internal.ValueBased
+interface OpenValueBased {
+  void g();
+}

java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/valuebased/SynchronizeOnValueBasedClassFixTest.java

@@ -0,0 +1,29 @@
+// Copyright 2000-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.java.codeInsight.daemon.valuebased;
+
+import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase;
+import com.intellij.codeInspection.LocalInspectionTool;
+import com.intellij.codeInspection.valuebased.SynchronizeOnValueBasedClassInspection;
+import com.intellij.testFramework.LightProjectDescriptor;
+import org.jetbrains.annotations.NonNls;
+import org.jetbrains.annotations.NotNull;
+
+import static com.intellij.testFramework.fixtures.LightJavaCodeInsightFixtureTestCase.JAVA_16;
+
+public class SynchronizeOnValueBasedClassFixTest extends LightQuickFixParameterizedTestCase {
+
+  @Override
+  protected @NonNls String getBasePath() {
+    return "/codeInsight/daemonCodeAnalyzer/valuebased/quickfix";
+  }
+
+  @Override
+  protected LocalInspectionTool @NotNull [] configureLocalInspectionTools() {
+    return new LocalInspectionTool[]{ new SynchronizeOnValueBasedClassInspection() };
+  }
+
+  @Override
+  protected @NotNull LightProjectDescriptor getProjectDescriptor() {
+    return JAVA_16;
+  }
+}

java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/valuebased/SynchronizeOnValueBasedClassHighlightTest.java

@@ -2,13 +2,13 @@
 package com.intellij.java.codeInsight.daemon.valuebased;
 
 import com.intellij.JavaTestUtil;
-import com.intellij.codeInspection.valuebased.ValueBasedWarningsInspection;
+import com.intellij.codeInspection.valuebased.SynchronizeOnValueBasedClassInspection;
 import com.intellij.testFramework.LightProjectDescriptor;
 import com.intellij.testFramework.fixtures.LightJavaCodeInsightFixtureTestCase;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
 
-public class ValueBasedWarningsTest extends LightJavaCodeInsightFixtureTestCase {
+public class SynchronizeOnValueBasedClassHighlightTest extends LightJavaCodeInsightFixtureTestCase {
   static final @NonNls String BASE_PATH = "/codeInsight/daemonCodeAnalyzer/valuebased";
 
   @Override
@@ -21,7 +21,7 @@ public class ValueBasedWarningsTest extends LightJavaCodeInsightFixtureTestCase
     myFixture.configureByFile(abstractValueBased);
     myFixture.configureByFile(ivalueBased);
     myFixture.configureByFile(openValueBased);
-    myFixture.enableInspections(new ValueBasedWarningsInspection());
+    myFixture.enableInspections(new SynchronizeOnValueBasedClassInspection());
   }
 
   public void testExtendValueBasedClass() { doTest(); }

plugins/InspectionGadgets/src/inspectionDescriptions/SynchronizeOnValueBasedClass.html

@@ -0,0 +1,14 @@
+<html>
+<body>
+Attempts to synchronize on an instance of a value-based class will produce compile-time warnings and raise run-time exceptions starting from Java 16.
+<p>
+  For example, java.lang.Double is annotated with <code>jdk.internal.ValueBased</code>, so the following code will produce a compile-time warning
+</p>
+<pre>
+Double d = 20.0;
+synchronized (d) { ... } // javac warning
+</pre>
+<!-- tooltip end -->
+<p>Since 2021.1</p>
+</body>
+</html>

plugins/InspectionGadgets/src/inspectionDescriptions/ValueBasedWarnings.html

@@ -1,7 +0,0 @@
-<html>
-<body>
-Value based warnings are meant to warn about invalid usage of value-based classes
-<!-- tooltip end -->
-<p>Since 2021.1</p>
-</body>
-</html>