Skip to content

Commit

Permalink
StatementSwitchToExpressionSwitch: in certain situations, the generat…
Browse files Browse the repository at this point in the history 
…ed autofix code for direct conversions may not compile. Add unit tests to guard against regression.

PiperOrigin-RevId: 651569415
  • Loading branch information
@markhbrady
markhbrady authored and Error Prone Team committed Jul 11, 2024
1 parent 03d15b5 commit 8c1a828
Show file tree
Hide file tree
Showing 2 changed files with 175 additions and 29 deletions.
54 changes: 25 additions & 29 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 @@ -63,7 +63,6 @@
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;
import com.sun.tools.javac.tree.JCTree.JCAssignOp;
import com.sun.tools.javac.tree.Pretty;
Expand Down Expand Up @@ -529,8 +528,7 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
// For readability, filter out trailing unlabelled break statement because these become a
// No-Op when used inside expression switches
ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);
String transformedBlockSource =
transformBlock(caseTree, state, cases, caseIndex, filteredStatements);
String transformedBlockSource = transformBlock(caseTree, state, filteredStatements);

if (firstCaseInGroup) {
groupedCaseCommentsAccumulator = new StringBuilder();
Expand Down Expand Up @@ -623,7 +621,7 @@ private static SuggestedFix convertToReturnSwitch(
boolean isDefaultCase = caseTree.getExpression() == null;

String transformedBlockSource =
transformReturnOrThrowBlock(caseTree, state, cases, caseIndex, getStatements(caseTree));
transformReturnOrThrowBlock(caseTree, state, getStatements(caseTree));

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

String transformedBlockSource =
transformAssignOrThrowBlock(caseTree, state, cases, caseIndex, filteredStatements);
transformAssignOrThrowBlock(caseTree, state, filteredStatements);

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

/** Transforms code for this case into the code under an expression switch. */
private static String transformBlock(
CaseTree caseTree,
VisitorState state,
List<? extends CaseTree> cases,
int caseIndex,
ImmutableList<StatementTree> filteredStatements) {
CaseTree caseTree, VisitorState state, ImmutableList<StatementTree> filteredStatements) {

StringBuilder transformedBlockBuilder = new StringBuilder();
int codeBlockStart = extractLhsComments(caseTree, state, transformedBlockBuilder);
int codeBlockEnd =
filteredStatements.isEmpty()
? getBlockEnd(state, caseTree, cases, caseIndex)
? getBlockEnd(state, caseTree)
: state.getEndPosition(Streams.findLast(filteredStatements.stream()).get());
transformedBlockBuilder.append(state.getSourceCode(), codeBlockStart, codeBlockEnd);

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

if (caseIndex == cases.size() - 1) {
List<? extends StatementTree> statements = caseTree.getStatements();
if (statements == null || statements.size() != 1) {
return state.getEndPosition(caseTree);
}

// Invariant: statements.size() == 1
StatementTree onlyStatement = getOnlyElement(statements);
if (!onlyStatement.getKind().equals(BLOCK)) {
return state.getEndPosition(caseTree);
}

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

/**
* Determines whether the supplied {@code case}'s logic should be expressed on the right of the
* arrow symbol without braces, incorporating both language and readabilitiy considerations.
* arrow symbol without braces, incorporating both language and readability considerations.
*/
private static boolean shouldTransformCaseWithoutBraces(
ImmutableList<StatementTree> statementTrees) {
Expand Down Expand Up @@ -995,17 +999,13 @@ private static boolean isSwitchExhaustive(
* transformed to {@code x+1;}.
*/
private static String transformReturnOrThrowBlock(
CaseTree caseTree,
VisitorState state,
List<? extends CaseTree> cases,
int caseIndex,
List<? extends StatementTree> statements) {
CaseTree caseTree, VisitorState state, List<? extends StatementTree> statements) {

StringBuilder transformedBlockBuilder = new StringBuilder();
int codeBlockStart;
int codeBlockEnd =
statements.isEmpty()
? getBlockEnd(state, caseTree, cases, caseIndex)
? getBlockEnd(state, caseTree)
: state.getEndPosition(Streams.findLast(statements.stream()).get());

if (statements.size() == 1 && statements.get(0).getKind().equals(RETURN)) {
Expand All @@ -1028,17 +1028,13 @@ private static String transformReturnOrThrowBlock(
* {@code >>=}).
*/
private static String transformAssignOrThrowBlock(
CaseTree caseTree,
VisitorState state,
List<? extends CaseTree> cases,
int caseIndex,
List<? extends StatementTree> statements) {
CaseTree caseTree, VisitorState state, List<? extends StatementTree> statements) {

StringBuilder transformedBlockBuilder = new StringBuilder();
int codeBlockStart;
int codeBlockEnd =
statements.isEmpty()
? getBlockEnd(state, caseTree, cases, caseIndex)
? getBlockEnd(state, caseTree)
: state.getEndPosition(Streams.findLast(statements.stream()).get());

if (!statements.isEmpty() && statements.get(0).getKind().equals(EXPRESSION_STATEMENT)) {
Expand Down
150 changes: 150 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 @@ -1230,6 +1230,156 @@ public void switchByEnum_caseHasOnlyComments_error() {
.doTest();
}

@Test
public void switchByEnum_surroundingBracesCannotRemove_error() {
// Can't remove braces around OBVERSE because break statements are not a member of
// KINDS_CONVERTIBLE_WITHOUT_BRACES
assumeTrue(RuntimeVersion.isAtLeast14());
helper
.addSourceLines(
"Test.java",
"class Test {",
" enum Side {OBVERSE, REVERSE};",
" public Test(int foo) {",
" }",
" ",
" public void foo(Side side) { ",
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
" switch(side) {",
" case OBVERSE: {",
" // The quick brown fox, jumps over the lazy dog, etc.",
" break;",
" }",
" ",
" default: { ",
" throw new RuntimeException(\"Invalid type.\");",
" }",
" }",
" }",
"}")
.setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
.doTest();

// Check correct generated code
refactoringHelper
.addInputLines(
"Test.java",
"class Test {",
" enum Side {OBVERSE, REVERSE};",
" public Test(int foo) {",
" }",
" ",
" public void foo(Side side) { ",
" switch(side) {",
" case OBVERSE: {",
" // The quick brown fox, jumps over the lazy dog, etc.",
" break;",
" }",
" ",
" default: { ",
" throw new RuntimeException(\"Invalid type.\");",
" }",
" }",
" }",
"}")
.addOutputLines(
"Test.java",
"class Test {",
" enum Side {OBVERSE, REVERSE};",
" public Test(int foo) {",
" }",
" ",
" public void foo(Side side) { ",
" switch(side) {",
" case OBVERSE -> {",
" // The quick brown fox, jumps over the lazy dog, etc.",
" break;",
" }",
" ",
" default -> ",
" throw new RuntimeException(\"Invalid type.\");",
" ",
" }",
" }",
"}")
.setArgs(
ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion"))
.doTest();
}

@Test
public void switchByEnum_surroundingBracesEmpty_error() {
// Test handling of cases with surrounding braces that are empty. The braces around OBVERSE
// can be removed because throw is a member of KINDS_CONVERTIBLE_WITHOUT_BRACES.
assumeTrue(RuntimeVersion.isAtLeast14());

helper
.addSourceLines(
"Test.java",
"class Test {",
" enum Side {OBVERSE, REVERSE};",
" public Test(int foo) {",
" }",
" ",
" public void foo(Side side) { ",
" // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]",
" switch(side) {",
" case OBVERSE: {",
" // The quick brown fox, jumps over the lazy dog, etc.",
" throw new RuntimeException(\"Invalid.\");",
" }",
" ",
" default: {",
" }",
" }",
" }",
"}")
.setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
.doTest();

// Check correct generated code
refactoringHelper
.addInputLines(
"Test.java",
"class Test {",
" enum Side {OBVERSE, REVERSE};",
" public Test(int foo) {",
" }",
" ",
" public void foo(Side side) { ",
" switch(side) {",
" case OBVERSE: {",
" // The quick brown fox, jumps over the lazy dog, etc.",
" throw new RuntimeException(\"Invalid.\");",
" }",
" ",
" default: {",
" }",
" }",
" }",
"}")
.addOutputLines(
"Test.java",
"class Test {",
" enum Side {OBVERSE, REVERSE};",
" public Test(int foo) {",
" }",
" ",
" public void foo(Side side) { ",
" switch(side) {",
" case OBVERSE -> ",
" // The quick brown fox, jumps over the lazy dog, etc.",
" throw new RuntimeException(\"Invalid.\");",
" default -> {}",
" ",
" }",
" }",
"}")
.setArgs(
ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion"))
.doTest();
}

/**********************************
*
* Return switch test cases
Expand Down

0 comments on commit 8c1a828

Please sign in to comment.