This is great work. Thanks for writing about it. I hold a stronger opinion than "the danger of nullness and the validity of the warnings depend on the code context": nullness is a solved problem, and type systems solved it, decades ago in some languages and more recently in Java. Nullness errors are a choice, and we should not choose to have them. Treat them as you would treat any other type error.
And I hold even strong opinions than that! Almost all the time, unclear nullness/typing has an underlying cause of lack of clarity in design and thinking. This is why fixing nullness almost always improves the code, but also why it can be difficult to do after the fact, because it can require fixing a bad or broken design. But sometimes it is easy, and you should do it with gusto in either way :-) And more! If checkerframework cannot determine the correctness of something, probably neither can you. You may think you can, but you are probably wrong. And even if you are correct today, the next person coming along will not have your internal mental state and will easily break it. As an example of both of the above: it can be very hard to fix API designs like DoFn (or JUnit classes, etc) of the start/doSomething/end variety that rely on a caller to invoke methods in a particular order and have significant inter-method stateful dependencies that are not at all guaranteed by their design. Such APIs are extremely fragile and the source of a huge number of bugs because it is very easy and common to invoke methods in an order or multiplicity that was not designed for. Not at all coincidentally, it is quite hard and annoying to add the needed "Requires" and "Ensures" annotations to get them to type check, and also very hard and annoying to satisfy those requirements when calling the methods. This is all a feature - the design was bad, and the difficulty setting up and satisfying the invariants helps you realize it. /lecture :-) Kenn On Tue, Apr 25, 2023 at 9:56 AM Damon Douglas <damondoug...@apache.org> wrote: > Hello Everyone, > > *For those experienced with Beam*: > > PR/26413 [1] begins the long journey of addressing [2] which is a subtask > of [3]. 千里之行，始於足下 :-) > > *For those beginning on the learning path*: > > The pull request referenced in the email is not directly related to Beam > but involves a Java annotation called SuppressWarnings. Per its namesake, > it suppresses warnings according to the values you set in the annotation. > In [2] and [3]'s context the particular setting is "nullness" that > suppresses any warnings related to potentially null references in the code. > > In the Beam repository, we utilize The Checker Framework [4] to help > maintain a clean codebase. When such annotation applies, it will > essentially mask these warnings and let the code pass without errors. When > removing the SuppressWarnings with "nullness" annotation, the checks fail. > > Why is this a big deal? The danger of nullness and the validity of the > warnings depend on the code context. However, as of this writing there are > many Java classes in our repository with the SuppressWarnings annotation. > What is the likelihood they are masking something serious? > > Here in our discussion would be a great place to be practical and relay > what can be done when receiving a null-related error from the checker > framework. However, I want to wait after I do more of these and find > validation through peer review. Then, I will follow-up with the dev list. > > Meanwhile, you can take a look at my attempt in the PR [1]. > > Best, > > Damon > > *References*: > 1. https://github.com/apache/beam/pull/26413 > 2. https://github.com/apache/beam/issues/20498 > 3. https://github.com/apache/beam/issues/20497 > 4. https://checkerframework.org/ > > >
