Hi,
I'll give my two cents here.
I'm not 100% sure that the 1-5% of bugs are as severe as other types of
bugs. Yes, throwing NPEs at user is not very polite. On the other hand,
many of these actually boil down to user errors. Then we might ask what
a correct solution would be. If we manage to figure out what the actual
problem is and tell user what specifically is missing or going wrong,
that would be just awesome. On the other hand, if a tool used for
avoiding "unexpected" NPEs forces us to code
Object value = Objects.requireNonNull(myNullableObject); // or
similar using Preconditions
value.doStuff();
instead of just
myNullableObject.doStuff()
what we actually did, is a) made a framework happy, and b) changed a
line at which NPE is thrown by 1 (and yes, internally prevented JVM from
thrown SIGSEGV at itself, but that is deeply technical thing). Nothing
changed semantically, from user perspective.
Now, given that the framework significantly rises compile time (due to
all the checks), causes many "bugs" being reported by static code
analysis tools (because the framework adds @Nonnull default annotations
everywhere, even when Beam's code actually counts with nullability of a
field), and given how much we currently suppress these checks ($ git
grep BEAM-10402 | wc -l -> 1981), I'd say this deserves a deeper discussion.
Jan
On 1/20/21 10:48 PM, Kenneth Knowles wrote:
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
<mailto: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
<mailto: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
<mailto: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
