From d78dd6d355db050d1420b2b88068566f515cfd1e Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 14 Dec 2023 04:09:45 -0800 Subject: [PATCH] Don't report NonFinalStaticField findings for fields modified in `@BeforeClass` methods PiperOrigin-RevId: 590890283 --- .../bugpatterns/NonFinalStaticField.java | 121 ++++++++++++------ .../bugpatterns/NonFinalStaticFieldTest.java | 16 +++ 2 files changed, 95 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java b/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java index 2628e3183d1..78622b104d0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java @@ -38,15 +38,16 @@ import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.CompoundAssignmentTree; +import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.UnaryTree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol.VarSymbol; import java.util.Objects; -import java.util.concurrent.atomic.AtomicBoolean; import javax.lang.model.element.Modifier; /** A BugPattern; see the summary. */ @@ -71,7 +72,11 @@ public Description matchVariable(VariableTree tree, VisitorState state) { .anyMatch(anno -> hasDirectAnnotationWithSimpleName(tree, anno))) { return NO_MATCH; } - if (!canBeRemoved(symbol, state) || isEverMutatedInSameCompilationUnit(symbol, state)) { + IsMutated everMutatedInSameCompilationUnit = isEverMutatedInSameCompilationUnit(symbol, state); + if (everMutatedInSameCompilationUnit == IsMutated.IN_BEFORE_METHOD) { + return NO_MATCH; + } + if (!canBeRemoved(symbol, state) || everMutatedInSameCompilationUnit == IsMutated.TRUE) { return describeMatch(tree); } return describeMatch( @@ -117,45 +122,77 @@ private static String getDefaultInitializer(VariableTree tree, VisitorState stat return "null"; } - private static boolean isEverMutatedInSameCompilationUnit(VarSymbol symbol, VisitorState state) { - AtomicBoolean seen = new AtomicBoolean(false); - new TreeScanner() { - @Override - public Void visitAssignment(AssignmentTree tree, Void unused) { - if (Objects.equals(getSymbol(tree.getVariable()), symbol)) { - seen.set(true); - } - return super.visitAssignment(tree, null); - } - - @Override - public Void visitCompoundAssignment(CompoundAssignmentTree tree, Void unused) { - if (Objects.equals(getSymbol(tree.getVariable()), symbol)) { - seen.set(true); - } - return super.visitCompoundAssignment(tree, null); - } - - @Override - public Void visitUnary(UnaryTree tree, Void unused) { - if (Objects.equals(getSymbol(tree.getExpression()), symbol) && isMutating(tree.getKind())) { - seen.set(true); - } - return super.visitUnary(tree, null); - } - - private boolean isMutating(Kind kind) { - switch (kind) { - case POSTFIX_DECREMENT: - case POSTFIX_INCREMENT: - case PREFIX_DECREMENT: - case PREFIX_INCREMENT: - return true; - default: - return false; - } - } - }.scan(state.getPath().getCompilationUnit(), null); - return seen.get(); + enum IsMutated { + TRUE, + FALSE, + IN_BEFORE_METHOD + } + + private static IsMutated isEverMutatedInSameCompilationUnit( + VarSymbol symbol, VisitorState state) { + var scanner = + new TreeScanner() { + IsMutated isMutated = IsMutated.FALSE; + + boolean inBeforeMethod = false; + + @Override + public Void visitAssignment(AssignmentTree tree, Void unused) { + if (Objects.equals(getSymbol(tree.getVariable()), symbol)) { + isMutated(); + } + return super.visitAssignment(tree, null); + } + + @Override + public Void visitCompoundAssignment(CompoundAssignmentTree tree, Void unused) { + if (Objects.equals(getSymbol(tree.getVariable()), symbol)) { + isMutated(); + } + return super.visitCompoundAssignment(tree, null); + } + + @Override + public Void visitUnary(UnaryTree tree, Void unused) { + if (Objects.equals(getSymbol(tree.getExpression()), symbol) + && isMutating(tree.getKind())) { + isMutated(); + } + return super.visitUnary(tree, null); + } + + private void isMutated() { + if (inBeforeMethod) { + isMutated = IsMutated.IN_BEFORE_METHOD; + } else if (isMutated.equals(IsMutated.FALSE)) { + isMutated = IsMutated.TRUE; + } + } + + private boolean isMutating(Kind kind) { + switch (kind) { + case POSTFIX_DECREMENT: + case POSTFIX_INCREMENT: + case PREFIX_DECREMENT: + case PREFIX_INCREMENT: + return true; + default: + return false; + } + } + + @Override + public Void visitMethod(MethodTree tree, Void unused) { + boolean prev = inBeforeMethod; + try { + inBeforeMethod |= ASTHelpers.hasAnnotation(tree, "org.junit.BeforeClass", state); + return super.visitMethod(tree, null); + } finally { + inBeforeMethod = prev; + } + } + }; + scanner.scan(state.getPath().getCompilationUnit(), null); + return scanner.isMutated; } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java index 028bdd284c5..6e350f629a6 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java @@ -211,4 +211,20 @@ public void volatileRemoved() { "}") .doTest(); } + + @Test + public void beforeClass() { + compilationTestHelper + .addSourceLines( + "Test.java", // + "import org.junit.BeforeClass;", + "public class Test {", + " private static String foo;", + " @BeforeClass", + " public static void setup() {", + " foo = \"\";", + " }", + "}") + .doTest(); + } }