Introduce RedundantStringEscape check
#1138
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| 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. */ | ||
| // XXX: Also cover `\"` sequences inside text blocks. Note that this requires a more subtle | ||
| // approach, as double-quote characters will need to remain escaped if removing the backslash would | ||
| // create a new sequence of three or more double-quotes. (TBD whether we'd like to enforce a | ||
| // "preferred" approach to escaping, e.g. by always escaping the last of a triplet, such that the | ||
| // over-all number of escaped characters is minimized.) | ||
| // XXX: Also flag `'\"'` char literals. | ||
| @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('\'') < 0) { | ||
|
Check warning on line 42 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java
|
||
| /* 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(); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<String> 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<String> 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<String> m() {", | ||
| " return Arrays.asList(", | ||
| " \"'\",", | ||
| " \"''\",", | ||
| " \"''\",", | ||
| " \"''\",", | ||
| " \"'ß'\",", | ||
| " (", | ||
| " /* Leading comment. */ \"'\" /* Trailing comment. */),", | ||
| " \"'foo\\\"bar'baz\\\"qux'\");", | ||
| " }", | ||
| "}") | ||
| .doTest(TestMode.TEXT_MATCH); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this covers all unnecessary escaping cases in "single-line strings", but not text blocks. With text blocks, escalating double quotes may also be unnecessary but only if it's not the last character (because the text block would end with
"""").I'm happy not (yet) to cover it, but should we document and make a note of it?
Or am I missing something since text blocks are a Java 17 feature and not yet an EPS concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I'll have a look whether this isn't too much trouble to add, and add an
XXXcomment otherwise 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started working on this, but there are quite some edge cases and performance aspects to consider. As such I pushed only a commit with a comment to follow up on this topic. Will push a draft branch once this PR is merged.