diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FieldCanBeStatic.java b/core/src/main/java/com/google/errorprone/bugpatterns/FieldCanBeStatic.java new file mode 100644 index 00000000000..9439854edd2 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FieldCanBeStatic.java @@ -0,0 +1,85 @@ +/* + * Copyright 2020 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.SUGGESTION; +import static com.google.errorprone.matchers.Description.NO_MATCH; + +import com.google.common.base.Ascii; +import com.google.common.base.CaseFormat; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Modifier; + +/** @author andrew@gaul.org (Andrew Gaul) */ +@BugPattern( + name = "FieldCanBeStatic", + summary = + "A final field initialized at compile-time with an instance of an immutable type can be" + + " static.", + severity = SUGGESTION) +public class FieldCanBeStatic extends BugChecker implements VariableTreeMatcher { + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + VarSymbol sym = ASTHelpers.getSymbol(tree); + if (!canBeStatic(sym)) { + return NO_MATCH; + } + SuggestedFix.Builder fix = SuggestedFix.builder(); + SuggestedFixes.addModifiers(tree, state, Modifier.STATIC).ifPresent(fix::merge); + String name = tree.getName().toString(); + if (!name.equals(Ascii.toUpperCase(name))) { + String renamed = CaseFormat.LOWER_CAMEL.to(CaseFormat.UPPER_UNDERSCORE, name); + fix.merge(SuggestedFixes.renameVariable(tree, renamed, state)); + } + return describeMatch(tree.getModifiers(), fix.build()); + } + + private static boolean canBeStatic(VarSymbol sym) { + if (sym == null) { + return false; + } + if (!sym.getKind().equals(ElementKind.FIELD)) { + return false; + } + if (!sym.getModifiers().contains(Modifier.FINAL)) { + return false; + } + if (!sym.isPrivate()) { + return false; + } + if (sym.isStatic()) { + return false; + } + if (sym.getConstantValue() == null) { + return false; + } + if (sym.hasAnnotations()) { + return false; + } + return true; + } +} 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 28a424f07bc..1fd0a5fa707 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -109,6 +109,7 @@ import com.google.errorprone.bugpatterns.FallThrough; import com.google.errorprone.bugpatterns.FieldCanBeFinal; import com.google.errorprone.bugpatterns.FieldCanBeLocal; +import com.google.errorprone.bugpatterns.FieldCanBeStatic; import com.google.errorprone.bugpatterns.Finally; import com.google.errorprone.bugpatterns.FloatCast; import com.google.errorprone.bugpatterns.FloatingPointAssertionWithinEpsilon; @@ -873,6 +874,7 @@ public static ScannerSupplier errorChecks() { ExtendsAutoValue.class, FieldCanBeFinal.class, FieldCanBeLocal.class, + FieldCanBeStatic.class, FieldMissingNullable.class, ForEachIterable.class, FunctionalInterfaceClash.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/FieldCanBeStaticTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/FieldCanBeStaticTest.java new file mode 100644 index 00000000000..4105e839917 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/FieldCanBeStaticTest.java @@ -0,0 +1,96 @@ +/* + * Copyright 2018 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; + +/** {@link FieldCanBeStatic}Test */ +@RunWith(JUnit4.class) +public class FieldCanBeStaticTest { + + @Test + public void positive() { + CompilationTestHelper.newInstance(FieldCanBeStatic.class, getClass()) + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains:", + " private final int primitive = 3;", + " // BUG: Diagnostic contains:", + " private final String string = \"string\";", + "}") + .doTest(); + } + + @Test + public void negative() { + CompilationTestHelper.newInstance(FieldCanBeStatic.class, getClass()) + .addSourceLines( + "Test.java", + "class Test {", + " private static final String staticFinalInitializer;", + " static {", + " staticFinalInitializer = \"string\";", + " }", + " private static final String staticFinal = \"string\";", + " private int nonFinal = 3;", + " private static int staticNontFinal = 4;", + " private final Object finalMutable = new Object();", + " private final int nonLiteral = new java.util.Random().nextInt();", + " private final Person pojo = new Person(\"Bob\", 42);", + " private final String nullString = null;", + " @Deprecated private final String annotatedField = \"\";", + " private static class Person {", + " final String name;", + " final int age;", + " Person(String name, int age) {", + " this.name = name;", + " this.age = age;", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void refactoring() { + BugCheckerRefactoringTestHelper.newInstance(new FieldCanBeStatic(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " private final int foo = 1;", + " private final int BAR_FIELD = 2;", + " int f() {", + " return foo + BAR_FIELD;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " private static final int FOO = 1;", + " private static final int BAR_FIELD = 2;", + " int f() {", + " return FOO + BAR_FIELD;", + " }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/FieldCanBeStatic.md b/docs/bugpattern/FieldCanBeStatic.md new file mode 100644 index 00000000000..7f5aa30ce51 --- /dev/null +++ b/docs/bugpattern/FieldCanBeStatic.md @@ -0,0 +1,19 @@ +`final` fields initialized with a literal can elide a per-instance reference by +adding the `static` keyword. Since the field is `final` it is unmodifiable and +since its initializer is a literal the value is immutable and thus sharing a +per-class field is safe. This also allows a simpler constant load bytecode +instead of a field lookup. + +That is, prefer this: + +```java +static final String string = "string"; +static final int number = 42; +``` + +Not this: + +```java +final String string = "string"; +final int number = 42; +```