Introduce RedundantStringEscape check
#1138
Conversation
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| @Override | ||
| public Description matchLiteral(LiteralTree tree, VisitorState state) { | ||
| String constant = ASTHelpers.constValue(tree, String.class); | ||
| if (constant == null || constant.indexOf('\'') == -1) { |
There was a problem hiding this comment.
Pitest is correct that the || constant.indexOf('\'') == -1 condition can be dropped. It's here for performance reasons.
| /** | ||
| * 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(); | ||
| } |
There was a problem hiding this comment.
Just noticed we could also use the upstream VisitorState#getConstantExpression method, though this variant is more efficient.
There was a problem hiding this comment.
I filed google/error-prone#4586; will update this PR.
There was a problem hiding this comment.
Rebased and added a commit.
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
1b69726 to
96c8208
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
96c8208 to
8ee3ce8
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
rickie
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
8ee3ce8 to
4ce0955
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
@werli are you up for being the second reviewer? Otherwise I'll ask @mohamedsamehsalah 😄 |
|
@Stephan202 Yes, I got pinged by Rick. ETA is EOD. 👍 |
|
GitHub should support the |
| import com.sun.source.tree.LiteralTree; | ||
| import tech.picnic.errorprone.utils.SourceCode; | ||
|
|
||
| /** A {@link BugChecker} that flags string constants with extraneous escaping. */ |
There was a problem hiding this comment.
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.
Great suggestion! I'll have a look whether this isn't too much trouble to add, and add an XXX comment otherwise 👍
There was a problem hiding this comment.
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.
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
4ce0955 to
3e30dc5
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
LGTM, let's merge 🚀 ? |
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.
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
3e30dc5 to
f200a92
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
deleted the
sschroevers/introduce-redundant-string-escape-check
branch
Suggested commit message: