Subtypes of Immutable classes should be consistently regarded as immu…

…table.

PiperOrigin-RevId: 456501408

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

@@ -124,19 +124,14 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
     if (info.isPresent()) {
       state.reportMatch(buildDescription(tree).setMessage(info.message()).build());
     }
-    if (!hasImmutableAnnotation(lambdaType, state)) {
+    if (!typeOrSuperHasImmutableAnnotation(lambdaType, state)) {
       return NO_MATCH;
     }
     checkClosedTypes(tree, state, lambdaType, analysis);
 
     return NO_MATCH;
   }
 
-  private boolean hasImmutableAnnotation(TypeSymbol tsym, VisitorState state) {
-    return immutableAnnotations.stream()
-        .anyMatch(annotation -> hasAnnotation(tsym, annotation, state));
-  }
-
   @Override
   public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) {
     // check instantiations of `@ImmutableTypeParameter`s in method references
@@ -150,7 +145,7 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s
     if (info.isPresent()) {
       state.reportMatch(buildDescription(tree).setMessage(info.message()).build());
     }
-    if (!hasImmutableAnnotation(memberReferenceType, state)) {
+    if (!typeOrSuperHasImmutableAnnotation(memberReferenceType, state)) {
       return NO_MATCH;
     }
     if (getSymbol(getReceiver(tree)) instanceof ClassSymbol) {
@@ -480,7 +475,7 @@ private void handleIdentifier(Symbol symbol) {
     for (var entry : typesClosed.asMap().entrySet()) {
       var classSymbol = entry.getKey();
       var methods = entry.getValue();
-      if (!hasImmutableAnnotation(classSymbol.type.tsym, state)) {
+      if (!typeOrSuperHasImmutableAnnotation(classSymbol.type.tsym, state)) {
         String message =
             format(
                 "%s, but accesses instance method(s) '%s' on '%s' which is not @Immutable.",
@@ -587,6 +582,15 @@ private Type immutableSupertype(Symbol sym, VisitorState state) {
     return null;
   }
 
+  private boolean hasImmutableAnnotation(TypeSymbol tsym, VisitorState state) {
+    return immutableAnnotations.stream()
+        .anyMatch(annotation -> hasAnnotation(tsym, annotation, state));
+  }
+
+  private boolean typeOrSuperHasImmutableAnnotation(TypeSymbol tsym, VisitorState state) {
+    return hasImmutableAnnotation(tsym, state) || immutableSupertype(tsym, state) != null;
+  }
+
   /**
    * Gets the set of in-scope immutable type parameters from the containerOf specs on
    * {@code @Immutable} annotations.

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

@@ -714,31 +714,36 @@ private AnnotationInfo getAnnotation(
     if (!(sym instanceof ClassSymbol)) {
       return null;
     }
-    Type superClass = ((ClassSymbol) sym).getSuperclass();
-    AnnotationInfo superAnnotation = getInheritedAnnotation(superClass.asElement(), state);
-    if (superAnnotation == null) {
-      return null;
-    }
-    // If an annotated super-type was found, look for any type arguments to the super-type that
-    // are in the super-type's containerOf spec, and where the arguments are type parameters
-    // of the current class.
-    // E.g. for `Foo<X> extends Super<X>` if `Super<Y>` is annotated
-    // `@ThreadSafeContainerAnnotation Y`
-    // then `Foo<X>` is has X implicitly annotated `@ThreadSafeContainerAnnotation X`
-    ImmutableList.Builder<String> containerOf = ImmutableList.builder();
-    for (int i = 0; i < superClass.getTypeArguments().size(); i++) {
-      Type arg = superClass.getTypeArguments().get(i);
-      TypeVariableSymbol formal = superClass.asElement().getTypeParameters().get(i);
-      if (!arg.hasTag(TypeTag.TYPEVAR)) {
+    for (Type superClass : state.getTypes().closure(sym.type)) {
+      if (superClass.tsym.equals(sym)) {
+        continue;
+      }
+      AnnotationInfo superAnnotation = getInheritedAnnotation(superClass.asElement(), state);
+      if (superAnnotation == null) {
         continue;
       }
-      TypeSymbol argSym = arg.asElement();
-      if (argSym.owner == sym
-          && superAnnotation.containerOf().contains(formal.getSimpleName().toString())) {
-        containerOf.add(argSym.getSimpleName().toString());
+      // If an annotated super-type was found, look for any type arguments to the super-type that
+      // are in the super-type's containerOf spec, and where the arguments are type parameters
+      // of the current class.
+      // E.g. for `Foo<X> extends Super<X>` if `Super<Y>` is annotated
+      // `@ThreadSafeContainerAnnotation Y`
+      // then `Foo<X>` is has X implicitly annotated `@ThreadSafeContainerAnnotation X`
+      ImmutableList.Builder<String> containerOf = ImmutableList.builder();
+      for (int i = 0; i < superClass.getTypeArguments().size(); i++) {
+        Type arg = superClass.getTypeArguments().get(i);
+        TypeVariableSymbol formal = superClass.asElement().getTypeParameters().get(i);
+        if (!arg.hasTag(TypeTag.TYPEVAR)) {
+          continue;
+        }
+        TypeSymbol argSym = arg.asElement();
+        if (argSym.owner == sym
+            && superAnnotation.containerOf().contains(formal.getSimpleName().toString())) {
+          containerOf.add(argSym.getSimpleName().toString());
+        }
       }
+      return AnnotationInfo.create(superAnnotation.typeName(), containerOf.build());
     }
-    return AnnotationInfo.create(superAnnotation.typeName(), containerOf.build());
+    return null;
   }
 
   /**

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

@@ -2006,7 +2006,8 @@ public void immutableUpperBoundAndContainerOfInconsistency() {
             "@Immutable class Test {",
             "  final WithContainerOf<ImmutableInterface> a = null;",
             "  final WithoutContainerOf<ImmutableInterface> b = null;",
-            "  // BUG: Diagnostic contains: field 'c' of type 'WithContainerOf<MutableImpl>'",
+            // Even though MutableImpl is mutable, it extends an @Immutable class, so we believe it
+            // is. (The violation is suppressed at the MutableImpl declaration.)
             "  final WithContainerOf<MutableImpl> c = null;",
             "  final WithoutContainerOf<MutableImpl> d = null;",
             "}")
@@ -2774,6 +2775,25 @@ public void methodReference_onMutableType() {
         .doTest();
   }
 
+  @Test
+  public void methodReference_onSubtypeOfImmutableType() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "import java.util.HashMap;",
+            "import java.util.Map;",
+            "abstract class Test {",
+            "  @Immutable interface ImmutableFunction { String apply(String b); }",
+            "  interface SubFunction extends ImmutableFunction {}",
+            "  void test(ImmutableFunction f) {",
+            "    SubFunction sf = null;",
+            "    test(sf::apply);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void methodReference_onExpressionWithMutableType() {
     compilationHelper