Skip to content

Commit

Permalink
Optimize VisitorState#getConstantExpression
Browse files Browse the repository at this point in the history
By avoiding a second pass over the string.

Fixes #4586

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4586 from PicnicSupermarket:sschroevers/optimize-getConstantExpression 45f3401
PiperOrigin-RevId: 711440462
  • Loading branch information
Stephan202 authored and Error Prone Team committed Jan 3, 2025
1 parent 85af056 commit 789b5c0
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 23 deletions.
27 changes: 27 additions & 0 deletions check_api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,22 @@
</jdkToolchain>
</configuration>
</execution>
<execution>
<id>java23</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<jdkToolchain>
<version>23</version>
</jdkToolchain>
<compileSourceRoots>
<compileSourceRoot>${basedir}/src/main/java23</compileSourceRoot>
</compileSourceRoots>
<!-- multiReleaseOutput requires setting release -->
<outputDirectory>${project.build.outputDirectory}/META-INF/versions/23</outputDirectory>
</configuration>
</execution>
<execution>
<id>java24</id>
<configuration>
Expand All @@ -197,6 +213,17 @@
</executions>

</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<configuration>
<archive>
<manifestEntries>
<Multi-Release>true</Multi-Release>
</manifestEntries>
</archive>
</configuration>
</plugin>
</plugins>
</build>

Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
24 changes: 4 additions & 20 deletions check_api/src/main/java/com/google/errorprone/VisitorState.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(", "))));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 789b5c0

Please sign in to comment.