Add support for switch expressions #520
Conversation
This reverts commit 9c4630c.
Pull Request Test Coverage Report for Build #610
💛 - Coveralls |
|
This PR is ready to review I think. We don't get Coveralls support for the new |
| ExpressionTree switchExpression = tree.getExpression(); | ||
| if (switchExpression instanceof ParenthesizedTree) { | ||
| switchExpression = ((ParenthesizedTree) switchExpression).getExpression(); | ||
| ExpressionTree switchSelectorExpression = tree.getExpression(); |
There was a problem hiding this comment.
This variable rename is to avoid confusion now that we have switch expressions. Also, add a comment to explain the stripping of one level of parens
|
JMH results. For This PR: So there's no slowdown on the benchmarks. |
| @@ -1944,8 +1949,13 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) { | |||
| exprMayBeNull = nullnessFromDataflow(state, expr); | |||
| break; | |||
| default: | |||
| throw new RuntimeException( | |||
| "whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr)); | |||
| // match switch expressions by comparing strings, so the code compiles on JDK versions < 12 | |||
There was a problem hiding this comment.
Unfortunate, but makes sense...
| SwitchExpressionNode node, TransferInput<Nullness, NullnessStore> input) { | ||
| // The cfg includes assignments of the value of each case body of the switch expression | ||
| // to the switch expression var (a synthetic local variale). So, the dataflow result | ||
| // for the switch expression is just the result for the switch expression var |
There was a problem hiding this comment.
Nice work on the CF part here!
| } | ||
|
|
||
| @Test | ||
| public void testSwitchExpression() { |
There was a problem hiding this comment.
These are a lot of test cases in a single test method. I am fine with landing as is, but... would it be too complicated to partition this into a few different test cases? (Basically one for each of the testX methods of SwitchExpr, since they all seem fairly independent)
There was a problem hiding this comment.
Sure, done in ae2900d
There was a problem hiding this comment.
LGTM. Ready to merge! (The Coveralls regression is a CI limitation and should be addressed soon by #521 )
Fixes #289
Checker Framework 3.21.0 added support for switch expressions; we build on that to support the expressions in NullAway. We also add a
jdk17-unit-testsmodule to test the support (which requires a JDK 12+ compiler).(Copy of #518, but now from a branch on the main repo so Coveralls works)