Optimize VisitorState#getConstantExpression

By avoiding a second pass over the string. Fixes #4586 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4586 from PicnicSupermarket:sschroevers/optimize-getConstantExpression ff02145 PiperOrigin-RevId: 677291745
google · Sep 22, 2024 · cfae81a · cfae81a
1 parent be99217
commit cfae81a
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 20 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/VisitorState.java b/check_api/src/main/java/com/google/errorprone/VisitorState.java
@@ -51,6 +51,7 @@
 import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
 import com.sun.tools.javac.tree.TreeMaker;
 import com.sun.tools.javac.util.Context;
+import com.sun.tools.javac.util.Convert;
 import com.sun.tools.javac.util.Name;
 import com.sun.tools.javac.util.Names;
 import com.sun.tools.javac.util.Options;
@@ -770,24 +771,20 @@ private static final class SharedState {
    * Like {@link Elements#getConstantExpression}, but doesn't over-escape single quotes in strings.
    */
   public String getConstantExpression(Object value) {
-    String escaped = getElements().getConstantExpression(value);
-    if (value instanceof String) {
-      // Don't escape single-quotes in string literals
-      StringBuilder sb = new StringBuilder();
-      for (int i = 0; i < escaped.length(); i++) {
-        char c = escaped.charAt(i);
-        if (c == '\\' && i + 1 < escaped.length()) {
-          char next = escaped.charAt(++i);
-          if (next != '\'') {
-            sb.append(c);
-          }
-          sb.append(next);
-        } else {
-          sb.append(c);
-        }
+    if (!(value instanceof CharSequence str)) {
+      return getElements().getConstantExpression(value);
+    }
+
+    // Don't escape single-quotes in string literals.
+    StringBuilder sb = new StringBuilder("\"");
+    for (int i = 0; i < str.length(); i++) {
+      char c = str.charAt(i);
+      if (c == '\'') {
+        sb.append('\'');
+      } else {
+        sb.append(Convert.quote(c));
       }
-      return sb.toString();
     }
-    return escaped;
+    return sb.append('"').toString();
   }
 }
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RobolectricShadowDirectlyOn.java b/core/src/main/java/com/google/errorprone/bugpatterns/RobolectricShadowDirectlyOn.java
@@ -73,7 +73,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     MethodSymbol symbol = getSymbol(parent);
     String argReplacement =
         Streams.concat(
-                Stream.of(state.getConstantExpression(symbol.getSimpleName().toString())),
+                Stream.of(state.getConstantExpression(symbol.getSimpleName())),
                 Streams.zip(
                     symbol.getParameters().stream(),
                     parent.getArguments().stream(),

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java
@@ -351,7 +351,7 @@ Description unwrapArguments(
     if (!fixed) {
       return NO_MATCH;
     }
-    fix.replace(tree.getArguments().get(0), state.getConstantExpression(sb.toString()));
+    fix.replace(tree.getArguments().get(0), state.getConstantExpression(sb));
     return describeMatch(tree, fix.build());
   }
 

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerStringConcatenation.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerStringConcatenation.java
@@ -112,7 +112,7 @@ protected Void defaultAction(Tree tree, Void unused) {
         tree,
         SuggestedFix.replace(
             argument,
-            state.getConstantExpression(formatString.toString())
+            state.getConstantExpression(formatString)
                 + ", "
                 + formatArguments.stream().map(state::getSourceForNode).collect(joining(", "))));
   }

diff --git a/core/src/test/java/com/google/errorprone/VisitorStateTest.java b/core/src/test/java/com/google/errorprone/VisitorStateTest.java
@@ -106,6 +106,8 @@ public void getConstantExpression() {
     assertThat(visitorState.getConstantExpression("hello \n world"))
         .isEqualTo("\"hello \\n world\"");
     assertThat(visitorState.getConstantExpression('\'')).isEqualTo("'\\''");
+    assertThat(visitorState.getConstantExpression(new StringBuilder("hello ' world")))
+        .isEqualTo("\"hello ' world\"");
   }
 
   // The following is taken from ErrorProneJavacPluginTest. There may be an easier way.