From 4ce095506271b7b168c2e058758d1afd4c9d4d70 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 21 Sep 2024 15:11:44 +0200 Subject: [PATCH] New pass --- .../errorprone/bugpatterns/StringJoin.java | 2 +- .../refasterrules/BugCheckerRules.java | 8 +++-- .../BugCheckerRulesTestOutput.java | 3 +- .../bugpatterns/BugPatternLink.java | 4 ++- .../ErrorProneRuntimeClasspath.java | 7 ++-- .../ExhaustiveRefasterTypeMigration.java | 6 ++-- .../picnic/errorprone/utils/SourceCode.java | 26 ++++++-------- .../errorprone/utils/SourceCodeTest.java | 36 ++++++++++++++----- 8 files changed, 59 insertions(+), 33 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java index be54bb14af..99b3380df3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java @@ -149,7 +149,7 @@ private Description trySuggestExplicitJoin( SuggestedFix.Builder fix = SuggestedFix.builder() .replace(tree.getMethodSelect(), "String.join") - .replace(arguments.next(), SourceCode.toStringConstantExpression(separator)); + .replace(arguments.next(), SourceCode.toStringConstantExpression(separator, state)); while (arguments.hasNext()) { ExpressionTree argument = arguments.next(); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java index dd274124cb..76c3dcf09c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java @@ -58,8 +58,9 @@ BugCheckerRefactoringTestHelper after( } /** - * Prefer {@link SourceCode#toStringConstantExpression(CharSequence)} over alternatives that - * unnecessarily escape single quote characters. + * Prefer {@link SourceCode#toStringConstantExpression(Object, + * com.google.errorprone.VisitorState)} over alternatives that unnecessarily escape single quote + * characters. */ static final class ConstantsFormat { @BeforeTemplate @@ -74,7 +75,8 @@ String before(String value) { @AfterTemplate String after(CharSequence value) { - return SourceCode.toStringConstantExpression(value); + return SourceCode.toStringConstantExpression( + value, Refaster.emitCommentBefore("REPLACEME", null)); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java index e7511a202e..524cf36f48 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java @@ -31,7 +31,8 @@ ImmutableSet testBugCheckerRefactoringTestHelpe ImmutableSet testConstantsFormat() { return ImmutableSet.of( - SourceCode.toStringConstantExpression("foo"), SourceCode.toStringConstantExpression("bar")); + SourceCode.toStringConstantExpression("foo", /* REPLACEME */ null), + SourceCode.toStringConstantExpression("bar", /* REPLACEME */ null)); } ImmutableSet testNameContentEquals() { diff --git a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java index d5c8ad43d1..b6274bd0e7 100644 --- a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java @@ -126,7 +126,9 @@ private static SuggestedFix suggestFix( state, "link", ImmutableList.of( - linkPrefix + " + " + SourceCode.toStringConstantExpression(tree.getSimpleName())))); + linkPrefix + + " + " + + SourceCode.toStringConstantExpression(tree.getSimpleName(), state)))); String linkType = SuggestedFixes.qualifyStaticImport( diff --git a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java index 1f7b249c9c..fb19c4dd0b 100644 --- a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java @@ -123,7 +123,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .setMessage("This type may not be on the runtime classpath; use a string literal instead") .addFix( SuggestedFix.replace( - tree, SourceCode.toStringConstantExpression(receiver.owner.getQualifiedName()))) + tree, + SourceCode.toStringConstantExpression(receiver.owner.getQualifiedName(), state))) .build(); } @@ -150,7 +151,9 @@ private static SuggestedFix suggestClassReference( original, identifier + ".class.getCanonicalName()" - + (suffix.isEmpty() ? "" : (" + " + SourceCode.toStringConstantExpression(suffix)))) + + (suffix.isEmpty() + ? "" + : (" + " + SourceCode.toStringConstantExpression(suffix, state)))) .build(); } diff --git a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java index 9874b0f4d8..ada6b19c7c 100644 --- a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -38,7 +38,6 @@ import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.TypeSymbol; -import com.sun.tools.javac.util.Constants; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -49,6 +48,7 @@ import javax.lang.model.element.AnnotationValue; import javax.lang.model.element.Modifier; import org.jspecify.annotations.Nullable; +import tech.picnic.errorprone.utils.SourceCode; /** * A {@link BugChecker} that validates the claim made by {@link @@ -129,7 +129,9 @@ public Description matchClass(ClassTree tree, VisitorState state) { migrationAnnotation, state, TYPE_MIGRATION_UNMIGRATED_METHODS_ELEMENT, - unmigratedMethods.stream().map(Constants::format).collect(toImmutableList())) + unmigratedMethods.stream() + .map(m -> SourceCode.toStringConstantExpression(m, state)) + .collect(toImmutableList())) .build()); } diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java index 3e50e53c39..6d29e96120 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java +++ b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java @@ -14,7 +14,6 @@ import com.google.errorprone.util.ErrorProneTokens; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; -import com.sun.tools.javac.util.Convert; import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; import com.sun.tools.javac.util.Position; import java.util.Optional; @@ -46,22 +45,19 @@ public static String treeToString(Tree tree, VisitorState state) { /** * Returns a Java string constant expression (i.e., a quoted string) representing the given input. * - * @apiNote This method differs from {@link com.sun.tools.javac.util.Constants#format(Object)} in - * that it does not superfluously escape single quote characters. - * @param str The string of interest. + * @param value The value of interest. + * @param state A {@link VisitorState} describing the context in which the given {@link Tree} is + * found. * @return A non-{@code null} string. + * @apiNote This method differs from {@link com.sun.tools.javac.util.Constants#format(Object)} in + * that it does not superfluously escape single quote characters. It is different from {@link + * VisitorState#getConstantExpression(Object)} in that it is more performant and accepts any + * {@link CharSequence} instance. */ - public static String toStringConstantExpression(CharSequence str) { - StringBuilder result = new StringBuilder("\""); - for (int i = 0; i < str.length(); i++) { - char c = str.charAt(i); - if (c == '\'') { - result.append('\''); - } else { - result.append(Convert.quote(c)); - } - } - return result.append('"').toString(); + // XXX: Drop this method if https://github.com/google/error-prone/pull/4586 is merged and released + // with the proposed `CharSequence` compatibility change. + public static String toStringConstantExpression(Object value, VisitorState state) { + return state.getConstantExpression(value instanceof CharSequence ? value.toString() : value); } /** diff --git a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java index a482a0d4ed..ed4b73d98b 100644 --- a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java +++ b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java @@ -11,6 +11,7 @@ import com.google.errorprone.bugpatterns.BugChecker.LiteralTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.util.ASTHelpers; @@ -19,6 +20,7 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import java.util.Optional; import javax.lang.model.element.Name; import org.junit.jupiter.api.Test; @@ -32,6 +34,8 @@ ToStringConstantExpressionTestChecker.class, getClass()) "A.java", "class A {", " String m() {", + " char a = 'c';", + " char b = '\\'';", " return \"foo\\\"bar\\'baz\\bqux\";", " }", "}") @@ -39,7 +43,9 @@ ToStringConstantExpressionTestChecker.class, getClass()) "A.java", "class A {", " String m() {", - " return \"foo\\\"bar'baz\\bqux\";", + " char a = 'c' /* 'c' */; /* \"a\" */", + " char b = '\\'' /* '\\'' */; /* \"b\" */", + " return \"foo\\\"bar\\'baz\\bqux\" /* \"foo\\\"bar'baz\\bqux\" */;", " }", "}") .doTest(TestMode.TEXT_MATCH); @@ -254,24 +260,38 @@ UnwrapMethodInvocationDroppingWhitespaceAndCommentsTestChecker.class, getClass() } /** - * A {@link BugChecker} that applies {@link SourceCode#toStringConstantExpression(CharSequence)} - * to string literals. + * A {@link BugChecker} that applies {@link SourceCode#toStringConstantExpression(Object, + * VisitorState)} to string literals. */ @BugPattern(severity = ERROR, summary = "Interacts with `SourceCode` for testing purposes") public static final class ToStringConstantExpressionTestChecker extends BugChecker - implements LiteralTreeMatcher { + implements LiteralTreeMatcher, VariableTreeMatcher { private static final long serialVersionUID = 1L; @Override public Description matchLiteral(LiteralTree tree, VisitorState state) { - return Optional.ofNullable(ASTHelpers.constValue(tree, String.class)) + // XXX: The character conversion is a workaround for the fact that `ASTHelpers#constValue` + // returns an `Integer` value for `char` constants. + return Optional.ofNullable(ASTHelpers.constValue(tree)) .map( constant -> - describeMatch( - tree, - SuggestedFix.replace(tree, SourceCode.toStringConstantExpression(constant)))) + ASTHelpers.isSubtype(ASTHelpers.getType(tree), state.getSymtab().charType, state) + ? (char) (int) constant + : constant) + .map(constant -> describeMatch(tree, addComment(tree, constant, state))) .orElse(Description.NO_MATCH); } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + return describeMatch( + tree, addComment(tree, ASTHelpers.getSymbol(tree).getSimpleName(), state)); + } + + private static SuggestedFix addComment(Tree tree, Object value, VisitorState state) { + return SuggestedFix.postfixWith( + tree, "/* %s */".formatted(SourceCode.toStringConstantExpression(value, state))); + } } /**