Skip to content

Commit

Permalink
Improve fixes for StatementSwitchToExpressionSwitch
Browse files Browse the repository at this point in the history 
If a case contains a single block statement, skip past the braces of the block statement.

Enables fixes like:

```
      case 0 -> System.out.println("0");
```

Instead of:

```
      case 0 -> {
        {
          System.out.println("0");
          break;
        }
      }
```

Fixes #4222

PiperOrigin-RevId: 593453909
  • Loading branch information
@cushon
cushon authored and Error Prone Team committed Jan 4, 2024
1 parent cb1dc06 commit a807e85
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 12 deletions.
39 changes: 28 additions & 11 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 @@ -19,10 +19,12 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.Iterables.getLast;
import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.sun.source.tree.Tree.Kind.BLOCK;
import static com.sun.source.tree.Tree.Kind.BREAK;
import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT;
import static com.sun.source.tree.Tree.Kind.RETURN;
Expand Down Expand Up @@ -214,7 +216,7 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree, VisitorSt
.collect(toImmutableSet()));
boolean isLastCaseInSwitch = caseIndex == cases.size() - 1;

List<? extends StatementTree> statements = caseTree.getStatements();
List<? extends StatementTree> statements = getStatements(caseTree);
CaseFallThru caseFallThru = CaseFallThru.MAYBE_FALLS_THRU;
if (statements == null) {
// This case must be of kind CaseTree.CaseKind.RULE, and thus this is already an expression
Expand Down Expand Up @@ -601,7 +603,7 @@ private static SuggestedFix convertToReturnSwitch(
boolean isDefaultCase = caseTree.getExpression() == null;

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

if (firstCaseInGroup) {
groupedCaseCommentsAccumulator = new StringBuilder();
Expand Down Expand Up @@ -794,7 +796,7 @@ private static Optional<String> extractCommentsBeforeRemovedBreak(
CaseTree caseTree, VisitorState state, ImmutableList<StatementTree> filteredStatements) {

// Was a trailing break removed and some expressions remain?
if (caseTree.getStatements().size() > filteredStatements.size()
if (getStatements(caseTree).size() > filteredStatements.size()
&& !filteredStatements.isEmpty()) {
// Extract any comments after what is now the last statement and before the removed
// break
Expand All @@ -803,8 +805,7 @@ private static Optional<String> extractCommentsBeforeRemovedBreak(
.getSourceCode()
.subSequence(
state.getEndPosition(Iterables.getLast(filteredStatements)),
getStartPosition(
caseTree.getStatements().get(caseTree.getStatements().size() - 1)))
getStartPosition(getStatements(caseTree).get(getStatements(caseTree).size() - 1)))
.toString()
.trim();
if (!commentsAfterNewLastStatement.isEmpty()) {
Expand All @@ -820,15 +821,31 @@ private static Optional<String> extractCommentsBeforeRemovedBreak(
*/
private static ImmutableList<StatementTree> filterOutRedundantBreak(CaseTree caseTree) {
boolean caseEndsWithUnlabelledBreak =
Streams.findLast(caseTree.getStatements().stream())
Streams.findLast(getStatements(caseTree).stream())
.filter(statement -> statement.getKind().equals(BREAK))
.filter(breakTree -> ((BreakTree) breakTree).getLabel() == null)
.isPresent();
return caseEndsWithUnlabelledBreak
? caseTree.getStatements().stream()
.limit(caseTree.getStatements().size() - 1)
? getStatements(caseTree).stream()
.limit(getStatements(caseTree).size() - 1)
.collect(toImmutableList())
: ImmutableList.copyOf(caseTree.getStatements());
: ImmutableList.copyOf(getStatements(caseTree));
}

/**
* Returns the statements of a {@link CaseTree}. If the only statement is a block statement,
* return the block's statements instead.
*/
private static List<? extends StatementTree> getStatements(CaseTree caseTree) {
List<? extends StatementTree> statements = caseTree.getStatements();
if (statements == null || statements.size() != 1) {
return statements;
}
StatementTree onlyStatement = getOnlyElement(statements);
if (!onlyStatement.getKind().equals(BLOCK)) {
return statements;
}
return ((BlockTree) onlyStatement).getStatements();
}

/** Transforms code for this case into the code under an expression switch. */
Expand Down Expand Up @@ -860,9 +877,9 @@ private static int extractLhsComments(

int lhsStart = getStartPosition(caseTree);
int lhsEnd =
caseTree.getStatements().isEmpty()
getStatements(caseTree).isEmpty()
? state.getEndPosition(caseTree)
: getStartPosition(caseTree.getStatements().get(0));
: getStartPosition(getStatements(caseTree).get(0));

// Accumulate comments into transformed block
state.getOffsetTokens(lhsStart, lhsEnd).stream()
Expand Down
43 changes: 42 additions & 1 deletion ...rc/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,7 @@ public void switchByEnumCardWithReturnNested1_error() {
" public void foo(Side side) { ",
" switch(side) {",
" case HEART-> System.out.println(\"heart\");",
" case DIAMOND -> {{ System.out.println(\"nested1\"); break; }}",
" case DIAMOND -> System.out.println(\"nested1\");",
" case SPADE, CLUB -> System.out.println(\"everything else\");",
" }",
" }",
Expand Down Expand Up @@ -2776,4 +2776,45 @@ public void switchByEnum_nonExhaustiveAssignmentSwitch_noError() {
"-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion"))
.doTest();
}

@Test
public void i4222() {
refactoringHelper
.addInputLines(
"Test.java",
"public class Test {",
" public static void main(String[] args) {",
" switch (args.length) {",
" case 0:",
" {",
" System.out.println(0);",
" break;",
" }",
" case 1:",
" System.out.println(1);",
" break;",
" case 2:",
" System.out.println(2);",
" System.out.println(2);",
" break;",
" }",
" }",
"}")
.addOutputLines(
"Test.java",
"public class Test {",
" public static void main(String[] args) {",
" switch (args.length) {",
" case 0 -> System.out.println(0);",
" case 1 -> System.out.println(1);",
" case 2 -> {",
" System.out.println(2);",
" System.out.println(2);",
" }",
" }",
" }",
"}")
.setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=true")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}
}

0 comments on commit a807e85

Please sign in to comment.