Skip to content

Commit 32997f7

Browse files
java-team-github-botError Prone Team
java-team-github-bot
authored and
Error Prone Team
committed
Bugfix assignment switch analysis in StatementSwitchToExpressionSwitch: if an assignment target is not a valid symbol, then don't attempt conversion.
PiperOrigin-RevId: 638716933
1 parent 2dde254 commit 32997f7

File tree

2 files changed

+188
-4
lines changed

2 files changed

+188
-4
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
import com.sun.source.tree.Tree;
6262
import com.sun.source.tree.Tree.Kind;
6363
import com.sun.source.util.TreePath;
64+
import com.sun.tools.javac.code.Symbol;
6465
import com.sun.tools.javac.code.Type;
6566
import com.sun.tools.javac.tree.JCTree;
6667
import com.sun.tools.javac.tree.JCTree.JCAssign;
@@ -74,6 +75,7 @@
7475
import java.util.Collections;
7576
import java.util.HashSet;
7677
import java.util.List;
78+
import java.util.Objects;
7779
import java.util.Optional;
7880
import java.util.Set;
7981
import java.util.regex.Pattern;
@@ -431,10 +433,8 @@ private static AssignmentSwitchAnalysisState analyzeCaseForAssignmentSwitch(
431433
// First assignment seen?
432434
(assignmentTargetOptional.isEmpty() && caseAssignmentTargetOptional.isPresent())
433435
// Not first assignment, but assigning to same symbol as the first assignment?
434-
|| (assignmentTargetOptional.isPresent()
435-
&& caseAssignmentTargetOptional.isPresent()
436-
&& getSymbol(assignmentTargetOptional.get())
437-
.equals(getSymbol(caseAssignmentTargetOptional.get())));
436+
|| isCompatibleWithFirstAssignment(
437+
assignmentTargetOptional, caseAssignmentTargetOptional);
438438

439439
if (compatibleOperator && compatibleReference) {
440440
caseQualifications =
@@ -459,6 +459,28 @@ && getSymbol(assignmentTargetOptional.get())
459459
: assignmentTreeOptional);
460460
}
461461

462+
/**
463+
* In a switch with multiple assignments, determine whether a subsequent assignment target is
464+
* compatible with the first assignment target.
465+
*/
466+
private static boolean isCompatibleWithFirstAssignment(
467+
Optional<ExpressionTree> assignmentTargetOptional,
468+
Optional<ExpressionTree> caseAssignmentTargetOptional) {
469+
470+
if (assignmentTargetOptional.isEmpty() || caseAssignmentTargetOptional.isEmpty()) {
471+
return false;
472+
}
473+
474+
Symbol assignmentTargetSymbol = getSymbol(assignmentTargetOptional.get());
475+
// For non-symbol assignment targets, multiple assignments are not currently supported
476+
if (assignmentTargetSymbol == null) {
477+
return false;
478+
}
479+
480+
Symbol caseAssignmentTargetSymbol = getSymbol(caseAssignmentTargetOptional.get());
481+
return Objects.equals(assignmentTargetSymbol, caseAssignmentTargetSymbol);
482+
}
483+
462484
/**
463485
* Determines whether the supplied case's {@code statements} are capable of being mapped to an
464486
* equivalent expression switch case (without repeating code), returning {@code true} if so.

‎core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2203,6 +2203,168 @@ public void switchByEnum_assignmentSwitchTwoAssignments_noError() {
22032203
.doTest();
22042204
}
22052205

2206+
@Test
2207+
public void switchByEnum_assignmentSwitchToSingleArray_error() {
2208+
assumeTrue(RuntimeVersion.isAtLeast14());
2209+
helper
2210+
.addSourceLines(
2211+
"Test.java",
2212+
"class Test {",
2213+
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
2214+
" int[] x;",
2215+
" public Test(int foo) {",
2216+
" x = null;",
2217+
" }",
2218+
" ",
2219+
" public int[] foo(Side side) { ",
2220+
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
2221+
" switch(side) {",
2222+
" case HEART:",
2223+
" throw new RuntimeException();",
2224+
" case DIAMOND:",
2225+
" x[6] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
2226+
" break;",
2227+
" case SPADE:",
2228+
" throw new RuntimeException();",
2229+
" default:",
2230+
" throw new NullPointerException();",
2231+
" }",
2232+
" return x;",
2233+
" }",
2234+
"}")
2235+
.setArgs(
2236+
ImmutableList.of(
2237+
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
2238+
.doTest();
2239+
2240+
// Check correct generated code
2241+
refactoringHelper
2242+
.addInputLines(
2243+
"Test.java",
2244+
"class Test {",
2245+
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
2246+
" int[] x;",
2247+
" public Test(int foo) {",
2248+
" x = null;",
2249+
" }",
2250+
" ",
2251+
" public int[] foo(Side side) { ",
2252+
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
2253+
" switch(side) {",
2254+
" case HEART:",
2255+
" throw new RuntimeException();",
2256+
" case DIAMOND:",
2257+
" x[6] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
2258+
" break;",
2259+
" case SPADE:",
2260+
" throw new RuntimeException();",
2261+
" default:",
2262+
" throw new NullPointerException();",
2263+
" }",
2264+
" return x;",
2265+
" }",
2266+
"}")
2267+
.addOutputLines(
2268+
"Test.java",
2269+
"class Test {",
2270+
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
2271+
" int[] x;",
2272+
" public Test(int foo) {",
2273+
" x = null;",
2274+
" }",
2275+
" ",
2276+
" public int[] foo(Side side) { ",
2277+
" x[6] <<= switch(side) {",
2278+
" case HEART -> throw new RuntimeException();",
2279+
" case DIAMOND -> (((x[6]+1) * (x[6]*x[5]) << 1));",
2280+
" case SPADE -> throw new RuntimeException();",
2281+
" default -> throw new NullPointerException();",
2282+
" };",
2283+
" return x;",
2284+
" }",
2285+
"}")
2286+
.setArgs(
2287+
ImmutableList.of(
2288+
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
2289+
.doTest();
2290+
}
2291+
2292+
@Test
2293+
public void switchByEnum_assignmentSwitchToMultipleArray_noError() {
2294+
// Multiple array dereferences or other non-variable left-hand-side expressions may (in
2295+
// principle) be convertible to assignment switches, but this feature is not supported at this
2296+
// time
2297+
assumeTrue(RuntimeVersion.isAtLeast14());
2298+
helper
2299+
.addSourceLines(
2300+
"Test.java",
2301+
"class Test {",
2302+
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
2303+
" int[] x;",
2304+
" public Test(int foo) {",
2305+
" x = null;",
2306+
" }",
2307+
" ",
2308+
" public int[] foo(Side side) { ",
2309+
" switch(side) {",
2310+
" case HEART:",
2311+
" // Inline comment",
2312+
" x[6] <<= 2;",
2313+
" break;",
2314+
" case DIAMOND:",
2315+
" x[6] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
2316+
" break;",
2317+
" case SPADE:",
2318+
" throw new RuntimeException();",
2319+
" default:",
2320+
" throw new NullPointerException();",
2321+
" }",
2322+
" return x;",
2323+
" }",
2324+
"}")
2325+
.setArgs(
2326+
ImmutableList.of(
2327+
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
2328+
.doTest();
2329+
}
2330+
2331+
@Test
2332+
public void switchByEnum_assignmentSwitchToMultipleDistinct_noError() {
2333+
// x[5] and x[6] are distinct assignment targets
2334+
assumeTrue(RuntimeVersion.isAtLeast14());
2335+
helper
2336+
.addSourceLines(
2337+
"Test.java",
2338+
"class Test {",
2339+
" enum Side {HEART, SPADE, DIAMOND, CLUB};",
2340+
" int[] x;",
2341+
" public Test(int foo) {",
2342+
" x = null;",
2343+
" }",
2344+
" ",
2345+
" public int[] foo(Side side) { ",
2346+
" switch(side) {",
2347+
" case HEART:",
2348+
" // Inline comment",
2349+
" x[6] <<= 2;",
2350+
" break;",
2351+
" case DIAMOND:",
2352+
" x[5] <<= (((x[6]+1) * (x[6]*x[5]) << 1));",
2353+
" break;",
2354+
" case SPADE:",
2355+
" throw new RuntimeException();",
2356+
" default:",
2357+
" throw new NullPointerException();",
2358+
" }",
2359+
" return x;",
2360+
" }",
2361+
"}")
2362+
.setArgs(
2363+
ImmutableList.of(
2364+
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
2365+
.doTest();
2366+
}
2367+
22062368
@Test
22072369
public void switchByEnum_assignmentSwitchMixedKinds_noError() {
22082370
// Different assignment types ("=" versus "+="). The check does not attempt to alter the

0 commit comments

Comments
 (0)