From 7e9a10089b731fcff39d711aab25bc2b8b8d0c5a Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Mon, 26 Aug 2024 11:34:45 -0700 Subject: [PATCH] Make ThreadSafeTypeParameter useful in the open-source version of ErrorProne. PiperOrigin-RevId: 667655941 --- .../errorprone/annotations/ThreadSafe.java | 5 +- .../threadsafety/ThreadSafety.java | 4 +- .../threadsafety/ThreadSafeCheckerTest.java | 150 ++++++++++++++++++ 3 files changed, 157 insertions(+), 2 deletions(-) diff --git a/annotations/src/main/java/com/google/errorprone/annotations/ThreadSafe.java b/annotations/src/main/java/com/google/errorprone/annotations/ThreadSafe.java index 547be45bb4e..190fe99e4f6 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/ThreadSafe.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/ThreadSafe.java @@ -83,7 +83,10 @@ *
  • it is annotated with {@link ThreadSafe}. * * - *

    This first requirement means the type is at least inherently shallowly thread-safe. + *

    This first requirement means the type is at least inherently shallowly thread-safe. For types + * with type parameters to be deemed deeply thread-safe, those of these types that denote + * containment must also be deemed thread-safe. A full explanation of this can be found in the + * {@link ThreadSafeTypeParameter} javadoc. * *

    Fields annotated with {@code javax.annotation.concurrent.GuardedBy} are likely the meat of a * mutable thread-safe class: these are things that need to be mutated, but should be done so in a diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java index 8e1ba318bd5..263bda79707 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java @@ -35,6 +35,7 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.Immutable; import com.google.errorprone.annotations.ThreadSafe; +import com.google.errorprone.annotations.ThreadSafeTypeParameter; import com.google.errorprone.bugpatterns.CanBeStaticAnalyzer; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; @@ -95,7 +96,8 @@ public static ThreadSafety.Builder threadSafeBuilder( .setPurpose(Purpose.FOR_THREAD_SAFE_CHECKER) .knownTypes(wellKnownThreadSafety) .markerAnnotations(ImmutableSet.of(ThreadSafe.class.getName())) - .acceptedAnnotations(ImmutableSet.of(Immutable.class.getName())); + .acceptedAnnotations(ImmutableSet.of(Immutable.class.getName())) + .typeParameterAnnotation(ImmutableSet.of(ThreadSafeTypeParameter.class.getName())); return builder; } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeCheckerTest.java index f8de59f8724..323a3e9f114 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeCheckerTest.java @@ -950,6 +950,29 @@ public void rawClass() { .doTest(); } + @Ignore("b/26797524 - add tests for generic arguments") + @Test + public void threadSafeTypeParam() { + compilationHelper + .addSourceLines( + "X.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.errorprone.annotations.ThreadSafe;", + "public class X {", + " final ImmutableList<@ThreadSafeTypeParameter ?> unknownSafeType;", + " X (ImmutableList<@ThreadSafeTypeParameter ?> unknownSafeType) {", + " this.unknownSafeType = unknownSafeType;", + " }", + "}") + .addSourceLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " final X badX = new X(ImmutableList.of(ImmutableList.of()));", + "}") + .doTest(); + } + @Ignore("b/26797524 - add tests for generic arguments") @Test public void mutableTypeParam() { @@ -1001,6 +1024,78 @@ public void knownThreadSafeFlag() { .doTest(); } + @Test + public void threadSafeTypeParameter() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "@ThreadSafe class Test<@ThreadSafeTypeParameter T> {", + " final T t = null;", + "}") + .doTest(); + } + + @Test + public void threadSafeTypeParameterInstantiation() { + compilationHelper + .addSourceLines( + "A.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "@ThreadSafe class A<@ThreadSafeTypeParameter T> {", + "}") + .addSourceLines( + "Test.java", + "class Test {", + " A f() {", + " return new A<>();", + " }", + " A g() {", + " // BUG: Diagnostic contains: instantiation of 'T' is not " + + "thread-safe, 'Object' is not thread-safe", + " return new A<>();", + " }", + "}") + .doTest(); + } + + @Test + public void threadSafeTypeParameterUsage() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "class Test {", + " static <@ThreadSafeTypeParameter T> void f() {}", + "}") + .doTest(); + } + + @Test + public void threadSafeTypeParameterUsage_interface() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "@ThreadSafe interface Test<@ThreadSafeTypeParameter T> {", + "}") + .doTest(); + } + + @Test + public void threadSafeTypeParameterMutableClass() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "// BUG: Diagnostic contains: @ThreadSafeTypeParameter is only supported on", + "class A<@ThreadSafeTypeParameter T> {}") + .doTest(); + } + @Test public void annotatedClassType() { compilationHelper @@ -1015,10 +1110,65 @@ public void annotatedClassType() { .doTest(); } + @Test + public void instantiationWithThreadSafeTypeParameter() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.errorprone.annotations.ThreadSafe;", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "@ThreadSafe public class Test<@ThreadSafeTypeParameter T> {", + " final ImmutableList xs = ImmutableList.of();", + "}") + .doTest(); + } + // Regression test for b/117937500 + @Test + public void notAllTypeVarsInstantiated() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "import java.util.function.Function;", + "class Test {", + " public final void f1(A transform) {}", + " public C f2(Function fn) {", + " return null;", + " }", + " public final void f3(Function fn) {", + " // BUG: Diagnostic contains: instantiation of 'C' is not thread-safe", + " // 'E' is a non-thread-safe type variable", + " f1(f2(fn));", + " }", + "}") + .doTest(); + } // javac does not instantiate type variables when they are not used for target typing, so we // cannot check whether their instantiations are thread-safe. + @Ignore + @Test + public void notAllTypeVarsInstantiated_shouldFail() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafeTypeParameter;", + "import java.util.function.Function;", + "class Test {", + " public final void f1(A transform) {}", + " public <@ThreadSafeTypeParameter B, C> C f2(Function fn) {", + " return null;", + " }", + " public final void f3(Function fn) {", + " // BUG: Diagnostic contains: instantiation of 'B' is not thread-safe", + " // 'D' is a non-thread-safe type variable", + " f1(f2(fn));", + " }", + "}") + .doTest(); + } @Test public void threadSafeUpperBound() {