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 11ec6a498be0..cf3a3ca05c73 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -509,7 +509,7 @@ private static boolean areStatementsConvertibleToExpressionSwitch( /** * Transforms the supplied statement switch into an expression switch directly. In this * conversion, each nontrivial statement block is mapped one-to-one to a new {@code Expression} or - * {@code StatementBlock} on the right-hand side. Comments are presevered where possible. + * {@code StatementBlock} on the right-hand side. Comments are preserved where possible. */ private static SuggestedFix convertDirectlyToExpressionSwitch( SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) { @@ -573,9 +573,7 @@ private static SuggestedFix convertDirectlyToExpressionSwitch( } else { // Extract comments (if any) preceding break that was removed as redundant Optional commentsBeforeRemovedBreak = - filteredStatements.isEmpty() - ? Optional.empty() - : extractCommentsBeforeRemovedBreak(caseTree, state, filteredStatements); + extractCommentsBeforeRemovedBreak(caseTree, state, filteredStatements); // Join together all comments and code, separating with newlines transformedBlockSource = @@ -638,6 +636,10 @@ private static SuggestedFix convertToReturnSwitch( List statementsToDelete = new ArrayList<>(); List cases = switchTree.getCases(); + ImmutableList allSwitchComments = + state.getTokensForNode(switchTree).stream() + .flatMap(errorProneToken -> errorProneToken.comments().stream()) + .collect(toImmutableList()); StringBuilder replacementCodeBuilder = new StringBuilder(); replacementCodeBuilder .append("return switch ") @@ -663,23 +665,46 @@ private static SuggestedFix convertToReturnSwitch( replacementCodeBuilder.append( isDefaultCase ? "default" : printCaseExpressions(caseTree, state)); + Optional commentsAfterCaseOptional = + extractCommentsAfterCase(switchTree, allSwitchComments, 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; + // Join together all comments and code, separating with newlines + transformedBlockSource = + Joiner.on("\n") + .skipNulls() + .join( + // This case is the last case in its group, so insert any comments from prior + // grouped cases first + groupedCaseCommentsAccumulator.length() == 0 + ? null + : groupedCaseCommentsAccumulator.toString(), + transformedBlockSource.isEmpty() ? null : transformedBlockSource, + commentsAfterCaseOptional.orElse(null)); } replacementCodeBuilder.append(" -> "); - // Single statement with no comments - no braces needed - replacementCodeBuilder.append(transformedBlockSource); + // No braces needed + replacementCodeBuilder.append("\n").append(transformedBlockSource); firstCaseInGroup = true; } // case loop @@ -761,6 +786,11 @@ private static SuggestedFix convertToAssignmentSwitch( SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) { List cases = switchTree.getCases(); + ImmutableList allSwitchComments = + state.getTokensForNode(switchTree).stream() + .flatMap(errorProneToken -> errorProneToken.comments().stream()) + .collect(toImmutableList()); + StringBuilder replacementCodeBuilder = new StringBuilder( state.getSourceForNode( @@ -800,32 +830,52 @@ private static SuggestedFix convertToAssignmentSwitch( replacementCodeBuilder.append( isDefaultCase ? "default" : printCaseExpressions(caseTree, state)); + Optional commentsAfterCaseOptional = + extractCommentsAfterCase(switchTree, allSwitchComments, 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 = + extractCommentsBeforeRemovedBreak(caseTree, state, filteredStatements); + + // Join together all comments and code, separating with newlines + transformedBlockSource = + Joiner.on("\n") + .skipNulls() + .join( + // This case is the last case in its group, so insert any comments from prior + // grouped cases first + groupedCaseCommentsAccumulator.length() == 0 + ? null + : groupedCaseCommentsAccumulator.toString(), + transformedBlockSource.isEmpty() ? null : transformedBlockSource, + commentsBeforeRemovedBreak.orElse(null), + commentsAfterCaseOptional.orElse(null)); } replacementCodeBuilder.append(" -> "); - // 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(); - } - - // Single statement with no comments - no braces needed - replacementCodeBuilder.append(transformedBlockSource); + // No braces needed + replacementCodeBuilder.append("\n").append(transformedBlockSource); firstCaseInGroup = true; } // case loop @@ -844,8 +894,8 @@ private static Optional extractCommentsBeforeRemovedBreak( CaseTree caseTree, VisitorState state, ImmutableList filteredStatements) { // Was a trailing break removed and some expressions remain? - if (getStatements(caseTree).size() > filteredStatements.size() - && !filteredStatements.isEmpty()) { + if (!filteredStatements.isEmpty() + && getStatements(caseTree).size() > filteredStatements.size()) { // Extract any comments after what is now the last statement and before the removed // break String commentsAfterNewLastStatement = 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 e1c4bf949f64..eab2d16735b4 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -215,9 +215,9 @@ public void switchByEnumCard_combinesCaseComments_error() { " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", " switch(side) {", " case HEART:", - " System.out.println(\"heart\");", + " System.out.println(\"heart2\");", " break;", - " case DIAMOND:", + " case /* sparkly */ DIAMOND /* Sparkly */:", " // Empty block comment 1", " // Fall through", " case SPADE:", @@ -247,7 +247,7 @@ public void switchByEnumCard_combinesCaseComments_error() { " case HEART:", " System.out.println(\"heart2\");", " break;", - " case /* sparkly */ DIAMOND:", + " case /* sparkly */ DIAMOND /* Sparkly */:", " // Empty block comment 1", " // Fall through", " case SPADE:", @@ -271,6 +271,7 @@ public void switchByEnumCard_combinesCaseComments_error() { " case HEART -> System.out.println(\"heart2\");", " case DIAMOND, SPADE, CLUB -> {", " /* sparkly */", + " /* Sparkly */", " // Empty block comment 1", " // Empty block comment 2", " // Start of block comment 1", @@ -1679,12 +1680,14 @@ public void switchByEnum_returnSwitchWithShouldNeverHappen_error() { " case SPADE -> throw new RuntimeException();", " case CLUB -> throw new NullPointerException();", " };", + " // This should never happen", + " ", " }", "}") .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -1812,7 +1815,7 @@ public void switchByEnum_exhaustiveWithDefault_error() { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -1915,6 +1918,7 @@ public void switchByEnum_returnSwitchWithShouldNeverHappen_errorAndRemoveShouldN " }", " // Custom comment - should never happen", " int z = invoke(/* block comment 0 */);", + " // Custom comment 2", " {z++;}", " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", " }", @@ -1955,6 +1959,7 @@ public void switchByEnum_returnSwitchWithShouldNeverHappen_errorAndRemoveShouldN " }", " // Custom comment - should never happen", " int z = invoke(/* block comment 0 */);", + " // Custom comment 2", " {z++;}", " throw new RuntimeException(\"Switch was not exhaustive at runtime \" + z);", " }", @@ -1986,6 +1991,9 @@ public void switchByEnum_returnSwitchWithShouldNeverHappen_errorAndRemoveShouldN " case CLUB -> throw new NullPointerException();", " };", " // Custom comment - should never happen", + "", + " // Custom comment 2", + "", " }", " System.out.println(\"don't delete 2\");", " return 0;", @@ -1994,7 +2002,7 @@ public void switchByEnum_returnSwitchWithShouldNeverHappen_errorAndRemoveShouldN .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -2107,7 +2115,136 @@ public void switchByEnum_returnSwitchNoFollowingStatementsInBlock_errorAndNoRemo .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void switchByEnum_groupedComments_errorAndNoRemoval() { + // The switch is exhaustive but doesn't have any statements immediately following it in the + // lowest ancestor statement block + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " System.out.println(\"don't delete 0\");", + " if (invoke() > 0) {", + " System.out.println(\"don't delete 1\");", + " // Preceding comment", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART /* lhs comment */: // rhs comment", + " // Another comment", + " case /* sparkly */ DIAMOND /* Sparkly */:", + " // Diamond", + " case SPADE:", + " // Before invoke", + " return invoke();", + " // After invoke", + " case CLUB:", + " throw new NullPointerException();", + " // After last case", + " }", + " }", + " // Custom comment - should never happen because invoke returns 123 or throws", + " int z = invoke(/* block comment 0 */);", + " {z++;}", + " throw new RuntimeException(\"Invoke <= 0 at runtime \");", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " System.out.println(\"don't delete 0\");", + " if (invoke() > 0) {", + " System.out.println(\"don't delete 1\");", + " // Preceding comment", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case HEART /* lhs comment */: // rhs comment", + " // Another comment", + " case /* sparkly */ DIAMOND /* Sparkly */:", + " // Diamond", + " case SPADE:", + " // Before invoke", + " return invoke();", + " // After invoke", + " /* More after invoke */", + " case CLUB:", + " throw new NullPointerException();", + " // After last case", + " }", + " }", + " // Custom comment - should never happen because invoke returns 123 or throws", + " int z = invoke(/* block comment 0 */);", + " {z++;}", + " throw new RuntimeException(\"Invoke <= 0 at runtime \");", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int invoke() {", + " return 123;", + " }", + " public int foo(Side side) { ", + " System.out.println(\"don't delete 0\");", + " if (invoke() > 0) {", + " System.out.println(\"don't delete 1\");", + " // Preceding comment", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " return switch(side) {", + " case HEART, DIAMOND, SPADE -> ", + " /* lhs comment */", + " // rhs comment", + " // Another comment", + " /* sparkly */", + " /* Sparkly */", + " // Diamond", + " // Before invoke", + " invoke();", + " // After invoke", + " /* More after invoke */", + " case CLUB -> throw new NullPointerException();", + " // After last case", + " };", + " }", + " // Custom comment - should never happen because invoke returns 123 or throws", + " int z = invoke(/* block comment 0 */);", + " {z++;}", + " throw new RuntimeException(\"Invoke <= 0 at runtime \");", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -2170,6 +2307,7 @@ public void switchByEnum_returnSwitchNoFollowingStatementsInBlock_errorAndNoRemo " case CLUB -> throw new NullPointerException();", " };", " // Custom comment - should never happen", + "", " };", " System.out.println(\"don't delete 2\");", " return lambda.get();", @@ -2178,7 +2316,7 @@ public void switchByEnum_returnSwitchNoFollowingStatementsInBlock_errorAndNoRemo .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion")) - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -2511,7 +2649,8 @@ public void switchByEnum_assignmentSwitchMixedReferences_error() { " ", " public int foo(Side side) { ", " this.x <<= switch(side) {", - " case HEART -> /* LHS comment */", + " case HEART ->", + " /* LHS comment */", " // Inline comment", " 2;", " case DIAMOND -> (((x+1) * (x*x)) << 1);", @@ -2524,7 +2663,7 @@ public void switchByEnum_assignmentSwitchMixedReferences_error() { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -3080,7 +3219,7 @@ public void switchByEnum_exhaustiveAssignmentSwitch_error() { .setArgs( ImmutableList.of( "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -3162,6 +3301,112 @@ public void switchByEnum_exhaustiveCompoundAssignmentSwitch_error() { .doTest(); } + @Test + public void switchByEnum_groupedComments_error() { + // Verify compound assignments (here, *=) with grouped comments + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case /* red suit */ HEART:", + " // Heart comment", + " case /* red suit */ DIAMOND: // sparkles", + " // Diamond comment", + " // Fall through", + " case /* black suit */ SPADE:", + " x *= 2;", + " // Before break comment", + " break;", + " // After break comment", + " case /* black suit */ CLUB:", + " // Club comment", + " throw new NullPointerException();", + " // Club after throw comment", + " }", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(side) {", + " case /* red suit */ HEART:", + " // Heart comment", + " case /* red suit */ DIAMOND: // sparkles", + " // Diamond comment", + " // Fall through", + " case /* black suit */ SPADE:", + " x *= 2;", + " // Before break comment", + " break;", + " // After break comment", + " case /* black suit */ CLUB:", + " // Club comment", + " throw new NullPointerException();", + " // Club after throw comment", + " }", + " return x;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Side {HEART, SPADE, DIAMOND, CLUB};", + " public Test(int foo) {", + " }", + " ", + " public int foo(Side side) { ", + " int x = 0;", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " x *= ", + " switch(side) {", + " case HEART, DIAMOND, SPADE ->", + " /* red suit */", + " // Heart comment", + " /* red suit */", + " // sparkles", + " // Diamond comment", + " /* black suit */", + " 2;", + " // Before break comment", + " // After break comment", + " case CLUB ->", + " /* black suit */", + " // Club comment", + " throw new NullPointerException();", + " // Club after throw comment", + " };", + " return x;", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion")) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + @Test public void switchByEnum_compoundAssignmentExampleInDocumentation_error() { // This code appears as an example in the documentation (added surrounding class) @@ -3177,7 +3422,7 @@ public void switchByEnum_compoundAssignmentExampleInDocumentation_error() { " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", " switch(suit) {", " case HEARTS:", - " // Fall thru", + " // Fall through", " case DIAMONDS:", " score += -1;", " break;", @@ -3203,7 +3448,7 @@ public void switchByEnum_compoundAssignmentExampleInDocumentation_error() { " private void updateScore(Suit suit) {", " switch(suit) {", " case HEARTS:", - " // Fall thru", + " // Fall through", " case DIAMONDS:", " score += -1;", " break;", @@ -3230,7 +3475,7 @@ public void switchByEnum_compoundAssignmentExampleInDocumentation_error() { " }", "}") .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion") - .doTest(); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test