Yes, completely sound nullability checking has been added to the project via checkerframework, based on a large number of NPE bugs (1-5% depending on how you search, but many other bugs likely attributable to nullness-based design errors) which are extra embarrassing because NPEs have were essentially solved, even in practice for Java, well before Beam existed.
Checker framework is a pluggable type system analysis with some amount of control flow sensitivity. Every value has a type that is either nullable or not, and certain control structures (like checking for null) can alter the type inside a scope. The best way to think about it is to consider every value in the program as either nullable or not, much like you think of every value as either a string or not, and to view method calls as inherently stateful/nondetermistic. This can be challenging in esoteric cases, but usually makes the overall code health better anyhow. Your example illustrates how problematic the design of the Java language is: the analysis cannot assume that `getDescription` is a pure function, and neither should you. Even if it is aware of boolean-short-circuit it would not be sound to accept this code. There is an annotation for this in the cases where it is true (like proto-generate getters): https://checkerframework.org/api/org/checkerframework/dataflow/qual/Pure.html The refactor for cases like this is trivial so there isn't a lot of value to thinking too hard about it. if (statusCode.equals(Code.INVALID_ARGUMENT) @Nullable String desc = statusCode.toStatus().getDescription(); if (desc != null && desc.contains("finalized")) { return false; } } To a casual eye, this may look like a noop change. To the analysis it makes all the difference. And IMO this difference is real. Humans may assume it is a noop and humans would be wrong. So many times when you think/expect/hope that `getXYZ()` is a trivial getter method you later learn that it was tweaked for some odd reason. I believe this code change makes the code better. Suppressing these errors should be exceptionally rare, and never in normal code. It is far better to improve your code than to suppress the issue. It would be very cool for the proto compiler to annotate enough that new-and-improved type checkers could make things more convenient. Kenn On Mon, Jan 11, 2021 at 8:53 PM Reuven Lax <re...@google.com> wrote: > I can use that trick. However I'm surprised that the check appears to be > so simplistic. > > For example, the following code triggers the check on > getDescription().contains(), since getDescription returns a Nullable > string. However even a simplistic analysis should realize that > getDescription() was checked for null first! I have a dozen or so cases > like this, and I question how useful such a simplistic check it will be. > > if (statusCode.equals(Code.INVALID_ARGUMENT) > && statusCode.toStatus().getDescription() != null > && statusCode.toStatus().getDescription().contains("finalized")) { > return false; > } > > > On Mon, Jan 11, 2021 at 8:32 PM Boyuan Zhang <boyu...@google.com> wrote: > >> Yeah it seems like the checker is enabled: >> https://issues.apache.org/jira/browse/BEAM-10402. I used >> @SuppressWarnings({"nullness" )}) to suppress the error when I think it's >> not really a concern. >> >> On Mon, Jan 11, 2021 at 8:28 PM Reuven Lax <re...@google.com> wrote: >> >>> Has extra Nullable checking been enabled in the Beam project? I have a >>> PR that was on hold for several months, and I'm struggling now with compile >>> failing to complaints about assigning something that is nullable to >>> something that is not nullable. Even when the immediate control flow makes >>> it absolutely impossible for the variable to be null. >>> >>> Has something changed here? >>> >>> Reuven >>> >>
