From c29a38e8340caca32c7ea4382edd6d5f35db7f07 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 21 Sep 2024 12:24:45 +0200 Subject: [PATCH 1/6] Optimize `VisitorState#getConstantExpression` By avoiding a second pass over the string. --- .../com/google/errorprone/VisitorState.java | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 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 index 95c6eb70a0f..58a2611572b 100644 --- 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; @@ -760,24 +761,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 String 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(); } } From dec3f8790e3002e783a3584503f65d144222c4bc Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 21 Sep 2024 12:33:14 +0200 Subject: [PATCH 2/6] Optional additional performance improvement --- check_api/src/main/java/com/google/errorprone/VisitorState.java | 2 +- .../errorprone/bugpatterns/RobolectricShadowDirectlyOn.java | 2 +- .../errorprone/bugpatterns/flogger/FloggerArgumentToString.java | 2 +- .../bugpatterns/flogger/FloggerStringConcatenation.java | 2 +- core/src/test/java/com/google/errorprone/VisitorStateTest.java | 2 ++ 5 files changed, 6 insertions(+), 4 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 index 58a2611572b..e385d0a1466 100644 --- a/check_api/src/main/java/com/google/errorprone/VisitorState.java +++ b/check_api/src/main/java/com/google/errorprone/VisitorState.java @@ -761,7 +761,7 @@ private static final class SharedState { * Like {@link Elements#getConstantExpression}, but doesn't over-escape single quotes in strings. */ public String getConstantExpression(Object value) { - if (!(value instanceof String str)) { + if (!(value instanceof CharSequence str)) { return getElements().getConstantExpression(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. From 624fa2d08ff7eac9f2b6ad1f9ed450dace2e6a4d Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 28 Dec 2024 20:05:49 +0100 Subject: [PATCH 3/6] Support JDK 23+ --- check_api/pom.xml | 27 +++++++++++ .../errorprone/ConstantStringExpressions.java | 45 +++++++++++++++++++ .../com/google/errorprone/VisitorState.java | 21 ++------- .../errorprone/ConstantStringExpressions.java | 34 ++++++++++++++ 4 files changed, 110 insertions(+), 17 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..9b988cfb44e --- /dev/null +++ b/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java @@ -0,0 +1,45 @@ +/* + * 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; + +/** + * @see VisitorState#getConstantExpression(Object) + */ +final class ConstantStringExpressions { + private ConstantStringExpressions() {} + + static String toConstantStringExpression(Object value, VisitorState state) { + if (!(value instanceof CharSequence)) { + return state.getElements().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 e385d0a1466..7d769a3799e 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; @@ -51,7 +52,6 @@ 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; @@ -758,23 +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) { - 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.append('"').toString(); + return toConstantStringExpression(value, this); } } 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..449e4c4f0bb --- /dev/null +++ b/check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java @@ -0,0 +1,34 @@ +/* + * 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.google.errorprone.VisitorState; +import com.sun.tools.javac.util.Convert; + +/** + * @see VisitorState#getConstantExpression(Object) + */ +final class ConstantStringExpressions { + private ConstantStringExpressions() {} + + static String toConstantStringExpression(Object value, VisitorState state) { + return state + .getElements() + .getConstantExpression( + value instanceof CharSequence charSequence ? charSequence.toString() : value); + } +} From 07dfafacaa5feeda6ceb201e363858aefc9af91c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 4 Jan 2025 13:41:43 +0100 Subject: [PATCH 4/6] Proposal to fix the build --- check_api/pom.xml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/check_api/pom.xml b/check_api/pom.xml index 78c7f0c3a25..ec02ab7afbe 100644 --- a/check_api/pom.xml +++ b/check_api/pom.xml @@ -210,6 +210,22 @@ ${project.build.outputDirectory}/META-INF/versions/24 + + + reset-output-directory + + compile + + + + ${basedir}/nonexistent-directory + + ${project.build.outputDirectory} + + From 4f61c1b3189b8ae6992fb9dacff66a0abda60fb8 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 4 Jan 2025 13:45:51 +0100 Subject: [PATCH 5/6] Clarify --- check_api/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/check_api/pom.xml b/check_api/pom.xml index ec02ab7afbe..9e8bc1e8df1 100644 --- a/check_api/pom.xml +++ b/check_api/pom.xml @@ -214,7 +214,7 @@ + directory of a module `A` on which it depends, rather than `A`'s installed JAR. --> reset-output-directory compile From 10b06e758767766c2712b655b5e3673aee66fc92 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 4 Jan 2025 20:46:56 +0100 Subject: [PATCH 6/6] Also address the other feedback --- .../com/google/errorprone/ConstantStringExpressions.java | 5 +++-- .../main/java/com/google/errorprone/VisitorState.java | 2 +- .../com/google/errorprone/ConstantStringExpressions.java | 9 ++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java b/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java index 9b988cfb44e..db42fce0e10 100644 --- a/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java +++ b/check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java @@ -17,6 +17,7 @@ package com.google.errorprone; import com.sun.tools.javac.util.Convert; +import javax.lang.model.util.Elements; /** * @see VisitorState#getConstantExpression(Object) @@ -24,9 +25,9 @@ final class ConstantStringExpressions { private ConstantStringExpressions() {} - static String toConstantStringExpression(Object value, VisitorState state) { + static String toConstantStringExpression(Object value, Elements elements) { if (!(value instanceof CharSequence)) { - return state.getElements().getConstantExpression(value); + return elements.getConstantExpression(value); } // Don't escape single-quotes in string literals. 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 7d769a3799e..1e27261cfa3 100644 --- a/check_api/src/main/java/com/google/errorprone/VisitorState.java +++ b/check_api/src/main/java/com/google/errorprone/VisitorState.java @@ -762,6 +762,6 @@ private static final class SharedState { * quotes in strings and (b) treats any {@link CharSequence} as a {@link String}. */ public String getConstantExpression(Object value) { - return toConstantStringExpression(value, this); + return toConstantStringExpression(value, 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 index 449e4c4f0bb..79de5093000 100644 --- a/check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java +++ b/check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java @@ -18,6 +18,7 @@ import com.google.errorprone.VisitorState; import com.sun.tools.javac.util.Convert; +import javax.lang.model.util.Elements; /** * @see VisitorState#getConstantExpression(Object) @@ -25,10 +26,8 @@ final class ConstantStringExpressions { private ConstantStringExpressions() {} - static String toConstantStringExpression(Object value, VisitorState state) { - return state - .getElements() - .getConstantExpression( - value instanceof CharSequence charSequence ? charSequence.toString() : value); + static String toConstantStringExpression(Object value, Elements elements) { + return elements.getConstantExpression( + value instanceof CharSequence charSequence ? charSequence.toString() : value); } }