This Question is Answered

17 Replies Last post: Jul 19, 2013 9:23 AM by Peter_Ivanov  
Peter_Ivanov Newbie 60 posts since
Mar 24, 2008
Currently Being Moderated

Jul 17, 2013 5:50 PM

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?

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 11:59 AM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 12:33 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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.

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 3:01 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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.

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 3:40 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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.

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 4:47 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 5:24 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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.

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 6:41 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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.

Peter Gromov Apprentice 850 posts since
Sep 12, 2002
Currently Being Moderated
Jul 18, 2013 3:43 PM in response to: Peter_Ivanov
Re: Constant conditions inspection became less paranoid in Idea 12 - is it good?

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.

More Like This

  • Retrieving data ...