Bug 401330

Comment 1 Stephan Herrmann 2013-02-20 13:23:17 EST

Let me see if I get your point:

(In reply to comment #0)
> I've been getting NPEs on my @NonNulls.
> 
> One scenario is
> 
> for (X anX : someXs) {
>     anX.doSomething();
>     doSomethingChecked(anX);
> }
> 
> where doSomethingChecked has an @NonNull argument.

So you want a warning re unchecked conversion at that call, right?
But how would you ever see an NPE inside doSomethingChecked if 
anX.doSomething() has been executed beforehand?

 
> Now if I add @NonNull on the iterator I get a warning that this is unsound,
> so the compiler knows that anX is not necessarily null, so why does the
> legacy call suppress the checking.

I'm not 100% sure what you mean by legacy call. Is that the anX.doSomethig()?


> Shouldn't NPE checking be enthusiastic?

sure :)

> I have enabled it, so I want it to
> help me rather than deceive me. Therefore the @NonNull argument on
> doSomethingChecked could propagate back to the anX declaration, provoking a
> warning on the "for".

We wouldn't even need the doSomethingChecked for that.
We could propagate back from all dereferences.
I could implement that anX.doSomething() requires anX to be @NonNull.
The requirement is no weaker than for a @NonNull parameter.
Unfortunately, people would kill me if I raised that kind of warning,
because at the start of migrating to null-safe code, you'd hardly see
a single line without that warning :)

I believe what you want is the final warning to be enabled when you believe
your program is fully annotated. That warning would recognize if the 
type parameter of the collection someXs is un-annotated, reminding
you to fill in that gap - but I'm running ahead of our Java7 present.

Comment 2 Ed Willink 2013-02-20 13:55:05 EST

(In reply to comment #1)
> So you want a warning re unchecked conversion at that call, right?
> But how would you ever see an NPE inside doSomethingChecked if 
> anX.doSomething() has been executed beforehand?

Indeed, but the current  behaviour is that becuase you have Bug 1 silently suppressed, you also get Bug silently suppressed where it should be overt.

> I'm not 100% sure what you mean by legacy call. Is that the anX.doSomethig()?

Yes. My understnading is that you take the view that anything that uses unnannotated functionality is ok.

> Unfortunately, people would kill me if I raised that kind of warning,
> because at the start of migrating to null-safe code, you'd hardly see
> a single line without that warning :)

Very true.

But may be I'm just first in line to kill you for deceiving me in to beleiving that I had more than sporadic checking in my code.

Maybe a "strict" option available to those who think their code is NPE-free.

Comment 3 Stephan Herrmann 2013-02-20 14:14:39 EST

(In reply to comment #2)
> Indeed, but the current  behaviour is that becuase you have Bug 1 silently
> suppressed, you also get Bug silently suppressed where it should be overt.

Here Ecj "thinks" like this:
Since null will explode at anX.doSomething() anyway, there isn't a second bug.
Everytime when doSomethingChecked(anX) is called, anX is nonnull. Promised!
That's the inherent logic behind flow analysis, isn't it?
 
> But may be I'm just first in line to kill you for deceiving me in to
> beleiving that I had more than sporadic checking in my code.

Now I know who to avoid meeting on the street :)
 
> Maybe a "strict" option available to those who think their code is NPE-free.

Eventually you will get that option, but I don't believe that before JSR 308
you'll keep it enabled more the 3 seconds (unless you're masochistic :)..)

So, thanks for the comment, but I don't see what we could improve as of now.
Do you?

Comment 4 Ed Willink 2013-02-20 14:40:46 EST

(In reply to comment #3)
> Here Ecj "thinks" like this:
> Since null will explode at anX.doSomething() anyway, there isn't a second
> bug.
> Everytime when doSomethingChecked(anX) is called, anX is nonnull. Promised!
> That's the inherent logic behind flow analysis, isn't it?

Yes, for flow analysis that doesn't change observable behaviour.

But for new-NPE analysis we have the presumption that there will be no NPEs, so we cannot argue that anX.doSomething() gave us an NPE guard; quite the converse, we should actually require that anX.doSomething() does not give an NPE, which implies that anX is provably non-null!

Comment 5 Stephan Herrmann 2013-02-20 14:49:48 EST

The requirement against anX.doSomething() is sound, but I don't see how
this situation could be distinguished from similar situations where
we just have to admit that we don't know yet...


And I believe we have other changes in the pipeline that make for more
significant progress than this particular request.