Skip to content

[IfChainToSwitch] when safe mode is enabled, enforce strict matching of predicates in the if-chain; disallows method invocations etc. because converting these can change semantics #5488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Feb 19, 2026
+101 −37
Merged

[IfChainToSwitch] when safe mode is enabled, enforce strict matching of predicates in the if-chain; disallows method invocations etc. because converting these can change semantics #5488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 30 additions & 21 deletions core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher;
import com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
Expand Down Expand Up @@ -103,12 +104,14 @@ enum Validity {
private final boolean enableMain;
private final boolean enableSafe;
private final int maxChainLength;
private final ConstantExpressions constantExpressions;

@Inject
IfChainToSwitch(ErrorProneFlags flags) {
IfChainToSwitch(ErrorProneFlags flags, ConstantExpressions constantExpressions) {
enableMain = flags.getBoolean("IfChainToSwitch:EnableMain").orElse(false);
enableSafe = flags.getBoolean("IfChainToSwitch:EnableSafe").orElse(false);
maxChainLength = flags.getInteger("IfChainToSwitch:MaxChainLength").orElse(50);
this.constantExpressions = constantExpressions;
}

@Override
Expand Down Expand Up @@ -796,7 +799,7 @@ private static boolean hasDominanceViolation(
* converting it to a switch statement. Returns the analysis state following the analysis of the
* if statement at this level.
*/
private static IfChainAnalysisState analyzeIfStatement(
private IfChainAnalysisState analyzeIfStatement(
IfChainAnalysisState ifChainAnalysisState,
ExpressionTree condition,
StatementTree conditionalBlock,
Expand Down Expand Up @@ -900,7 +903,7 @@ public Boolean reduce(@Nullable Boolean left, @Nullable Boolean right) {
* so even if this method returns {@code Optional.empty()} the predicate may still be convertible
* by some other (unsupported) means.
*/
private static Optional<ExpressionTree> validatePredicateForSubject(
private Optional<ExpressionTree> validatePredicateForSubject(
ExpressionTree predicate,
Optional<ExpressionTree> subject,
VisitorState state,
Expand Down Expand Up @@ -1029,7 +1032,23 @@ private static Optional<ExpressionTree> validatePredicateForSubject(
return Optional.empty();
}

private static Optional<ExpressionTree> validateInstanceofForSubject(
/**
* Determines whether the {@code subject} expression "matches" the given {@code expression}. If
* {@code enableSafe} is true, then matching means that the subject must be referring to the same
* variable or constant expression. If {@code enableSafe} is false, then we also allow expressions
* that have potential side-effects.
*/
private boolean subjectMatches(
ExpressionTree subject, ExpressionTree expression, VisitorState state) {

boolean sameVariable = sameVariable(subject, expression);

return enableSafe
? sameVariable || constantExpressions.isSame(subject, expression, state)
: sameVariable || expressionSourceMatches(subject, expression, state);
}

private Optional<ExpressionTree> validateInstanceofForSubject(
ExpressionTree at,
InstanceOfTree instanceOfTree,
Optional<ExpressionTree> subject,
Expand All @@ -1044,10 +1063,7 @@ private static Optional<ExpressionTree> validateInstanceofForSubject(

ExpressionTree expression = at;
// Does this expression and the subject (if present) refer to the same thing?
if (subject.isPresent()
&& !(sameVariable(subject.get(), expression)
|| subject.get().equals(expression)
|| expressionSourceMatches(subject, expression, state))) {
if (subject.isPresent() && !subjectMatches(subject.get(), expression, state)) {
return Optional.empty();
}

Expand Down Expand Up @@ -1134,7 +1150,7 @@ private static Optional<ExpressionTree> validateInstanceofForSubject(
return Optional.of(expression);
}

private static Optional<ExpressionTree> validateCompileTimeConstantForSubject(
private Optional<ExpressionTree> validateCompileTimeConstantForSubject(
ExpressionTree lhs,
ExpressionTree rhs,
Optional<ExpressionTree> subject,
Expand All @@ -1150,10 +1166,7 @@ private static Optional<ExpressionTree> validateCompileTimeConstantForSubject(
ExpressionTree testExpression = compileTimeConstantOnLhs ? rhs : lhs;
ExpressionTree compileTimeConstant = compileTimeConstantOnLhs ? lhs : rhs;

if (subject.isPresent()
&& !(sameVariable(subject.get(), testExpression)
|| subject.get().equals(testExpression)
|| expressionSourceMatches(subject, testExpression, state))) {
if (subject.isPresent() && !subjectMatches(subject.get(), testExpression, state)) {
// Predicate not compatible with predicate of preceding if statement
return Optional.empty();
}
Expand Down Expand Up @@ -1210,7 +1223,7 @@ private static Optional<ExpressionTree> validateCompileTimeConstantForSubject(
return Optional.of(testExpression);
}

private static Optional<ExpressionTree> validateEnumPredicateForSubject(
private Optional<ExpressionTree> validateEnumPredicateForSubject(
ExpressionTree lhs,
ExpressionTree rhs,
Optional<ExpressionTree> subject,
Expand Down Expand Up @@ -1240,10 +1253,7 @@ private static Optional<ExpressionTree> validateEnumPredicateForSubject(
ExpressionTree compileTimeConstant = lhsIsEnumConstant ? lhs : rhs;
ExpressionTree testExpression = lhsIsEnumConstant ? rhs : lhs;

if (subject.isPresent()
&& !(sameVariable(subject.get(), testExpression)
|| subject.get().equals(testExpression)
|| expressionSourceMatches(subject, testExpression, state))) {
if (subject.isPresent() && !subjectMatches(subject.get(), testExpression, state)) {
return Optional.empty();
}

Expand Down Expand Up @@ -1298,10 +1308,9 @@ private static Optional<ExpressionTree> validateEnumPredicateForSubject(
* comments, etc.
*/
private static boolean expressionSourceMatches(
Optional<ExpressionTree> subject, ExpressionTree expression, VisitorState state) {
ExpressionTree subject, ExpressionTree expression, VisitorState state) {

return subject.isPresent()
&& state.getSourceForNode(subject.get()).equals(state.getSourceForNode(expression));
return state.getSourceForNode(subject).equals(state.getSourceForNode(expression));
}

/** Retrieves a list of all statements (if any) following the current path, if any. */
Expand Down
87 changes: 71 additions & 16 deletions core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,18 +327,16 @@ public void ifChain_dontAlwaysPullUp_error() {
import java.lang.Number;

class Test {
private Object suit;

public void foo(Suit s) {
this.suit = null;
Object suit = s;
System.out.println("yo");
if (this.suit instanceof String) {
if (suit instanceof String) {
System.out.println("It's a string!");
} else if (suit instanceof Number) {
System.out.println("It's a number!");
} else if (suit instanceof Suit) {
System.out.println("It's a Suit!");
} else if (this.suit instanceof Object o) {
} else if (suit instanceof Object o) {
System.out.println("It's an object!");
}
throw new AssertionError();
Expand All @@ -351,10 +349,8 @@ public void foo(Suit s) {
import java.lang.Number;

class Test {
private Object suit;

public void foo(Suit s) {
this.suit = null;
Object suit = s;
System.out.println("yo");
switch (suit) {
case String unused -> System.out.println("It's a string!");
Expand All @@ -381,18 +377,16 @@ public void ifChain_dontAlwaysPullUpSafe_error() {
import java.lang.Number;

class Test {
private Object suit;

public void foo(Suit s) {
this.suit = null;
Object suit = s;
System.out.println("yo");
if (this.suit instanceof String) {
if (suit instanceof String) {
System.out.println("It's a string!");
} else if (suit instanceof Number) {
System.out.println("It's a number!");
} else if (suit instanceof Suit) {
System.out.println("It's a Suit!");
} else if (this.suit instanceof Object o) {
} else if (suit instanceof Object o) {
System.out.println("It's an object!");
}
throw new AssertionError();
Expand All @@ -405,10 +399,8 @@ public void foo(Suit s) {
import java.lang.Number;

class Test {
private Object suit;

public void foo(Suit s) {
this.suit = null;
Object suit = s;
System.out.println("yo");
switch (suit) {
case String unused -> System.out.println("It's a string!");
Expand Down Expand Up @@ -2375,6 +2367,69 @@ public void foo(Suit s) {
.doTest();
}

@Test
public void ifChain_methodInvocation_error() {
refactoringHelper
.addInputLines(
"Test.java",
"""
class Test {
public void foo(Suit s) {
Object suit = s;
if (suit.hashCode() == 1) {
System.out.println("Hash code 1");
} else if (suit.hashCode() == 2) {
System.out.println("Hash code 2");
} else if (suit.hashCode() == 3) {
System.out.println("Hash code 3");
} else throw new AssertionError("Some other hash code");
}
}
""")
.addOutputLines(
"Test.java",
"""
class Test {
public void foo(Suit s) {
Object suit = s;
switch (suit.hashCode()) {
case 1 -> System.out.println("Hash code 1");
case 2 -> System.out.println("Hash code 2");
case 3 -> System.out.println("Hash code 3");
default -> throw new AssertionError("Some other hash code");
}
}
}
""")
.setArgs("-XepOpt:IfChainToSwitch:EnableMain")
.doTest();
}

@Test
public void ifChain_methodInvocationSafe_noError() {
// Same code as ifChain_methodInvocation_error, but should not refactor in safe mode because the
// subject is not the same variable.
helper
.addSourceLines(
"Test.java",
"""
class Test {
public void foo(Suit s) {
Object suit = s;
if (suit.hashCode() == 1) {
System.out.println("Hash code 1");
} else if (suit.hashCode() == 2) {
System.out.println("Hash code 2");
} else if (suit.hashCode() == 3) {
System.out.println("Hash code 3");
} else throw new AssertionError("Some other hash code");
}
}
""")
.setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe=true")
.doTest();
}

@Test
public void ifChain_parameterizedTypeSafe_error() {
// Raw types are converted to the wildcard type.
Expand Down
Loading