diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java index 76919adee26..f8136a249bb 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -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; @@ -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; @@ -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 = @@ -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 assignmentTargetOptional, + Optional 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. diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java index 0d945107dcd..7a8eef5ac38 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -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