Skip to content

Commit

Permalink
Introduce ThreadSafeRefactor, roughly the same as ImmutableRefactor b…
Browse files Browse the repository at this point in the history
…ut for @threadsafe.

This migrates JSR-305 ThreadSafe to EP's if possible, and if not, comments it out.

PiperOrigin-RevId: 514253080
  • Loading branch information
graememorgan authored and Error Prone Team committed Mar 9, 2023
1 parent 7a4507a commit 350912f
Show file tree
Hide file tree
Showing 3 changed files with 236 additions and 0 deletions.
@@ -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<? extends ImportTree> 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<ClassTree> notOk = new HashSet<>();
new TreePathScanner<Void, Void>() {
@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();
}
}
Expand Up @@ -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;
Expand Down Expand Up @@ -1136,6 +1137,7 @@ public static ScannerSupplier errorChecks() {
SystemExitOutsideMain.class,
SystemOut.class,
TestExceptionChecker.class,
ThreadSafeRefactoring.class,
ThrowSpecificExceptions.class,
ThrowsUncheckedException.class,
TimeUnitMismatch.class,
Expand Down
@@ -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);
}
}

0 comments on commit 350912f

Please sign in to comment.