I will be blunt about my opinions about the general issue: - NullPointerExceptions (and similar) are a solved problem. * They have been since 2003 at the latest [1] (this is when the types were hacked into Java - the foundation dates back to the 70s or earlier) * Checkerframework is a _pluggable_ system where that nullness type system is a "hello, world" level demo, since 2008 at the latest [2]. * Our users should know this and judge us accordingly.
- Checkerframework should be thought of and described as type checking, because it is. It is not heuristic nor approximate. - If your code was unclear about whether something could be null, it was probably unclear to a person reading it also, and very likely to have actual bugs. - APIs that accept a lot of nullable parameters are, generally speaking, bad APIs. They are hard to use correctly, less readable, and very likely to cause actual bugs. You are forcing your users to deal with accidental complexity you left behind. * Corollary to the above two points: Almost all the time, the changes to clearify nullness make your code better, more readable, easier for users or editors. - It is true that there is a learning curve to programming in this way. - There are certainly common patterns in Java that do not work very well, and suppression is sometimes the best option. * Example: JUnit's @Setup and @Test conventions do not work very well and it is not worth the effort to make them work. This is actually because if it were "normal code" it would be bad code. There are complex inter-method dependencies enforced only by convention. This matters: sometimes a JUnit test class is called from another class, used as "normal code". This does go wrong in practice. Plain old JUnit test cases frequently go wrong as well. And here is my opinion when it comes to Beam: - "Community over code" is not an excuse for negligent practices that cause easily avoidable risk to our users. I will be very disappointed if we choose that. - I think having tooling that helps newcomers write better code by default is better for the community too. Just like having automatic formatting is better. Less to haggle about in review, etc. - A simple search reveals about 170 bugs that we know of [4]. - So far in almost every module I have fixed I discovered actual new null errors. Many examples at [5]. - It is extremely easy to suppress the type checking. Almost all of our classes have it suppressed already (I did this work, to allow existing errors while protecting new code). - Including the annotations in the shipped jars is an important feature. Without this, users cannot write null-safe code themselves. * Reuven highlighted this: when methods are not annotated, we have to use/implement workarounds. Actually Guava does use checkerframework annotations [6] and the problem is that it requires its *input* to already be non-null so actually you cannot even use it to convert nullable values to non-nullable values. * Beam has its own [7] that is annotated, actually for yet another reason: when Guava's checkNotNull fails, it throws NPE when it should throw IllegalArgumentException. Guava's checkNotNull should not be used for input validation! - It is unfortunate that IntelliJ inserts a bunch of annotations in user code. I wonder if there is something we can do about that. At the Java level, if they are not on the classpath they should be ignored and not affect users. Coincidentally, the JDK has had NullPointerExceptions in this area :-) [8]. I understand the pain of longer compile times slowing people down. That is actually a problem to be solved which does not require lowering our standards of quality. How about we try moving it to a separate CI job and see how it goes? In my experience stories like Reuven's are much more frustrating in a separate CI job because you find out quite late that your code has flaws. Like when spotless fails, but much more work to fix, and would have been prevented long ago if it were integrated into the compile. Kenn [1] https://web.eecs.umich.edu/~bchandra/courses/papers/Fahndrich_NonNull.pdf [2] https://homes.cs.washington.edu/~mernst/pubs/pluggable-checkers-issta2008.pdf [3] https://github.com/google/guava/blob/fe3fda0ca54076a2268d060725e9a6e26f867a5e/pom.xml#L275 [4] https://issues.apache.org/jira/issues/?jql=project%20%3D%20BEAM%20AND%20(summary%20~%20%22NPE%22%20OR%20summary%20~%20%22NullPointerException%22%20OR%20description%20~%20%22NPE%22%20OR%20description%20~%20%22NullPointerException%22%20OR%20comment%20~%20%22NPE%22%20OR%20comment%20~%20%22NullPointerException%22) [5] https://github.com/apache/beam/pull/12284 and https://github.com/apache/beam/pull/12162 and [6] https://github.com/google/guava/blob/fe3fda0ca54076a2268d060725e9a6e26f867a5e/guava/src/com/google/common/base/Preconditions.java#L878 [7] https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/util/Preconditions.java [8] https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8152174 On Mon, Mar 15, 2021 at 2:12 PM Reuven Lax <re...@google.com> wrote: > I have some deeper concerns with the null checks. The fact that many > libraries we use (including guava) don't always annotate their methods > forces a lot of workarounds. As a very simple example, the return value > from Preconditions.checkNotNull clearly can never be null, yet the > nullability checks don't know this. This and other similar cases require > constantly adding extra unnecessary null checks in the code just to make > the checker happy. There have been other cases where I haven't been able to > figure out a way to make the checker happy (often these seem to involve > using lambdas), and after 10-15 minutes of investigation have given up and > disabled the check. > > Now you might say that it's worth the extra pain and ugliness of writing > "useless" code to ensure that we have null-safe code. However I think this > ignores a sociological aspect of software development. I have a > higher tolerance than many for this sort of pain, and I'm willing to spend > some time figuring out how to rewrite my code such that it makes the > checker happy (even though often it forced me to write much more awkward > code). However even I have often found myself giving up and just disabling > the check. Many others will have less tolerance than me, and will simply > disable the checks. At that point we'll have a codebase littered with > @SuppressWarnings("nullness"), which doesn't really get us where we want to > be. I've seen similar struggles in other codebases, and generally having a > static checker with too many false positives often ends up being worse than > having no checker. > > On Mon, Mar 15, 2021 at 10:37 AM Ismaël Mejía <ieme...@gmail.com> wrote: > >> +1 >> >> Even if I like the strictness for Null checking, I also think that >> this is adding too much extra time for builds (that I noticed locally >> when enabled) and also I agree with Jan that the annotations are >> really an undesired side effect. For reference when you try to auto >> complete some method signatures on IntelliJ on downstream projects >> with C-A-v it generates some extra Checkers annotations like @NonNull >> and others even if the user isn't using them which is not desirable. >> >> >> >> On Mon, Mar 15, 2021 at 6:04 PM Kyle Weaver <kcwea...@google.com> wrote: >> >> >> >> Big +1 for moving this to separate CI job. I really don't like what >> annotations are currently added to the code we ship. Tools like Idea add >> these annotations to code they generate when overriding classes and that's >> very annoying. Users should not be exposed to internal tools like >> nullability checking. >> > >> > >> > I was only planning on moving this to a separate CI job. The job would >> still be release blocking, so the same annotations would still be required. >> > >> > I'm not sure which annotations you are concerned about. There are two >> annotations involved with nullness checking, @SuppressWarnings and >> @Nullable. @SuppressWarnings has retention policy SOURCE, so it shouldn't >> be exposed to users at all. @Nullable is not just for internal tooling, it >> also provides useful information about our APIs to users. The user should >> not have to guess whether a method argument etc. can be null or not, and >> for better or worse, these annotations are the standard way of expressing >> that in Java. >> >
