On Mon, Mar 15, 2021 at 11:42 PM Reuven Lax <re...@google.com
<mailto:re...@google.com>> wrote:
On Mon, Mar 15, 2021 at 9:12 PM Kenneth Knowles
<k...@apache.org <mailto:k...@apache.org>> wrote:
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)
Huh - Fahndrich tried to hire me once to work on a project
called Singularity. Small world. Also note that Sanjay
Ghemawat is listed in the citations!
Umm, I was confusing names. Fahndrich is actually a former
coworker at Google :)
* 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.
I agree with the above in a closed system. However as
mentioned, some of the APIs we use suffer from this.
In a previous life, I saw up close an effort to add such
analysis to a large codebase. Two separate tools called
Prefix and Prefast were used (the difference between the two
is actually quite interesting, but not relevant here).
However in order to make this analysis useful, there was a
massive effort to properly annotate the entire codebase,
including all libraries used. This isn't a perfect example -
this was a C++ codebase which is much harder to analyze, and
these tools identified far more than simply nullness errors
(resource leaks, array indices, proper string null
termination, exception behavior, etc.). However the closer we
can get to properly annotating the transitive closure of our
dependencies, the better this framework will work.
- 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.
I agree with this. I prefer to be able to detect on my
computer that there are failures, and not have to wait for
submission. The complaint was that some people are
experiencing trouble on their local machine however, so it
seems reasonable to add an opt-out flag (though I would
prefer opt out to opt in).
Kenn
[1]
https://web.eecs.umich.edu/~bchandra/courses/papers/Fahndrich_NonNull.pdf
<https://web.eecs.umich.edu/~bchandra/courses/papers/Fahndrich_NonNull.pdf>
[2]
https://homes.cs.washington.edu/~mernst/pubs/pluggable-checkers-issta2008.pdf
<https://homes.cs.washington.edu/~mernst/pubs/pluggable-checkers-issta2008.pdf>
[3]
https://github.com/google/guava/blob/fe3fda0ca54076a2268d060725e9a6e26f867a5e/pom.xml#L275
<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)
<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
<https://github.com/apache/beam/pull/12284> and
https://github.com/apache/beam/pull/12162
<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
<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
<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
<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 <mailto: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 <mailto: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
<mailto: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.
