diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java b/core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java new file mode 100644 index 00000000000..26dedbc399a --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java @@ -0,0 +1,133 @@ +/* + * 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.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getSymbol; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.MultimapBuilder; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Consider inlining this constant", severity = WARNING) +public class InlineTrivialConstant extends BugChecker implements CompilationUnitTreeMatcher { + + @AutoValue + abstract static class TrivialConstant { + abstract VariableTree tree(); + + abstract String replacement(); + + static TrivialConstant create(VariableTree tree, String replacement) { + return new AutoValue_InlineTrivialConstant_TrivialConstant(tree, replacement); + } + } + + @Override + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + Map fields = new HashMap<>(); + new SuppressibleTreePathScanner(state) { + @Override + public Void visitVariable(VariableTree tree, Void unused) { + VarSymbol sym = getSymbol(tree); + isTrivialConstant(tree, sym, state) + .ifPresent(r -> fields.put(sym, TrivialConstant.create(tree, r))); + return super.visitVariable(tree, null); + } + }.scan(state.getPath(), null); + ListMultimap uses = MultimapBuilder.hashKeys().arrayListValues().build(); + new TreePathScanner() { + @Override + public Void visitMemberSelect(MemberSelectTree tree, Void unused) { + handle(tree); + return super.visitMemberSelect(tree, null); + } + + @Override + public Void visitIdentifier(IdentifierTree tree, Void unused) { + handle(tree); + return super.visitIdentifier(tree, null); + } + + private void handle(Tree tree) { + Symbol sym = getSymbol(tree); + if (sym == null) { + return; + } + if (fields.containsKey(sym)) { + uses.put(sym, tree); + } + } + }.scan(state.getPath(), null); + for (Map.Entry e : fields.entrySet()) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + TrivialConstant value = e.getValue(); + fix.delete(value.tree()); + uses.get(e.getKey()).forEach(x -> fix.replace(x, value.replacement())); + state.reportMatch(describeMatch(value.tree(), fix.build())); + } + return NO_MATCH; + } + + private static Optional isTrivialConstant( + VariableTree tree, VarSymbol sym, VisitorState state) { + if (!(sym.getKind().equals(ElementKind.FIELD) + && sym.getModifiers().contains(Modifier.PRIVATE) + && sym.isStatic() + && sym.getModifiers().contains(Modifier.FINAL))) { + return Optional.empty(); + } + if (!tree.getName().contentEquals("EMPTY_STRING")) { + return Optional.empty(); + } + if (!ASTHelpers.isSameType(sym.asType(), state.getSymtab().stringType, state)) { + return Optional.empty(); + } + ExpressionTree initializer = tree.getInitializer(); + if (initializer.getKind().equals(Tree.Kind.STRING_LITERAL)) { + String value = (String) ((LiteralTree) initializer).getValue(); + if (Objects.equals(value, "")) { + return Optional.of("\"\""); + } + } + return Optional.empty(); + } +} 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 a0eee7e0808..404340f81b3 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -172,6 +172,7 @@ import com.google.errorprone.bugpatterns.InfiniteRecursion; import com.google.errorprone.bugpatterns.InitializeInline; import com.google.errorprone.bugpatterns.InjectOnBugCheckers; +import com.google.errorprone.bugpatterns.InlineTrivialConstant; import com.google.errorprone.bugpatterns.InputStreamSlowMultibyteRead; import com.google.errorprone.bugpatterns.InsecureCipherMode; import com.google.errorprone.bugpatterns.InstanceOfAndCastMatchWrongType; @@ -890,6 +891,7 @@ public static ScannerSupplier errorChecks() { InjectOnConstructorOfAbstractClass.class, InjectedConstructorAnnotations.class, InlineFormatString.class, + InlineTrivialConstant.class, Inliner.class, InputStreamSlowMultibyteRead.class, InstanceOfAndCastMatchWrongType.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/InlineTrivialConstantTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/InlineTrivialConstantTest.java new file mode 100644 index 00000000000..3a29e06541b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/InlineTrivialConstantTest.java @@ -0,0 +1,75 @@ +/* + * 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 class InlineTrivialConstantTest { + @Test + public void positive() { + BugCheckerRefactoringTestHelper.newInstance(InlineTrivialConstant.class, getClass()) + .addInputLines( + "Test.java", + "class Test {", + " private static final String EMPTY_STRING = \"\";", + " void f() {", + " System.err.println(EMPTY_STRING);", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void f() {", + " System.err.println(\"\");", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + CompilationTestHelper.newInstance(InlineTrivialConstant.class, getClass()) + .addSourceLines( + "Test.java", + "class Test {", + " static class NonPrivate {", + " static final String EMPTY_STRING = \"\";", + " }", + " static class NonStatic {", + " private final String EMPTY_STRING = \"\";", + " }", + " static class NonFinal {", + " private static String EMPTY_STRING = \"\";", + " }", + " static class NonString {", + " private static final Object EMPTY_STRING = \"\";", + " }", + " static class WrongName {", + " private static final String LAUNCH_CODES = \"\";", + " }", + " static class WrongValue {", + " private static final String EMPTY_STRING = \"hello\";", + " }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/InlineTrivialConstant.md b/docs/bugpattern/InlineTrivialConstant.md new file mode 100644 index 00000000000..4636fafd369 --- /dev/null +++ b/docs/bugpattern/InlineTrivialConstant.md @@ -0,0 +1,20 @@ +Constants should be given names that emphasize the semantic meaning of the +value. If the name of the constant doesn't convey any information that isn't +clear from the value, consider inlining it. + +For example, prefer this: + +```java +System.err.println(1); +System.err.println(""); +``` + +to this: + +```java +private static final int ONE = 1; +private static final String EMPTY_STRING = ""; +... +System.err.println(ONE); +System.err.println(EMPTY_STRING); +```