From 159ae7f9c55bf4030df6e0ca33bc5e75745a8bcc Mon Sep 17 00:00:00 2001
From: ghm <ghm@google.com>
Date: Mon, 16 Oct 2023 09:13:19 -0700
Subject: [PATCH] Remove `volatile` when finalling `static` fields.

Please do validate my claim, but I think there's simply no point in ever putting volatile on a static final field.

PiperOrigin-RevId: 573830549
---
 .../bugpatterns/NonFinalStaticField.java         |  7 ++++++-
 .../bugpatterns/NonFinalStaticFieldTest.java     | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

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 9c7a2e936a2..2628e3183d1 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java
@@ -20,6 +20,7 @@
 import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
 import static com.google.errorprone.fixes.SuggestedFix.merge;
 import static com.google.errorprone.fixes.SuggestedFixes.addModifiers;
+import static com.google.errorprone.fixes.SuggestedFixes.removeModifiers;
 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;
@@ -46,6 +47,7 @@
 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. */
 @BugPattern(summary = "Static fields should almost always be final.", severity = WARNING)
@@ -75,7 +77,10 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
     return describeMatch(
         tree,
         merge(
-            addModifiers(tree, state, FINAL).orElse(emptyFix()),
+            addModifiers(tree, tree.getModifiers(), state, ImmutableSet.of(FINAL))
+                .orElse(emptyFix()),
+            removeModifiers(tree.getModifiers(), state, ImmutableSet.of(Modifier.VOLATILE))
+                .orElse(emptyFix()),
             addDefaultInitializerIfNecessary(tree, state)));
   }
 
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 40cf01d2ab5..028bdd284c5 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java
@@ -195,4 +195,20 @@ public void exemptedAnnotation_noFinding() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void volatileRemoved() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java", //
+            "public class Test {",
+            "  private volatile static String FOO = \"\";",
+            "}")
+        .addOutputLines(
+            "Test.java", //
+            "public class Test {",
+            "  private static final String FOO = \"\";",
+            "}")
+        .doTest();
+  }
 }