Accept type variables that have a thread-safe upper bound as thread-s…

…afe types.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=191953714

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

@@ -83,8 +83,8 @@ Violation isThreadSafeType(
         allowContainerTypeParameters, containerTypeParameters, type);
   }
 
-  boolean isImmutableTypeParameter(TypeVariableSymbol sym) {
-    return threadSafety.isThreadSafeTypeParameter(sym);
+  boolean hasThreadSafeTypeParameterAnnotation(TypeVariableSymbol sym) {
+    return threadSafety.hasThreadSafeTypeParameterAnnotation(sym);
   }
 
   Violation checkInstantiation(

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

@@ -134,7 +134,7 @@ public Description matchTypeParameter(TypeParameterTree tree, VisitorState state
       return NO_MATCH;
     }
     ImmutableAnalysis analysis = new ImmutableAnalysis(this, state, wellKnownMutability);
-    if (!analysis.isImmutableTypeParameter((TypeVariableSymbol) sym)) {
+    if (!analysis.hasThreadSafeTypeParameterAnnotation((TypeVariableSymbol) sym)) {
       return NO_MATCH;
     }
     switch (sym.owner.getKind()) {
@@ -196,7 +196,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
             .filter(
                 e ->
                     annotation.containerOf().contains(e.getKey())
-                        && analysis.isImmutableTypeParameter(e.getValue()))
+                        && analysis.hasThreadSafeTypeParameterAnnotation(e.getValue()))
             .map(Entry::getKey)
             .collect(toImmutableSet());
     if (!immutableAndContainer.isEmpty()) {

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

@@ -30,7 +30,6 @@
 import com.google.common.collect.Streams;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.CanBeStaticAnalyzer;
-import com.google.errorprone.bugpatterns.threadsafety.ThreadSafety.Violation;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ClassTree;
 import com.sun.tools.javac.code.Attribute;
@@ -53,6 +52,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -77,6 +77,9 @@ public final class ThreadSafety {
   @Nullable private final Class<? extends Annotation> suppressAnnotation;
   @Nullable private final Class<? extends Annotation> typeParameterAnnotation;
 
+  /** Stores recursive invocations of {@link #isTypeParameterThreadSafe} */
+  private final Set<TypeVariableSymbol> recursiveThreadSafeTypeParameter = new HashSet<>();
+
   public static Builder builder() {
     return new Builder();
   }
@@ -278,7 +281,7 @@ public Violation threadSafeInstantiation(
     }
     for (int i = 0; i < type.tsym.getTypeParameters().size(); i++) {
       TypeVariableSymbol typaram = type.tsym.getTypeParameters().get(i);
-      boolean immutableTypeParameter = isThreadSafeTypeParameter(typaram);
+      boolean immutableTypeParameter = hasThreadSafeTypeParameterAnnotation(typaram);
       if (annotation.containerOf().contains(typaram.getSimpleName().toString())
           || immutableTypeParameter) {
         Type tyarg = type.getTypeArguments().get(i);
@@ -355,7 +358,8 @@ private boolean containerOfSubtyping(
     }
     // (2)
     if (!containerTypeParameters.contains(tyargument.asElement().getSimpleName().toString())
-        || isThreadSafeTypeParameter((TypeVariableSymbol) tyargument.asElement())) {
+        || isTypeParameterThreadSafe(
+            (TypeVariableSymbol) tyargument.asElement(), containerTypeParameters)) {
       return false;
     }
     // (3)
@@ -416,7 +420,7 @@ public Violation visitTypeVar(TypeVar type, Void s) {
       if (containerTypeParameters.contains(tyvar.getSimpleName().toString())) {
         return Violation.absent();
       }
-      if (isThreadSafeTypeParameter(tyvar)) {
+      if (isTypeParameterThreadSafe(tyvar, containerTypeParameters)) {
         return Violation.absent();
       }
       String message;
@@ -485,14 +489,51 @@ public Violation visitType(Type type, Void s) {
    * Returns true if the given type parameter's declaration is annotated with {@link
    * #typeParameterAnnotation} indicated it will only ever be instantiated with thread-safe types.
    */
-  public boolean isThreadSafeTypeParameter(TypeVariableSymbol symbol) {
+  public boolean hasThreadSafeTypeParameterAnnotation(TypeVariableSymbol symbol) {
     return typeParameterAnnotation != null
         && symbol
             .getAnnotationMirrors()
             .stream()
             .anyMatch(t -> t.type.tsym.flatName().contentEquals(typeParameterAnnotation.getName()));
   }
 
+  /**
+   * Returns whether a type parameter is thread-safe.
+   *
+   * <p>This is true if either the type parameter's declaration is annotated with {@link
+   * #typeParameterAnnotation} (indicating it can only be instantiated with thread-safe types), or
+   * the type parameter has a thread-safe upper bound (sub-classes of thread-safe types are also
+   * thread-safe).
+   *
+   * <p>If a type has a recursive bound, we recursively assume that this type satisfies all
+   * thread-safety constraints. Recursion can only happen with type variables that have recursive
+   * type bounds. These type variables do not need to be called out in the "containerOf" attribute
+   * or annotated with {@link #typeParameterAnnotation}.
+   *
+   * <p>Recursion does not apply to other kinds of types because all declared types must be
+   * annotated thread-safe, which means that thread-safety checkers don't need to analyze all
+   * referenced types recursively.
+   */
+  private boolean isTypeParameterThreadSafe(
+      TypeVariableSymbol symbol, Set<String> containerTypeParameters) {
+    if (!recursiveThreadSafeTypeParameter.add(symbol)) {
+      return true;
+    }
+    // TODO(b/77695285): Prevent type variables that are immutable because of an immutable upper
+    // bound to be marked thread-safe via containerOf or typeParameterAnnotation.
+    try {
+      for (Type bound : symbol.getBounds()) {
+        if (!isThreadSafeType(true, containerTypeParameters, bound).isPresent()) {
+          // A type variable is thread-safe if any upper bound is thread-safe.
+          return true;
+        }
+      }
+      return hasThreadSafeTypeParameterAnnotation(symbol);
+    } finally {
+      recursiveThreadSafeTypeParameter.remove(symbol);
+    }
+  }
+
   /**
    * Gets the {@link Symbol}'s annotation info, either from a marker annotation on the symbol, from
    * an accepted annotation on the symbol, or from the list of well-known types.
@@ -701,7 +742,7 @@ public Violation checkInstantiation(
             typeParameters.stream(),
             typeArguments.stream(),
             (sym, type) -> {
-              if (!isThreadSafeTypeParameter(sym)) {
+              if (!hasThreadSafeTypeParameterAnnotation(sym)) {
                 return Violation.absent();
               }
               Violation info =
@@ -725,7 +766,7 @@ public Violation checkInvocation(Type methodType, Symbol symbol) {
       return Violation.absent();
     }
     Collection<TypeVariableSymbol> typeParameters = symbol.getTypeParameters();
-    if (typeParameters.stream().noneMatch(this::isThreadSafeTypeParameter)) {
+    if (typeParameters.stream().noneMatch(this::hasThreadSafeTypeParameterAnnotation)) {
       // fast path
       return Violation.absent();
     }

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

@@ -1505,21 +1505,36 @@ public void immutableTypeParameter() {
   @Test
   public void immutableTypeParameterInstantiation() {
     compilationHelper
+        .addSourceLines(
+            "MyImmutableType.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable class MyImmutableType {}")
+        .addSourceLines(
+            "MyMutableType.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "class MyMutableType {}")
         .addSourceLines(
             "A.java",
             "import com.google.errorprone.annotations.ImmutableTypeParameter;",
             "import com.google.errorprone.annotations.Immutable;",
             "import com.google.common.collect.ImmutableList;",
-            "@Immutable class A<@ImmutableTypeParameter T> {",
-            "}")
+            "@Immutable class A<@ImmutableTypeParameter T> {}")
         .addSourceLines(
             "Test.java",
             "class Test {",
             "  A<String> f() {",
             "    return new A<>();",
             "  }",
             "  A<Object> g() {",
-            "   // BUG: Diagnostic contains: instantiation of 'T' is mutable, 'Object' is mutable",
+            "    // BUG: Diagnostic contains: instantiation of 'T' is mutable, 'Object' is mutable",
+            "    return new A<>();",
+            "  }",
+            "  <T extends MyImmutableType> A<T> h() {",
+            "    return new A<>();",
+            "  }",
+            "  <T extends MyMutableType> A<T> i() {",
+            "    // BUG: Diagnostic contains: "
+                + "instantiation of 'T' is mutable, 'T' is a mutable type variable",
             "    return new A<>();",
             "  }",
             "}")
@@ -1678,4 +1693,88 @@ public void immutableInterfaceImplementationCapturesMutableState() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void immutableUpperBound() {
+    compilationHelper
+        .addSourceLines(
+            "MyImmutableType.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable class MyImmutableType {}")
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.collect.ImmutableList;",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable class Test<T extends MyImmutableType, U extends T> {",
+            "  final T t = null;",
+            "  final U u = null;",
+            "  final ImmutableList<? extends U> v = null;",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void immutableRecursiveUpperBound() {
+    compilationHelper
+        .addSourceLines(
+            "Recursive.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable",
+            "abstract class Recursive<T extends Recursive<T>> {",
+            "  final T x = null;",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void immutableRecursiveUpperBound_notImmutable() {
+    compilationHelper
+        .addSourceLines(
+            "Recursive.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "import java.util.List;",
+            "@Immutable",
+            "abstract class Recursive<T extends Recursive<T>> {",
+            "  final T x = null;",
+            "  // BUG: Diagnostic contains: 'Recursive' has field 'y' of type 'java.util.List<T>'",
+            "  final List<T> y = null;",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void immutableUpperBoundAndContainerOfInconsistency() {
+    compilationHelper
+        .addSourceLines(
+            "ImmutableInterface.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable interface ImmutableInterface {}")
+        .addSourceLines(
+            "MutableImpl.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@SuppressWarnings(\"Immutable\") class MutableImpl implements ImmutableInterface {",
+            "  int mutableField;",
+            "}")
+        .addSourceLines(
+            "WithContainerOf.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable(containerOf=\"T\")",
+            "class WithContainerOf<T extends ImmutableInterface> { final T x = null; }")
+        .addSourceLines(
+            "WithoutContainerOf.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable",
+            "class WithoutContainerOf<T extends ImmutableInterface> { final T x = null; }")
+        .addSourceLines(
+            "Test.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "@Immutable class Test {",
+            "  final WithContainerOf<ImmutableInterface> a = null;",
+            "  final WithoutContainerOf<ImmutableInterface> b = null;",
+            "  // BUG: Diagnostic contains: field 'c' of type 'WithContainerOf<MutableImpl>'",
+            "  final WithContainerOf<MutableImpl> c = null;",
+            "  final WithoutContainerOf<MutableImpl> d = null;",
+            "}")
+        .doTest();
+  }
 }