diff --git a/core/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java b/core/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java index a33cbd45f9c..1b20681e58e 100644 --- a/core/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java +++ b/core/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java @@ -72,12 +72,15 @@ public boolean isEmpty() { public void onDescribed(Description description) { // Use only first (most likely) suggested fix if (description.fixes.size() > 0) { - Fix fix = description.fixes.get(0); - importsToAdd.addAll(fix.getImportsToAdd()); - importsToRemove.addAll(fix.getImportsToRemove()); - for (Replacement replacement : fix.getReplacements(endPositions)) { - replacements.add(replacement); - } + handleFix(description.fixes.get(0)); + } + } + + public void handleFix(Fix fix) { + importsToAdd.addAll(fix.getImportsToAdd()); + importsToRemove.addAll(fix.getImportsToRemove()); + for (Replacement replacement : fix.getReplacements(endPositions)) { + replacements.add(replacement); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java new file mode 100644 index 00000000000..86df89ed10c --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java @@ -0,0 +1,88 @@ +/* + * Copyright 2016 Google Inc. All Rights Reserved. + * + * 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.Category.JDK; +import static com.google.errorprone.BugPattern.MaturityLevel.MATURE; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; + +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.Fix; +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; +import com.sun.tools.javac.tree.JCTree; +import com.sun.tools.javac.tree.TreeScanner; + +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Modifier; + +/** @author cushon@google.com (Liam Miller-Cushon) */ +@BugPattern( + name = "ConstantField", + category = JDK, + summary = "Field name is CONSTANT_CASE, but field is not static and final", + severity = SUGGESTION, + maturity = MATURE +) +public class ConstantField extends BugChecker implements VariableTreeMatcher { + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + Symbol.VarSymbol sym = ASTHelpers.getSymbol(tree); + if (sym == null || sym.getKind() != ElementKind.FIELD) { + return Description.NO_MATCH; + } + if (sym.isStatic() && sym.getModifiers().contains(Modifier.FINAL)) { + return Description.NO_MATCH; + } + String name = sym.getSimpleName().toString(); + if (!name.equals(name.toUpperCase())) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .addFix(SuggestedFixes.addModifiers(tree, state, Modifier.FINAL, Modifier.STATIC)) + .addFix(renameFix(tree, state, name)) + .build(); + } + + private Fix renameFix(VariableTree tree, VisitorState state, String name) { + int pos = ((JCTree) tree).getStartPosition() + state.getSourceForNode(tree).indexOf(name); + final String renamed = CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, name); + final SuggestedFix.Builder fix = + SuggestedFix.builder().replace(pos, pos + name.length(), renamed); + final Symbol.VarSymbol sym = ASTHelpers.getSymbol(tree); + ((JCTree) state.getPath().getCompilationUnit()) + .accept( + new TreeScanner() { + @Override + public void visitIdent(JCTree.JCIdent tree) { + if (sym.equals(ASTHelpers.getSymbol(tree))) { + fix.replace(tree, renamed); + } + } + }); + return fix.build(); + } +} 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 6acfc61226f..f2d3999339b 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -41,6 +41,7 @@ import com.google.errorprone.bugpatterns.ComparisonContractViolated; import com.google.errorprone.bugpatterns.ComparisonOutOfRange; import com.google.errorprone.bugpatterns.CompileTimeConstantChecker; +import com.google.errorprone.bugpatterns.ConstantField; import com.google.errorprone.bugpatterns.DeadException; import com.google.errorprone.bugpatterns.DepAnn; import com.google.errorprone.bugpatterns.DivZero; @@ -342,6 +343,7 @@ public static ScannerSupplier errorChecks() { ProtoStringFieldReferenceEquality.class, RemoveUnusedImports.class, ScopeAnnotationOnInterfaceOrAbstractClass.class, + ConstantField.class, ScopeOrQualifierAnnotationRetention.class, SelfComparison.class, SelfEquality.class, diff --git a/core/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java b/core/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java index a4aed0d0901..cc3fdf673bc 100644 --- a/core/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java +++ b/core/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java @@ -27,6 +27,8 @@ import com.google.errorprone.apply.DescriptionBasedDiff; import com.google.errorprone.apply.SourceFile; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.matchers.Description; import com.google.errorprone.scanner.ErrorProneScanner; import com.google.errorprone.scanner.ErrorProneScannerTransformer; import com.google.testing.compile.JavaFileObjects; @@ -41,6 +43,7 @@ import java.io.IOException; import java.util.HashMap; +import java.util.List; import java.util.Map; import javax.tools.DiagnosticCollector; @@ -58,11 +61,11 @@ public class BugCheckerRefactoringTestHelper { /** * Test mode for matching refactored source against expected source. */ - public static enum TestMode { + public enum TestMode { TEXT_MATCH { @Override - void verifyMatch(JavaFileObject refactoredSource, - JavaFileObject expectedSource) throws IOException { + void verifyMatch(JavaFileObject refactoredSource, JavaFileObject expectedSource) + throws IOException { assertThat(refactoredSource.getCharContent(false).toString()) .isEqualTo(expectedSource.getCharContent(false).toString()); } @@ -74,16 +77,34 @@ void verifyMatch(JavaFileObject refactoredSource, JavaFileObject expectedSource) } }; - abstract void verifyMatch(JavaFileObject refactoredSource, - JavaFileObject expectedSource) throws IOException; + abstract void verifyMatch(JavaFileObject refactoredSource, JavaFileObject expectedSource) + throws IOException; + } + + /** + * For checks that provide multiple possible fixes, chooses the one that will be applied for the + * test. + * */ + public interface FixChooser { + Fix choose(List fixes); + } + + enum FixChoosers implements FixChooser { + FIRST { + @Override + public Fix choose(List fixes) { + return fixes.get(0); + } + } } private final Map sources = new HashMap<>(); private final BugChecker refactoringBugChecker; private final ErrorProneInMemoryFileManager fileManager; - private BugCheckerRefactoringTestHelper( - BugChecker refactoringBugChecker, Class clazz) { + private FixChooser fixChooser = FixChoosers.FIRST; + + private BugCheckerRefactoringTestHelper(BugChecker refactoringBugChecker, Class clazz) { this.refactoringBugChecker = refactoringBugChecker; this.fileManager = new ErrorProneInMemoryFileManager(clazz); } @@ -102,6 +123,11 @@ public BugCheckerRefactoringTestHelper.ExpectOutput addInputLines(String path, S return new ExpectOutput(fileManager.forSourceLines(path, input)); } + public BugCheckerRefactoringTestHelper setFixChooser(FixChooser chooser) { + this.fixChooser = chooser; + return this; + } + public void doTest() throws IOException { this.doTest(TestMode.AST_MATCH); } @@ -127,18 +153,32 @@ private void runTestOnPair(JavaFileObject input, JavaFileObject output, TestMode testMode.verifyMatch(transformed, output); } - private JavaFileObject applyDiff(JavaFileObject sourceFileObject, - JavacTaskImpl task, JCCompilationUnit tree) throws IOException { - DescriptionBasedDiff diff = DescriptionBasedDiff.create(tree); - transformer(refactoringBugChecker).apply(new TreePath(tree), task.getContext(), diff); + private JavaFileObject applyDiff( + JavaFileObject sourceFileObject, JavacTaskImpl task, JCCompilationUnit tree) + throws IOException { + final DescriptionBasedDiff diff = DescriptionBasedDiff.create(tree); + transformer(refactoringBugChecker) + .apply( + new TreePath(tree), + task.getContext(), + new DescriptionListener() { + @Override + public void onDescribed(Description description) { + if (!description.fixes.isEmpty()) { + diff.handleFix(fixChooser.choose(description.fixes)); + } + } + }); SourceFile sourceFile = SourceFile.create(sourceFileObject); diff.applyDifferences(sourceFile); - JavaFileObject transformed = JavaFileObjects.forSourceString( - Iterables.getOnlyElement(Iterables.filter(tree.getTypeDecls(), JCClassDecl.class)) - .sym.getQualifiedName() - .toString(), - sourceFile.getSourceText()); + JavaFileObject transformed = + JavaFileObjects.forSourceString( + Iterables.getOnlyElement(Iterables.filter(tree.getTypeDecls(), JCClassDecl.class)) + .sym + .getQualifiedName() + .toString(), + sourceFile.getSourceText()); return transformed; } @@ -192,15 +232,15 @@ public ExpectOutput(JavaFileObject input) { this.input = input; } - public BugCheckerRefactoringTestHelper addOutputLines(String path, String ... output) { + public BugCheckerRefactoringTestHelper addOutputLines(String path, String... output) { assertThat(fileManager.exists(path)).isFalse(); - return addInputAndOutput(input, fileManager.forSourceLines(path, output)); + return addInputAndOutput(input, fileManager.forSourceLines(path, output)); } public BugCheckerRefactoringTestHelper addOutput(String outputFilename) { return addInputAndOutput(input, fileManager.forResource(outputFilename)); } - + public BugCheckerRefactoringTestHelper expectUnchanged() { return addInputAndOutput(input, input); } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ConstantFieldTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ConstantFieldTest.java new file mode 100644 index 00000000000..e166e1e0faa --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ConstantFieldTest.java @@ -0,0 +1,110 @@ +/* + * Copyright 2016 Google Inc. All Rights Reserved. + * + * 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.BugCheckerRefactoringTestHelper.FixChooser; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.fixes.Fix; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +import java.io.IOException; +import java.util.List; + +/** {@link ConstantField}Test */ +@RunWith(JUnit4.class) +public class ConstantFieldTest { + CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(ConstantField.class, getClass()); + + @Test + public void positive() { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains: static final Object CONSTANT1 = 42;", + " Object CONSTANT1 = 42;", + " // BUG: Diagnostic contains: @Deprecated static final Object CONSTANT2 = 42;", + " @Deprecated Object CONSTANT2 = 42;", + " // BUG: Diagnostic contains: static final Object CONSTANT3 = 42;", + " static Object CONSTANT3 = 42;", + " // BUG: Diagnostic contains: static final Object CONSTANT4 = 42;", + " final Object CONSTANT4 = 42;", + " // BUG: Diagnostic contains:" + + " @Deprecated private static final Object CONSTANT5 = 42;", + " @Deprecated private Object CONSTANT5 = 42;", + "}") + .doTest(); + } + + @Test + public void rename() { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains: 'Object constantCaseName = 42;'", + " Object CONSTANT_CASE_NAME = 42;", + "}") + .doTest(); + } + + @Test + public void negative() { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " static final Object CONSTANT = 42;", + " Object nonConst = 42;", + "}") + .doTest(); + } + + @Test + public void renameUsages() throws IOException { + BugCheckerRefactoringTestHelper.newInstance(new ConstantField(), getClass()) + .addInputLines( + "in/Test.java", + "class Test {", + " Object CONSTANT_CASE = 42;", + " void f() {", + " System.err.println(CONSTANT_CASE);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "class Test {", + " Object constantCase = 42;", + " void f() {", + " System.err.println(constantCase);", + " }", + "}") + .setFixChooser( + new FixChooser() { + @Override + public Fix choose(List fixes) { + return fixes.get(1); + } + }) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } +} diff --git a/docs/bugpattern/ConstantField.md b/docs/bugpattern/ConstantField.md new file mode 100644 index 00000000000..0341a6d8211 --- /dev/null +++ b/docs/bugpattern/ConstantField.md @@ -0,0 +1,8 @@ +The [Google Java Style Guide ยง5.2.4][style] requires constant names to use +`CONSTANT_CASE`. + +[style]: https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names + +When naming a field with `CONSTANT_CASE`, make sure the field is `static`, +`final`, and of immutable type. If the field doesn't meet those criteria, use +`lowerCamelCase` instead.