Fun with @Nullable

Hope you'll enjoy this (if you don't like puzzles, please skip this
posting):

in our code we had a class of the following pattern:

class Sample {
private @Nullable Object field;

Sample() (Object _field) {
field = _field;
}

void foo() {
if (field != null) {
field.foo();
field.bar();
}
}
}

The call to bar() is marked as a candidate for NPE.
The easy puzzle for you is to explain the warning and propose a good fix for
it:)

Eugene.


19 comments

No fix needed. This is not a false positive. The call to foo() could reset field to null. The warning is a good indicator that, if you do something squirrelly in foo(), you'll get bitten.

Now, if field was marked "final", then I would hope that the dataflow engine would be smart enough not to give a warning. Similarly, in a perfect world where we could mark foo() as "@Const", it should also not give the "may be null" warning, although it would certainly give other warnings. Something like 'Call to a @Const method with result ignored', at very least.

--Dave Griffith

0


"Dave Griffith" <dave.griffith@cnn.com> wrote in message
news:29647684.1133791693433.JavaMail.javamailuser@localhost...

No fix needed. This is not a false positive. The call to foo() could
reset field to null. The warning is a good indicator that, if you do
something squirrelly in foo(), you'll get bitten.

Indeed it could. However, as a side note, only in presence of cyclic data
structure could this happen.

Now, if field was marked "final", then I would hope that the dataflow
engine would be smart enough not to give a warning.

Yep, this is a fix for this problem:) I wonder if any machinery could handle
this though...

Similarly, in a perfect world where we could mark foo() as "@Const"
--Dave Griffith



We had a heavy thought on @Const, but could not come to a satisfactory
conclusion that would allow at least the PSI getters to be @Const:(

Eugene.


0

Eugene Vigdorchik (JetBrains) wrote:

We had a heavy thought on @Const, but could not come to a satisfactory
conclusion that would allow at least the PSI getters to be @Const:(


How does calling PSI getter change externally visible state?

0

Most of them are lazy and memo-izing, so they change a lot of internal state, without changing anything external. That means that @Const would be difficult/impossible to verify, even though it could easily still be used, as in this example for nullness checking.

--Dave Griffith

0

I think Const would be useful even if it was not verified to be true.

Dave Griffith wrote:

Most of them are lazy and memo-izing, so they change a lot of internal state, without changing anything external. That means that @Const would be difficult/impossible to verify, even though it could easily still be used, as in this example for nullness checking.

--Dave Griffith

0

Still we need a formal definition, don't we?

"Keith Lea" <keith@cs.oswego.edu> wrote in message
news:dn1s4g$khh$1@is.intellij.net...
>I think Const would be useful even if it was not verified to be true.
>

Dave Griffith wrote:

>> Most of them are lazy and memo-izing, so they change a lot of internal
>> state, without changing anything external. That means that @Const would
>> be difficult/impossible to verify, even though it could easily still be
>> used, as in this example for nullness checking.
>>
>> --Dave Griffith


0

I think formal definition could be that executing the method will not
change result of any future call to that object, don't you think?

I see problems with objects which change state of other objects, like this:

class A {
B b;
B getB() { }
String getBName() { return b.getName(); }
}

class B {
String name;
String getName() {return name;}
void setName(String s) {name = s;}
}

Now, getB and getBName are both @Const, but how is IDEA supposed to know
that calling A.getB().setName() will alter state of A?

Maybe we need a new annotation @Leak which would annotate getB. IDEA
would ignore any @Const once it sees a @Leak method has been called, and
will assume any method call could alter any object.

What do you think of this proposal, Dave and Eugene?

Eugene Vigdorchik (JetBrains) wrote:

Still we need a formal definition, don't we?

"Keith Lea" <keith@cs.oswego.edu> wrote in message
news:dn1s4g$khh$1@is.intellij.net...

>>I think Const would be useful even if it was not verified to be true.
>>
>>Dave Griffith wrote:
>>
>>>Most of them are lazy and memo-izing, so they change a lot of internal
>>>state, without changing anything external. That means that @Const would
>>>be difficult/impossible to verify, even though it could easily still be
>>>used, as in this example for nullness checking.
>>>
>>>--Dave Griffith


0

Quite a straight definition IMO, though the problem of verifying this is
clearly computationally undecidable:(
If we agree on lack of verification, then it is probably ok to me.

Eugene.

"Keith Lea" <keith@cs.oswego.edu> wrote in message
news:dn1tao$2sb$1@is.intellij.net...
>I think formal definition could be that executing the method will not
>change result of any future call to that object, don't you think?
>

I see problems with objects which change state of other objects, like
this:

>

class A {
B b;
B getB() { }
String getBName() { return b.getName(); }
}

>

class B {
String name;
String getName() {return name;}
void setName(String s) {name = s;}
}

>

Now, getB and getBName are both @Const, but how is IDEA supposed to know
that calling A.getB().setName() will alter state of A?

>

Maybe we need a new annotation @Leak which would annotate getB. IDEA would
ignore any @Const once it sees a @Leak method has been called, and will
assume any method call could alter any object.

>

What do you think of this proposal, Dave and Eugene?

>

Eugene Vigdorchik (JetBrains) wrote:

>> Still we need a formal definition, don't we?
>>
>> "Keith Lea" <keith@cs.oswego.edu> wrote in message
>> news:dn1s4g$khh$1@is.intellij.net...
>>
>>>I think Const would be useful even if it was not verified to be true.
>>>
>>>Dave Griffith wrote:
>>>
>>>>Most of them are lazy and memo-izing, so they change a lot of internal
>>>>state, without changing anything external. That means that @Const would
>>>>be difficult/impossible to verify, even though it could easily still be
>>>>used, as in this example for nullness checking.
>>>>
>>>>--Dave Griffith
>>
>>

0

There could be small heuristic inspections, like "const method modifies
field." If it's not too complex we could add @Cache annotation for
fields, to make them exempt from the inspection.

Eugene Vigdorchik (JetBrains) wrote:

Quite a straight definition IMO, though the problem of verifying this is
clearly computationally undecidable:(
If we agree on lack of verification, then it is probably ok to me.

Eugene.

"Keith Lea" <keith@cs.oswego.edu> wrote in message
news:dn1tao$2sb$1@is.intellij.net...

>>I think formal definition could be that executing the method will not
>>change result of any future call to that object, don't you think?
>>
>>I see problems with objects which change state of other objects, like
>>this:
>>
>>class A {
>> B b;
>> B getB() { }
>> String getBName() { return b.getName(); }
>>}
>>
>>class B {
>> String name;
>> String getName() {return name;}
>> void setName(String s) {name = s;}
>>}
>>
>>Now, getB and getBName are both @Const, but how is IDEA supposed to know
>>that calling A.getB().setName() will alter state of A?
>>
>>Maybe we need a new annotation @Leak which would annotate getB. IDEA would
>>ignore any @Const once it sees a @Leak method has been called, and will
>>assume any method call could alter any object.
>>
>>What do you think of this proposal, Dave and Eugene?
>>
>>Eugene Vigdorchik (JetBrains) wrote:
>>
>>>Still we need a formal definition, don't we?
>>>
>>>"Keith Lea" <keith@cs.oswego.edu> wrote in message
>>>news:dn1s4g$khh$1@is.intellij.net...
>>>
>>>
>>>>I think Const would be useful even if it was not verified to be true.
>>>>
>>>>Dave Griffith wrote:
>>>>
>>>>
>>>>>Most of them are lazy and memo-izing, so they change a lot of internal
>>>>>state, without changing anything external. That means that @Const would
>>>>>be difficult/impossible to verify, even though it could easily still be
>>>>>used, as in this example for nullness checking.
>>>>>
>>>>>--Dave Griffith
>>>
>>>

0

Additionally, a good heuristic for un-annotated methods would be that method calls that return void could be assumed to be non-const, and other methods assumed const. Special cases for the collections methods that should return void, but don't.

Perfection is probably unacheivable along these lines, but a lot of power could probably be gotten very quickly. Particularly since a lot of @Const methods can be clearly proven const (they are just getters, or small stateless calculations, or only modify locally created collections which do not escape).

--Dave Griffith

0

Keith Lea wrote:

If it's not too complex we could add @Cache annotation for
fields, to make them exempt from the inspection.


Mutable :), as in C++.


--
Best regards,
Maxim Mossienko
IntelliJ Labs / JetBrains Inc.
http://www.intellij.com
"Develop with pleasure!"

0

Hello Keith,

That's all good but doesn't solve an original puzzle. Call to foo() can indeed
be state changing. It's just doesn't mean it changes value of the annotated
field in far the most cases.

-


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

I think formal definition could be that executing the method will not
change result of any future call to that object, don't you think?

I see problems with objects which change state of other objects, like
this:

class A {
B b;
B getB() { }
String getBName() { return b.getName(); }
}
class B {
String name;
String getName() {return name;}
void setName(String s) {name = s;}
}
Now, getB and getBName are both @Const, but how is IDEA supposed to
know that calling A.getB().setName() will alter state of A?

Maybe we need a new annotation @Leak which would annotate getB. IDEA
would ignore any @Const once it sees a @Leak method has been called,
and will assume any method call could alter any object.

What do you think of this proposal, Dave and Eugene?

Eugene Vigdorchik (JetBrains) wrote:

>> Still we need a formal definition, don't we?
>>
>> "Keith Lea" <keith@cs.oswego.edu> wrote in message
>> news:dn1s4g$khh$1@is.intellij.net...
>>
>>> I think Const would be useful even if it was not verified to be
>>> true.
>>>
>>> Dave Griffith wrote:
>>>
>>>> Most of them are lazy and memo-izing, so they change a lot of
>>>> internal state, without changing anything external. That means
>>>> that @Const would be difficult/impossible to verify, even though it
>>>> could easily still be used, as in this example for nullness
>>>> checking.
>>>>
>>>> --Dave Griffith
>>>>


0

I think const is a good start. But you're right it doesn't solve the
puzzle. Maybe in another year as CPU gets cheaper, IDEA can afford to
start statically analyzing all classes, and determining in a vague,
unreliable way, which methods modify which fields.

Maxim Shafirov (JetBrains) wrote:

Hello Keith,

That's all good but doesn't solve an original puzzle. Call to foo() can
indeed be state changing. It's just doesn't mean it changes value of the
annotated field in far the most cases.

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

>> I think formal definition could be that executing the method will not
>> change result of any future call to that object, don't you think?
>>
>> I see problems with objects which change state of other objects, like
>> this:
>>
>> class A {
>> B b;
>> B getB() { }
>> String getBName() { return b.getName(); }
>> }
>> class B {
>> String name;
>> String getName() {return name;}
>> void setName(String s) {name = s;}
>> }
>> Now, getB and getBName are both @Const, but how is IDEA supposed to
>> know that calling A.getB().setName() will alter state of A?
>>
>> Maybe we need a new annotation @Leak which would annotate getB. IDEA
>> would ignore any @Const once it sees a @Leak method has been called,
>> and will assume any method call could alter any object.
>>
>> What do you think of this proposal, Dave and Eugene?
>>
>> Eugene Vigdorchik (JetBrains) wrote:
>>
>>> Still we need a formal definition, don't we?
>>>
>>> "Keith Lea" <keith@cs.oswego.edu> wrote in message
>>> news:dn1s4g$khh$1@is.intellij.net...
>>>
>>>> I think Const would be useful even if it was not verified to be
>>>> true.
>>>>
>>>> Dave Griffith wrote:
>>>>
>>>>> Most of them are lazy and memo-izing, so they change a lot of
>>>>> internal state, without changing anything external. That means
>>>>> that @Const would be difficult/impossible to verify, even though it
>>>>> could easily still be used, as in this example for nullness
>>>>> checking.
>>>>>
>>>>> --Dave Griffith
>>>>>


0

If we could provide some way to express our pssible knowledge of the fact
that the object is not a part of a cycle, then the original warning could be
eliminated.


"Keith Lea" <keith@cs.oswego.edu> wrote in message
news:dn2486$6h6$1@is.intellij.net...

I think const is a good start. But you're right it doesn't solve the
puzzle. Maybe in another year as CPU gets cheaper, IDEA can afford to
start statically analyzing all classes, and determining in a vague,
unreliable way, which methods modify which fields.

>

Maxim Shafirov (JetBrains) wrote:

Hello Keith,

>

That's all good but doesn't solve an original puzzle. Call to foo() can
indeed be state changing. It's just doesn't mean it changes value of the
annotated field in far the most cases.

>

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

>
>> I think formal definition could be that executing the method will not
>> change result of any future call to that object, don't you think?
>>
>> I see problems with objects which change state of other objects, like
>> this:
>>
>> class A {
>> B b;
>> B getB() { }
>> String getBName() { return b.getName(); }
>> }
>> class B {
>> String name;
>> String getName() {return name;}
>> void setName(String s) {name = s;}
>> }
>> Now, getB and getBName are both @Const, but how is IDEA supposed to
>> know that calling A.getB().setName() will alter state of A?
>>
>> Maybe we need a new annotation @Leak which would annotate getB. IDEA
>> would ignore any @Const once it sees a @Leak method has been called,
>> and will assume any method call could alter any object.
>>
>> What do you think of this proposal, Dave and Eugene?
>>
>> Eugene Vigdorchik (JetBrains) wrote:
>>
>>> Still we need a formal definition, don't we?
>>>
>>> "Keith Lea" <keith@cs.oswego.edu> wrote in message
>>> news:dn1s4g$khh$1@is.intellij.net...
>>>
>>>> I think Const would be useful even if it was not verified to be
>>>> true.
>>>>
>>>> Dave Griffith wrote:
>>>>
>>>>> Most of them are lazy and memo-izing, so they change a lot of
>>>>> internal state, without changing anything external. That means
>>>>> that @Const would be difficult/impossible to verify, even though it
>>>>> could easily still be used, as in this example for nullness
>>>>> checking.
>>>>>
>>>>> --Dave Griffith
>>>>>
>
>



0


In any case you'll have to keep threading issues in mind but that's another
long story. I remember Doug came up with thread safety annotation ideas,
which might work perfectly in line with what we're talking about here.

-


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

If we could provide some way to express our pssible knowledge of the
fact that the object is not a part of a cycle, then the original
warning could be eliminated.

"Keith Lea" <keith@cs.oswego.edu> wrote in message
news:dn2486$6h6$1@is.intellij.net...

>> I think const is a good start. But you're right it doesn't solve the
>> puzzle. Maybe in another year as CPU gets cheaper, IDEA can afford to
>> start statically analyzing all classes, and determining in a vague,
>> unreliable way, which methods modify which fields.
>>
>> Maxim Shafirov (JetBrains) wrote:
>>
>>> Hello Keith,
>>>
>>> That's all good but doesn't solve an original puzzle. Call to foo()
>>> can indeed be state changing. It's just doesn't mean it changes
>>> value of the annotated field in far the most cases.
>>>
>>> -


>>> Maxim Shafirov
>>> JetBrains, Inc
>>> http://www.jetbrains.com
>>> "Develop with pleasure!"
>>>> I think formal definition could be that executing the method will
>>>> not change result of any future call to that object, don't you
>>>> think?
>>>>
>>>> I see problems with objects which change state of other objects,
>>>> like this:
>>>>
>>>> class A {
>>>> B b;
>>>> B getB() { }
>>>> String getBName() { return b.getName(); }
>>>> }
>>>> class B {
>>>> String name;
>>>> String getName() {return name;}
>>>> void setName(String s) {name = s;}
>>>> }
>>>> Now, getB and getBName are both @Const, but how is IDEA supposed to
>>>> know that calling A.getB().setName() will alter state of A?
>>>> Maybe we need a new annotation @Leak which would annotate getB.
>>>> IDEA would ignore any @Const once it sees a @Leak method has been
>>>> called, and will assume any method call could alter any object.
>>>>
>>>> What do you think of this proposal, Dave and Eugene?
>>>>
>>>> Eugene Vigdorchik (JetBrains) wrote:
>>>>
>>>>> Still we need a formal definition, don't we?
>>>>>
>>>>> "Keith Lea" <keith@cs.oswego.edu> wrote in message
>>>>> news:dn1s4g$khh$1@is.intellij.net...
>>>>>
>>>>>> I think Const would be useful even if it was not verified to be
>>>>>> true.
>>>>>>
>>>>>> Dave Griffith wrote:
>>>>>>
>>>>>>> Most of them are lazy and memo-izing, so they change a lot of
>>>>>>> internal state, without changing anything external. That means
>>>>>>> that @Const would be difficult/impossible to verify, even though
>>>>>>> it could easily still be used, as in this example for nullness
>>>>>>> checking.
>>>>>>>
>>>>>>> --Dave Griffith
>>>>>>>


0

As I understand it, the FindBugs people are going to be working on detectors for the Doug Lea et. al concurrency annotations. I'd love to do the same for InspectionGadgets, when they are publicly available.

--Dave Griffith

0


"Dave Griffith" <dave.griffith@cnn.com> wrote in message
news:20279704.1133814764062.JavaMail.javamailuser@localhost...

As I understand it, the FindBugs people are going to be working on
detectors for the Doug Lea et. al concurrency annotations. I'd love to do
the same for InspectionGadgets, when they are publicly available.


BTW, do you know if they have a paper on those concurrency annotations? I'd
love to read it if they had.


0

Sorry, but can someone explained how field can be modified in foo method called from not null instance?

0

I can reproduce only in this form, but this one - is not very good code, is not it?

0

Please sign in to leave a comment.