SonarLint does not detect NullPointerException problems in Java (regression?)

[Zoltán Csorba]

I'm using   SonarLint for Eclipse    3.1.0.201705051000
With Eclipse  Version: Mars.2 Release (4.5.2)

I've attached two files with sample code, that should cause sonar warnings.

First problem: map.get(key) may return a null value.

    protected void nullPointerProblemWithMapNOTDetectedBySonar() {
        Map<String, String> someMap = new HashMap<>();
        Object valueFound = someMap.get(SOME_KEY);
        LOG.debug("Value is empty: "+valueFound.toString());  // NOT CORRECT: valueFound can be null
    }

In the next case, Sonar should report at least the "always false" condition, or detect that the variable is used inconsistently:

    protected void nullPointerProblemNotDetectedBySonar() {
        Map<String, String> someMap = new HashMap<>();
        someMap.put(KEY, VALUE);
        String valueFound = someMap.get(SOME_KEY);
        if (testValue != null && valueFound.equals(testValue)) {  // NOT CORRECT: possible NPE error should be reported here
            LOG.debug("EQUALS");
        } else {
            LOG.debug("NOT EQUALS");
        }
        if (valueFound == null) {  // NOT CORRECT: condition always false, as it was already used as non-null value
            LOG.debug("Map entry is missing or null!");
        }
    }

In contradiction, the following case was detected, although it is almost the same:

    protected void nullPointerProblemWithVariableNotDetectedBySonar() {
        String valueFound = nullValue;
        if (valueFound.equals(testValue)) { // NOT CORRECT: possible NPE error should be reported here
            LOG.debug("EQUALS");
        } else {
            LOG.debug("NOT EQUALS");
        }
        if (valueFound == null) { // CORRECT?: condition always false, as it was already used as non-null value
            LOG.debug("Map entry is missing or null!");
        }
    }

I think this could be a regression issue, as previously these type of problems has been detected by Sonar (but I'm not 100% sure).

[Matthias Kolley]

I'm using
Eclipse for RCP and RAP Developers

Version: Neon.3 Release (4.6.3)
Build id: 20170314-1500

with SonarLint 3.1.0.201705051000

and I reproduced the

private final String nullValue = null;
    private String            testValue    = "not found";

    protected void checkNull() {
        final String valueFound = nullValue;
        if (valueFound.equals(testValue)) {
            //do somthing
        }
    }

And I am getting a major issue from sonarlint:

[Zoltán Csorba]

Hi Matthias,
  thanks for the reply.
As I see, there is an important difference between the sample sources.
In your case, the nullValue field is final, in my case, it is not, just it was not set anywhere else.

If I use the final modifier, then I get the same sonar warnings as you.

Still, I think the main problem is, that Sonar does not find the inconsistency of the null checks.

Regards,
  Zoltán

[Matthias Kolley]

That is absolutely correct.
But imagine the following situation:

public class Example {

    private String    nullValue    = null;

    private String    testValue    = "not found";

    protected void checkNull() {
        final String valueFound = nullValue;
        if (valueFound.equals(testValue)) {

            //do something
        }
    }

    protected void changeValues() {
        nullValue = "";

        testValue = "";
    }
}

How should sonarlint find out wether the nullValue is really null?
Sonarlint cant know wether "changeValue()" was called before or not.

[Csorba Zoltán]

Exactly, but Sonarlint could detect the problem, that the value can be null.

A similar case is detected when the method that can return a null value is not checked. In this case, SonarLint reports the possibility of a NullPointerException.

--
You received this message because you are subscribed to a topic in the Google Groups "SonarLint" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarlint/03C1fjq0FmI/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sonarlint+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarlint/1110b7c4-3b0d-4f7c-bb35-b356c13861d2%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Zoltán Csorba]

Summarizing my opinion:

1. There is an inconsistency in possible null checks. Sonar can recognize if a method may return null value that was not checked, but it does not report a problem on the same if you get the value from a Map, that can be null.
2. Using an object first and check if null afterward should always be reported as a problem. In the second example of the original post, it does not happen. See below: valueFound.equals(testValue) in 5th row, then valueFound == null in 10th row.
   

    protected void nullPointerProblemNotDetectedBySonar() {
        Map<String, String> someMap = new HashMap<>();
        someMap.put(KEY, VALUE);
        String valueFound = someMap.get(SOME_KEY);
        if (testValue != null && valueFound.equals(testValue)) {  // NOT CORRECT: possible NPE error should be reported here
            LOG.debug("EQUALS");
        } else {
            LOG.debug("NOT EQUALS");
        }
        if (valueFound == null) {  // NOT CORRECT: condition always false, as it was already used as non-null value
            LOG.debug("Map entry is missing or null!");
        }
    }

But maybe I'm wrong.
Any ideas or disagreement?

[Nicolas Peru]

Hi, 

Just a small explanation before answering you precisely : dataflow engine of SonarJava (used by SonarLint) consider values can be NULL, NOT-NULL or : we don't know. In order to avoid false positives we raise issues if and only if you dereference a NULL value. 

1- If I understand correctly what you are suggesting is pretty simple : consider that get method returns value that could be null. As of today we try to avoid hardcoded behaviors like this and rely on annotations @CheckForNull and @Nullable on methods (and Map.get method is not annotated).

2- Then when a value is used a non-null way it is considered as NonNull like you suggest. However in your example there is one path to reach the last if without having dereferenced "valueFound" : don't forget && is short circuit so if testValue is null, then we have no idea about the nullness of "valuefound". So we can't say anything on the last condition. 

Note one thing : the cases you are mentioning are false negative. We are far more keen on having false negative over false positive as we are aiming to raise a tool which provides valuable feedback with the smallest noise possible.

HTH, 

Cheers, 

--
You received this message because you are subscribed to the Google Groups "SonarLint" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarlint+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarlint/4fca4f7f-9198-4a01-9c45-25646691ccfd%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Nicolas Peru | SonarSource

Senior Developer

http://twitter.com/benzonico

http://sonarsource.com

[Csorba Zoltán]

Hi Nicolas,

  thank you for the clarification. It definitely helped me to understand the reasons behind. I can accept that this is the good approach, and it is better to avoid false positives and lower the amount of "noise".

I take this as a resolution for my question.

Regards,

  Zoltán

To unsubscribe from this group and stop receiving emails from it, send an email to sonarlint+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarlint/4fca4f7f-9198-4a01-9c45-25646691ccfd%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.