Skip to content

Commit

Permalink
Handle switch block consisting of switch rules in Reachability.
Browse files Browse the repository at this point in the history 
And change `canCompleteNormally(CaseTree)` to `canFallThrough(CaseTree)`, because I think that's what the usage sites actually want.

PiperOrigin-RevId: 697660962
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Nov 19, 2024
1 parent 2aea991 commit e8a64ad
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 34 deletions.
72 changes: 50 additions & 22 deletions check_api/src/main/java/com/google/errorprone/util/Reachability.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.BreakTree;
import com.sun.source.tree.CaseTree;
import com.sun.source.tree.CaseTree.CaseKind;
import com.sun.source.tree.CatchTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ContinueTree;
Expand Down Expand Up @@ -69,20 +70,26 @@ public static boolean canCompleteNormally(StatementTree statement) {
}

/**
* Returns true if the given case tree can complete normally, as defined by JLS 14.21.
* Returns true if the given case tree can fall through to the next case tree.
*
* <p>An exception is made for {@code System.exit}, which cannot complete normally in practice.
*/
public static boolean canCompleteNormally(CaseTree caseTree) {
List<? extends StatementTree> statements = caseTree.getStatements();
if (statements.isEmpty()) {
return true;
}
// We only care whether the last statement completes; javac would have already
// reported an error if that statement wasn't reachable, and the answer is
// independent of any preceding statements.
// TODO(cushon): This isn't really making an exception for System.exit in the prior statements.
return canCompleteNormally(getLast(statements));
public static boolean canFallThrough(CaseTree caseTree) {
return switch (caseTree.getCaseKind()) {
case STATEMENT -> {
List<? extends StatementTree> statements = caseTree.getStatements();
if (statements.isEmpty()) {
yield true;
}
// We only care whether the last statement completes; javac would have already
// reported an error if that statement wasn't reachable, and the answer is
// independent of any preceding statements.
// TODO(cushon): This isn't really making an exception for System.exit in the prior
// statements.
yield canCompleteNormally(getLast(statements));
}
case RULE -> false;
};
}

private static class CanCompleteNormallyVisitor extends SimpleTreeVisitor<Boolean, Void> {
Expand Down Expand Up @@ -203,14 +210,14 @@ public Boolean visitAssert(AssertTree tree, Void unused) {
}

/*
* A switch statement can complete normally iff at least one of the
* A non-arrow switch statement can complete normally iff at least one of the
* following is true:
*
* 1) The switch block is empty or contains only switch labels.
* 2) The last statement in the switch block can complete normally.
* 3) There is at least one switch label after the last switch block
* 1) The switch block does not contain a default label.
* 2) The switch block is empty or contains only switch labels.
* 3) The last statement in the switch block can complete normally.
* 4) There is at least one switch label after the last switch block
* statement group.
* 4) The switch block does not contain a default label.
* 5) There is a reachable break statement that exits the switch statement.
*
* A switch block is reachable iff its switch statement is reachable.
Expand All @@ -226,23 +233,44 @@ public Boolean visitAssert(AssertTree tree, Void unused) {
@Override
public Boolean visitSwitch(SwitchTree tree, Void unused) {
// (1)
if (tree.getCases().stream().allMatch(c -> c.getStatements().isEmpty())) {
if (tree.getCases().stream().noneMatch(c -> isSwitchDefault(c))) {
return true;
}
// A switch statement whose switch block consists of switch rules can complete normally iff at
// least one of the following is true:
// (1 above) The switch statement is not enhanced (§14.11.2) and its switch block does not
// contain a default label.
// (2) One of the switch rules introduces a switch rule expression (which is necessarily a
// statement expression).
// (3) One of the switch rules introduces a switch rule block that can complete normally.
// (4) One of the switch rules introduces a switch rule block that contains a reachable
// break statement which exits the switch statement.
if (tree.getCases().stream().anyMatch(c -> c.getCaseKind().equals(CaseKind.RULE))) {
// (2) and (3)
if (tree.getCases().stream().anyMatch(c -> scan(c.getBody()))) {
return true;
}
// (4)
if (breaks.contains(tree)) {
return true;
}
return false;
}
// Past this point, we know each case is a statement.
// (2)
if (tree.getCases().stream().allMatch(c -> c.getStatements().isEmpty())) {
return true;
}
// (3)
boolean lastCompletes = true;
for (CaseTree c : tree.getCases()) {
lastCompletes = scan(c.getStatements());
}
if (lastCompletes) {
return true;
}
// (3)
if (getLast(tree.getCases()).getStatements().isEmpty()) {
return true;
}
// (4)
if (tree.getCases().stream().noneMatch(c -> isSwitchDefault(c))) {
if (getLast(tree.getCases()).getStatements().isEmpty()) {
return true;
}
// (5)
Expand Down
35 changes: 35 additions & 0 deletions check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,41 @@ public static List<Object[]> parameters() {
"System.err.println();",
"// BUG: Diagnostic contains:",
},
{
"switch (42) {", //
" case 1 -> {}",
"}",
"// BUG: Diagnostic contains:",
},
{
"switch (42) {", //
" case 1 -> throw new AssertionError();",
"}",
"// BUG: Diagnostic contains:",
},
{
"switch (42) {", //
" case 1 -> throw new AssertionError();",
" case 2 -> throw new AssertionError();",
" default -> {}",
"}",
"// BUG: Diagnostic contains:",
},
{
"switch (42) {", //
" case 1 -> throw new AssertionError();",
" case 2 -> throw new AssertionError();",
" default -> throw new AssertionError();",
"}",
},
{
"switch (42) {", //
" case 1 -> { break; }",
" case 2 -> throw new AssertionError();",
" default -> throw new AssertionError();",
"}",
"// BUG: Diagnostic contains:",
},
};
return Arrays.stream(parameters).map(x -> new Object[] {x}).collect(toList());
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/SwitchDefault.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) {
if (it.hasNext()) {
// If there are trailing cases after the default statement group, move the default to the end.
// Only emit a fix if the default doesn't fall through.
if (!Reachability.canCompleteNormally(getLast(defaultStatementGroup))) {
if (!Reachability.canFallThrough(getLast(defaultStatementGroup))) {
int start = getStartPosition(defaultStatementGroup.get(0));
int end = state.getEndPosition(getLast(defaultStatementGroup));
String replacement;
Expand All @@ -97,7 +97,7 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) {
// If the last statement group falls out of the switch, add a `break;` before moving
// the default to the end.
CaseTree last = getLast(tree.getCases());
if (isSwitchDefault(last) || Reachability.canCompleteNormally(last)) {
if (isSwitchDefault(last) || Reachability.canFallThrough(last)) {
replacement = "break;\n" + replacement;
}
fix.replace(start, end, "").postfixWith(getLast(tree.getCases()), replacement);
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryDefaultInEnumSwitch.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
import static com.google.errorprone.util.ASTHelpers.isSwitchDefault;
import static com.google.errorprone.util.Reachability.canCompleteNormally;
import static com.google.errorprone.util.Reachability.canFallThrough;

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
Expand Down Expand Up @@ -165,8 +166,8 @@ private Description fixDefault(
if (!SuggestedFixes.compilesWithFix(SuggestedFix.delete(defaultCase), state)) {
return NO_MATCH; // case (3)
}
if (!canCompleteNormally(caseBeforeDefault)) {
// case (2) -- If the case before the default can't complete normally,
if (!canFallThrough(caseBeforeDefault)) {
// case (2) -- If the case before the default can't fall through,
// it's OK to to delete the default.
return buildDescription(defaultCase)
.setMessage(DESCRIPTION_REMOVED_DEFAULT)
Expand Down
10 changes: 2 additions & 8 deletions core/src/main/java/com/google/errorprone/bugpatterns/WrongOneof.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.google.errorprone.bugpatterns;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.Iterables.getLast;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.predicates.TypePredicates.isDescendantOf;
Expand All @@ -26,7 +25,7 @@
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.stripParentheses;
import static com.google.errorprone.util.Reachability.canCompleteNormally;
import static com.google.errorprone.util.Reachability.canFallThrough;

import com.google.common.base.CaseFormat;
import com.google.common.collect.ImmutableSet;
Expand All @@ -41,11 +40,9 @@
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.SwitchTree;
import com.sun.source.util.TreePath;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.inject.Inject;

Expand Down Expand Up @@ -101,10 +98,7 @@ private void processSwitch(

scanForInvalidGetters(getters, allowableGetters, caseTree, constantReceiver, state);

List<? extends StatementTree> statements = caseTree.getStatements();
if (statements != null
&& !statements.isEmpty()
&& !canCompleteNormally(getLast(statements))) {
if (!canFallThrough(caseTree)) {
allowableGetters.clear();
}
}
Expand Down

0 comments on commit e8a64ad

Please sign in to comment.