NonFinalStaticField: flag static fields which are non-final.

But only suggest adding a `final` if we're pretty sure it's OK.

PiperOrigin-RevId: 570982775

core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java

@@ -0,0 +1,106 @@
+/*
+ * 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;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
+import static com.google.errorprone.fixes.SuggestedFixes.addModifiers;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.canBeRemoved;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
+import static com.google.errorprone.util.ASTHelpers.isStatic;
+import static javax.lang.model.element.ElementKind.FIELD;
+import static javax.lang.model.element.Modifier.FINAL;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.CompoundAssignmentTree;
+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;
+
+/** A BugPattern; see the summary. */
+@BugPattern(summary = "Static fields should almost always be final.", severity = WARNING)
+public final class NonFinalStaticField extends BugChecker implements VariableTreeMatcher {
+  @Override
+  public Description matchVariable(VariableTree tree, VisitorState state) {
+    var symbol = getSymbol(tree);
+    if (!symbol.getKind().equals(FIELD)) {
+      return NO_MATCH;
+    }
+    if (!isStatic(symbol)) {
+      return NO_MATCH;
+    }
+    if (isConsideredFinal(symbol)) {
+      return NO_MATCH;
+    }
+    if (!canBeRemoved(symbol, state) || isEverMutatedInSameCompilationUnit(symbol, state)) {
+      return describeMatch(tree);
+    }
+    return describeMatch(tree, addModifiers(tree, state, FINAL).orElse(emptyFix()));
+  }
+
+  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();
+  }
+}

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -269,6 +269,7 @@
 import com.google.errorprone.bugpatterns.NonCanonicalStaticMemberImport;
 import com.google.errorprone.bugpatterns.NonCanonicalType;
 import com.google.errorprone.bugpatterns.NonFinalCompileTimeConstant;
+import com.google.errorprone.bugpatterns.NonFinalStaticField;
 import com.google.errorprone.bugpatterns.NonOverridingEquals;
 import com.google.errorprone.bugpatterns.NonRuntimeAnnotation;
 import com.google.errorprone.bugpatterns.NullOptional;
@@ -1158,6 +1159,7 @@ public static ScannerSupplier warningChecks() {
           MutableGuiceModule.class,
           NoAllocationChecker.class,
           NonCanonicalStaticMemberImport.class,
+          NonFinalStaticField.class, // Intentionally disabled in OSS.
           PackageLocation.class,
           ParameterComment.class,
           ParameterMissingNullable.class,

core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java

@@ -0,0 +1,158 @@
+/*
+ * 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;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public final class NonFinalStaticFieldTest {
+  private final CompilationTestHelper compilationTestHelper =
+      CompilationTestHelper.newInstance(NonFinalStaticField.class, getClass());
+
+  private final BugCheckerRefactoringTestHelper refactoringTestHelper =
+      BugCheckerRefactoringTestHelper.newInstance(NonFinalStaticField.class, getClass());
+
+  @Test
+  public void positiveFixable() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  private static String FOO = \"\";",
+            "}")
+        .addOutputLines(
+            "Test.java", //
+            "public class Test {",
+            "  private static final String FOO = \"\";",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void positiveButNotFixable_noRefactoring() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  public static String FOO = \"\";",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void positiveNotFixable_finding() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  // BUG: Diagnostic contains:",
+            "  public static String FOO = \"\";",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void negative() {
+    compilationTestHelper
+        .addSourceLines(
+            "Test.java", //
+            "public class Test {",
+            "  private static final String FOO = \"\";",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void reassigned_noFix() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  // BUG: Diagnostic contains:",
+            "  private static String foo = \"\";",
+            "  public void set(String s) {",
+            "    foo = s;",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void reassigned_finding() {
+    compilationTestHelper
+        .addSourceLines(
+            "Test.java", //
+            "public class Test {",
+            "  // BUG: Diagnostic contains:",
+            "  private static String foo = \"\";",
+            "  public void set(String s) {",
+            "    foo = s;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void incremented_noFix() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  // BUG: Diagnostic contains:",
+            "  private static int foo = 0;",
+            "  public void increment() {",
+            "    foo++;",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void incrementedAnotherWay_noFix() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  // BUG: Diagnostic contains:",
+            "  private static int foo = 0;",
+            "  public void increment() {",
+            "    foo += 1;",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void negativeInterface() {
+    compilationTestHelper
+        .addSourceLines(
+            "Test.java", //
+            "public interface Test {",
+            "  String FOO = \"\";",
+            "}")
+        .doTest();
+  }
+}

docs/bugpattern/NonFinalStaticField.md

@@ -0,0 +1,13 @@
+`static` fields should almost always be both `final` and deeply immutable.
+
+Instead of:
+
+```java
+private static String FOO = "foo";
+```
+
+Prefer:
+
+```java
+private static final String FOO = "foo";
+```