From 96c8208b3e61d7a510b7d3582e827de48d575f6b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Apr 2024 09:57:19 +0200 Subject: [PATCH] Introduce `RedundantStringEscape` check This check aims to simplify string constants by dropping redundant single quote escape sequences. The check is optimized for performance. While there, update existing checks such that they do not introduce violations of the type flagged by this new check. --- .../bugpatterns/RedundantStringEscape.java | 89 ++++++++++++++++++ .../bugpatterns/Slf4jLogStatement.java | 4 +- .../errorprone/bugpatterns/StringJoin.java | 3 +- .../refasterrules/BugCheckerRules.java | 15 +++- .../RedundantStringEscapeTest.java | 90 +++++++++++++++++++ .../BugCheckerRulesTestInput.java | 7 +- .../BugCheckerRulesTestOutput.java | 8 +- .../bugpatterns/BugPatternLink.java | 4 +- .../ErrorProneRuntimeClasspath.java | 6 +- .../picnic/errorprone/utils/SourceCode.java | 22 +++++ .../errorprone/utils/SourceCodeTest.java | 46 ++++++++++ 11 files changed, 276 insertions(+), 18 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscapeTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java new file mode 100644 index 00000000000..e2bb0f53264 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java @@ -0,0 +1,89 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.LiteralTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.LiteralTree; +import tech.picnic.errorprone.utils.SourceCode; + +/** A {@link BugChecker} that flags string constants with extraneous escaping. */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Inside string expressions single quotes do not need to be escaped", + link = BUG_PATTERNS_BASE_URL + "RedundantStringEscape", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class RedundantStringEscape extends BugChecker implements LiteralTreeMatcher { + private static final long serialVersionUID = 1L; + + /** Instantiates a new {@link RedundantStringEscape} instance. */ + public RedundantStringEscape() {} + + @Override + public Description matchLiteral(LiteralTree tree, VisitorState state) { + String constant = ASTHelpers.constValue(tree, String.class); + if (constant == null || constant.indexOf('\'') == -1) { + /* Fast path: this isn't a string constant with a single quote. */ + return Description.NO_MATCH; + } + + String source = SourceCode.treeToString(tree, state); + if (!containsBannedEscapeSequence(source)) { + /* Semi-fast path: this expression doesn't contain an escaped single quote. */ + return Description.NO_MATCH; + } + + /* Slow path: suggest dropping the escape characters. */ + return describeMatch(tree, SuggestedFix.replace(tree, dropRedundantEscapeSequences(source))); + } + + /** + * Tells whether the given string constant source expression contains an escaped single quote. + * + * @implNote As the input is a literal Java string expression, it will start and end with a double + * quote; as such any found backslash will not be the string's final character. + */ + private static boolean containsBannedEscapeSequence(String source) { + for (int p = source.indexOf('\\'); p != -1; p = source.indexOf('\\', p + 2)) { + if (source.charAt(p + 1) == '\'') { + return true; + } + } + + return false; + } + + /** + * Simplifies the given string constant source expression by dropping the backslash preceding an + * escaped single quote. + * + * @implNote Note that this method does not delegate to {@link + * SourceCode#toStringConstantExpression}, as that operation may replace other Unicode + * characters with their associated escape sequence. + * @implNote As the input is a literal Java string expression, it will start and end with a double + * quote; as such any found backslash will not be the string's final character. + */ + private static String dropRedundantEscapeSequences(String source) { + StringBuilder result = new StringBuilder(); + + for (int p = 0; p < source.length(); p++) { + char c = source.charAt(p); + if (c != '\\' || source.charAt(p + 1) != '\'') { + result.append(c); + } + } + + return result.toString(); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java index ccf73b82580..7836961f9e3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java @@ -41,7 +41,7 @@ tags = LIKELY_ERROR) public final class Slf4jLogStatement extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher MARKER = isSubtypeOf("org.slf4j.Marker"); + private static final Matcher SLF4J_MARKER = isSubtypeOf("org.slf4j.Marker"); private static final Matcher THROWABLE = isSubtypeOf(Throwable.class); private static final Matcher SLF4J_LOGGER_INVOCATION = instanceMethod() @@ -71,7 +71,7 @@ private static List getTrimmedArguments( * SLF4J log statements may accept a "marker" as a first argument, before the format string. * We ignore such markers. */ - int lTrim = MARKER.matches(args.get(0), state) ? 1 : 0; + int lTrim = SLF4J_MARKER.matches(args.get(0), state) ? 1 : 0; /* * SLF4J treats the final argument to a log statement specially if it is a `Throwabe`: it * will always choose to render the associated stacktrace, even if the argument has a 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 4ab098b3cb7..be54bb14af9 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 @@ -23,7 +23,6 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.util.Constants; import java.util.Formattable; import java.util.Iterator; import java.util.List; @@ -150,7 +149,7 @@ private Description trySuggestExplicitJoin( SuggestedFix.Builder fix = SuggestedFix.builder() .replace(tree.getMethodSelect(), "String.join") - .replace(arguments.next(), Constants.format(separator)); + .replace(arguments.next(), SourceCode.toStringConstantExpression(separator)); 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 db21ddb7115..20e2cddc362 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 @@ -10,6 +10,7 @@ import com.sun.tools.javac.util.Constants; import com.sun.tools.javac.util.Convert; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.utils.SourceCode; /** Refaster rules related to {@link com.google.errorprone.bugpatterns.BugChecker} classes. */ @OnlineDocumentation @@ -55,16 +56,24 @@ BugCheckerRefactoringTestHelper after( } } - /** Prefer using the {@link Constants} API over more verbose alternatives. */ + /** + * Prefer {@link SourceCode#toStringConstantExpression(CharSequence)} over alternatives that + * unnecessarily escape single quote characters. + */ static final class ConstantsFormat { + @BeforeTemplate + String before(CharSequence value) { + return Constants.format(value); + } + @BeforeTemplate String before(String value) { return String.format("\"%s\"", Convert.quote(value)); } @AfterTemplate - String after(String value) { - return Constants.format(value); + String after(CharSequence value) { + return SourceCode.toStringConstantExpression(value); } } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscapeTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscapeTest.java new file mode 100644 index 00000000000..c3f2151196c --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscapeTest.java @@ -0,0 +1,90 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class RedundantStringEscapeTest { + @Test + void identification() { + CompilationTestHelper.newInstance(RedundantStringEscape.class, getClass()) + .addSourceLines( + "A.java", + "import java.util.Arrays;", + "import java.util.List;", + "", + "class A {", + " List m() {", + " return Arrays.asList(", + " \"foo\",", + " \"ß\",", + " \"'\",", + " \"\\\"\",", + " \"\\\\\",", + " \"\\\\'\",", + " \"'\\\\\",", + " // BUG: Diagnostic contains:", + " \"\\\\\\'\",", + " // BUG: Diagnostic contains:", + " \"\\'\\\\\",", + " // BUG: Diagnostic contains:", + " \"\\'\",", + " // BUG: Diagnostic contains:", + " \"'\\'\",", + " // BUG: Diagnostic contains:", + " \"\\''\",", + " // BUG: Diagnostic contains:", + " \"\\'\\'\",", + " (", + " // BUG: Diagnostic contains:", + " /* Leading comment. */ \"\\'\" /* Trailing comment. */),", + " // BUG: Diagnostic contains:", + " \"\\'foo\\\"bar\\'baz\\\"qux\\'\");", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(RedundantStringEscape.class, getClass()) + .addInputLines( + "A.java", + "import java.util.Arrays;", + "import java.util.List;", + "", + "class A {", + " List m() {", + " return Arrays.asList(", + " \"\\'\",", + " \"'\\'\",", + " \"\\''\",", + " \"\\'\\'\",", + " \"\\'ß\\'\",", + " (", + " /* Leading comment. */ \"\\'\" /* Trailing comment. */),", + " \"\\'foo\\\"bar\\'baz\\\"qux\\'\");", + " }", + "}") + .addOutputLines( + "A.java", + "import java.util.Arrays;", + "import java.util.List;", + "", + "class A {", + " List m() {", + " return Arrays.asList(", + " \"'\",", + " \"''\",", + " \"''\",", + " \"''\",", + " \"'ß'\",", + " (", + " /* Leading comment. */ \"'\" /* Trailing comment. */),", + " \"'foo\\\"bar'baz\\\"qux'\");", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestInput.java index 91b0bf59628..bb86d0970cf 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestInput.java @@ -4,13 +4,14 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; import com.google.errorprone.bugpatterns.BugChecker; +import com.sun.tools.javac.util.Constants; import com.sun.tools.javac.util.Convert; import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; final class BugCheckerRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(Convert.class, FixChoosers.class); + return ImmutableSet.of(Constants.class, Convert.class, FixChoosers.class); } ImmutableSet testBugCheckerRefactoringTestHelperIdentity() { @@ -28,7 +29,7 @@ ImmutableSet testBugCheckerRefactoringTestHelpe .addOutputLines("A.java", "class A {}"); } - String testConstantsFormat() { - return String.format("\"%s\"", Convert.quote("foo")); + ImmutableSet testConstantsFormat() { + return ImmutableSet.of(Constants.format("foo"), String.format("\"%s\"", Convert.quote("bar"))); } } 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 013617aad6a..f5d4a773e8d 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 @@ -7,11 +7,12 @@ import com.sun.tools.javac.util.Constants; import com.sun.tools.javac.util.Convert; import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; +import tech.picnic.errorprone.utils.SourceCode; final class BugCheckerRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(Convert.class, FixChoosers.class); + return ImmutableSet.of(Constants.class, Convert.class, FixChoosers.class); } ImmutableSet testBugCheckerRefactoringTestHelperIdentity() { @@ -27,7 +28,8 @@ ImmutableSet testBugCheckerRefactoringTestHelpe .expectUnchanged(); } - String testConstantsFormat() { - return Constants.format("foo"); + ImmutableSet testConstantsFormat() { + return ImmutableSet.of( + SourceCode.toStringConstantExpression("foo"), SourceCode.toStringConstantExpression("bar")); } } 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 3d050b2e850..d5c8ad43d1e 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 @@ -30,8 +30,8 @@ import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.Tree.Kind; -import com.sun.tools.javac.util.Constants; import javax.lang.model.element.Name; +import tech.picnic.errorprone.utils.SourceCode; /** * A {@link BugChecker} that flags {@link BugChecker} declarations inside {@code @@ -126,7 +126,7 @@ private static SuggestedFix suggestFix( state, "link", ImmutableList.of( - linkPrefix + " + " + Constants.format(tree.getSimpleName().toString())))); + linkPrefix + " + " + SourceCode.toStringConstantExpression(tree.getSimpleName())))); 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 1a13aba94f0..1f7b249c9ca 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 @@ -26,8 +26,8 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.util.Constants; import java.util.regex.Pattern; +import tech.picnic.errorprone.utils.SourceCode; import tech.picnic.errorprone.utils.ThirdPartyLibrary; /** @@ -123,7 +123,7 @@ 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, Constants.format(receiver.owner.getQualifiedName().toString()))) + tree, SourceCode.toStringConstantExpression(receiver.owner.getQualifiedName()))) .build(); } @@ -150,7 +150,7 @@ private static SuggestedFix suggestClassReference( original, identifier + ".class.getCanonicalName()" - + (suffix.isEmpty() ? "" : (" + " + Constants.format(suffix)))) + + (suffix.isEmpty() ? "" : (" + " + SourceCode.toStringConstantExpression(suffix)))) .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 824b864942e..3e50e53c39e 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,6 +14,7 @@ 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; @@ -42,6 +43,27 @@ public static String treeToString(Tree tree, VisitorState state) { return src != null ? src : tree.toString(); } + /** + * 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. + * @return A non-{@code null} string. + */ + 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(); + } + /** * Creates a {@link SuggestedFix} for the deletion of the given {@link Tree}, including any * whitespace that follows it. 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 0b61a05413d..a482a0d4ed7 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 @@ -8,18 +8,43 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +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.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; +import java.util.Optional; import javax.lang.model.element.Name; import org.junit.jupiter.api.Test; final class SourceCodeTest { + @Test + void toStringConstantExpression() { + BugCheckerRefactoringTestHelper.newInstance( + ToStringConstantExpressionTestChecker.class, getClass()) + .addInputLines( + "A.java", + "class A {", + " String m() {", + " return \"foo\\\"bar\\'baz\\bqux\";", + " }", + "}") + .addOutputLines( + "A.java", + "class A {", + " String m() {", + " return \"foo\\\"bar'baz\\bqux\";", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + @Test void deleteWithTrailingWhitespaceAnnotations() { BugCheckerRefactoringTestHelper.newInstance( @@ -228,6 +253,27 @@ UnwrapMethodInvocationDroppingWhitespaceAndCommentsTestChecker.class, getClass() .doTest(TestMode.TEXT_MATCH); } + /** + * A {@link BugChecker} that applies {@link SourceCode#toStringConstantExpression(CharSequence)} + * to string literals. + */ + @BugPattern(severity = ERROR, summary = "Interacts with `SourceCode` for testing purposes") + public static final class ToStringConstantExpressionTestChecker extends BugChecker + implements LiteralTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchLiteral(LiteralTree tree, VisitorState state) { + return Optional.ofNullable(ASTHelpers.constValue(tree, String.class)) + .map( + constant -> + describeMatch( + tree, + SuggestedFix.replace(tree, SourceCode.toStringConstantExpression(constant)))) + .orElse(Description.NO_MATCH); + } + } + /** * A {@link BugChecker} that uses {@link SourceCode#deleteWithTrailingWhitespace(Tree, * VisitorState)} to suggest the deletion of annotations and methods with a name containing