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.
