From ea7db3564bb1936a18c5078afc12e46a3313e029 Mon Sep 17 00:00:00 2001 From: markbrady Date: Wed, 17 Jul 2024 08:28:14 -0700 Subject: [PATCH] StatementSwitchToExpressionSwitch: Enhance code comment handling for direct conversion, including retaining comments after the final statement in a case PiperOrigin-RevId: 653242592 --- .../StatementSwitchToExpressionSwitch.java | 89 +++++++-- ...StatementSwitchToExpressionSwitchTest.java | 187 ++++++++++++++++-- 2 files changed, 252 insertions(+), 24 deletions(-) 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 82011eb8dc73..87edc33e905c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -33,6 +33,7 @@ import static java.util.stream.Collectors.joining; import com.google.auto.value.AutoValue; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -45,6 +46,7 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matchers; import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ErrorProneComment; import com.google.errorprone.util.Reachability; import com.google.errorprone.util.SourceVersion; import com.sun.source.tree.AssignmentTree; @@ -540,19 +542,50 @@ private static SuggestedFix convertDirectlyToExpressionSwitch( replacementCodeBuilder.append( isDefaultCase ? "default" : printCaseExpressions(caseTree, state)); + Optional commentsAfterCaseOptional = + extractCommentsAfterCase(switchTree, state, caseIndex); if (analysisResult.groupedWithNextCase().get(caseIndex)) { firstCaseInGroup = false; replacementCodeBuilder.append(", "); // Capture comments from this case so they can be added to the group's transformed case if (!transformedBlockSource.trim().isEmpty()) { - groupedCaseCommentsAccumulator.append(removeFallThruLines(transformedBlockSource)); + String commentsToAppend = removeFallThruLines(transformedBlockSource); + if (groupedCaseCommentsAccumulator.length() > 0) { + groupedCaseCommentsAccumulator.append("\n"); + } + groupedCaseCommentsAccumulator.append(commentsToAppend); + } + + if (commentsAfterCaseOptional.isPresent()) { + if (groupedCaseCommentsAccumulator.length() > 0) { + groupedCaseCommentsAccumulator.append("\n"); + } + groupedCaseCommentsAccumulator.append(commentsAfterCaseOptional.get()); } + // Add additional cases to the list on the lhs of the arrow continue; } else { // This case is the last case in its group, so insert the collected comments from the lhs of // the colon here - transformedBlockSource = groupedCaseCommentsAccumulator + transformedBlockSource; + + // Extract comments (if any) preceding break that was removed as redundant + Optional commentsBeforeRemovedBreak = + filteredStatements.isEmpty() + ? Optional.empty() + : extractCommentsBeforeRemovedBreak(caseTree, state, filteredStatements); + + // Join together all comments and code, separating with newlines + transformedBlockSource = + Joiner.on("\n") + .skipNulls() + .join( + groupedCaseCommentsAccumulator.length() == 0 + ? null + : groupedCaseCommentsAccumulator.toString(), + transformedBlockSource.isEmpty() ? null : transformedBlockSource, + commentsBeforeRemovedBreak.orElse(null), + commentsAfterCaseOptional.orElse(null)); } replacementCodeBuilder.append(" -> "); @@ -570,17 +603,10 @@ private static SuggestedFix convertDirectlyToExpressionSwitch( } } else { // Transformed block has code - // Extract comments (if any) for break that was removed as redundant - Optional commentsBeforeRemovedBreak = - extractCommentsBeforeRemovedBreak(caseTree, state, filteredStatements); - if (commentsBeforeRemovedBreak.isPresent()) { - transformedBlockSource = transformedBlockSource + "\n" + commentsBeforeRemovedBreak.get(); - } - // To improve readability, don't use braces on the rhs if not needed if (shouldTransformCaseWithoutBraces(filteredStatements)) { - // Single statement with no comments - no braces needed - replacementCodeBuilder.append(transformedBlockSource); + // No braces needed + replacementCodeBuilder.append("\n").append(transformedBlockSource); } else { // Use braces on the rhs replacementCodeBuilder.append("{\n").append(transformedBlockSource).append("\n}"); @@ -599,7 +625,7 @@ private static SuggestedFix convertDirectlyToExpressionSwitch( /** * Transforms the supplied statement switch into a {@code return 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. + * expression on 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. */ @@ -903,6 +929,45 @@ private static int extractLhsComments( return lhsEnd; } + /** + * Extracts any comments appearing after the specified {@code caseIndex} but before the subsequent + * case or end of the {@code switchTree}. + */ + private static Optional extractCommentsAfterCase( + SwitchTree switchTree, VisitorState state, int caseIndex) { + + ImmutableList comments = + state.getTokensForNode(switchTree).stream() + .flatMap(errorProneToken -> errorProneToken.comments().stream()) + .collect(toImmutableList()); + + int switchStart = getStartPosition(switchTree); + + // Indexing relative to the start position of the switch statement + // Invariant: caseEndIndex >= 0 + int caseEndIndex = state.getEndPosition(switchTree.getCases().get(caseIndex)) - switchStart; + // Invariant: nextCaseStartIndex >= caseEndIndex + int nextCaseStartIndex = + caseIndex == switchTree.getCases().size() - 1 + ? state.getEndPosition(switchTree) - switchStart + : getStartPosition(switchTree.getCases().get(caseIndex + 1)) - switchStart; + + String filteredComments = + comments.stream() + // Comments after the end of the current case and before the start of the next case + .filter( + comment -> + comment.getPos() >= caseEndIndex && comment.getPos() < nextCaseStartIndex) + .map(ErrorProneComment::getText) + // Remove "fall thru" comments + .map(commentText -> removeFallThruLines(commentText)) + // Remove empty comments + .filter(commentText -> !commentText.isEmpty()) + .collect(joining("\n")); + + return filteredComments.isEmpty() ? Optional.empty() : Optional.of(filteredComments); + } + /** * Finds the position in source corresponding to the end of the code block of the supplied {@code * caseIndex} within all {@code cases}. 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 d1e2d6c144e3..08f4a892626c 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -110,6 +110,7 @@ public void switchByEnum_removesRedundantBreak_error() { " // Middle comment", " System.out.println(\"obverse\");", " // Break comment", + " // End comment", " }", " case REVERSE -> System.out.println(\"reverse\");", " }", @@ -268,7 +269,8 @@ public void switchByEnumCard_combinesCaseComments_error() { " public void foo(Side side) { ", " switch(side) {", " case HEART -> System.out.println(\"heart2\");", - " case DIAMOND, SPADE, CLUB -> { /* sparkly */", + " case DIAMOND, SPADE, CLUB -> {", + " /* sparkly */", " // Empty block comment 1", " // Empty block comment 2", " // Start of block comment 1", @@ -354,7 +356,7 @@ public void switchByEnumCard2_removesRedundantBreaks_error() { " case HEART -> ", " System.out.println(\"heart\");", " // Pre break comment", - " ", + " // Post break comment", " case DIAMOND -> {", " // Diamond break comment", " break;", @@ -444,7 +446,8 @@ public void switchByEnumCard_onlyExpressionsAndThrowAreBraceless_error() { " case SPADE -> {", " return;", " }", - " case CLUB -> throw new AssertionError();", + " case CLUB ->", + " throw new AssertionError();", " }", " }", " }", @@ -591,10 +594,11 @@ public void switchWithDefaultInMiddle_error() { " System.out.println(\"diamond\");", " return;", " }", - " default -> /* comment: */", + " default -> ", + " /* comment: */", " System.out.println(\"club\");", - " ", - " case SPADE -> System.out.println(\"spade\");", + " case SPADE -> ", + " System.out.println(\"spade\");", " }", " }", "}") @@ -675,8 +679,11 @@ public void switchWithLabelledBreak_error() { " System.out.println(\"will return\");", " return;", " }", - " case DIAMOND -> {break outer;}", - " case SPADE, CLUB -> System.out.println(\"everything else\");", + " case DIAMOND -> {", + " break outer;", + " }", + " case SPADE, CLUB -> ", + " System.out.println(\"everything else\");", " }", " }", " }", @@ -886,9 +893,12 @@ public void switchByEnumCardWithReturnNested1_error() { " ", " public void foo(Side side) { ", " switch(side) {", - " case HEART-> System.out.println(\"heart\");", - " case DIAMOND -> System.out.println(\"nested1\");", - " case SPADE, CLUB -> System.out.println(\"everything else\");", + " case HEART-> ", + " System.out.println(\"heart\");", + " case DIAMOND -> ", + " System.out.println(\"nested1\");", + " case SPADE, CLUB -> ", + " System.out.println(\"everything else\");", " }", " }", "}") @@ -1217,7 +1227,6 @@ public void switchByEnum_caseHasOnlyComments_error() { " // more comments.", " // Diamond comment", " System.out.println(\"Heart or diamond\");", - " ", " case SPADES, CLUBS -> {", " bar();", " System.out.println(\"Black suit\");", @@ -1230,6 +1239,86 @@ public void switchByEnum_caseHasOnlyComments_error() { .doTest(); } + @Test + public void switchByEnum_accumulatedComments_error() { + // Comments should be aggregated across multiple cases + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " public Test() {}", + " private void foo(Suit suit) {", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(suit) {", + " case /* red */ HEARTS:", + " // A comment here", + " // more comments.", + " case /* red */ DIAMONDS:", + " // Diamonds comment", + " case /* black */SPADES:", + " // Spades comment", + " case /* black */CLUBS:", + " bar();", + " System.out.println(\"Any suit\");", + " }", + " }", + " private void bar() {}", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " public Test() {}", + " private void foo(Suit suit) {", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(suit) {", + " case /* red */ HEARTS:", + " // A comment here", + " // more comments.", + " case /* red */ DIAMONDS:", + " // Diamonds comment", + " case /* black */SPADES:", + " // Spades comment", + " case /* black */CLUBS:", + " bar();", + " System.out.println(\"Any suit\");", + " }", + " }", + " private void bar() {}", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " public Test() {}", + " private void foo(Suit suit) {", + " switch(suit) {", + " case HEARTS, DIAMONDS, SPADES, CLUBS -> {", + " /* red */", + " // A comment here", + " /* red */", + " // more comments.", + " // Diamonds comment", + " /* black */", + " // Spades comment", + " /* black */", + " bar();", + " System.out.println(\"Any suit\");", + " }", + " }", + " }", + " private void bar() {}", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + } + @Test public void switchByEnum_surroundingBracesCannotRemove_error() { // Can't remove braces around OBVERSE because break statements are not a member of @@ -1380,6 +1469,80 @@ public void switchByEnum_surroundingBracesEmpty_error() { .doTest(); } + @Test + public void switchByEnum_afterReturnComments_error() { + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Suit {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Suit suit) { ", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(suit) {", + " case HEART:", + " // before return comment", + " return 123;", + " // after return comment", + " /* more comments */", + " default:", + " }", + " return 0;", + " }", + "}") + .setArgs( + ImmutableList.of("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")) + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Suit {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Suit suit) { ", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(suit) {", + " case HEART:", + " // before return comment", + " return 123;", + " // after return comment", + " /* more comments */", + " default:", + " //default comment", + " }", + " return 0;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Suit {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {}", + " public int foo(Suit suit) {", + " switch(suit) {", + " case HEART -> {", + " // before return comment", + " return 123;", + " // after return comment", + " /* more comments */", + " }", + " default -> {", + " //default comment", + " }", + " }", + " return 0;", + " }", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + } + /********************************** * * Return switch test cases