Support switch expressions #289
Assignees
Comments
|
Thanks for the report, @michaelhixson! For the For the |
Any idea how I can craft a test case that uses Checker Dataflow directly, without NullAway, to demonstrate the problem to them? I gave it a shot and couldn't figure out how to use anything in the |
|
Yeah a standalone example will be tricky. You can probably just point them to this issue. Here is the relevant part of the exception stack that you can paste: And here is the line throwing the exception on the Checker Framework master branch. |
|
Ok, I filed the upstream issue here: typetools/checker-framework#2373 |
|
Does NullAway cover standard switch statements on enums today? I noticed this in our codebase today
|
|
Good catch, @ZacSweers! That one should be easy to fix. Can you file a separate issue for it? |
|
Done - #299 |
@msridhar What do you think of an approach like this? diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
index a600c5d..ce823b6 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -1962,6 +1962,10 @@ public class NullAway extends BugChecker
exprMayBeNull = nullnessFromDataflow(state, expr);
break;
default:
+ if (expr.getKind().name().equals("SWITCH_EXPRESSION")) {
+ exprMayBeNull = nullnessFromDataflow(state, expr);
+ break;
+ }
throw new RuntimeException(
"whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr));
}With that change to NullAway and the following change to dataflow, my code with switch expressions compiles. diff --git a/dataflow/src/main/java/org/checkerframework/dataflow/cfg/CFGBuilder.java b/dataflow/src/main/java/org/checkerframework/dataflow/cfg/CFGBuilder.java
index 505b5a3ec..58ed6738b 100644
--- a/dataflow/src/main/java/org/checkerframework/dataflow/cfg/CFGBuilder.java
+++ b/dataflow/src/main/java/org/checkerframework/dataflow/cfg/CFGBuilder.java
@@ -3382,11 +3382,6 @@ public class CFGBuilder {
}
}
- @Override
- public Node visitCase(CaseTree tree, Void p) {
- throw new AssertionError("case visitor is implemented in SwitchBuilder");
- }
-
@Override
public Node visitCatch(CatchTree tree, Void p) {
scan(tree.getParameter(), p);This doesn't catch nullness errors involving switch expressions, but it seems like a good intermediate step. |
|
@michaelhixson I'm ok with a change like this, though as you know we'll also need an updated Checker Framework dataflow library. |
|
Any updates or a work around? Ran into this problem on a Java 14 project. |
|
@lessthanoptimal unfortunately, no. We would still need a fix for typetools/checker-framework#2373 before we could ship a NullAway fix for this problem. We will definitely post an update here when we make some progress |
msridhar
added a commit
that referenced
this issue
Dec 7, 2021
The key new feature here is that the dataflow library no longer crashes on switch expressions / arrow case labels in switch statements: https://github.com/typetools/checker-framework/blob/checker-framework-3.20.0/docs/CHANGELOG.md#version-3200-december-6-2021 This is an initial step toward fixing #289, though we will need precise control-flow graphs (which is still a WIP in Checker dataflow) to really handle these constructs properly.
|
With #510 landed the latest NullAway snapshot should no longer crash on the new kinds of |
|
It threw this exception: Here's how I added the dependency: |
|
Thanks! I should have known that something wouldn't work. I will work on fixing that crash and putting together a proper test case (may take a bit of time as we currently don't test on JDK 17). |
|
Sounds good an thanks for working on this issue! FYI I got that error in JDK 15. |
Thanks! I'm sure the same error will appear in JDK 17. FWIW I don't think we have resources to test and fix NullAway bugs on non-LTS JDK releases, though if it's an easy fix we will try. |
This was referenced Dec 17, 2021
msridhar
added a commit
that referenced
this issue
Dec 21, 2021
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-tests` module to test the support (which requires a JDK 12+ compiler).
|
@lessthanoptimal this should be fixed now! Any chance you can again test the snapshot build on your code base? |
|
@msridhar Just tried it and the SNAPSHOT is working! Thanks! I tested it in two projects. |
|
Thanks for checking! We'll get this out in a new release soon though it may be after the holidays |
|
Celebration might be premature. Was in the process of adding null checks to a project and ran into a situation where it blew up. |
|
@lessthanoptimal oof that looks like a Checker Framework issue. Any chance you can reduce to a self-contained small-ish example with the same crash? |
|
Also, if you add |
|
@msridhar Adding I'll see if I can create a stand alone example. This particular project is fairly large. Not sure if this is relevant, but the switch expression is inside a lambda which is not typical in my use cases. |
|
Ok, thanks! I may be able to repro just based on your excerpt; will report back. |
|
Managed to reproduce; opened #524 on it. While we work to fix, you'll have to use |
|
FYI our current switch expression support is now released in version 0.9.3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Support switch expressions, which are a preview feature in Java 12.
Currently, if I use a switch expression in my code, NullAway will throw an error during compilation. Removing NullAway from my
<annotationProcessorPaths>and removing the NullAway-related compiler arguments makes the error go away.I've seen two kinds of errors so far.
Error 1: "java.lang.AssertionError: case visitor is implemented in SwitchBuilder"
Source code:
Output:
Error 2: "java.lang.RuntimeException: whoops, better handle SWITCH_EXPRESSION"
Source code:
Output:
The text was updated successfully, but these errors were encountered: