Skip to content

Commit 8c1a828

Browse files
markhbradyError Prone Team
markhbrady
authored and
Error Prone Team
committed
StatementSwitchToExpressionSwitch: in certain situations, the generated autofix code for direct conversions may not compile. Add unit tests to guard against regression.
PiperOrigin-RevId: 651569415
1 parent 03d15b5 commit 8c1a828

2 files changed

Lines changed: 175 additions & 29 deletions

File tree

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

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@
6363
import com.sun.source.util.TreePath;
6464
import com.sun.tools.javac.code.Symbol;
6565
import com.sun.tools.javac.code.Type;
66-
import com.sun.tools.javac.tree.JCTree;
6766
import com.sun.tools.javac.tree.JCTree.JCAssign;
6867
import com.sun.tools.javac.tree.JCTree.JCAssignOp;
6968
import com.sun.tools.javac.tree.Pretty;
@@ -529,8 +528,7 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
529528
// For readability, filter out trailing unlabelled break statement because these become a
530529
// No-Op when used inside expression switches
531530
ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);
532-
String transformedBlockSource =
533-
transformBlock(caseTree, state, cases, caseIndex, filteredStatements);
531+
String transformedBlockSource = transformBlock(caseTree, state, filteredStatements);
534532

535533
if (firstCaseInGroup) {
536534
groupedCaseCommentsAccumulator = new StringBuilder();
@@ -623,7 +621,7 @@ private static SuggestedFix convertToReturnSwitch(
623621
boolean isDefaultCase = caseTree.getExpression() == null;
624622

625623
String transformedBlockSource =
626-
transformReturnOrThrowBlock(caseTree, state, cases, caseIndex, getStatements(caseTree));
624+
transformReturnOrThrowBlock(caseTree, state, getStatements(caseTree));
627625

628626
if (firstCaseInGroup) {
629627
groupedCaseCommentsAccumulator = new StringBuilder();
@@ -725,7 +723,7 @@ private static int findBlockStatementIndex(TreePath treePath, BlockTree blockTre
725723
/**
726724
* Transforms the supplied statement switch into an assignment switch style of expression switch.
727725
* In this conversion, each nontrivial statement block is mapped one-to-one to a new expression on
728-
* the right-hand side of the arrow. Comments are presevered where possible. Precondition: the
726+
* the right-hand side of the arrow. Comments are preserved where possible. Precondition: the
729727
* {@code AnalysisResult} for the {@code SwitchTree} must have deduced that this conversion is
730728
* possible.
731729
*/
@@ -760,7 +758,7 @@ private static SuggestedFix convertToAssignmentSwitch(
760758
ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);
761759

762760
String transformedBlockSource =
763-
transformAssignOrThrowBlock(caseTree, state, cases, caseIndex, filteredStatements);
761+
transformAssignOrThrowBlock(caseTree, state, filteredStatements);
764762

765763
if (firstCaseInGroup) {
766764
groupedCaseCommentsAccumulator = new StringBuilder();
@@ -870,17 +868,13 @@ private static List<? extends StatementTree> getStatements(CaseTree caseTree) {
870868

871869
/** Transforms code for this case into the code under an expression switch. */
872870
private static String transformBlock(
873-
CaseTree caseTree,
874-
VisitorState state,
875-
List<? extends CaseTree> cases,
876-
int caseIndex,
877-
ImmutableList<StatementTree> filteredStatements) {
871+
CaseTree caseTree, VisitorState state, ImmutableList<StatementTree> filteredStatements) {
878872

879873
StringBuilder transformedBlockBuilder = new StringBuilder();
880874
int codeBlockStart = extractLhsComments(caseTree, state, transformedBlockBuilder);
881875
int codeBlockEnd =
882876
filteredStatements.isEmpty()
883-
? getBlockEnd(state, caseTree, cases, caseIndex)
877+
? getBlockEnd(state, caseTree)
884878
: state.getEndPosition(Streams.findLast(filteredStatements.stream()).get());
885879
transformedBlockBuilder.append(state.getSourceCode(), codeBlockStart, codeBlockEnd);
886880

@@ -913,19 +907,29 @@ private static int extractLhsComments(
913907
* Finds the position in source corresponding to the end of the code block of the supplied {@code
914908
* caseIndex} within all {@code cases}.
915909
*/
916-
private static int getBlockEnd(
917-
VisitorState state, CaseTree caseTree, List<? extends CaseTree> cases, int caseIndex) {
910+
private static int getBlockEnd(VisitorState state, CaseTree caseTree) {
918911

919-
if (caseIndex == cases.size() - 1) {
912+
List<? extends StatementTree> statements = caseTree.getStatements();
913+
if (statements == null || statements.size() != 1) {
914+
return state.getEndPosition(caseTree);
915+
}
916+
917+
// Invariant: statements.size() == 1
918+
StatementTree onlyStatement = getOnlyElement(statements);
919+
if (!onlyStatement.getKind().equals(BLOCK)) {
920920
return state.getEndPosition(caseTree);
921921
}
922922

923-
return ((JCTree) cases.get(caseIndex + 1)).getStartPosition();
923+
// The RHS of the case has a single enclosing block { ... }
924+
List<? extends StatementTree> blockStatements = ((BlockTree) onlyStatement).getStatements();
925+
return blockStatements.isEmpty()
926+
? state.getEndPosition(caseTree)
927+
: state.getEndPosition(blockStatements.get(blockStatements.size() - 1));
924928
}
925929

926930
/**
927931
* Determines whether the supplied {@code case}'s logic should be expressed on the right of the
928-
* arrow symbol without braces, incorporating both language and readabilitiy considerations.
932+
* arrow symbol without braces, incorporating both language and readability considerations.
929933
*/
930934
private static boolean shouldTransformCaseWithoutBraces(
931935
ImmutableList<StatementTree> statementTrees) {
@@ -995,17 +999,13 @@ private static boolean isSwitchExhaustive(
995999
* transformed to {@code x+1;}.
9961000
*/
9971001
private static String transformReturnOrThrowBlock(
998-
CaseTree caseTree,
999-
VisitorState state,
1000-
List<? extends CaseTree> cases,
1001-
int caseIndex,
1002-
List<? extends StatementTree> statements) {
1002+
CaseTree caseTree, VisitorState state, List<? extends StatementTree> statements) {
10031003

10041004
StringBuilder transformedBlockBuilder = new StringBuilder();
10051005
int codeBlockStart;
10061006
int codeBlockEnd =
10071007
statements.isEmpty()
1008-
? getBlockEnd(state, caseTree, cases, caseIndex)
1008+
? getBlockEnd(state, caseTree)
10091009
: state.getEndPosition(Streams.findLast(statements.stream()).get());
10101010

10111011
if (statements.size() == 1 && statements.get(0).getKind().equals(RETURN)) {
@@ -1028,17 +1028,13 @@ private static String transformReturnOrThrowBlock(
10281028
* {@code >>=}).
10291029
*/
10301030
private static String transformAssignOrThrowBlock(
1031-
CaseTree caseTree,
1032-
VisitorState state,
1033-
List<? extends CaseTree> cases,
1034-
int caseIndex,
1035-
List<? extends StatementTree> statements) {
1031+
CaseTree caseTree, VisitorState state, List<? extends StatementTree> statements) {
10361032

10371033
StringBuilder transformedBlockBuilder = new StringBuilder();
10381034
int codeBlockStart;
10391035
int codeBlockEnd =
10401036
statements.isEmpty()
1041-
? getBlockEnd(state, caseTree, cases, caseIndex)
1037+
? getBlockEnd(state, caseTree)
10421038
: state.getEndPosition(Streams.findLast(statements.stream()).get());
10431039

10441040
if (!statements.isEmpty() && statements.get(0).getKind().equals(EXPRESSION_STATEMENT)) {

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

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,6 +1230,156 @@ public void switchByEnum_caseHasOnlyComments_error() {
12301230
.doTest();
12311231
}
12321232

1233+
@Test
1234+
public void switchByEnum_surroundingBracesCannotRemove_error() {
1235+
// Can't remove braces around OBVERSE because break statements are not a member of
1236+
// KINDS_CONVERTIBLE_WITHOUT_BRACES
1237+
assumeTrue(RuntimeVersion.isAtLeast14());
1238+
helper
1239+
.addSourceLines(
1240+
"Test.java",
1241+
"class Test {",
1242+
" enum Side {OBVERSE, REVERSE};",
1243+
" public Test(int foo) {",
1244+
" }",
1245+
" ",
1246+
" public void foo(Side side) { ",
1247+
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
1248+
" switch(side) {",
1249+
" case OBVERSE: {",
1250+
" // The quick brown fox, jumps over the lazy dog, etc.",
1251+
" break;",
1252+
" }",
1253+
" ",
1254+
" default: { ",
1255+
" throw new RuntimeException(\"Invalid type.\");",
1256+
" }",
1257+
" }",
1258+
" }",
1259+
"}")
1260+
.setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
1261+
.doTest();
1262+
1263+
// Check correct generated code
1264+
refactoringHelper
1265+
.addInputLines(
1266+
"Test.java",
1267+
"class Test {",
1268+
" enum Side {OBVERSE, REVERSE};",
1269+
" public Test(int foo) {",
1270+
" }",
1271+
" ",
1272+
" public void foo(Side side) { ",
1273+
" switch(side) {",
1274+
" case OBVERSE: {",
1275+
" // The quick brown fox, jumps over the lazy dog, etc.",
1276+
" break;",
1277+
" }",
1278+
" ",
1279+
" default: { ",
1280+
" throw new RuntimeException(\"Invalid type.\");",
1281+
" }",
1282+
" }",
1283+
" }",
1284+
"}")
1285+
.addOutputLines(
1286+
"Test.java",
1287+
"class Test {",
1288+
" enum Side {OBVERSE, REVERSE};",
1289+
" public Test(int foo) {",
1290+
" }",
1291+
" ",
1292+
" public void foo(Side side) { ",
1293+
" switch(side) {",
1294+
" case OBVERSE -> {",
1295+
" // The quick brown fox, jumps over the lazy dog, etc.",
1296+
" break;",
1297+
" }",
1298+
" ",
1299+
" default -> ",
1300+
" throw new RuntimeException(\"Invalid type.\");",
1301+
" ",
1302+
" }",
1303+
" }",
1304+
"}")
1305+
.setArgs(
1306+
ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion"))
1307+
.doTest();
1308+
}
1309+
1310+
@Test
1311+
public void switchByEnum_surroundingBracesEmpty_error() {
1312+
// Test handling of cases with surrounding braces that are empty. The braces around OBVERSE
1313+
// can be removed because throw is a member of KINDS_CONVERTIBLE_WITHOUT_BRACES.
1314+
assumeTrue(RuntimeVersion.isAtLeast14());
1315+
1316+
helper
1317+
.addSourceLines(
1318+
"Test.java",
1319+
"class Test {",
1320+
" enum Side {OBVERSE, REVERSE};",
1321+
" public Test(int foo) {",
1322+
" }",
1323+
" ",
1324+
" public void foo(Side side) { ",
1325+
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
1326+
" switch(side) {",
1327+
" case OBVERSE: {",
1328+
" // The quick brown fox, jumps over the lazy dog, etc.",
1329+
" throw new RuntimeException(\"Invalid.\");",
1330+
" }",
1331+
" ",
1332+
" default: {",
1333+
" }",
1334+
" }",
1335+
" }",
1336+
"}")
1337+
.setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
1338+
.doTest();
1339+
1340+
// Check correct generated code
1341+
refactoringHelper
1342+
.addInputLines(
1343+
"Test.java",
1344+
"class Test {",
1345+
" enum Side {OBVERSE, REVERSE};",
1346+
" public Test(int foo) {",
1347+
" }",
1348+
" ",
1349+
" public void foo(Side side) { ",
1350+
" switch(side) {",
1351+
" case OBVERSE: {",
1352+
" // The quick brown fox, jumps over the lazy dog, etc.",
1353+
" throw new RuntimeException(\"Invalid.\");",
1354+
" }",
1355+
" ",
1356+
" default: {",
1357+
" }",
1358+
" }",
1359+
" }",
1360+
"}")
1361+
.addOutputLines(
1362+
"Test.java",
1363+
"class Test {",
1364+
" enum Side {OBVERSE, REVERSE};",
1365+
" public Test(int foo) {",
1366+
" }",
1367+
" ",
1368+
" public void foo(Side side) { ",
1369+
" switch(side) {",
1370+
" case OBVERSE -> ",
1371+
" // The quick brown fox, jumps over the lazy dog, etc.",
1372+
" throw new RuntimeException(\"Invalid.\");",
1373+
" default -> {}",
1374+
" ",
1375+
" }",
1376+
" }",
1377+
"}")
1378+
.setArgs(
1379+
ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion"))
1380+
.doTest();
1381+
}
1382+
12331383
/**********************************
12341384
*
12351385
* Return switch test cases

0 commit comments

Comments
 (0)