Method invocation XXX may produce NPE

This inspection usually works pretty but now I got some wrong results.
Ascii art follows :)

@Nullable private RfxStructureNode parent;

public boolean isLast()
{
if(parent==null) return true;
return parent.indexOf(this)+1==parent.getChildCount();
^^^^^^^^^^^^^^^^^^^^^^
}
public RfxNode getPreviousSibling()
{
if(parent==null) return null;
int pos=parent.indexOf(this);
return pos>0 ? parent.getChildren().get(pos-1) : null;
^^^^^^^^^^^^^^^^^^^^
}
public RfxNode getNextSibling()
{
if(parent==null) return null;
int pos=parent.indexOf(this);
return pos+1<parent.getChildCount() ?
^^^^^^^^^^^^^^^^^^^^^^
parent.getChildren().get(pos+1) : null;
^^^^^^^^^^^^^^^^^^^^
}
Occurs in build 3419.

19 comments
Comment actions Permalink

These cases come about essentially because JB are taking into account
the fact that your app might be multi-threaded, and so the field parent
can be legally set to null at any time.

See, e.g. http://www.jetbrains.net/jira/browse/IDEA-2068
R

0
Comment actions Permalink

I can reproduce this in build 3425 and I think it a bug. Have you
submitted a JIRA issue?

Bas

Sven Steiniger wrote:

This inspection usually works pretty but now I got some wrong results.
Ascii art follows :)

@Nullable private RfxStructureNode parent;

public boolean isLast()
{
if(parent==null) return true;
return parent.indexOf(this)+1==parent.getChildCount();
^^^^^^^^^^^^^^^^^^^^^^
}
public RfxNode getPreviousSibling()
{
if(parent==null) return null;
int pos=parent.indexOf(this);
return pos>0 ? parent.getChildren().get(pos-1) : null;
^^^^^^^^^^^^^^^^^^^^
}
public RfxNode getNextSibling()
{
if(parent==null) return null;
int pos=parent.indexOf(this);
return pos+1<parent.getChildCount() ?
^^^^^^^^^^^^^^^^^^^^^^
parent.getChildren().get(pos+1) : null;
^^^^^^^^^^^^^^^^^^^^
}
Occurs in build 3419.

0
Comment actions Permalink

the fact that your app might be multi-threaded

Not only multi-threaded but with current annotation system we can't be sure
calling parent.indexOf(...) will not set parent to null via some chained
calls.

-


Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"


0
Comment actions Permalink

So workaround would be introducing local variable for the parent.
-


Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"


0
Comment actions Permalink

Whoops, I guess I was too tired to see that obviousness there.

Bas

Maxim Shafirov (JetBrains) wrote:
>> the fact that your app might be multi-threaded

Not only multi-threaded but with current annotation system we can't be sure
calling parent.indexOf(...) will not set parent to null via some chained
calls.

-------------------
Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"

0
Comment actions Permalink

!?!! I can accept the reason that parent.indexOf() may change the parent
field. That's ugly, but correct (at least if you don't analyse the
called method :)

But multi-threaded access should not considered at all. In this case any
test would fail. eq. "if(parent!=null) parent.doSomething()" can fail in
multi-thread access.

Maxim Shafirov (JetBrains) schrieb:
>> the fact that your app might be multi-threaded


Not only multi-threaded but with current annotation system we can't be
sure calling parent.indexOf(...) will not set parent to null via some
chained calls.

-------------------
Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"

0
Comment actions Permalink

I disagree about ugliness of workaround. I personally create local
variables for any field which I reference more than once in a method,
and I know others who suggest such style.

Max, maybe in this case the inspection could suggest creating a local
variable for the field. People have posted about this several times.

Maxim Shafirov (JetBrains) wrote:

So workaround would be introducing local variable for the
parent.
-------------------
Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"


0
Comment actions Permalink

Sven Steiniger wrote:

!?!! I can accept the reason that parent.indexOf() may change the parent
field. That's ugly, but correct (at least if you don't analyse the
called method :)

But multi-threaded access should not considered at all. In this case any
test would fail. eq. "if(parent!=null) parent.doSomething()" can fail in
multi-thread access.


That's true, and you should not use that construct, and IDEA should warn
about it, I think.

0
Comment actions Permalink

That's why 'synchronized' has been invented... :)

Keith Lea wrote:

Sven Steiniger wrote:

>> !?!! I can accept the reason that parent.indexOf() may change the parent
>> field. That's ugly, but correct (at least if you don't analyse the
>> called method :)
>>
>> But multi-threaded access should not considered at all. In this case
>> any test would fail. eq. "if(parent!=null) parent.doSomething()" can
>> fail in
>> multi-thread access.


That's true, and you should not use that construct, and IDEA should warn
about it, I think.


--
Martin Fuhrer
Fuhrer Engineering AG
http://www.fuhrer.com

0
Comment actions Permalink


Sadly, that would result in too many false positives. There's plenty of valid use cases for such constructs. For instance, you may know that all instances of a class are used locally to one thread, and thus no synchronization is necessary.

I'm pretty sure that the data-flow inspections don't take multi-threading into account.

--Dave Griffith

0
Comment actions Permalink

Even without multithreading, I think reading that field over and over is
going to be slower than reading it into a local variable once, then
reading the variable over and over.

Dave Griffith wrote:

Sadly, that would result in too many false positives. There's plenty
of valid use cases for such constructs. For instance, you may know
that all instances of a class are used locally to one thread, and
thus no synchronization is necessary.

I'm pretty sure that the data-flow inspections don't take
multi-threading into account.

--Dave Griffith

0
Comment actions Permalink

One of the problems is that adding @Nullable to a field just produces a
huge swathe of yellow in your code. I produced a small hunk of test code
and discovered at least one bug (in the ordinary, unannotated case) - I
would be eager to hear the comments of this thread's participants.
http://www.jetbrains.net/jira/browse/IDEA-4126
R

0
Comment actions Permalink

Quite true, at least for non-final fields, and I agree that multiple reads should usually be replaced by cached local variables. There's actually an inspection for multiple accesses of a field in-product already, under the new "J2ME inspections" category.

--Dave Griffith

0
Comment actions Permalink

Dave Griffith wrote:

Quite true, at least for non-final fields, and I agree that multiple reads should usually be replaced by cached local variables. There's actually an inspection for multiple accesses of a field in-product already, under the new "J2ME inspections" category.


Is it really that slow? I was under the impression that the reason the
volatile keyword exists is because the JVM may locally cache the value
of a field, thus the volatile keyword would force the JVM to fetch the
value every time the field was referenced.

Ciao,
Gordon

--
Gordon Tyler (Software Developer)
Quest Software <http://www.quest.com/>
260 King Street East, Toronto, Ontario M5A 4L5, Canada
Voice: (416) 933-5046 | Fax: (416) 933-5001

0
Comment actions Permalink

Gordon Tyler wrote:

Dave Griffith wrote:

>> Quite true, at least for non-final fields, and I agree that multiple
>> reads should usually be replaced by cached local variables. There's
>> actually an inspection for multiple accesses of a field in-product
>> already, under the new "J2ME inspections" category.


Is it really that slow? I was under the impression that the reason the
volatile keyword exists is because the JVM may locally cache the value
of a field, thus the volatile keyword would force the JVM to fetch the
value every time the field was referenced.


I think performance is one reason not to do it. I don't know if it will
cache it. There are absolutely no semantics for it - it might cache, it
might not. That scares me, so I always use a local variable.

0
Comment actions Permalink


You'd have to profile your apps to be sure if it's worthwhile. The core java classes (particularly in the collections API) copy multiply-accessed fields to locals quite often, so at very least someone at Sun thinks it's worth doing.

In J2ME land, there's also the issue that field accesses result in larger class-size footprints than local-variable accesses. If you're shaving every byte off of a download, that sort of thing can add up.

--Dave Griffith

0
Comment actions Permalink

Dave Griffith wrote:

You'd have to profile your apps to be sure if it's worthwhile. The core java classes (particularly in the collections API) copy multiply-accessed fields to locals quite often, so at very least someone at Sun thinks it's worth doing.


This may sound strange, but I wouldn't hold the JDK library up as a
paragon of code perfection. I've seen some stupid stuff going on in there.

Anyway, I'm not really disagreeing with you, it just seems unnecessary
to me, except perhaps for J2ME like you say. If I were looking to
increase the performance of my application, this would be one of the
last things I would do; algorithm optimizations being the first.

Ciao,
Gordon

--
Gordon Tyler (Software Developer)
Quest Software <http://www.quest.com/>
260 King Street East, Toronto, Ontario M5A 4L5, Canada
Voice: (416) 933-5046 | Fax: (416) 933-5001

0
Comment actions Permalink


Agreed, on all counts. That's why I moved this inspection from "Performance issues" to the considerably more specific "J2ME issues".

--Dave Griffith

0
Comment actions Permalink

I agree that it may be slower and produces smaller classfiles, but
that's not the point. As Gordon already said, optimizing usually starts
with optimizing algorithms. This peephole stuff is only done for very
performance ciritical parts.

I think the main problem is that it's not obvious. I just annotated some
code using @Nullable/@NotNull and IDEA shows some yellow spots which I
simply can't understand. Why? Because I am the coder and know that
indexOf() has no side-effects! I sat there and though what will be wrong
here and didn't get the point. That's why I started this thread.

Under the assumption that any method call may have side-effects (which
are not analysed by IDEA yet), the warnings are ok. But IDEA has to tell
me why! Otherwise many users may just assume it's an false positive.

Sven.

Keith Lea schrieb:

Even without multithreading, I think reading that field over and over is
going to be slower than reading it into a local variable once, then
reading the variable over and over.

Dave Griffith wrote:

>> Sadly, that would result in too many false positives. There's plenty
>> of valid use cases for such constructs. For instance, you may know
>> that all instances of a class are used locally to one thread, and
>> thus no synchronization is necessary.
>>
>> I'm pretty sure that the data-flow inspections don't take
>> multi-threading into account.
>>
>> --Dave Griffith

0

Please sign in to leave a comment.