diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java b/core/src/main/java/com/google/errorprone/bugpatterns/NonFinalStaticField.java new file mode 100644 index 00000000000..456a2d7fff8 --- /dev/null +++ b/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() { + @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(); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 792e8321900..4fdb6d2f5f9 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/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, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NonFinalStaticFieldTest.java new file mode 100644 index 00000000000..1119819b308 --- /dev/null +++ b/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(); + } +} diff --git a/docs/bugpattern/NonFinalStaticField.md b/docs/bugpattern/NonFinalStaticField.md new file mode 100644 index 00000000000..588842c4eee --- /dev/null +++ b/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"; +```