New pass

PicnicSupermarket · Sep 21, 2024 · 8ee3ce8 · 8ee3ce8
1 parent 670314e
commit 8ee3ce8
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 33 deletions.
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
@@ -149,7 +149,7 @@ private Description trySuggestExplicitJoin(
     SuggestedFix.Builder fix =
         SuggestedFix.builder()
             .replace(tree.getMethodSelect(), "String.join")
-            .replace(arguments.next(), SourceCode.toStringConstantExpression(separator));
+            .replace(arguments.next(), SourceCode.toStringConstantExpression(separator, state));
 
     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
@@ -57,8 +57,9 @@ BugCheckerRefactoringTestHelper after(
   }
 
   /**
-   * Prefer {@link SourceCode#toStringConstantExpression(CharSequence)} over alternatives that
-   * unnecessarily escape single quote characters.
+   * Prefer {@link SourceCode#toStringConstantExpression(Object,
+   * com.google.errorprone.VisitorState)} over alternatives that unnecessarily escape single quote
+   * characters.
    */
   static final class ConstantsFormat {
     @BeforeTemplate
@@ -73,7 +74,8 @@ String before(String value) {
 
     @AfterTemplate
     String after(CharSequence value) {
-      return SourceCode.toStringConstantExpression(value);
+      return SourceCode.toStringConstantExpression(
+          value, Refaster.emitCommentBefore("REPLACEME", null));
     }
   }
 }
diff --git a/...ib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java b/...ib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestOutput.java
@@ -30,6 +30,7 @@ ImmutableSet<BugCheckerRefactoringTestHelper> testBugCheckerRefactoringTestHelpe
 
   ImmutableSet<String> testConstantsFormat() {
     return ImmutableSet.of(
-        SourceCode.toStringConstantExpression("foo"), SourceCode.toStringConstantExpression("bar"));
+        SourceCode.toStringConstantExpression("foo", /* REPLACEME */ null),
+        SourceCode.toStringConstantExpression("bar", /* REPLACEME */ null));
   }
 }
diff --git a/...uidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java b/...uidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java
@@ -126,7 +126,9 @@ private static SuggestedFix suggestFix(
             state,
             "link",
             ImmutableList.of(
-                linkPrefix + " + " + SourceCode.toStringConstantExpression(tree.getSimpleName()))));
+                linkPrefix
+                    + " + "
+                    + SourceCode.toStringConstantExpression(tree.getSimpleName(), state))));
 
     String linkType =
         SuggestedFixes.qualifyStaticImport(

diff --git a/...c/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java b/...c/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ErrorProneRuntimeClasspath.java
@@ -123,7 +123,8 @@ 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, SourceCode.toStringConstantExpression(receiver.owner.getQualifiedName())))
+                tree,
+                SourceCode.toStringConstantExpression(receiver.owner.getQualifiedName(), state)))
         .build();
   }
 
@@ -150,7 +151,9 @@ private static SuggestedFix suggestClassReference(
             original,
             identifier
                 + ".class.getCanonicalName()"
-                + (suffix.isEmpty() ? "" : (" + " + SourceCode.toStringConstantExpression(suffix))))
+                + (suffix.isEmpty()
+                    ? ""
+                    : (" + " + SourceCode.toStringConstantExpression(suffix, state))))
         .build();
   }
 

diff --git a/...n/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java b/...n/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java
@@ -38,7 +38,6 @@
 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.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 Description matchClass(ClassTree tree, VisitorState state) {
                 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());
   }
 

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
@@ -14,7 +14,6 @@
 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;
@@ -46,22 +45,19 @@ public static String treeToString(Tree tree, VisitorState state) {
   /**
    * 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.
+   * @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.
    */
-  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();
+  // 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);
   }
 
   /**

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
@@ -11,6 +11,7 @@
 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;
@@ -19,6 +20,7 @@
 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;
@@ -32,14 +34,18 @@ ToStringConstantExpressionTestChecker.class, getClass())
             "A.java",
             "class A {",
             "  String m() {",
+            "    char a = 'c';",
+            "    char b = '\\'';",
             "    return \"foo\\\"bar\\'baz\\bqux\";",
             "  }",
             "}")
         .addOutputLines(
             "A.java",
             "class A {",
             "  String m() {",
-            "    return \"foo\\\"bar'baz\\bqux\";",
+            "    char a = 'c' /* 'c' */; /* \"a\" */",
+            "    char b = '\\'' /* '\\'' */; /* \"b\" */",
+            "    return \"foo\\\"bar\\'baz\\bqux\" /* \"foo\\\"bar'baz\\bqux\" */;",
             "  }",
             "}")
         .doTest(TestMode.TEXT_MATCH);
@@ -254,24 +260,38 @@ UnwrapMethodInvocationDroppingWhitespaceAndCommentsTestChecker.class, getClass()
   }
 
   /**
-   * A {@link BugChecker} that applies {@link SourceCode#toStringConstantExpression(CharSequence)}
-   * to string literals.
+   * 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 {
+      implements LiteralTreeMatcher, VariableTreeMatcher {
     private static final long serialVersionUID = 1L;
 
     @Override
     public Description matchLiteral(LiteralTree tree, VisitorState state) {
-      return Optional.ofNullable(ASTHelpers.constValue(tree, String.class))
+      // 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 ->
-                  describeMatch(
-                      tree,
-                      SuggestedFix.replace(tree, SourceCode.toStringConstantExpression(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)));
+    }
   }
 
   /**