Skip to content

Commit cecce2e

Browse files
graememorganError Prone Team
authored and
Error Prone Team
committed
Retain comments around unreachable statements if they contain "LINT.".
This is heuristicy, but helps preserve load-bearing comments even when we want to delete the unreachable code. PiperOrigin-RevId: 756398882
1 parent bf94fcf commit cecce2e

File tree

2 files changed

+73
-16
lines changed

2 files changed

+73
-16
lines changed

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

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import com.google.common.collect.ImmutableMap;
4242
import com.google.common.collect.ImmutableSet;
4343
import com.google.common.collect.Iterables;
44-
import com.google.common.collect.Range;
4544
import com.google.common.collect.Streams;
4645
import com.google.errorprone.BugPattern;
4746
import com.google.errorprone.ErrorProneFlags;
@@ -1021,7 +1020,7 @@ private static SuggestedFix convertToReturnSwitch(
10211020
AnalysisResult analysisResult,
10221021
boolean removeDefault) {
10231022

1024-
List<Range<Integer>> regionsToDelete = new ArrayList<>();
1023+
SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
10251024
List<? extends CaseTree> cases = switchTree.getCases();
10261025
ImmutableList<ErrorProneComment> allSwitchComments =
10271026
state.getTokensForNode(switchTree).stream()
@@ -1119,39 +1118,50 @@ private static SuggestedFix convertToReturnSwitch(
11191118
if (tree instanceof BlockTree blockTree) {
11201119
TreePath rootToCurrentPath = TreePath.getPath(state.getPath(), switchTree);
11211120
int indexInBlock = findBlockStatementIndex(rootToCurrentPath, blockTree);
1121+
var statements = blockTree.getStatements();
11221122
// A single mock of the immediate child statement block (or switch) is sufficient to
11231123
// analyze reachability here; deeper-nested statements are not relevant.
11241124
boolean nextStatementReachable =
11251125
Reachability.canCompleteNormally(
1126-
blockTree.getStatements().get(indexInBlock),
1127-
ImmutableMap.of(cannotCompleteNormallyTree, false));
1126+
statements.get(indexInBlock), ImmutableMap.of(cannotCompleteNormallyTree, false));
11281127
// If we continue to the ancestor statement block, it will be because the end of this
11291128
// statement block is not reachable
11301129
cannotCompleteNormallyTree = blockTree;
11311130
if (nextStatementReachable) {
11321131
break;
11331132
}
11341133

1135-
// If a next statement in this block exists, then it is not reachable; similarly, none of
1136-
// the following statements in this block are reachable either. So, iff further statements
1137-
// exist, delete them all (including their comments).
1138-
if (indexInBlock < blockTree.getStatements().size() - 1) {
1139-
regionsToDelete.add(
1140-
Range.closed(
1141-
state.getEndPosition(blockTree.getStatements().get(indexInBlock)),
1142-
state.getEndPosition(blockTree)));
1134+
// If a next statement in this block exists, then it is not reachable.
1135+
if (indexInBlock < statements.size() - 1) {
1136+
String deletedRegion =
1137+
state
1138+
.getSourceCode()
1139+
.subSequence(
1140+
state.getEndPosition(statements.get(indexInBlock)),
1141+
state.getEndPosition(blockTree))
1142+
.toString();
1143+
// If the region we would delete looks interesting, bail out and just delete the orphaned
1144+
// statements.
1145+
if (deletedRegion.contains("LINT.")) {
1146+
statements
1147+
.subList(indexInBlock + 1, statements.size())
1148+
.forEach(suggestedFixBuilder::delete);
1149+
} else {
1150+
// If the region doesn't seem to contain interesting comments, delete it along with
1151+
// comments: those comments are often just of the form "Unreachable code".
1152+
suggestedFixBuilder.replace(
1153+
state.getEndPosition(statements.get(indexInBlock)),
1154+
state.getEndPosition(blockTree),
1155+
"}");
1156+
}
11431157
}
11441158
}
11451159
}
11461160

1147-
SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
11481161
if (removeDefault) {
11491162
suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
11501163
}
11511164
suggestedFixBuilder.replace(switchTree, replacementCodeBuilder.toString());
1152-
// Delete dead code and its comments
1153-
regionsToDelete.forEach(
1154-
r -> suggestedFixBuilder.replace(r.lowerEndpoint(), r.upperEndpoint(), "}"));
11551165
return suggestedFixBuilder.build();
11561166
}
11571167

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2102,6 +2102,53 @@ public int foo(Suit suit) {
21022102
.doTest(TEXT_MATCH);
21032103
}
21042104

2105+
@Test
2106+
public void switchByEnum_returnSwitchWithAllEnumValues_retainTrailingLintComment() {
2107+
refactoringHelper
2108+
.addInputLines(
2109+
"Test.java",
2110+
"""
2111+
class Test {
2112+
public int foo(Suit suit) {
2113+
// LINT.
2114+
switch (suit) {
2115+
case HEART:
2116+
return 1;
2117+
case DIAMOND:
2118+
return 2;
2119+
case SPADE:
2120+
return 3;
2121+
case CLUB:
2122+
return 4;
2123+
}
2124+
// LINT.
2125+
throw new AssertionError("unreachable!");
2126+
}
2127+
}
2128+
""")
2129+
.addOutputLines(
2130+
"Test.java",
2131+
"""
2132+
class Test {
2133+
public int foo(Suit suit) {
2134+
// LINT.
2135+
return switch (suit) {
2136+
case HEART -> 1;
2137+
case DIAMOND -> 2;
2138+
case SPADE -> 3;
2139+
case CLUB -> 4;
2140+
};
2141+
// LINT.
2142+
2143+
}
2144+
}
2145+
""")
2146+
.setArgs(
2147+
"-XepOpt:StatementSwitchToExpressionSwitch:EnableReturnSwitchConversion",
2148+
"-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=false")
2149+
.doTest(TEXT_MATCH);
2150+
}
2151+
21052152
@Test
21062153
public void switchByEnum_returnSwitchNoFollowingStatementsInBlock_errorAndNoRemoval() {
21072154
// The switch is exhaustive but doesn't have any statements immediately following it in the

0 commit comments

Comments
 (0)