Skip to content

Commit 308c6a5

Browse files
nick-someoneError Prone Team
nick-someone
authored and
Error Prone Team
committed
When an inlined method refers to method arguments in the true or false
branch of a conditional statement, refuse to allow it to be inlined. Before calling the method in question, the caller has definitely executed the expressions that yield the values passed to the method. However, after inlining the body of the method into the callsite, one of the two expressions will no longer be executed, leading to a subtle behavior change if the un-executed expression was side-effecting. PiperOrigin-RevId: 505200171
1 parent ec13312 commit 308c6a5

File tree

5 files changed

+98
-69
lines changed

5 files changed

+98
-69
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/inlineme/InlinabilityResult.java‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.common.collect.ImmutableSet;
2626
import com.google.errorprone.VisitorState;
2727
import com.google.errorprone.util.ASTHelpers;
28+
import com.sun.source.tree.ConditionalExpressionTree;
2829
import com.sun.source.tree.ExpressionStatementTree;
2930
import com.sun.source.tree.ExpressionTree;
3031
import com.sun.source.tree.IdentifierTree;
@@ -36,6 +37,7 @@
3637
import com.sun.source.tree.ReturnTree;
3738
import com.sun.source.tree.StatementTree;
3839
import com.sun.source.tree.Tree;
40+
import com.sun.source.tree.Tree.Kind;
3941
import com.sun.source.util.TreePath;
4042
import com.sun.source.util.TreePathScanner;
4143
import com.sun.source.util.TreeScanner;
@@ -106,7 +108,7 @@ enum InlineValidationErrorReason {
106108
"InlineMe cannot be applied when the implementation references deprecated or less visible"
107109
+ " API elements:"),
108110
API_IS_PRIVATE("InlineMe cannot be applied to private APIs."),
109-
LAMBDA_CAPTURES_PARAMETER(
111+
BODY_WOULD_EVALUATE_DIFFERENTLY(
110112
"Inlining this method will result in a change in evaluation timing for one or more"
111113
+ " arguments to this method."),
112114
METHOD_CAN_BE_OVERIDDEN_AND_CANT_BE_FIXED(
@@ -179,14 +181,14 @@ static InlinabilityResult forMethod(MethodTree tree, VisitorState state) {
179181

180182
// TODO(kak): declare a list of all the types we don't want to allow (e.g., ClassTree) and use
181183
// contains
182-
if (body.toString().contains("{")) {
184+
if (body.toString().contains("{") || body.getKind() == Kind.CONDITIONAL_EXPRESSION) {
183185
return fromError(InlineValidationErrorReason.COMPLEX_STATEMENT, body);
184186
}
185187

186-
Symbol usedMultipliedTimes = usesVariablesMultipleTimes(body, methSymbol.params(), state);
187-
if (usedMultipliedTimes != null) {
188+
Symbol usedMultipleTimes = usesVariablesMultipleTimes(body, methSymbol.params(), state);
189+
if (usedMultipleTimes != null) {
188190
return fromError(
189-
InlineValidationErrorReason.REUSE_OF_ARGUMENTS, body, usedMultipliedTimes.toString());
191+
InlineValidationErrorReason.REUSE_OF_ARGUMENTS, body, usedMultipleTimes.toString());
190192
}
191193

192194
Tree privateOrDeprecatedApi =
@@ -198,8 +200,8 @@ static InlinabilityResult forMethod(MethodTree tree, VisitorState state) {
198200
state.getSourceForNode(privateOrDeprecatedApi));
199201
}
200202

201-
if (hasLambdaCapturingParameters(tree, body)) {
202-
return fromError(InlineValidationErrorReason.LAMBDA_CAPTURES_PARAMETER, body);
203+
if (hasArgumentInPossiblyNonExecutedPosition(tree, body)) {
204+
return fromError(InlineValidationErrorReason.BODY_WOULD_EVALUATE_DIFFERENTLY, body);
203205
}
204206

205207
if (ASTHelpers.methodCanBeOverridden(methSymbol)) {
@@ -363,27 +365,41 @@ private static Visibility getVisibility(Symbol symbol) {
363365
}
364366
}
365367

366-
private static boolean hasLambdaCapturingParameters(MethodTree meth, ExpressionTree statement) {
368+
private static boolean hasArgumentInPossiblyNonExecutedPosition(
369+
MethodTree meth, ExpressionTree statement) {
367370
AtomicBoolean paramReferred = new AtomicBoolean(false);
368371
ImmutableSet<VarSymbol> params =
369372
meth.getParameters().stream().map(ASTHelpers::getSymbol).collect(toImmutableSet());
370373
new TreeScanner<Void, Void>() {
371-
LambdaExpressionTree currentLambdaTree = null;
374+
Tree currentContextTree = null;
372375

373376
@Override
374377
public Void visitLambdaExpression(LambdaExpressionTree lambdaExpressionTree, Void o) {
375-
LambdaExpressionTree lastContext = currentLambdaTree;
376-
currentLambdaTree = lambdaExpressionTree;
378+
Tree lastContext = currentContextTree;
379+
currentContextTree = lambdaExpressionTree;
377380
scan(lambdaExpressionTree.getBody(), null);
378-
currentLambdaTree = lastContext;
381+
currentContextTree = lastContext;
382+
return null;
383+
}
384+
385+
@Override
386+
public Void visitConditionalExpression(ConditionalExpressionTree ceTree, Void o) {
387+
scan(ceTree.getCondition(), null);
388+
// If the variables show up in the left or right side, they may conditionally not be
389+
// executed.
390+
Tree lastContext = currentContextTree;
391+
currentContextTree = ceTree;
392+
scan(ceTree.getTrueExpression(), null);
393+
scan(ceTree.getFalseExpression(), null);
394+
currentContextTree = lastContext;
379395
return null;
380396
}
381397

382398
@Override
383399
public Void visitIdentifier(IdentifierTree identifierTree, Void aVoid) {
384400
// If the lambda captures method parameters, inlining the method body can change the
385401
// timing of the evaluation of the arguments.
386-
if (currentLambdaTree != null && params.contains(getSymbol(identifierTree))) {
402+
if (currentContextTree != null && params.contains(getSymbol(identifierTree))) {
387403
paramReferred.set(true);
388404
}
389405
return super.visitIdentifier(identifierTree, null);

‎core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ && stringContainsComments(state.getSourceForNode(tree), state.context)) {
183183
boolean varargsWithEmptyArguments = false;
184184
if (symbol.isVarArgs()) {
185185
// If we're calling a varargs method, its inlining *should* have the varargs parameter in a
186-
// reasonable position. If there are are 0 arguments, we'll need to do more surgery
186+
// reasonable position. If there are 0 arguments, we'll need to do more surgery
187187
if (callingVars.size() == varNames.size() - 1) {
188188
varargsWithEmptyArguments = true;
189189
} else {

‎core/src/test/java/com/google/errorprone/bugpatterns/inlineme/InlinerTest.java‎

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,55 +1141,6 @@ public void customInlineMe() {
11411141
.doTest();
11421142
}
11431143

1144-
@Test
1145-
public void ternaryInlining_b266848535() {
1146-
refactoringTestHelper
1147-
.addInputLines(
1148-
"Client.java",
1149-
"import com.google.common.collect.ImmutableList;",
1150-
"import com.google.errorprone.annotations.InlineMe;",
1151-
"import java.util.Optional;",
1152-
"public final class Client {",
1153-
" @Deprecated",
1154-
" @InlineMe(",
1155-
" replacement = ",
1156-
"\"this.getList().isEmpty() ? Optional.empty() : Optional.of(this.getList().get(0))\",",
1157-
" imports = {\"java.util.Optional\"})",
1158-
" public Optional<String> getFoo() {",
1159-
" return getList().isEmpty() ? Optional.empty() : Optional.of(getList().get(0));",
1160-
" }",
1161-
" public ImmutableList<String> getList() {",
1162-
" return ImmutableList.of();",
1163-
" }",
1164-
"}")
1165-
.expectUnchanged()
1166-
.addInputLines(
1167-
"Caller.java",
1168-
"import static com.google.common.truth.Truth.assertThat;",
1169-
"public final class Caller {",
1170-
" public void doTest() {",
1171-
" Client client = new Client();",
1172-
" assertThat(client.getFoo().get()).isEqualTo(\"hi\");",
1173-
" }",
1174-
"}")
1175-
.addOutputLines(
1176-
"out/Caller.java",
1177-
"import static com.google.common.truth.Truth.assertThat;",
1178-
"import java.util.Optional;",
1179-
"public final class Caller {",
1180-
" public void doTest() {",
1181-
" Client client = new Client();",
1182-
// TODO(b/266848535): this is a bug; we need to add parens around the ternary
1183-
" assertThat(",
1184-
" client.getList().isEmpty() ",
1185-
" ? Optional.empty()",
1186-
" : Optional.of(client.getList().get(0)).get())",
1187-
" .isEqualTo(\"hi\");",
1188-
" }",
1189-
"}")
1190-
.doTest();
1191-
}
1192-
11931144
@Test
11941145
public void varArgs_b268215956() {
11951146
refactoringTestHelper

‎core/src/test/java/com/google/errorprone/bugpatterns/inlineme/SuggesterTest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -678,9 +678,9 @@ public void ternaryOverMultipleLines() {
678678
"public final class Client {",
679679
" @Deprecated",
680680
" public Duration getDeadline(Duration deadline) {",
681-
" return deadline.compareTo(Duration.ZERO) > 0",
681+
" return (deadline.compareTo(Duration.ZERO) > 0",
682682
" ? Duration.ofSeconds(42)",
683-
" : Duration.ZERO;",
683+
" : Duration.ZERO);",
684684
" }",
685685
"}")
686686
.addOutputLines(
@@ -689,14 +689,14 @@ public void ternaryOverMultipleLines() {
689689
"import com.google.errorprone.annotations.InlineMe;",
690690
"import java.time.Duration;",
691691
"public final class Client {",
692-
" @InlineMe(replacement = \"deadline.compareTo(Duration.ZERO) > 0 ?"
693-
+ " Duration.ofSeconds(42) : Duration.ZERO\", ",
692+
" @InlineMe(replacement = \"(deadline.compareTo(Duration.ZERO) > 0 ?"
693+
+ " Duration.ofSeconds(42) : Duration.ZERO)\", ",
694694
"imports = \"java.time.Duration\")",
695695
" @Deprecated",
696696
" public Duration getDeadline(Duration deadline) {",
697-
" return deadline.compareTo(Duration.ZERO) > 0",
697+
" return (deadline.compareTo(Duration.ZERO) > 0",
698698
" ? Duration.ofSeconds(42)",
699-
" : Duration.ZERO;",
699+
" : Duration.ZERO);",
700700
" }",
701701
"}")
702702
.doTest();

‎core/src/test/java/com/google/errorprone/bugpatterns/inlineme/ValidatorTest.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,68 @@ public void instanceMethod_withLambda() {
200200
.doTest();
201201
}
202202

203+
@Test
204+
public void instanceMethod_ternaryExpression_varsInCondition() {
205+
helper
206+
.addSourceLines(
207+
"Client.java",
208+
"import com.google.errorprone.annotations.InlineMe;",
209+
"public final class Client {",
210+
// OK, since x and y are always evaluated
211+
" @InlineMe(replacement = \"this.after(x == y ? true : false)\")",
212+
" @Deprecated",
213+
" public boolean before(int x, int y) {",
214+
" return after(x == y ? true : false);",
215+
" }",
216+
" public boolean after(boolean b) {",
217+
" return !b;",
218+
" }",
219+
"}")
220+
.doTest();
221+
}
222+
223+
@Test
224+
public void instanceMethod_ternaryExpression_varsInOtherArms() {
225+
helper
226+
.addSourceLines(
227+
"Client.java",
228+
"import com.google.errorprone.annotations.InlineMe;",
229+
"public final class Client {",
230+
" @InlineMe(replacement = \"this.after(x > 0 ? y : false)\")",
231+
" @Deprecated",
232+
" // BUG: Diagnostic contains: evaluation timing",
233+
" public boolean before(int x, boolean y) {",
234+
" return after(x > 0 ? y : true);",
235+
" }",
236+
" public boolean after(boolean b) {",
237+
" return !b;",
238+
" }",
239+
"}")
240+
.doTest();
241+
}
242+
243+
@Test
244+
public void instanceMethod_topLevelTernary() {
245+
helper
246+
.addSourceLines(
247+
"Client.java",
248+
"import com.google.errorprone.annotations.InlineMe;",
249+
"public final class Client {",
250+
" @InlineMe(replacement = \"x == y ? this.a() : this.b()\")",
251+
" // BUG: Diagnostic contains: complex statement",
252+
" public boolean before(int x, int y) {",
253+
" return x == y ? a() : b();",
254+
" }",
255+
" public boolean a() {",
256+
" return true;",
257+
" }",
258+
" public boolean b() {",
259+
" return false;",
260+
" }",
261+
"}")
262+
.doTest();
263+
}
264+
203265
@Test
204266
public void instanceMethod_withLambdaAndVariable() {
205267
helper

0 commit comments

Comments
 (0)