From 350912fab4bd594db76360aeee3671ba3c228ede Mon Sep 17 00:00:00 2001 From: ghm Date: Sun, 5 Mar 2023 15:57:04 -0800 Subject: [PATCH] Introduce ThreadSafeRefactor, roughly the same as ImmutableRefactor but for @ThreadSafe. This migrates JSR-305 ThreadSafe to EP's if possible, and if not, comments it out. PiperOrigin-RevId: 514253080 --- .../threadsafety/ThreadSafeRefactoring.java | 109 +++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../ThreadSafeRefactoringTest.java | 125 ++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoring.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoringTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoring.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoring.java new file mode 100644 index 00000000000..e32e5ee40b4 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoring.java @@ -0,0 +1,109 @@ +/* + * Copyright 2018 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.threadsafety; + +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.util.ASTHelpers.getAnnotationsWithSimpleName; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.ImportTree; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.code.Symbol; +import java.util.HashSet; +import java.util.Optional; +import java.util.Set; +import javax.inject.Inject; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "Refactors uses of the JSR 305 @ThreadSafe to Error Prone's annotation", + severity = SUGGESTION) +public final class ThreadSafeRefactoring extends BugChecker implements CompilationUnitTreeMatcher { + private final WellKnownThreadSafety wellKnownThreadSafety; + + @Inject + public ThreadSafeRefactoring(ErrorProneFlags flags) { + this.wellKnownThreadSafety = WellKnownThreadSafety.fromFlags(flags); + } + + @Override + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + Optional threadSafeImport = + tree.getImports().stream() + .filter( + i -> { + Symbol s = ASTHelpers.getSymbol(i.getQualifiedIdentifier()); + return s != null + && s.getQualifiedName() + .contentEquals(javax.annotation.concurrent.ThreadSafe.class.getName()); + }) + .findFirst(); + if (threadSafeImport.isEmpty()) { + return Description.NO_MATCH; + } + Set notOk = new HashSet<>(); + new TreePathScanner() { + @Override + public Void visitClass(ClassTree node, Void unused) { + if (!ASTHelpers.hasAnnotation( + node, javax.annotation.concurrent.ThreadSafe.class.getName(), state)) { + return super.visitClass(node, null); + } + boolean isThreadSafe = isThreadSafe(node, state); + if (!isThreadSafe) { + notOk.add(node); + } + return super.visitClass(node, null); + } + }.scan(state.getPath(), null); + + SuggestedFix.Builder fixBuilder = + SuggestedFix.builder() + .removeImport(javax.annotation.concurrent.ThreadSafe.class.getName()) + .addImport(com.google.errorprone.annotations.ThreadSafe.class.getName()); + for (ClassTree classTree : notOk) { + getAnnotationsWithSimpleName(classTree.getModifiers().getAnnotations(), "ThreadSafe") + .forEach(fixBuilder::delete); + fixBuilder.prefixWith( + classTree, + "// This class was annotated with javax.annotation.concurrent.ThreadSafe, but didn't seem" + + " to be provably thread safe." + + "\n"); + } + return describeMatch(threadSafeImport.get(), fixBuilder.build()); + } + + private boolean isThreadSafe(ClassTree tree, VisitorState state) { + var threadSafety = new ThreadSafeAnalysis(this, state, wellKnownThreadSafety); + + var violation = + threadSafety.checkForThreadSafety( + /* tree= */ Optional.empty(), + threadSafety.threadSafeTypeParametersInScope(ASTHelpers.getSymbol(tree)), + ASTHelpers.getType(tree)); + return !violation.isPresent(); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 6102472ab5f..35b17c531ce 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -529,6 +529,7 @@ import com.google.errorprone.bugpatterns.threadsafety.StaticGuardedByInstance; import com.google.errorprone.bugpatterns.threadsafety.SynchronizeOnNonFinalField; import com.google.errorprone.bugpatterns.threadsafety.ThreadPriorityCheck; +import com.google.errorprone.bugpatterns.threadsafety.ThreadSafeRefactoring; import com.google.errorprone.bugpatterns.time.DateChecker; import com.google.errorprone.bugpatterns.time.DurationFrom; import com.google.errorprone.bugpatterns.time.DurationGetTemporalUnit; @@ -1136,6 +1137,7 @@ public static ScannerSupplier errorChecks() { SystemExitOutsideMain.class, SystemOut.class, TestExceptionChecker.class, + ThreadSafeRefactoring.class, ThrowSpecificExceptions.class, ThrowsUncheckedException.class, TimeUnitMismatch.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoringTest.java new file mode 100644 index 00000000000..eb206dd6fc8 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafeRefactoringTest.java @@ -0,0 +1,125 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.threadsafety; + +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class ThreadSafeRefactoringTest { + private final BugCheckerRefactoringTestHelper compilationHelper = + BugCheckerRefactoringTestHelper.newInstance(ThreadSafeRefactoring.class, getClass()); + + @Test + public void positive() { + compilationHelper + .addInputLines( + "Test.java", + "import javax.annotation.concurrent.ThreadSafe;", + "@ThreadSafe class Test {", + " final int a = 42;", + " final String b = null;", + "}") + .addOutputLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "@ThreadSafe class Test {", + " final int a = 42;", + " final String b = null;", + "}") + .doTest(); + } + + @Test + public void someImmutableSomeNot() { + compilationHelper + .addInputLines( + "Test.java", + "import javax.annotation.concurrent.ThreadSafe;", + "@ThreadSafe class Test {", + " int a = 42;", + " @ThreadSafe static class Inner {", + " final int a = 43;", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "// This class was annotated with javax.annotation.concurrent.ThreadSafe, but didn't" + + " seem to be provably thread safe.", + "class Test {", + " int a = 42;", + " @ThreadSafe ", + " static class Inner {", + " final int a = 43;", + " }", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void negative() { + compilationHelper + .addInputLines( + "Test.java", + "import javax.annotation.concurrent.ThreadSafe;", + "@ThreadSafe class Test {", + " int a = 42;", + "}") + .addOutputLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "// This class was annotated with javax.annotation.concurrent.ThreadSafe, but didn't" + + " seem to be provably thread safe.", + "class Test {", + " int a = 42;", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void negative_multipleClasses() { + compilationHelper + .addInputLines( + "Test.java", + "import javax.annotation.concurrent.ThreadSafe;", + "@ThreadSafe class Test {", + " int a = 42;", + " @ThreadSafe static class Inner {", + " int a = 43;", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.errorprone.annotations.ThreadSafe;", + "// This class was annotated with javax.annotation.concurrent.ThreadSafe, but didn't" + + " seem to be provably thread safe.", + "class Test {", + " int a = 42;", + " // This class was annotated with javax.annotation.concurrent.ThreadSafe, but didn't" + + " seem to be provably thread safe.", + " static class Inner {", + " int a = 43;", + " }", + "}") + .doTest(TEXT_MATCH); + } +}