Skip to content

Commit

Permalink
When an inlined method refers to method arguments in the true or false
Browse files Browse the repository at this point in the history 
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: 617624766
  • Loading branch information
@nick-someone
nick-someone authored and Error Prone Team committed Mar 20, 2024
1 parent ec13312 commit 437293d
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 69 deletions.
42 changes: 29 additions & 13 deletions core/src/main/java/com/google/errorprone/bugpatterns/inlineme/InlinabilityResult.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionStatementTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
Expand All @@ -36,6 +37,7 @@
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;
import com.sun.source.util.TreePathScanner;
import com.sun.source.util.TreeScanner;
Expand Down Expand Up @@ -106,7 +108,7 @@ enum InlineValidationErrorReason {
"InlineMe cannot be applied when the implementation references deprecated or less visible"
+ " API elements:"),
API_IS_PRIVATE("InlineMe cannot be applied to private APIs."),
LAMBDA_CAPTURES_PARAMETER(
BODY_WOULD_EVALUATE_DIFFERENTLY(
"Inlining this method will result in a change in evaluation timing for one or more"
+ " arguments to this method."),
METHOD_CAN_BE_OVERIDDEN_AND_CANT_BE_FIXED(
Expand Down Expand Up @@ -179,14 +181,14 @@ static InlinabilityResult forMethod(MethodTree tree, VisitorState state) {

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

Symbol usedMultipliedTimes = usesVariablesMultipleTimes(body, methSymbol.params(), state);
if (usedMultipliedTimes != null) {
Symbol usedMultipleTimes = usesVariablesMultipleTimes(body, methSymbol.params(), state);
if (usedMultipleTimes != null) {
return fromError(
InlineValidationErrorReason.REUSE_OF_ARGUMENTS, body, usedMultipliedTimes.toString());
InlineValidationErrorReason.REUSE_OF_ARGUMENTS, body, usedMultipleTimes.toString());
}

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

if (hasLambdaCapturingParameters(tree, body)) {
return fromError(InlineValidationErrorReason.LAMBDA_CAPTURES_PARAMETER, body);
if (hasArgumentInPossiblyNonExecutedPosition(tree, body)) {
return fromError(InlineValidationErrorReason.BODY_WOULD_EVALUATE_DIFFERENTLY, body);
}

if (ASTHelpers.methodCanBeOverridden(methSymbol)) {
Expand Down Expand Up @@ -363,27 +365,41 @@ private static Visibility getVisibility(Symbol symbol) {
}
}

private static boolean hasLambdaCapturingParameters(MethodTree meth, ExpressionTree statement) {
private static boolean hasArgumentInPossiblyNonExecutedPosition(
MethodTree meth, ExpressionTree statement) {
AtomicBoolean paramReferred = new AtomicBoolean(false);
ImmutableSet<VarSymbol> params =
meth.getParameters().stream().map(ASTHelpers::getSymbol).collect(toImmutableSet());
new TreeScanner<Void, Void>() {
LambdaExpressionTree currentLambdaTree = null;
Tree currentContextTree = null;

@Override
public Void visitLambdaExpression(LambdaExpressionTree lambdaExpressionTree, Void o) {
LambdaExpressionTree lastContext = currentLambdaTree;
currentLambdaTree = lambdaExpressionTree;
Tree lastContext = currentContextTree;
currentContextTree = lambdaExpressionTree;
scan(lambdaExpressionTree.getBody(), null);
currentLambdaTree = lastContext;
currentContextTree = lastContext;
return null;
}

@Override
public Void visitConditionalExpression(ConditionalExpressionTree ceTree, Void o) {
scan(ceTree.getCondition(), null);
// If the variables show up in the left or right side, they may conditionally not be
// executed.
Tree lastContext = currentContextTree;
currentContextTree = ceTree;
scan(ceTree.getTrueExpression(), null);
scan(ceTree.getFalseExpression(), null);
currentContextTree = lastContext;
return null;
}

@Override
public Void visitIdentifier(IdentifierTree identifierTree, Void aVoid) {
// If the lambda captures method parameters, inlining the method body can change the
// timing of the evaluation of the arguments.
if (currentLambdaTree != null && params.contains(getSymbol(identifierTree))) {
if (currentContextTree != null && params.contains(getSymbol(identifierTree))) {
paramReferred.set(true);
}
return super.visitIdentifier(identifierTree, null);
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ && stringContainsComments(state.getSourceForNode(tree), state.context)) {
boolean varargsWithEmptyArguments = false;
if (symbol.isVarArgs()) {
// If we're calling a varargs method, its inlining *should* have the varargs parameter in a
// reasonable position. If there are are 0 arguments, we'll need to do more surgery
// reasonable position. If there are 0 arguments, we'll need to do more surgery
if (callingVars.size() == varNames.size() - 1) {
varargsWithEmptyArguments = true;
} else {
Expand Down
49 changes: 0 additions & 49 deletions core/src/test/java/com/google/errorprone/bugpatterns/inlineme/InlinerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1141,55 +1141,6 @@ public void customInlineMe() {
.doTest();
}

@Test
public void ternaryInlining_b266848535() {
refactoringTestHelper
.addInputLines(
"Client.java",
"import com.google.common.collect.ImmutableList;",
"import com.google.errorprone.annotations.InlineMe;",
"import java.util.Optional;",
"public final class Client {",
" @Deprecated",
" @InlineMe(",
" replacement = ",
"\"this.getList().isEmpty() ? Optional.empty() : Optional.of(this.getList().get(0))\",",
" imports = {\"java.util.Optional\"})",
" public Optional<String> getFoo() {",
" return getList().isEmpty() ? Optional.empty() : Optional.of(getList().get(0));",
" }",
" public ImmutableList<String> getList() {",
" return ImmutableList.of();",
" }",
"}")
.expectUnchanged()
.addInputLines(
"Caller.java",
"import static com.google.common.truth.Truth.assertThat;",
"public final class Caller {",
" public void doTest() {",
" Client client = new Client();",
" assertThat(client.getFoo().get()).isEqualTo(\"hi\");",
" }",
"}")
.addOutputLines(
"out/Caller.java",
"import static com.google.common.truth.Truth.assertThat;",
"import java.util.Optional;",
"public final class Caller {",
" public void doTest() {",
" Client client = new Client();",
// TODO(b/266848535): this is a bug; we need to add parens around the ternary
" assertThat(",
" client.getList().isEmpty() ",
" ? Optional.empty()",
" : Optional.of(client.getList().get(0)).get())",
" .isEqualTo(\"hi\");",
" }",
"}")
.doTest();
}

@Test
public void varArgs_b268215956() {
refactoringTestHelper
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/java/com/google/errorprone/bugpatterns/inlineme/SuggesterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -678,9 +678,9 @@ public void ternaryOverMultipleLines() {
"public final class Client {",
" @Deprecated",
" public Duration getDeadline(Duration deadline) {",
" return deadline.compareTo(Duration.ZERO) > 0",
" return (deadline.compareTo(Duration.ZERO) > 0",
" ? Duration.ofSeconds(42)",
" : Duration.ZERO;",
" : Duration.ZERO);",
" }",
"}")
.addOutputLines(
Expand All @@ -689,14 +689,14 @@ public void ternaryOverMultipleLines() {
"import com.google.errorprone.annotations.InlineMe;",
"import java.time.Duration;",
"public final class Client {",
" @InlineMe(replacement = \"deadline.compareTo(Duration.ZERO) > 0 ?"
+ " Duration.ofSeconds(42) : Duration.ZERO\", ",
" @InlineMe(replacement = \"(deadline.compareTo(Duration.ZERO) > 0 ?"
+ " Duration.ofSeconds(42) : Duration.ZERO)\", ",
"imports = \"java.time.Duration\")",
" @Deprecated",
" public Duration getDeadline(Duration deadline) {",
" return deadline.compareTo(Duration.ZERO) > 0",
" return (deadline.compareTo(Duration.ZERO) > 0",
" ? Duration.ofSeconds(42)",
" : Duration.ZERO;",
" : Duration.ZERO);",
" }",
"}")
.doTest();
Expand Down
62 changes: 62 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/inlineme/ValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,68 @@ public void instanceMethod_withLambda() {
.doTest();
}

@Test
public void instanceMethod_ternaryExpression_varsInCondition() {
helper
.addSourceLines(
"Client.java",
"import com.google.errorprone.annotations.InlineMe;",
"public final class Client {",
// OK, since x and y are always evaluated
" @InlineMe(replacement = \"this.after(x == y ? true : false)\")",
" @Deprecated",
" public boolean before(int x, int y) {",
" return after(x == y ? true : false);",
" }",
" public boolean after(boolean b) {",
" return !b;",
" }",
"}")
.doTest();
}

@Test
public void instanceMethod_ternaryExpression_varsInOtherArms() {
helper
.addSourceLines(
"Client.java",
"import com.google.errorprone.annotations.InlineMe;",
"public final class Client {",
" @InlineMe(replacement = \"this.after(x > 0 ? y : false)\")",
" @Deprecated",
" // BUG: Diagnostic contains: evaluation timing",
" public boolean before(int x, boolean y) {",
" return after(x > 0 ? y : true);",
" }",
" public boolean after(boolean b) {",
" return !b;",
" }",
"}")
.doTest();
}

@Test
public void instanceMethod_topLevelTernary() {
helper
.addSourceLines(
"Client.java",
"import com.google.errorprone.annotations.InlineMe;",
"public final class Client {",
" @InlineMe(replacement = \"x == y ? this.a() : this.b()\")",
" // BUG: Diagnostic contains: complex statement",
" public boolean before(int x, int y) {",
" return x == y ? a() : b();",
" }",
" public boolean a() {",
" return true;",
" }",
" public boolean b() {",
" return false;",
" }",
"}")
.doTest();
}

@Test
public void instanceMethod_withLambdaAndVariable() {
helper
Expand Down

0 comments on commit 437293d

Please sign in to comment.