-
Notifications
You must be signed in to change notification settings - Fork 745
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.
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
StatementSwitchToExpressionSwitch: skip rule cases #3656
StatementSwitchToExpressionSwitch: skip rule cases #3656
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some notes. This PR avoids the NPE mentioned here.
@@ -122,7 +134,7 @@ private static AnalysisResult analyzeSwitchTree(SwitchTree switchTree) { | |||
|
|||
List<? extends StatementTree> statements = caseTree.getStatements(); | |||
CaseFallThru caseFallThru = CaseFallThru.MAYBE_FALLS_THRU; | |||
if (statements == null || statements.isEmpty()) { | |||
if (statements.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts the change in #3645, because there are actually many more CaseTree#getStatements()
calls in this class, and they all assume a non-null
result. The isRuleSwitch
check now guarantees that.
private static boolean isRuleSwitch(SwitchTree switchTree) { | ||
return switchTree.getCases().stream().anyMatch(StatementSwitchToExpressionSwitch::isRuleCase); | ||
} | ||
|
||
private static boolean isRuleCase(CaseTree caseTree) { | ||
try { | ||
return "RULE".equals(CaseTree.class.getMethod("getCaseKind").invoke(caseTree).toString()); | ||
} catch (ReflectiveOperationException e) { | ||
throw new LinkageError(e.getMessage(), e); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the documented invariant on which we now rely, perhaps it's more accurate to test for "STATEMENT"
. But it seems unlikely that a third Kind
will be introduced any time soon, and this way we avoid a negation at the call site. But I'm happy to switch this around.
@@ -1030,6 +1030,25 @@ public void emptySwitchCases_noMatch() { | |||
" }", | |||
" }", | |||
"}") | |||
.setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this for symmetry with other tests, and because in theory it covers more code paths (except that in practice isRuleSwitch(switchTree)
is tested before the flag is consulted).
NB: the other tests use .setArgs(ImmutableList.of(...))
; I wouldn't mind opening a separate cleanup PR for that.
The cases in a switch statement must all be of kind `STATEMENT` or `RULE`. In the latter case `StatementSwitchToExpressionSwitch` does not apply.
Rebased and resolved conflict. One way to look at this PR is that without it, it is unlikely that JDK 14+ users can enable |
4a7b6bf
to
b0632d0
Compare
The solution in #3676 doesn't require reflection; will close this PR. |
The cases in a switch statement must all be of kind
STATEMENT
orRULE
. In the latter caseStatementSwitchToExpressionSwitch
does notapply.