Skip to content

Commit 9212a2c

Browse files
cushonError Prone Team
cushon
authored and
Error Prone Team
committed
Add FormatStringShouldUsePlaceholders
PiperOrigin-RevId: 830952980
1 parent c573889 commit 9212a2c

File tree

3 files changed

+288
-0
lines changed

3 files changed

+288
-0
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/formatstring/FormatStringShouldUsePlaceholders.java‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* Copyright 2025 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns.formatstring;
18+
19+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
20+
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
21+
22+
import com.google.common.collect.ImmutableList;
23+
import com.google.errorprone.BugPattern;
24+
import com.google.errorprone.VisitorState;
25+
import com.google.errorprone.bugpatterns.BugChecker;
26+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
27+
import com.google.errorprone.fixes.Fix;
28+
import com.google.errorprone.fixes.SuggestedFix;
29+
import com.google.errorprone.matchers.Description;
30+
import com.google.errorprone.util.ASTHelpers;
31+
import com.sun.source.tree.BinaryTree;
32+
import com.sun.source.tree.ExpressionTree;
33+
import com.sun.source.tree.MethodInvocationTree;
34+
import com.sun.source.tree.Tree;
35+
import com.sun.source.tree.Tree.Kind;
36+
import com.sun.source.tree.TreeVisitor;
37+
import com.sun.source.util.SimpleTreeVisitor;
38+
import java.util.ArrayList;
39+
import java.util.List;
40+
41+
/** A BugPattern; see the summary. */
42+
@BugPattern(
43+
summary = "Using a format string avoids string concatenation in the common case.",
44+
explanation =
45+
"It usually hurts performance to eagerly generate error messages with +, as you pay the"
46+
+ " cost of the string conversion whether or not the condition fails. It's usually more"
47+
+ " efficient to use %s as a placeholder and to pass the additional variables as"
48+
+ " further arguments.",
49+
severity = WARNING)
50+
public class FormatStringShouldUsePlaceholders extends BugChecker
51+
implements MethodInvocationTreeMatcher {
52+
private static final TreeVisitor<Void, List<ExpressionTree>> CONCATENATIONS =
53+
new SimpleTreeVisitor<Void, List<ExpressionTree>>() {
54+
@Override
55+
protected Void defaultAction(Tree tree, List<ExpressionTree> concats) {
56+
concats.add((ExpressionTree) tree);
57+
return null;
58+
}
59+
60+
@Override
61+
public Void visitBinary(BinaryTree tree, List<ExpressionTree> concats) {
62+
if (tree.getKind() == Kind.PLUS) {
63+
tree.getLeftOperand().accept(this, concats);
64+
tree.getRightOperand().accept(this, concats);
65+
return null;
66+
} else {
67+
return super.visitBinary(tree, concats);
68+
}
69+
}
70+
};
71+
72+
private static ImmutableList<ExpressionTree> formatArguments(
73+
MethodInvocationTree tree, VisitorState state) {
74+
ImmutableList<ExpressionTree> args = FormatStringUtils.formatMethodArguments(tree, state);
75+
if (!args.isEmpty()) {
76+
return args;
77+
}
78+
int index = LenientFormatStringUtils.getLenientFormatStringPosition(tree, state);
79+
if (index != -1) {
80+
return ImmutableList.copyOf(tree.getArguments().subList(index, tree.getArguments().size()));
81+
}
82+
return ImmutableList.of();
83+
}
84+
85+
@Override
86+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
87+
final ImmutableList<? extends ExpressionTree> arguments = formatArguments(tree, state);
88+
if (arguments.isEmpty()) {
89+
return Description.NO_MATCH;
90+
}
91+
ExpressionTree formatString = arguments.getFirst();
92+
93+
// If the value is a compile-time constant, either it has no concatenations or the
94+
// concatenations result in a constant expression that doesn't need inlining.
95+
if (ASTHelpers.constValue(formatString, String.class) != null) {
96+
return Description.NO_MATCH;
97+
}
98+
99+
List<ExpressionTree> concats = new ArrayList<ExpressionTree>();
100+
formatString.accept(CONCATENATIONS, concats);
101+
if (concats.size() <= 1) {
102+
return Description.NO_MATCH;
103+
}
104+
105+
StringBuilder newMessage = new StringBuilder("\"");
106+
StringBuilder newArgs = new StringBuilder();
107+
108+
// Start this at the error string.
109+
// We will increment this as we pick up existing format string args and append them.
110+
int lastAddedArgPosition = 0;
111+
112+
for (ExpressionTree concat : concats) {
113+
if (concat.getKind() == Kind.STRING_LITERAL) {
114+
String sourceString = state.getSourceForNode(concat);
115+
// 0th and last char is a double quote i.e. '"', so collect string without double quotes.
116+
newMessage.append(sourceString.subSequence(1, sourceString.length() - 1));
117+
118+
// Figure out how far to advance the argument pointer.
119+
int len = sourceString.length();
120+
for (int i = 0; i < len; ++i) {
121+
char c = sourceString.charAt(i);
122+
if (c == '%' && i < len - 1) {
123+
if (sourceString.charAt(++i) == 's' && lastAddedArgPosition < arguments.size() - 1) {
124+
newArgs
125+
.append(", ")
126+
.append(state.getSourceForNode(arguments.get(++lastAddedArgPosition)));
127+
}
128+
}
129+
}
130+
} else {
131+
newMessage.append("%s");
132+
newArgs.append(", ").append(state.getSourceForNode(concat));
133+
}
134+
}
135+
// If we have any remaining args, add them as well. Though this is a bug, so if this ever
136+
// gets triggered maybe we should just revert this until we can make a more proper fix.
137+
if (lastAddedArgPosition < arguments.size() - 1) {
138+
return Description.NO_MATCH;
139+
}
140+
if (newArgs.length() == 0) {
141+
return Description.NO_MATCH;
142+
}
143+
144+
// Match the number of %s in format string and number of arguments.
145+
int numberPercentS = newMessage.toString().split("%s", -1).length - 1;
146+
int numberArgs = newArgs.toString().split(",", -1).length - 1;
147+
148+
// we attempt to fix an extra `%s` at the end.
149+
if ((numberPercentS == numberArgs + 1) && newMessage.toString().endsWith("%s")) {
150+
// there is exactly one `%s` extra and that too at the end, lets remove it
151+
// For example, we'll have transformed the erroneous log("result: %s" + result) into
152+
// log("result: %s%s", result), which can be fixed by removing a trailing %s.
153+
newMessage.delete(newMessage.length() - 2, newMessage.length());
154+
} else if (numberArgs != numberPercentS) {
155+
return Description.NO_MATCH;
156+
}
157+
158+
// Replace all the arguments from the "format string %s message " + stuff, all the way
159+
// to the end of the arg list. Here's a drawing:
160+
// replace between: v v
161+
// checkState(foo != false, "old message %s " + stuff, foo);
162+
// ->
163+
// checkState(foo != false, "new message %s %s", foo, stuff);
164+
// `arguments` already contains only the format string and the values to be substituted in.
165+
int start = getStartPosition(arguments.getFirst());
166+
int end = state.getEndPosition(arguments.getLast());
167+
168+
String replacement = new StringBuilder(newMessage).append("\"").append(newArgs).toString();
169+
Fix fix = SuggestedFix.replace(start, end, replacement);
170+
return describeMatch(tree, fix);
171+
}
172+
}

‎core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,7 @@
516516
import com.google.errorprone.bugpatterns.formatstring.AnnotateFormatMethod;
517517
import com.google.errorprone.bugpatterns.formatstring.FormatString;
518518
import com.google.errorprone.bugpatterns.formatstring.FormatStringAnnotationChecker;
519+
import com.google.errorprone.bugpatterns.formatstring.FormatStringShouldUsePlaceholders;
519520
import com.google.errorprone.bugpatterns.formatstring.InlineFormatString;
520521
import com.google.errorprone.bugpatterns.formatstring.LenientFormatStringValidation;
521522
import com.google.errorprone.bugpatterns.inject.AssistedInjectAndInjectOnConstructors;
@@ -967,6 +968,7 @@ public static ScannerSupplier warningChecks() {
967968
FloggerArgumentToString.class,
968969
FloggerPerWithoutRateLimit.class,
969970
FloggerStringConcatenation.class,
971+
FormatStringShouldUsePlaceholders.class,
970972
FragmentInjection.class,
971973
FragmentNotInstantiable.class,
972974
FutureReturnValueIgnored.class,

‎core/src/test/java/com/google/errorprone/bugpatterns/formatstring/FormatStringShouldUsePlaceholdersTest.java‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/*
2+
* Copyright 2025 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns.formatstring;
18+
19+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
20+
import org.junit.Test;
21+
import org.junit.runner.RunWith;
22+
import org.junit.runners.JUnit4;
23+
24+
@RunWith(JUnit4.class)
25+
public class FormatStringShouldUsePlaceholdersTest {
26+
27+
private final BugCheckerRefactoringTestHelper refactoringHelper =
28+
BugCheckerRefactoringTestHelper.newInstance(
29+
FormatStringShouldUsePlaceholders.class, getClass());
30+
31+
@Test
32+
public void verifyRefactoring() {
33+
refactoringHelper
34+
.addInputLines(
35+
"Test.java",
36+
"""
37+
import com.google.common.base.Verify;
38+
39+
public class Test {
40+
public void verify(int i, Object o) {
41+
Verify.verify(false, "Raspberry " + i + " irradiates " + o);
42+
Verify.verifyNotNull(o, o + " is null");
43+
Verify.verify(false, "%s" + i);
44+
Verify.verify(false, "%s%s" + i);
45+
}
46+
}
47+
""")
48+
.addOutputLines(
49+
"Test.java",
50+
"""
51+
import com.google.common.base.Verify;
52+
53+
public class Test {
54+
public void verify(int i, Object o) {
55+
Verify.verify(false, "Raspberry %s irradiates %s", i, o);
56+
Verify.verifyNotNull(o, "%s is null", o);
57+
Verify.verify(false, "%s", i);
58+
Verify.verify(false, "%s%s" + i);
59+
}
60+
}
61+
""")
62+
.doTest();
63+
}
64+
65+
@Test
66+
public void preconditionsRefactoring() {
67+
refactoringHelper
68+
.addInputLines(
69+
"Test.java",
70+
"""
71+
import com.google.common.base.Preconditions;
72+
73+
public class Test {
74+
private static final String STRING_CONST = "string constant";
75+
76+
public void preconditions(int i, Object o) {
77+
Preconditions.checkArgument(false, "Raspberry " + i + " irradiates " + o);
78+
Preconditions.checkArgument(false, "Concat " + i + " and argument %s", o);
79+
Preconditions.checkArgument(false, i + " begin and argument %s", o);
80+
Preconditions.checkArgument(false, "Argument %s and concat " + i, o);
81+
Preconditions.checkArgument(false, "How about %s the middle " + i + " as well %s?", o, o);
82+
Preconditions.checkArgument(false, "But string concat %s " + "string is ok", i);
83+
Preconditions.checkArgument(false, "And string concat " + STRING_CONST + " is ok");
84+
Preconditions.checkState(false, i + " defenestrates " + o);
85+
Preconditions.checkNotNull(o);
86+
Preconditions.checkState(o != null, o);
87+
}
88+
}
89+
""")
90+
.addOutputLines(
91+
"Test.java",
92+
"""
93+
import com.google.common.base.Preconditions;
94+
95+
public class Test {
96+
private static final String STRING_CONST = "string constant";
97+
98+
public void preconditions(int i, Object o) {
99+
Preconditions.checkArgument(false, "Raspberry %s irradiates %s", i, o);
100+
Preconditions.checkArgument(false, "Concat %s and argument %s", i, o);
101+
Preconditions.checkArgument(false, "%s begin and argument %s", i, o);
102+
Preconditions.checkArgument(false, "Argument %s and concat %s", o, i);
103+
Preconditions.checkArgument(false, "How about %s the middle %s as well %s?", o, i, o);
104+
Preconditions.checkArgument(false, "But string concat %s " + "string is ok", i);
105+
Preconditions.checkArgument(false, "And string concat " + STRING_CONST + " is ok");
106+
Preconditions.checkState(false, "%s defenestrates %s", i, o);
107+
Preconditions.checkNotNull(o);
108+
Preconditions.checkState(o != null, o);
109+
}
110+
}
111+
""")
112+
.doTest();
113+
}
114+
}

0 commit comments

Comments
 (0)