Skip to content

Commit

Permalink
Add FieldCanBeStatic checker
Browse files Browse the repository at this point in the history
This finds final fields which are initialized with a literal.  Adding
static reduces memory overhead from per-instance to per-class and
allows initialization at compile-time instead of run-time.

Fixes #930

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=312137814
  • Loading branch information
gaul authored and cgdecker committed May 20, 2020
1 parent 5fef0ab commit 46f2ddd
Show file tree
Hide file tree
Showing 4 changed files with 202 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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 [email protected] (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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -873,6 +874,7 @@ public static ScannerSupplier errorChecks() {
ExtendsAutoValue.class,
FieldCanBeFinal.class,
FieldCanBeLocal.class,
FieldCanBeStatic.class,
FieldMissingNullable.class,
ForEachIterable.class,
FunctionalInterfaceClash.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
19 changes: 19 additions & 0 deletions docs/bugpattern/FieldCanBeStatic.md
Original file line number Diff line number Diff line change
@@ -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;
```

0 comments on commit 46f2ddd

Please sign in to comment.