Skip to content

[IfChainToSwitch] Add new flag for safe mode, and emit null-handling logic when it is enabled #5455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: master
Choose a base branch
from
+993 −50
Open

[IfChainToSwitch] Add new flag for safe mode, and emit null-handling logic when it is enabled #5455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 95 additions & 14 deletions core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,13 @@ enum Validity {
}

private final boolean enableMain;
private final boolean enableSafe;
private final int maxChainLength;

@Inject
IfChainToSwitch(ErrorProneFlags flags) {
enableMain = flags.getBoolean("IfChainToSwitch:EnableMain").orElse(false);
enableSafe = flags.getBoolean("IfChainToSwitch:EnableSafe").orElse(false);
maxChainLength = flags.getInteger("IfChainToSwitch:MaxChainLength").orElse(50);
}

Expand Down Expand Up @@ -255,7 +257,9 @@ private static String prettyPrint(
StringBuilder sb = new StringBuilder();
sb.append("switch (").append(state.getSourceForNode(subject)).append(") {\n");
for (CaseIr caseIr : cases) {
if (caseIr.hasCaseNull()) {
if (caseIr.hasCaseNull() && caseIr.hasDefault()) {
sb.append("case null, default");
} else if (caseIr.hasCaseNull()) {
sb.append("case null");
} else if (caseIr.hasDefault()) {
sb.append("default");
Expand Down Expand Up @@ -399,13 +403,37 @@ private static String printRawTypesAsWildcards(
return sb.toString();
}

/**
* Determines whether a {@code switch} having the given {@code subject} and {@code cases} would
* implicitly throw in the event that the {@code subject} is {@code null} at runtime. Here,
* implicitly throwing means that an exception would be thrown, and further that the {@code throw}
* would not be caused by logic in any of the supplied {@code cases}. (If the subject cannot be
* assigned {@code null}, returns {@code false}.)
*/
private static boolean switchOnNullWouldImplicitlyThrow(
ExpressionTree subject, List<CaseIr> cases) {
Type type = getType(subject);
if (type.isPrimitive()) {
return false;
}

// If there is an explicit `case null` already, then there can't be an implicit throw caused by
// null
boolean hasCaseNull = cases.stream().anyMatch(CaseIr::hasCaseNull);
if (hasCaseNull) {
return false;
}

return true;
}

/**
* Analyzes the supplied case IRs for a switch statement for issues related default/unconditional
* cases. If deemed necessary, this method injects a `default` case into the supplied case IRs. If
* the supplied case IRs cannot be used to form a syntactically valid switch statement, returns
* `Optional.empty()`.
* cases. If deemed necessary, this method injects a `default` and/or `case null` into the
* supplied case IRs. If the supplied case IRs cannot be used to form a syntactically valid switch
* statement, returns `Optional.empty()`.
*/
private static Optional<List<CaseIr>> maybeFixDefaultAndUnconditional(
private Optional<List<CaseIr>> maybeFixDefaultNullAndUnconditional(
List<CaseIr> cases,
ExpressionTree subject,
StatementTree ifTree,
Expand All @@ -417,6 +445,10 @@ private static Optional<List<CaseIr>> maybeFixDefaultAndUnconditional(
Range<Integer> ifTreeSourceRange) {

boolean hasDefault = cases.stream().anyMatch(CaseIr::hasDefault);
boolean hasCaseNull = cases.stream().anyMatch(CaseIr::hasCaseNull);
boolean recheckDominanceNeeded = false;

boolean switchOnNullWouldImplicitlyThrow = switchOnNullWouldImplicitlyThrow(subject, cases);

// Has an unconditional case, meaning that any non-null value of the subject will be matched
long unconditionalCount =
Expand Down Expand Up @@ -467,7 +499,7 @@ && isSubtype(
.orElse(ifTreeSourceRange.lowerEndpoint());
casesCopy.add(
new CaseIr(
/* hasCaseNull= */ false,
/* hasCaseNull= */ !hasCaseNull && (enableSafe && switchOnNullWouldImplicitlyThrow),
/* hasDefault= */ true,
/* instanceOfOptional= */ Optional.empty(),
/* guardOptional= */ Optional.empty(),
Expand All @@ -476,6 +508,45 @@ && isSubtype(
/* caseSourceCodeRange= */ Range.closedOpen(
previousCaseEndPosition, previousCaseEndPosition)));
cases = casesCopy;
recheckDominanceNeeded = true;
} else if (enableSafe && !hasCaseNull && switchOnNullWouldImplicitlyThrow) {
List<CaseIr> casesCopy = new ArrayList<>(cases);
if (hasDefault) {
// Upgrade existing `default` to `case null, default`.
casesCopy =
casesCopy.stream()
.map(
caseIr -> {
if (caseIr.hasDefault()) {
return new CaseIr(
/* hasCaseNull= */ true,
/* hasDefault= */ true,
/* instanceOfOptional= */ caseIr.instanceOfOptional(),
/* guardOptional= */ caseIr.guardOptional(),
/* expressionsOptional= */ caseIr.expressionsOptional(),
/* arrowRhsOptional= */ caseIr.arrowRhsOptional(),
/* caseSourceCodeRange= */ caseIr.caseSourceCodeRange());
}
return caseIr;
})
.collect(toImmutableList());
recheckDominanceNeeded = true;
} else {
// Invariant: enableSafe=true
// Inject new empty `case null`
casesCopy.add(
new CaseIr(
/* hasCaseNull= */ true,
/* hasDefault= */ false,
/* instanceOfOptional= */ Optional.empty(),
/* guardOptional= */ Optional.empty(),
/* expressionsOptional= */ Optional.empty(),
/* arrowRhsOptional= */ Optional.empty(),
/* caseSourceCodeRange= */ Range.closedOpen(
ifTreeSourceRange.lowerEndpoint(), ifTreeSourceRange.lowerEndpoint())));
recheckDominanceNeeded = true;
}
cases = casesCopy;
}

// Given any possible changes, is the code after the switch reachable?
Expand Down Expand Up @@ -548,7 +619,7 @@ && isSubtype(
}
}

return Optional.of(cases);
return recheckDominanceNeeded ? maybeFixDominance(cases, state, subject) : Optional.of(cases);
}

/**
Expand All @@ -558,7 +629,7 @@ && isSubtype(
* original case IRs. Note that this method does not fully validate the resulting case IRs, but
* rather only partially validates them with respect to pull-up.
*/
private static Optional<List<CaseIr>> maybePullUp(
private Optional<List<CaseIr>> maybePullUp(
List<CaseIr> cases,
VisitorState state,
IfChainAnalysisState ifChainAnalysisState,
Expand All @@ -570,6 +641,13 @@ private static Optional<List<CaseIr>> maybePullUp(
return Optional.empty();
}

boolean hasCaseNull = cases.stream().anyMatch(CaseIr::hasCaseNull);
if (hasCaseNull) {
// Don't support pulling up when there is already an explicit `case null`, since this
// could implicate duplication of the pulled-up statements into both `case null` and `default`
return Optional.empty();
}

List<CaseIr> casesCopy = new ArrayList<>(cases);

if (ifChainAnalysisState.at().getElseStatement() == null
Expand All @@ -584,15 +662,16 @@ private static Optional<List<CaseIr>> maybePullUp(
// Statements containing break or yield cannot be pulled up
break;
}

int startPos =
casesCopy.isEmpty()
? ifTreeRange.lowerEndpoint()
: casesCopy.getLast().caseSourceCodeRange().upperEndpoint();
int endPos = state.getEndPosition(statement);
casesCopy.add(
new CaseIr(
/* hasCaseNull= */ false,
/* hasCaseNull= */ enableSafe
&& switchOnNullWouldImplicitlyThrow(
ifChainAnalysisState.subjectOptional().get(), cases),
/* hasDefault= */ true,
/* instanceOfOptional= */ Optional.empty(),
/* guardOptional= */ Optional.empty(),
Expand Down Expand Up @@ -1283,7 +1362,7 @@ private static Optional<Type> unboxed(Tree tree, VisitorState state) {
}

/** Performs a detailed analysis of the if-chain, generating suggested fixes as needed. */
private static List<SuggestedFix> deepAnalysisOfIfChain(
private List<SuggestedFix> deepAnalysisOfIfChain(
List<CaseIr> cases,
IfChainAnalysisState finalIfChainAnalysisState,
IfTree ifTree,
Expand Down Expand Up @@ -1322,7 +1401,7 @@ private static List<SuggestedFix> deepAnalysisOfIfChain(
.flatMap(caseList -> maybeFixDominance(caseList, state, subject))
.flatMap(
x ->
maybeFixDefaultAndUnconditional(
maybeFixDefaultNullAndUnconditional(
x,
subject,
ifTree,
Expand All @@ -1344,7 +1423,7 @@ private static List<SuggestedFix> deepAnalysisOfIfChain(
.flatMap(caseList -> maybeFixDominance(caseList, state, subject))
.flatMap(
caseList ->
maybeFixDefaultAndUnconditional(
maybeFixDefaultNullAndUnconditional(
caseList,
subject,
ifTree,
Expand All @@ -1353,7 +1432,9 @@ private static List<SuggestedFix> deepAnalysisOfIfChain(
/* numberPulledUp= */ 0,
finalIfChainAnalysisState.handledEnumValues(),
switchType,
ifTreeSourceRange));
ifTreeSourceRange))
// Changing default/null can affect dominance
.flatMap(caseList -> maybeFixDominance(caseList, state, subject));
}

List<SuggestedFix> suggestedFixes = new ArrayList<>();
Expand Down
Loading
Loading