Constant conditions inspection became less paranoid in Idea 12 - is it good?

Hi,

For ages is was so that such code:

if(a.b != null)
  a.b.foo();

triggered inspection. It used to say "potential NPE", and it was right, beacause a.b could be changed in another thread, or modified be some random code that we executed between these lines.
Naturally, many people were bothered, because even in trivial single-thread code it forced them to introduce safe variables, but we lived with it.

In Idea 12, all these alerts disappeared. It doesnt matter whether you call some methods or your code is not thread-safe, inspection says nothing. And there is no option to enable the old-school behavior.

Was it intended this way? :)

17 comments

Yes, this was intentional, because of too many false alarms. Now it only reports the issues that it's more sure about.

0

Thanks for answer. I still have some questions, though:

1. Is this final? We fear that in some future releases this inspection may get more conservative again, and we will have over 9000 violations by then :) It is uncomfortable to us even more beacause of the fact that this inspection contains numerous "possible NPE" detections, and it would be impossible to turn it off without loosing all of them.
2. Is is possible to tune this somehow, perhaps to introduce more inspection options for some corner cases, so that user could decide whether he wants "safe" or "easy"?
3. What are the cases that this inspection still detects?

In particular, I can conjure up some examples in which inspection detects nothng - would it be useful? Some of our cases may be specific, like

if(a.b != null) {
  call(foo()); // this is acually an prc call
  a.b.bar(); // this is not safe, at least because the control flow was actually broken at previous line, and foo() invocation might have changed the state of "a" itself
}

0

Well, we have some tests ensuring this policy and they are not very likely to be consciously removed :) Some bugs might be found which will have to be fixed. This can introduce some yellow code back, but I hope it won't result over 9000 warnings, and that any new warnings would be justified. And you can always report as bugs any warnings that you consider to be wrong.

There's currently no way of tuning the inspection in this respect. It would be a complication of the settings without a clear justification. It can be added if there's a demand and some convincing use cases.

The current policy is that after a method call, all non-final and non-volatile field references are considered to be in unknown state, so no conditions about them are believed to be constant until IDEA can somehow get to know about their state again (e.g. on reassignment). So constant conditions related to local variables are still fully reported. Final and volatile field analysis is unaffected as well.

0

Well, my code example above is one of those use cases: currently inspecton in Idea 12.1.4 Ultimate does not report this as an error, both when a.b is used to invoke bar() and when it is provided as @NotNull parameter to some other call. It doesn't seem right, and from what you've said I should report this as a bug. And even is this is not considered a bug in common case, some of the calls may break the control flow and therefore make all earlier assumptions possiblly false.
Also, in some code even the very first example from the original post is not safe, and some people would like to configure inspection this way at least in sertain Idea Scope.

BTW,

after a method call, all non-final and non-volatile field references are considered to be in unknown state

Why volatile? :) Is it because they are always considered to be in unknown state?

0

I see. In your examples this warning would make sense if your codebase is abound with such mutator method calls, and there are more of those than innocent equals/get* etc. I'd personally consider them a code smell. Anyway, you can file a request to bring the old behavior back optionally, and if it proves to be needed (e.g. gets enough votes), we could add it (although it's not that simple, especially if one wants to have it only in some parts of the code, not all).

Yes, volatile fields are always in unknown state.

0

Considering my second example, I would be perfectly happy if inspection reported any case where it cannot prove that method call between condition and field usage mutates nothing. Does it make sense for you? :) In simple cases, like trivial final methods or get/equal calls, it could be proved easily enough.
Moreover, your own statement

after a method call, all non-final and non-volatile field references are considered to be in unknown state

fits well with this requirement. But currently there seems to be a bug in inspection, or there is more to this rule than just "any method call". Is it only a method call on an instance itself that invalidates its fields reference state assumption, or any call?

0

Some examples of current behaviour:

final int previousValue = getValue(); setValue(value); final int newValue = getValue(); if (newValue <= previousValue) { //here, inspection states that this is always true

if(a.b != null) {
  a.foo(); // possibly mutates a
  this.bar(); //possibly mutates anything
  Helper.call(/*@NotNull*/ a.b); //here, inspection does not report that call() may receive null parameter
}


Should I file any of this as a bug?

0

Proving that a call mutates nothing is undecidable in general. For final methods, it could be done, but not for equals/hashCode/get* ones (unless they're final as well, or we assume general programmer's sanity, which is not always the case). So in most real-life calls, this heuristic wouldn't help anyway. Even for final methods, this requires a complex analysis involving potentially many source files and performing potentially exponential algorithm on them all. Now IDEA only analyzes the method itself and we have no plans yet of expanding the scope.

Unfortunately it's any method call. People write code with all sorts of complex logic which mutates fields on parametres, on their fiels, etc. Example: http://youtrack.jetbrains.com/issue/IDEA-107604. So the purpose of the change was to accept that and don't warn in cases we're not sure. Do no harm.

0

First code sample looks like a bug, so it would be nice of you to file a request (possibly with a self-contained class code).
Second sample isn't considered a bug by me, because it's not clear if bar mutates anything, so the inspection remains silent. You can still file a request about this, but it'd have to be proven to be demanded.

0

The state of affairs is clear, thank you :)

0

My question seems answered, so now it is simple curiosity that is in play :)

In the code example of the bug above, the mistake was that inspection did not take method calls into account, and thus the invalidation of assumption (challengeSet.id == 0) after the method call did the trick.
In my examples, it seems to work another way around: after any method calls the assumption (a.b != null) should be invalidated, but for some reason it is not. The inspection, it seems, works in defensive manner: this invalidation works only to negate violations, not trigger ones. Is that so?

0

No. All assumptions are invalidated after a method call. If you replaced "!=" with "==", you'd also get no warning (and not "always false").

0

But it is "no assumption" state that should cause warnings in this case, is it not?

// at this point, a.b may be null, because field b is @Nullable in A
if( a.b != null ) {
  @NotNull b1 = a.b; // good, because at this point (a.b != null) assumption is considered true
  foo(); // assumptions are invalidated
  bar(/*@NotNull*/ a.b); // at this point, all we know about a.b is that it is @Nullable, so a warning should be fired, but it is not
}


The only other thing that explains it to me is that @Nullable state of a.b is invalidated as well, but that seems too harsh, because any call in method body would render all @NotNull/@Nullable invormation invalid.

0

It's a special kind of "no assumption" state, which is specifically designed to produce no warnings at all. @Nullable is forgotten as well until you use the field again in comparison or reassign. @NotNull is preserved.

0

This sounds a bit odd to me, because if you have no information on method side-effects and therefore on the fields that could be changed, it seems you will have to discard all information at all - retaining only @NotNull status, according to you. It does not appear to be the case at the moment.
Again, an example (Idea 12.1.4):

public final class InspectionExample {   @Nullable   private Object obj;   @Nullable   private Object obj1;   public static void doIt() {     InspectionExample a = new InspectionExample();     a.doThings();     if (a.obj != null) { // assumption 1       a.doThings(); // invalidation       if (a.obj == null) { // here, there is a "always false" warning, though it was expexted that assumption 1 is invalidated by this point
      }       a.obj1.hashCode(); // here, there is a "potential NPE" warning, so @Nullable status seems to be retained       a.obj.hashCode(); // here, there is no warning, meaning that there are more assumptions retained about a.obj than about a.obj1     }   }   @Nullable   public Object getObj() {     return obj;   }   public void doThings() {     obj = null;   } }


All of this looks like the assumptions survive perfectly well through method calls - any method calls, as there are no deep analisys.

0

Sorry, I didn't realize we're talking about 12.1.4. In fact, in 12.1.5 EAP there were some changes, so in your code sample there's no "always false" warning anymore after "doThings" call. Before that call, IDEA assumed a.obj to be not-null, after the call this assumption was dropped. If it were highlighted as either "always true" or "always false", there'd anyway come somebody complaining that it's invalid in their situation.

As for a.obj1, that warning is retained, because IDEA didn't have any assumptions on it before "doThings" calls and thus there's no assumptions to remove. So the field annotation is used and potential NPE warning is issued, hopefully, useful.

0

My bad, too, I've heard rumors about some relevant changes in EAP :)
Thanks for clarification.

0

Please sign in to leave a comment.