Erroneous NullPointerException varning?

In piece of code below Demetra complains that pv.getPriorVersion() may
produce NullPointerException (see comment below). getPriorVersion() method
is declared as @Nullable

@Nullable public TemplateNode getOldestVersion() {
TemplateNode pv = this.getPriorVersion();
if (pv == null) {
return null;
}
int count = 0;
while (pv.getPriorVersion() != null) { //Demetra complains that
pv.getPriorVersion() may produce NullPointerException here
CompositeValidation.checkLoop(count++, this, "priorVersion");
pv = pv.getPriorVersion();
}
return pv;
}


8 comments
Comment actions Permalink

Another simpler case

if (nextVersion != null) {
nextVersion.setPriorVersion(getPriorVersion());
}

Demetra complains that method invocation
nextVersion.setPriorVersion(getPriorVersion()) can produce
NullPointerException

nextVersion is declared as @Nullable


"Alex Roytman" <nospam@domain.com> wrote in message
news:e561rd$nqn$1@is.intellij.net...

In piece of code below Demetra complains that pv.getPriorVersion() may
produce NullPointerException (see comment below). getPriorVersion() method
is declared as @Nullable

>

@Nullable public TemplateNode getOldestVersion() {
TemplateNode pv = this.getPriorVersion();
if (pv == null) {
return null;
}
int count = 0;
while (pv.getPriorVersion() != null) { //Demetra complains that
pv.getPriorVersion() may produce NullPointerException here
CompositeValidation.checkLoop(count++, this, "priorVersion");
pv = pv.getPriorVersion();
}
return pv;
}



0
Comment actions Permalink

Nothing wrong about that. IDEA doesn't know that two successive calls to pv.getPriorVersion() on the same object will return the same value, so it cannot assume that just because the while statement's check passed, the call inside the loop will not return null.

As a result, idea decides that the while() condition might execute with pv == null (after at lest one loop body invocation), and gives warning.

(Note: this reply applies to your first example only.)

0
Comment actions Permalink

Wait a second, do you mean this:

If you do, this code cannot be statically analyzed as safe, see my other post. Instead do:

0
Comment actions Permalink

Yes of course silly of me. When thinking simple getter/setter it is so easy
to forget that it is actually a method call :)

The second example is still strange.

By changing

if (nextVersion != null) {
nextVersion.setPriorVersion(getPriorVersion());
}

To

if (nextVersion != null) {
nextVersion.setPriorVersion(priorVersion);
}

I avoid the warning. Both priorVersion field and getPriorVersion() getter
are declared as Nullable, the setter has no annotation (it should)

"Walter Mundt" <emage@spamcop.net> wrote in message
news:5880315.1148619765602.JavaMail.itn@is.intellij.net...

Nothing wrong about that. IDEA doesn't know that two successive calls to
pv.getPriorVersion() on the same object will return the same value, so it
cannot assume that just because the if statement passed, the call
inside will succeed.



0
Comment actions Permalink

Thanks Walter,

By changing getters to field access I got rid of warnings as expected.

Alex

"Walter Mundt" <emage@spamcop.net> wrote in message
news:5880315.1148619765602.JavaMail.itn@is.intellij.net...

Nothing wrong about that. IDEA doesn't know that two successive calls to
pv.getPriorVersion() on the same object will return the same value, so it
cannot assume that just because the if statement passed, the call
inside will succeed.



0
Comment actions Permalink

I wonder if it is possible to treat simple getters and setters as fields
when doing Null analysis so subsequent calls to getter without calling
setter or assigning to the field in between considered to return the same
value


"Walter Mundt" <emage@spamcop.net> wrote in message
news:5880315.1148619765602.JavaMail.itn@is.intellij.net...

Nothing wrong about that. IDEA doesn't know that two successive calls to
pv.getPriorVersion() on the same object will return the same value, so it
cannot assume that just because the if statement passed, the call
inside will succeed.



0
Comment actions Permalink

Possible? Yes.

Worth bothering? Hard to say. Keep in mind that the whole logic behind having getters and setters is that one might eventually want to make them do something more elaborate than just operating on a single field. Even though most of the time that never has to happen, perhaps it's a good idea to avoid assuming a getters/setters will remain "simple" when analyzing 'client' code (outside the class).

And, IMHO at least, code inside a class shouldn't use generally getters and setter. Why? Changes to those functions can be transformed into the appropriate changes at the usage-within-class level without expanding the file-scope of the modificaitons necessary. (I apologize if that was not very clear -- it's a somewhat difficult concept to express concisely.)

0

Please sign in to leave a comment.