Skip to content

Commit

Permalink
Bugfix assignment switch analysis in StatementSwitchToExpressionSwitc…
Browse files Browse the repository at this point in the history 
…h: if an assignment target is not a valid symbol, then don't attempt conversion.

PiperOrigin-RevId: 638716933
  • Loading branch information
@java-team-github-bot
java-team-github-bot authored and Error Prone Team committed May 30, 2024
1 parent 2dde254 commit 32997f7
Show file tree
Hide file tree
Showing 2 changed files with 188 additions and 4 deletions.
30 changes: 26 additions & 4 deletions core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.tree.JCTree.JCAssign;
Expand All @@ -74,6 +75,7 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -431,10 +433,8 @@ private static AssignmentSwitchAnalysisState analyzeCaseForAssignmentSwitch(
// First assignment seen?
(assignmentTargetOptional.isEmpty() && caseAssignmentTargetOptional.isPresent())
// Not first assignment, but assigning to same symbol as the first assignment?
|| (assignmentTargetOptional.isPresent()
&& caseAssignmentTargetOptional.isPresent()
&& getSymbol(assignmentTargetOptional.get())
.equals(getSymbol(caseAssignmentTargetOptional.get())));
|| isCompatibleWithFirstAssignment(
assignmentTargetOptional, caseAssignmentTargetOptional);

if (compatibleOperator && compatibleReference) {
caseQualifications =
Expand All @@ -459,6 +459,28 @@ && getSymbol(assignmentTargetOptional.get())
: assignmentTreeOptional);
}

/**
* In a switch with multiple assignments, determine whether a subsequent assignment target is
* compatible with the first assignment target.
*/
private static boolean isCompatibleWithFirstAssignment(
Optional<ExpressionTree> assignmentTargetOptional,
Optional<ExpressionTree> caseAssignmentTargetOptional) {

if (assignmentTargetOptional.isEmpty() || caseAssignmentTargetOptional.isEmpty()) {
return false;
}

Symbol assignmentTargetSymbol = getSymbol(assignmentTargetOptional.get());
// For non-symbol assignment targets, multiple assignments are not currently supported
if (assignmentTargetSymbol == null) {
return false;
}

Symbol caseAssignmentTargetSymbol = getSymbol(caseAssignmentTargetOptional.get());
return Objects.equals(assignmentTargetSymbol, caseAssignmentTargetSymbol);
}

/**
* Determines whether the supplied case's {@code statements} are capable of being mapped to an
* equivalent expression switch case (without repeating code), returning {@code true} if so.
Expand Down
162 changes: 162 additions & 0 deletions ...rc/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2203,6 +2203,168 @@ public void switchByEnum_assignmentSwitchTwoAssignments_noError() {
.doTest();
}

@Test
public void switchByEnum_assignmentSwitchToSingleArray_error() {
assumeTrue(RuntimeVersion.isAtLeast14());
helper
.addSourceLines(
"Test.java",
"class Test {",
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
" int[] x;",
" public Test(int foo) {",
" x = null;",
" }",
" ",
" public int[] foo(Side side) { ",
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
" switch(side) {",
" case HEART:",
" throw new RuntimeException();",
" case DIAMOND:",
" x[6] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
" break;",
" case SPADE:",
" throw new RuntimeException();",
" default:",
" throw new NullPointerException();",
" }",
" return x;",
" }",
"}")
.setArgs(
ImmutableList.of(
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
.doTest();

// Check correct generated code
refactoringHelper
.addInputLines(
"Test.java",
"class Test {",
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
" int[] x;",
" public Test(int foo) {",
" x = null;",
" }",
" ",
" public int[] foo(Side side) { ",
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
" switch(side) {",
" case HEART:",
" throw new RuntimeException();",
" case DIAMOND:",
" x[6] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
" break;",
" case SPADE:",
" throw new RuntimeException();",
" default:",
" throw new NullPointerException();",
" }",
" return x;",
" }",
"}")
.addOutputLines(
"Test.java",
"class Test {",
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
" int[] x;",
" public Test(int foo) {",
" x = null;",
" }",
" ",
" public int[] foo(Side side) { ",
" x[6] <<= switch(side) {",
" case HEART -> throw new RuntimeException();",
" case DIAMOND -> (((x[6]+1) * (x[6]*x[5]) << 1));",
" case SPADE -> throw new RuntimeException();",
" default -> throw new NullPointerException();",
" };",
" return x;",
" }",
"}")
.setArgs(
ImmutableList.of(
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
.doTest();
}

@Test
public void switchByEnum_assignmentSwitchToMultipleArray_noError() {
// Multiple array dereferences or other non-variable left-hand-side expressions may (in
// principle) be convertible to assignment switches, but this feature is not supported at this
// time
assumeTrue(RuntimeVersion.isAtLeast14());
helper
.addSourceLines(
"Test.java",
"class Test {",
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
" int[] x;",
" public Test(int foo) {",
" x = null;",
" }",
" ",
" public int[] foo(Side side) { ",
" switch(side) {",
" case HEART:",
" // Inline comment",
" x[6] <<= 2;",
" break;",
" case DIAMOND:",
" x[6] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
" break;",
" case SPADE:",
" throw new RuntimeException();",
" default:",
" throw new NullPointerException();",
" }",
" return x;",
" }",
"}")
.setArgs(
ImmutableList.of(
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
.doTest();
}

@Test
public void switchByEnum_assignmentSwitchToMultipleDistinct_noError() {
// x[5] and x[6] are distinct assignment targets
assumeTrue(RuntimeVersion.isAtLeast14());
helper
.addSourceLines(
"Test.java",
"class Test {",
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
" int[] x;",
" public Test(int foo) {",
" x = null;",
" }",
" ",
" public int[] foo(Side side) { ",
" switch(side) {",
" case HEART:",
" // Inline comment",
" x[6] <<= 2;",
" break;",
" case DIAMOND:",
" x[5] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
" break;",
" case SPADE:",
" throw new RuntimeException();",
" default:",
" throw new NullPointerException();",
" }",
" return x;",
" }",
"}")
.setArgs(
ImmutableList.of(
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
.doTest();
}

@Test
public void switchByEnum_assignmentSwitchMixedKinds_noError() {
// Different assignment types ("=" versus "+="). The check does not attempt to alter the
Expand Down

0 comments on commit 32997f7

Please sign in to comment.