Nullness, and the way IDEA infers nullness / not-nullness

Consider the following code:

import org.jetbrains.annotations.NotNull;
public class TestNotNullAssignment {
void foo(final Object foo) {
final @NotNull Object bar = foo;
if (foo == null) System.out.println("Hello");
}
}

To be strict, I would like to ensure that a @NotNull variable, such as
foo, can only be assigned a @NotNull reference (or a reference that is
guaranteed to be non-null using data flow analysis). Then there would
be a warning in the "bar=foo" line (there isn't).

It might be argued that this is too strict, because there's too much
legacy unannotated code which would lead to too many warnings. But that
leads to some other negative consequences, like the fact that IDEA
currently infers from the assignment that foo cannot be null (because
then it couldn't have been assigned to a @NotNull variable), and
therefore warns me that "foo == null" can never be true.

I was about to delete an entire section of code dealing with the
"impossible" case where foo was null before I realized the error was in
fact that bar should not have been annotated @NotNull.

Perhaps IDEA should not infer that foo cannot be null simply because it
is assigned to the @NotNull variable bar? But IDEA would still make the
assumption that bar cannot be null, since bar is explicitly annotated,
so while "foo==null" would not generate a warning, "bar==null" still
would -- despite the fact that we know that foo==bar. This isn't very
consistent.

So I can't help coming back to the first conclusion: That there should
be a warning whenever a @NotNull variable is assigned the value of an
unannotated reference (as opposed to the current behaviour, which only
warns when it is assigned the value of a @Nullable reference). After
all, if you do know that your unannotated reference cannot be null, you
can quite easily suppress the false warning.

Any other thoughts, ideas or opinions about this?

8 comments
Comment actions Permalink

To be strict, I would like to ensure that a @NotNull
variable, such as
foo, can only be assigned a @NotNull reference (or a
reference that is
guaranteed to be non-null using data flow analysis).


A perfect valid propositon, +1 for this!

It might be argued that this is too strict, because
there's too much
legacy unannotated code which would lead to too many
warnings.


If you're using the nullness annotations, you're dealing with a mix with new and legacy code. You're writing a new service/function/object/whatever that makes use of legacy code. Of course, if you annotate a field with @NotNull, you want to take full advantage of that, and checking that only @NotNull expressions are assigned to this field is part of that -- and we always have @SuppressWarnings for the "bridge" portions of code that deal with the legacy, non-annotated code.

0
Comment actions Permalink

I filed a few issues about the way things currently work (2087 seems the
closest to what you are talking about).
http://www.jetbrains.net/jira/browse/IDEA-2068
http://www.jetbrains.net/jira/browse/IDEA-2087
http://www.jetbrains.net/jira/browse/IDEADEV-1134
http://www.jetbrains.net/jira/browse/IDEABKL-2949

A big problem I can see is that there are currently a few ways that a
@NotNull reference can be set to null without getting any warning. So if
you think of the guarantee in terms of "code that compiles with no
warnings will never produce a NullPointerException" (which is what the
Java team said for generics about ClassCastException) then you're going
to run into problems, because the implementation as it stands doesn't
make that guarantee.

I just asked for an option to treat all non-annotated variables as
potentially null. Not sure if that's the smartest way to do it, or how
it would combine with Idea's flow analysis ...

R
PS I would like to hear Dave G's comments on this, since he keeps
trumpeting the number of bugs he has found using annotations!

0
Comment actions Permalink

Robert Gibson wrote:

I just asked for an option to treat all non-annotated variables as
potentially null. Not sure if that's the smartest way to do it, or how
it would combine with Idea's flow analysis ...


Just to be clear, I believe there are two slightly different suggestions
here:

1) Treat non-annotated variables exactly as @Nullable variables are
treated now. If you do this without further heuristics, then even
System.out.println("") will trigger an NPE warning, because System.out
is non-annotated and would be assumed to be @Nullable. You'd have to
place this within "if (System.out != null)".

2) Only treat non-annotated variables differently in an assignment to a
@NotNull variable. This will only trigger warnings in those parts of
your code that you've actually annotated with @NotNull, that is, those
parts where you've explicitly stated that you want a warning if the
variable might ever be assigned the value null. This is the behaviour I
would like to see (I think...).

0
Comment actions Permalink

Jonas Kvarnström wrote:

2) Only treat non-annotated variables differently in an assignment to a
@NotNull variable. This will only trigger warnings in those parts of
your code that you've actually annotated with @NotNull, that is, those
parts where you've explicitly stated that you want a warning if the
variable might ever be assigned the value null. This is the behaviour I
would like to see (I think...).


Yep, that sounds right to me - good analysis, please add your comments
to IDEA-2087 and let's see if we can get this one nailed into 5.0.

R

0
Comment actions Permalink

Jonas Kvarnström wrote:

2) Only treat non-annotated variables differently in an assignment to a
@NotNull variable.


By the way, for this to work properly it needs to be integrated with the
IDEA dataflow analysis engine. I could implement a partial version of
this inspection myself by simply checking annotations, but then
something like this would still trigger the warning:

void foo(Object a) {
if (a != null) {
// a can't be null, but simple @NotNull analysis
// won't discover this
@NotNull Object b = a;
}
}

(Actually I did implement a few nullness inspections myself as an
extension to IG, but since I don't really know IG or the PSI API they're
just quick copy/paste hacks that sometimes give false warnings and would
most likely crash on partially completed code...)

0
Comment actions Permalink

>2) Only treat non-annotated variables differently in an assignment to a @NotNull variable.

This sounds like a reasonable compromise, although there will still be a bunch of false positives.

@NotNull foo = myList.get(0);

would be null, since get() on a List isn't annotated (and if it were annotated, it would have to be @Nullable).

--Dave Griffith

0
Comment actions Permalink

I would like to hear Dave G's comments on this, since he keeps trumpeting the number of bugs he has found using annotations!

It should be noted that the bugs I found are due to the fact that the third-party library I'm coding against is being annotated. I knew there were going to issues with un-annotated libraries, but believe that even with those issues using nullity annotations are quite valuable. Hopefully, that will be enough to overcome the network-effects issues, where nullity annotations only acheive their full potential when everybody uses them...

--Dave Griffith

0
Comment actions Permalink

Dave Griffith wrote:

This sounds like a reasonable compromise, although there will still
be a bunch of false positives.


Yes, that's certainly true. But at least you'll be able to do something
about it locally rather than having your entire code base turn yellow
immediately.

@NotNull foo = myList.get(0);

would be null, since get() on a List isn't annotated (and if it were
annotated, it would have to be @Nullable).


Which means that this is the kind of false positive that you would have
to deal with even if libraries were completely annotated. Either by
suppressing warnings or by using something like this ugly construction:

bar = myList.get(0);
assert bar != null;
@NotNull foo = bar;

I guess this is all because this nullability mechanism is not powerful
enough to handle all kinds of constraints, even though it does handle
quite a lot of constraints without requiring all the power and
complexity of a true pre/postcondition mechanism.

In this particular special case one could perhaps get by without
warnings or false positives by adding further container-related nullness
annotations stating whether the container that a reference points to can
contain null elements (@CanContainNull and @CannotContainNull), and
special annotations for get() and add()-style methods
(@NullableIfContainerCanContainNull get(int index) and
add(@NullableIfContainerCanContainNull Object obj)), but I think you'd
quickly reach the point of diminishing returns for such specialized
annotations.

Not to mention the case where you know that if myStructure.isBar() is
true, myStructure.getFoo() cannot return null -- this is popping upp in
a number of places in my own projects.

0

Please sign in to leave a comment.