Why exactly does it report that a condition is always true?

[David Karr]

I have the following block of code:

public class Foo {
    private Stuff stuff = new Stuff();
    public void method() {
        Bar bar = stuff.makeBar();
        String var = bar.getSomething();
        if (var != null) { // line 1
            System.out.println("Got here.");
        }
        System.out.println("var[" + var.length() + "]");
    }
}

On "line 1", SonarLint says 'Change this condition so that it does not always evaluate to "true"'.  I'm trying to understand exactly it thinks this.

If it matters, here is the current contents of "Bar.java":

import javax.annotation.Nonnull;
//import org.eclipse.jdt.annotation.NonNull;

public class Bar {
    //@NonNull
    @Nonnull
    public String getSomething() {
        return "abc";
    }
}

Notice that I am actually specifying that "getSomething()" always returns non-null.  Under those circumstances, I suppose it's possible that's what SonarLint is seeing.

What's odd is that Eclipse itself is not convinced at this point that "getSomething()" always returns non-null.  That's because I'm using the "other" null annotations package, which apparently Eclipse is not 100% compatible with (I have a current ticket with Eclipse open, where we're discussing this point).

When I convert "Bar" to use the Eclipse annotations, I still get that warning, and Eclipse's warnings also indicate that it believes that "getSomething()" cannot return null.

The fact that SonarLint gives me the warning no matter what makes me wonder whether it's seeing the annotation, or whether something else is making it think this.

[Michael Gumowski]

Hello David,

The "condition always true rule" (squid:S2583) from the SonarJava analyzer is relying on our Symbolic Execution (SE) engine. The purpose of the engine is to explore the graph of the possible states which can occur during the execution of a method, based on its control flow graph (CFG).

I'm going to simplify a bit things to make things clear, but basically, when there is a method invocation in the body of the method, two things can happen:

- The called method is declared within the same file, and the Engine will be able to deduce a behavior of the method, based on exploration of the syntax tree, or annotations.

- The called method is declared in another file, and we (currently) do not try to deduce the full behavior of the method, but we may still learns things from the bytecode.

In any case, in both scenarios, the SE engine will look at the annotations which are used (on return values, on parameters). In the scenario you are describing, the engine deduce, that because the method is annotated with @javax.annotation.Nonnull, the return value CAN NOT be null. Consequently, the engine is able to determine by exploring all the possible states after the method invocation that the next conditions will always be true, and the issue is reported.

Now, if you switch to the other annotations, from the eclipse project, I would expect SonarLint to have the issue related to "condition always true" disappear (at least that's what I expect and observe using SonarJava 4.9 with SonarLint), but another one to appear, saying that a null dereference could be thrown.

This second behavior comes from 2 things:

• We currently do not recognize correctly the eclipse annotations, so we don't consider result of the method invocation as being necessarily non-null. This will be fixed when handling SONARJAVA-1993.
• Knowing that we do not handle the eclipse annotation correctly, the SE engine learn nothing on "var" when calling "bar.getSomething()". However, because of the test "var != null" from the next line, the SE engine learns that the returned value was potentially nullable (why making the test otherwise?). Now, because "var" is nullable, then their is risk of null dereference immediately after the if-statement, because the if statements does not discard the possibility of reaching "var.length()" with "var" being null. Consequently, we raise another issue (squid:S2259).

So to answer your question, in the case you describe, we will rely on annotations.

Hope this helps,

Michael

--
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/893dc722-e885-46f9-97f9-d12fac73dd68%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Michael Gumowski | SonarSource

Software Developer, Language Team
http://sonarsource.com

[David Karr]

On Friday, May 5, 2017 at 2:30:42 AM UTC-7, michael.gumowski wrote:

Hello David,

The "condition always true rule" (squid:S2583) from the SonarJava analyzer is relying on our Symbolic Execution (SE) engine. The purpose of the engine is to explore the graph of the possible states which can occur during the execution of a method, based on its control flow graph (CFG).

The other funny thing about this is that the error message is sort of odd.  The message that Eclipse itself provides for the same deduction addresses the real issue, saying "Redundant null check: The variable var cannot be null at this location". However, if SonarLint said the same thing, it would be hard for me to tell them apart. :)
 

I'm going to simplify a bit things to make things clear, but basically, when there is a method invocation in the body of the method, two things can happen:

- The called method is declared within the same file, and the Engine will be able to deduce a behavior of the method, based on exploration of the syntax tree, or annotations.

- The called method is declared in another file, and we (currently) do not try to deduce the full behavior of the method, but we may still learns things from the bytecode.

In any case, in both scenarios, the SE engine will look at the annotations which are used (on return values, on parameters). In the scenario you are describing, the engine deduce, that because the method is annotated with @javax.annotation.Nonnull, the return value CAN NOT be null. Consequently, the engine is able to determine by exploring all the possible states after the method invocation that the next conditions will always be true, and the issue is reported.

Now, if you switch to the other annotations, from the eclipse project, I would expect SonarLint to have the issue related to "condition always true" disappear (at least that's what I expect and observe using SonarJava 4.9 with SonarLint), but another one to appear, saying that a null dereference could be thrown.

And in my original posting, I said that SonarLint was giving me the same warning, no matter which annotation set I was using.  What I now realize is that Eclipse and SonarLint use different triggers to run their analysis.  When I changed the annotations used in Bar and saved the file, Eclipse appears to run its analysis, changing the warnings shown in in Foo.  However, SonarLint doesn't appear to rerun the analysis in Foo until I make a change to Foo and save it. When I was doing my test, if I changed Bar from the javax annotations to the Eclipse annotations and saved Bar, then went back to Foo, I saw both the Eclipse and SonarLint warning on line 1. When I simply added and deleted a space to Foo and saved it, then the SonarLint warning went away.
 

This second behavior comes from 2 things:

• We currently do not recognize correctly the eclipse annotations, so we don't consider result of the method invocation as being necessarily non-null. This will be fixed when handling SONARJAVA-1993.

The irony is of course that Eclipse checks the Eclipse annotations and SonarLint checks the findbugs/javax annotations, although SonarQube itself (according to that bug report) may eventually support both.

Note that I also filed an Eclipse bug report to explore these discrepancies: https://bugs.eclipse.org/bugs/show_bug.cgi?id=516202 .