From 12910072a60f89b6b011a9a4ef03763efb78e8cb Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 2 Jan 2025 09:04:50 -0800 Subject: [PATCH] Optimize VisitorState#getConstantExpression By avoiding a second pass over the string. Fixes #4586 FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/error-prone/pull/4586 from PicnicSupermarket:sschroevers/optimize-getConstantExpression 45f34017c3148325841e492b2908af04957966a7 PiperOrigin-RevId: 711440462 --- check_api/pom.xml | 27 +++++++++++ .../errorprone/ConstantStringExpressions.java | 46 +++++++++++++++++++ .../com/google/errorprone/VisitorState.java | 24 ++-------- .../errorprone/ConstantStringExpressions.java | 31 +++++++++++++ .../RobolectricShadowDirectlyOn.java | 2 +- .../flogger/FloggerArgumentToString.java | 2 +- .../flogger/FloggerStringConcatenation.java | 2 +- .../google/errorprone/VisitorStateTest.java | 2 + 8 files changed, 113 insertions(+), 23 deletions(-) create mode 100644 check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java create mode 100644 check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java diff --git a/check_api/pom.xml b/check_api/pom.xml index 061e83e0e6b..78c7f0c3a25 100644 --- a/check_api/pom.xml +++ b/check_api/pom.xml @@ -181,6 +181,22 @@ + + java23 + + compile + + + + 23 + + + ${basedir}/src/main/java23 + + + ${project.build.outputDirectory}/META-INF/versions/23 + + java24 @@ -197,6 +213,17 @@ + + org.apache.maven.plugins + maven-jar-plugin + + + + true + + + + diff --git a/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java b/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java new file mode 100644 index 00000000000..db42fce0e10 --- /dev/null +++ b/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java @@ -0,0 +1,46 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone; + +import com.sun.tools.javac.util.Convert; +import javax.lang.model.util.Elements; + +/** + * @see VisitorState#getConstantExpression(Object) + */ +final class ConstantStringExpressions { + private ConstantStringExpressions() {} + + static String toConstantStringExpression(Object value, Elements elements) { + if (!(value instanceof CharSequence)) { + return elements.getConstantExpression(value); + } + + // Don't escape single-quotes in string literals. + CharSequence str = (CharSequence) value; + 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.append('"').toString(); + } +} diff --git a/check_api/src/main/java/com/google/errorprone/VisitorState.java b/check_api/src/main/java/com/google/errorprone/VisitorState.java index 95c6eb70a0f..09ace3a98f4 100644 --- a/check_api/src/main/java/com/google/errorprone/VisitorState.java +++ b/check_api/src/main/java/com/google/errorprone/VisitorState.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.errorprone.ConstantStringExpressions.toConstantStringExpression; import static com.google.errorprone.util.ASTHelpers.getStartPosition; import com.google.common.annotations.VisibleForTesting; @@ -757,27 +758,10 @@ private static final class SharedState { /** * Returns the Java source code for a constant expression representing the given constant value. - * Like {@link Elements#getConstantExpression}, but doesn't over-escape single quotes in strings. + * Like {@link Elements#getConstantExpression}, but (a) before JDK 23, doesn't over-escape single + * quotes in strings and (b) treats any {@link CharSequence} as a {@link String}. */ 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); - } - } - return sb.toString(); - } - return escaped; + return toConstantStringExpression(value, this.getElements()); } } diff --git a/check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java b/check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java new file mode 100644 index 00000000000..704b51d3859 --- /dev/null +++ b/check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java @@ -0,0 +1,31 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone; + +import javax.lang.model.util.Elements; + +/** + * @see VisitorState#getConstantExpression(Object) + */ +final class ConstantStringExpressions { + private ConstantStringExpressions() {} + + static String toConstantStringExpression(Object value, Elements elements) { + return elements.getConstantExpression( + value instanceof CharSequence charSequence ? charSequence.toString() : value); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RobolectricShadowDirectlyOn.java b/core/src/main/java/com/google/errorprone/bugpatterns/RobolectricShadowDirectlyOn.java index 5e9bada9f6c..7b3f264b42d 100644 --- 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 index 4389a3888ac..4d49402dd6c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java @@ -352,7 +352,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 index ddf535ec835..0b5ea9a0f74 100644 --- 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 index d90b912fb96..9a85d84588a 100644 --- 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.