Don't report NonFinalStaticField findings for fields modified in `@Be…

…foreClass` methods PiperOrigin-RevId: 590890283
google · Dec 14, 2023 · d78dd6d · d78dd6d
1 parent aadfdc3
commit d78dd6d
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 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
@@ -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<Void, Void>() {
-      @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<Void, Void>() {
+          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
@@ -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();
+  }
 }