Skip to content

Commit

Permalink
Introduce RedundantStringEscape check
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Stephan202 committed Apr 14, 2024
1 parent 9e23030 commit 1b69726
Show file tree
Hide file tree
Showing 11 changed files with 276 additions and 18 deletions.
Original file line number Diff line number Diff line change
@@ -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) {

Check warning on line 36 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 36 without causing a test to fail

removed conditional - replaced equality check with false (covered by 2 tests RemoveConditionalMutator_EQUAL_ELSE)
/* 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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExpressionTree> MARKER = isSubtypeOf("org.slf4j.Marker");
private static final Matcher<ExpressionTree> SLF4J_MARKER = isSubtypeOf("org.slf4j.Marker");
private static final Matcher<ExpressionTree> THROWABLE = isSubtypeOf(Throwable.class);
private static final Matcher<ExpressionTree> SLF4J_LOGGER_INVOCATION =
instanceMethod()
Expand Down Expand Up @@ -71,7 +71,7 @@ private static List<? extends ExpressionTree> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
}
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Convert.class, FixChoosers.class);
return ImmutableSet.of(Constants.class, Convert.class, FixChoosers.class);
}

ImmutableSet<BugCheckerRefactoringTestHelper> testBugCheckerRefactoringTestHelperIdentity() {
Expand All @@ -28,7 +29,7 @@ ImmutableSet<BugCheckerRefactoringTestHelper> testBugCheckerRefactoringTestHelpe
.addOutputLines("A.java", "class A {}");
}

String testConstantsFormat() {
return String.format("\"%s\"", Convert.quote("foo"));
ImmutableSet<String> testConstantsFormat() {
return ImmutableSet.of(Constants.format("foo"), String.format("\"%s\"", Convert.quote("bar")));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Convert.class, FixChoosers.class);
return ImmutableSet.of(Constants.class, Convert.class, FixChoosers.class);
}

ImmutableSet<BugCheckerRefactoringTestHelper> testBugCheckerRefactoringTestHelperIdentity() {
Expand All @@ -27,7 +28,8 @@ ImmutableSet<BugCheckerRefactoringTestHelper> testBugCheckerRefactoringTestHelpe
.expectUnchanged();
}

String testConstantsFormat() {
return Constants.format("foo");
ImmutableSet<String> testConstantsFormat() {
return ImmutableSet.of(
SourceCode.toStringConstantExpression("foo"), SourceCode.toStringConstantExpression("bar"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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();
}

Expand All @@ -150,7 +150,7 @@ private static SuggestedFix suggestClassReference(
original,
identifier
+ ".class.getCanonicalName()"
+ (suffix.isEmpty() ? "" : (" + " + Constants.format(suffix))))
+ (suffix.isEmpty() ? "" : (" + " + SourceCode.toStringConstantExpression(suffix))))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
Loading

0 comments on commit 1b69726

Please sign in to comment.