Bug 436511 - [compiler][null][correlation] Extracted if condition in a local variable wrongly analyzed as potential null pointer access
Summary: [compiler][null][correlation] Extracted if condition in a local variable wron...
Status: CLOSED DUPLICATE of bug 538421
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.2.2   Edit
Hardware: PC Windows 7
: P4 normal (vote)
Target Milestone: 4.7 M1   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2014-06-03 15:56 EDT by Baptiste Mathus CLA
Modified: 2018-08-30 10:27 EDT (History)
6 users (show)

See Also:


Attachments
Example of null analysis simple correlation in IntelliJ IDEA (25.45 KB, image/png)
2014-06-11 06:18 EDT, Baptiste Mathus CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste Mathus CLA 2014-06-03 15:56:19 EDT
Simple example demoing the issue:

  public static void main(String[] args)
  {
    // Calling an external API without @Nonnull or @Nullable information on purpose for the example
    String s = String.format("%s", "hop");
    boolean a = s != null;
    if (a)
    {
      // Leads to "Potential null pointer access: The variable s may be null at this location"
      s.charAt(0);
    }
    if (s != null)
    {
      // OK
      s.charAt(0);
    }
  }

If you need any additional thing, please let me know.

Thanks a lot.

Cheers
Comment 1 Stephan Herrmann CLA 2014-06-03 17:53:16 EDT
This is a direct consequence of the design of null analysis:

(a) At the point where we see "s != null" we assume that the programmer is unsure about the value of s, hence we mark that s could be null.

(b) At the point of the first "s.charAt(0)" we only have the information from (a) (no update on s since then) and hence raise a warning.

You probably expect that the compiler analyses the correlation between variables s and a. Such correlation has been requested many times (search for "[correlation]"), but unfortunately this would be a huge change, and I'm not even sure if this is *possible* with acceptable performance.

Hence we have no plans for implementing such a feature.

Rule of thumb: we are doing flow analysis, but the compiler is not a complete theorem prover that could deduce arbitrary properties from a mixed set of facts.
Comment 2 Baptiste Mathus CLA 2014-06-11 06:10:49 EDT
Thanks for your answer Stephan. Though I understand the design choices you state, I guess this is a bit unfortunate since this is at first sight a very simple case. I feel the possibilities offered by Eclipse here are great (even more through JSR308), but are unfortunately limited by this.

Thanks again
Comment 3 Baptiste Mathus CLA 2014-06-11 06:18:29 EDT
Created attachment 244139 [details]
Example of null analysis simple correlation in IntelliJ IDEA

Example showing that the same thing works in IntelliJ. 
Note I'm just adding that for reference and information, not for any form of finger pointing. I'm already quite happy with what Eclipse provides currently and just think that would be great to consider doing more some day :-).
Thanks.
Comment 4 Stephan Herrmann CLA 2014-06-11 13:18:28 EDT
(In reply to Baptiste Mathus from comment #3)
> Created attachment 244139 [details]
> Example of null analysis simple correlation in IntelliJ IDEA

AFAICT, that's not correlation analysis, yet. It's tracking the value of a single boolean variable. This would actually be feasible with the existing machinery in ECJ: for each variable we have a vector of 4 bits to denote its nullness (to encode values like def.null, pot.null, unknown, pot.unknown/pot.nn etc..). For a boolean variable these bits are unused and could be re-interpreted where 'null' is replace by 'false' and 'nonnull' by 'true'. Not sure if this would be worth the effort. How many boolean *variables* with statically known values do normal programs have?

All this doesn't involve the correlation between values of *different variables*. One of the difficulties of that would be having to track multiple triggers for invalidating the correlation etc. pp. Not to mention the question: which correlation predicates should we track? The number of potentially interesting predicates being infinite ...
Comment 5 Stephan Herrmann CLA 2014-07-17 06:37:05 EDT
no plans to fix this, currently.
Comment 6 Stephan Herrmann CLA 2016-06-28 17:17:07 EDT
Bulk closing all compiler bugs tagged [null][correlation], because we have no plans to add such a feature: it would be a tremendous implementation effort, beyond our current man power, and may be impossible to achieve within the desired performance bounds.

If s.o. has a viable strategy or even implementation for such a feature, I'm all ears.
Comment 7 Mickael Istria CLA 2016-06-29 04:20:37 EDT
(In reply to Stephan Herrmann from comment #6)
> If s.o. has a viable strategy or even implementation for such a feature, I'm
> all ears.

I believe marking it as WONTFIX sends the wrong signal despite this comment: many people interpret WONTFIX as "not even desired", which is not the case here. IMHO, if you want to at least hope someone to consider this issue, it has to remain open, unassigned, and without a target. milestone.
Comment 8 Gautier de SAINT MARTIN LACAZE CLA 2016-06-29 06:02:14 EDT
Maybe we can add the keyword "helpwanted" like some other bugs ?
Comment 9 Stephan Herrmann CLA 2016-06-29 10:52:20 EDT
(In reply to Mickael Istria from comment #7)
> (In reply to Stephan Herrmann from comment #6)
> > If s.o. has a viable strategy or even implementation for such a feature, I'm
> > all ears.
> 
> I believe marking it as WONTFIX sends the wrong signal despite this comment:
> many people interpret WONTFIX as "not even desired", which is not the case
> here. IMHO, if you want to at least hope someone to consider this issue, it
> has to remain open, unassigned, and without a target. milestone.

Frankly, I see no real hope for this issue (other than through a miracle). By saying "helpwanted" I would send the signal that I believe a solution is feasible, which I don't (until proven wrong).

We have loads of open bugs that will see no action within the foreseeable future. How else can we reduce that huge pile, if not by closing those that nobody will work on? Any other ideas how to ever see the bottom of > 2k bugs for just the leaf component JDT/Core?

IMHO, WONTFIX is the most open, most honest answer I can give here.

Anyway, let's not bury this sentence in our discussion:

(In reply to Stephan Herrmann from comment #6)
> If s.o. has a viable strategy or even implementation for such a feature,
> I'm all ears.

:)
Comment 10 Mickael Istria CLA 2016-06-29 11:04:10 EDT
(In reply to Stephan Herrmann from comment #9)
> We have loads of open bugs that will see no action within the foreseeable
> future. How else can we reduce that huge pile, if not by closing those that
> nobody will work on? Any other ideas how to ever see the bottom of > 2k bugs
> for just the leaf component JDT/Core?

Bugs such as this one are valid and the reported task or idea isn't closed nor resolved; I believe focusing on reducing the size of the open bugs bucket leads to erroneous usage of bug status and bad signals sent to the community.
FYI, I joined this discussion after a comment from a sad contributor who had the feeling this idea was thrown away.

> IMHO, WONTFIX is the most open, most honest answer I can give here.

You can say that you won't fix it; it doesn't mean that this bug is a WONTFIX: anyone else is free to try to fix it. The project and this bug have a lifecycle which is not tied to your plans and roadmap (there is causality from your roadmap to the project, but not the other way round).
Setting status to RESOLVED WONTFIX is strictly wrong according to the definion of "resolved" and unless you see what the future is made of, it seems dangerous to say "won't fix" in the name of the project for its whole life.

> Anyway, let's not bury this sentence in our discussion:

If the bug status was remained as "OPEN" then there wouldn't be need for a sentence to have people understand that this is still open for consideration.


I can imagine a way to fix the reported issue without changing the current JDT behavior. We could imagine another static analysis pass dedicated to null check that could remove this warning for example.
I believe this issue deserves more hope than you are putting in it ;)
Comment 11 Aurelien Pupier CLA 2016-06-30 04:40:43 EDT
It seems that SonarLint is able to provide this kind of analysis.
Maybe we can collaborate with them to find a solution inside JDT? Or just point users with this null-check need to SonarLint?


see https://groups.google.com/forum/#!topic/sonarlint/EKF7r0sRXmo
Comment 12 Stephan Herrmann CLA 2016-06-30 05:38:44 EDT
(In reply to Aurelien Pupier from comment #11)
> It seems that SonarLint is able to provide this kind of analysis.
> Maybe we can collaborate with them to find a solution inside JDT? 

When you say "we": are you willing to invest your time in such feature? Would you like to drive such an initiative?
Comment 13 Aurelien Pupier CLA 2016-06-30 05:59:45 EDT
(In reply to Stephan Herrmann from comment #12)
> (In reply to Aurelien Pupier from comment #11)
> > It seems that SonarLint is able to provide this kind of analysis.
> > Maybe we can collaborate with them to find a solution inside JDT? 
> 
> When you say "we": are you willing to invest your time in such feature?
> Would you like to drive such an initiative?

By participating in this discussion and trying to provide ideas to go further, I already invested time in this feature.

You asked previously "If s.o. has a viable strategy or even implementation for such a feature, I'm all ears."
So I provided an idea to find a solution. Maybe it is a bad one, maybe not, at least I tried.

I don't plan to drive this feature myself but I might be able to punctually provide some help.
Comment 14 Stephan Herrmann CLA 2016-06-30 07:24:06 EDT
(In reply to Aurelien Pupier from comment #13)
> I don't plan to drive this feature myself but I might be able to punctually
> provide some help.

That's your decision, and I might say something similar for myself.

With this, this cluster of bugs is in the same state for more than ten years now (see bug 128806). Neither Aurelien nor myself will turn the fate of this issue. If s.o. steps up to drive and lead this feature, that might qualify as the "miracle" I mentioned above :) In that case REOPEN would be the normal and appropriate action. But nobody should "hope" for a solution by the existing JDT team, let's be honest.
Comment 15 Mickael Istria CLA 2016-06-30 08:17:42 EDT
(In reply to Stephan Herrmann from comment #14)
> With this, this cluster of bugs is in the same state for more than ten years
> now (see bug 128806).

You can use search request that do not show too old bugs if you're not personally interested in them: https://bugs.eclipse.org/bugs/buglist.cgi?chfield=%5BBug%20creation%5D&chfieldfrom=2006-06-30&chfieldto=Now&classification=Eclipse&list_id=14696339&product=JDT&query_format=advanced&resolution=---

> Neither Aurelien nor myself will turn the fate of this
> issue. If s.o. steps up to drive and lead this feature, that might qualify
> as the "miracle" I mentioned above :) In that case REOPEN would be the
> normal and appropriate action.

If someone steps up to work on it, they the right flow is to move STATUS from OPEN (to anyone) to ASSIGNED (to someone in particular).
I'll repeat it, but WONTFIX sends wrong signals to the community; if the report is interesting but that there is no-one to work on it, then it's better being left open without assignee nor target milestone. This state is self-explicit and is less disappointing for the reporter.
Comment 16 Stephan Herrmann CLA 2016-06-30 09:05:33 EDT
(In reply to Mickael Istria from comment #15)

Mickael, the two of us won't agree on what's right and what's wrong here. Please see that I have my reasons, too. I informed the corresponding leads to give them a chance to have their say, but I really have to leave this discussion and get back to bug fixing.
Comment 17 Jay Arthanareeswaran CLA 2016-08-03 07:57:56 EDT
Verified for 4.7 M1.
Comment 18 Stephan Herrmann CLA 2018-08-30 10:27:32 EDT
I created a new umbrella RFE outlining what would be needed to address this issue.

*** This bug has been marked as a duplicate of bug 538421 ***