Introduce RedundantStringEscape check (#1138)

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.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringEscape.java

@@ -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) {
+      /* 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();
+  }
+}

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java

@@ -41,7 +41,7 @@ import tech.picnic.errorprone.utils.SourceCode;
     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()
@@ -71,7 +71,7 @@ public final class Slf4jLogStatement extends BugChecker implements MethodInvocat
      * 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

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java

@@ -23,7 +23,6 @@ import com.google.errorprone.util.ASTHelpers;
 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 @@ public final class StringJoin extends BugChecker implements MethodInvocationTree
     SuggestedFix.Builder fix =
         SuggestedFix.builder()
             .replace(tree.getMethodSelect(), "String.join")
-            .replace(arguments.next(), Constants.format(separator));
+            .replace(arguments.next(), SourceCode.toStringConstantExpression(separator, state));
 
     while (arguments.hasNext()) {
       ExpressionTree argument = arguments.next();

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

@@ -11,6 +11,7 @@ import com.sun.tools.javac.util.Constants;
 import com.sun.tools.javac.util.Convert;
 import javax.lang.model.element.Name;
 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
@@ -56,16 +57,26 @@ final class BugCheckerRules {
     }
   }
 
-  /** Prefer using the {@link Constants} API over more verbose alternatives. */
+  /**
+   * Prefer {@link SourceCode#toStringConstantExpression(Object,
+   * com.google.errorprone.VisitorState)} 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, Refaster.emitCommentBefore("REPLACEME", null));
     }
   }
 

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<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);
+  }
+}

error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestInput.java

@@ -4,6 +4,7 @@ import com.google.common.collect.ImmutableSet;
 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 javax.lang.model.element.Name;
 import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;
@@ -11,7 +12,7 @@ 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() {
@@ -29,8 +30,8 @@ final class BugCheckerRulesTest implements RefasterRuleCollectionTestCase {
         .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")));
   }
 
   ImmutableSet<Boolean> testNameContentEquals() {

error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java

@@ -8,11 +8,12 @@ import com.sun.tools.javac.util.Constants;
 import com.sun.tools.javac.util.Convert;
 import javax.lang.model.element.Name;
 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() {
@@ -28,8 +29,10 @@ final class BugCheckerRulesTest implements RefasterRuleCollectionTestCase {
         .expectUnchanged();
   }
 
-  String testConstantsFormat() {
-    return Constants.format("foo");
+  ImmutableSet<String> testConstantsFormat() {
+    return ImmutableSet.of(
+        SourceCode.toStringConstantExpression("foo", /* REPLACEME */ null),
+        SourceCode.toStringConstantExpression("bar", /* REPLACEME */ null));
   }
 
   ImmutableSet<Boolean> testNameContentEquals() {

error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java

@@ -30,8 +30,8 @@ import com.sun.source.tree.BinaryTree;
 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,9 @@ public final class BugPatternLink extends BugChecker implements ClassTreeMatcher
             state,
             "link",
             ImmutableList.of(
-                linkPrefix + " + " + Constants.format(tree.getSimpleName().toString()))));
+                linkPrefix
+                    + " + "
+                    + SourceCode.toStringConstantExpression(tree.getSimpleName(), state))));
 
     String linkType =
         SuggestedFixes.qualifyStaticImport(

error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java

@@ -26,8 +26,8 @@ import com.sun.source.tree.LiteralTree;
 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,8 @@ public final class ErrorProneRuntimeClasspath extends BugChecker
         .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(), state)))
         .build();
   }
 
@@ -150,7 +151,9 @@ public final class ErrorProneRuntimeClasspath extends BugChecker
             original,
             identifier
                 + ".class.getCanonicalName()"
-                + (suffix.isEmpty() ? "" : (" + " + Constants.format(suffix))))
+                + (suffix.isEmpty()
+                    ? ""
+                    : (" + " + SourceCode.toStringConstantExpression(suffix, state))))
         .build();
   }
 

error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java

@@ -38,7 +38,6 @@ import com.sun.tools.javac.code.Attribute;
 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.AnnotationMirror;
 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 final class ExhaustiveRefasterTypeMigration extends BugChecker implements
                 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());
   }
 

error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java

@@ -42,6 +42,24 @@ public final class SourceCode {
     return src != null ? src : tree.toString();
   }
 
+  /**
+   * Returns a Java string constant expression (i.e., a quoted string) representing the given input.
+   *
+   * @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.
+   */
+  // 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);
+  }
+
   /**
    * Creates a {@link SuggestedFix} for the deletion of the given {@link Tree}, including any
    * whitespace that follows it.

error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java

@@ -8,18 +8,49 @@ import com.google.errorprone.BugPattern;
 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.bugpatterns.BugChecker.VariableTreeMatcher;
+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 com.sun.source.tree.VariableTree;
+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() {",
+            "    char a = 'c';",
+            "    char b = '\\'';",
+            "    return \"foo\\\"bar\\'baz\\bqux\";",
+            "  }",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "class A {",
+            "  String m() {",
+            "    char a = 'c' /* 'c' */; /* \"a\" */",
+            "    char b = '\\'' /* '\\'' */; /* \"b\" */",
+            "    return \"foo\\\"bar\\'baz\\bqux\" /* \"foo\\\"bar'baz\\bqux\" */;",
+            "  }",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
+
   @Test
   void deleteWithTrailingWhitespaceAnnotations() {
     BugCheckerRefactoringTestHelper.newInstance(
@@ -228,6 +259,41 @@ final class SourceCodeTest {
         .doTest(TestMode.TEXT_MATCH);
   }
 
+  /**
+   * 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, VariableTreeMatcher {
+    private static final long serialVersionUID = 1L;
+
+    @Override
+    public Description matchLiteral(LiteralTree tree, VisitorState state) {
+      // 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 ->
+                  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)));
+    }
+  }
+
   /**
    * A {@link BugChecker} that uses {@link SourceCode#deleteWithTrailingWhitespace(Tree,
    * VisitorState)} to suggest the deletion of annotations and methods with a name containing