False positive (and negative) for Squid:S2589 (Remove this expression that always evaluates to true)

[Paul Wagland]

Hi all,

SonarQube is showing a lot issues, and the vast majority of issues that it reports we tend to fix. That is a testament to a great product!

However, sometimes false positives do pop up :-)

Here is an example of one. I have tried to make it as simple as possible, but anytime that I make the code slightly simpler the false-positive goes away. For example, if I remove gotOption then it correctly detects that gotLow is not always 0. If I change the loop from an Iterator based one to a for-each style loop, it correctly detects that gotLow is not always 0. If I merge the final if condition into a return, then it determines that gotLow is not always 0. Only when the code is as it is below can I make the false positive turn up. Unfortunately in our real code, I think that the if is clearer, and we can't remove gotOption since that is one of the things that we are checking for. Nor can I convert to a for-each loop, since we get an iterator() at the top of the loop, not an Iterable.

The good/bad news is that while trying to simplify the code to show the false positive, I also managed to produce a test case showing a false negative, and that code is much simpler, or at least much shorter ;-)

Anyway, the code in all its glory:

// -------------- ✁ --------------

package org.kungfoocoder.sonarqube.bugs;

import java.util.Arrays;

import java.util.Iterator;

public class SquidS2589Tests {

public boolean testFalsePositiveSquidS2589(final String... params) {

boolean gotSign = false;

boolean gotOption = false;

int gotLow = 0;

int gotHigh = 0;

for (final Iterator<String> it = Arrays.asList(params).iterator(); it

.hasNext();) {

final String sp = it.next();

final String paramName = sp.toUpperCase();

if ("SIGN".equals(paramName)) {

gotSign = true;

} else if ("OPTION".equals(paramName)) {

gotOption = true;

} else if (paramName.indexOf("LOW") >= 0) {

gotLow++;

if (gotLow > 1) {

return false;

}

} else if (paramName.indexOf("HIGH") >= 0) {

gotHigh++;

if (gotHigh > 1) {

return false;

}

}

}

if (!gotSign || !gotOption) {

return false;

}

if (gotHigh > 0 && gotLow == 0) {

//                                 ^^ Remove this expression which always evaluates to "true"

// we can get here if called as:

// testFalsePositiveSquidS2589("SIGN", "OPTION", "HIGH");

// In this case gotHigh == 1 and gotLow == 0

return false;

}

// we can get here if called as:

// testFalsePositiveSquidS2589("SIGN", "OPTION", "HIGH", "LOW");

// In this case gotHigh == 1 and gotLow == 1

return true;

}

public boolean testFalseNegativeSquidS2589(final String... params) {

int gotHigh = 0;

int gotElse = 0;

for (String sp : Arrays.asList(params)) {

if (sp.indexOf("HIGH") >= 0) {

gotHigh++;

if (gotHigh > 1 || gotElse > 0) {

//                                                 ^^ false negative, gotElse is never modified and so cannot be > 0

return false;

}

}

}

return true;

}

}

// -------------- ✃ --------------

Cheers,

Paul

[Tibor Blenessy]

Hello Paul,

which version of SonarJava are you using? I was able to reproduce the behavior you describe with SonarJava 4.10,  good news is that with the latest released version 4.14 false negative you describe is detected, and false positive is removed.

However there is new false positive on the first part of the condition gotHigh > 0 , I need to investigate bit further, it's most likely related to the way loops are interpreted in the symbolic execution engine, which has some limits with more complex control flows.

Can you upgrade to the latest version and confirm my findings? 

Best regards

Tibor

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/11cb390d-c480-4847-afc9-855fa0011046%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Tibor Blenessy | SonarSource

SonarJava Developer

https://sonarsource.com 

[Paul Wagland]

Hi Tibor,

I believe that I tested this with 4.12 or 4.13 (I'll try to add the version next time I submit a report ;-) )

I can confirm your findings, however have updated my test case slightly, as I found a new false negative.

// -------------- ✁ --------------

package org.kungfoocoder.sonarqube.bugs;

import java.util.Arrays;

import java.util.Iterator;

public class FalsePositiveSquidS2589 {

public boolean testFalsePositiveSquidS2589(final String... params) {

boolean gotSign = false;

boolean gotOption = false;

int gotLow = 0;

int gotHigh = 0;

for (final Iterator<String> it = Arrays.asList(params).iterator(); it.hasNext();) {

final String sp = it.next();

final String paramName = sp.toUpperCase();

if ("SIGN".equals(paramName)) {

gotSign = true;

} else if ("OPTION".equals(paramName)) {

gotOption = true;

} else if (paramName.indexOf("LOW") >= 0) {

gotLow++;

if (gotLow > 1) {

return false;

}

} else if (paramName.indexOf("HIGH") >= 0) {

gotHigh++;

if (gotHigh > 1) {

return false;

}

if (gotHigh > 1 && gotLow == 0) {

//                                  ^^ false negative, it was tested a few lines above

return false;

}

}

}

if (!gotSign || !gotOption) {

return false;

}

if (gotHigh > 0 && gotLow == 0) {

//                  ^^ Remove this expression which always evaluates to "true"

// we can get here if called as:

// testFalsePositiveSquidS2589("SIGN", "OPTION", "HIGH");

// In this case gotHigh == 1 and gotLow == 0

return false;

}

// we can get here if called as:

// testFalsePositiveSquidS2589("SIGN", "OPTION", "HIGH", "LOW");

// In this case gotHigh == 1 and gotLow == 1

return true;

}

}

// -------------- ✃ --------------

Cheers,

Paul

[Tibor Blenessy]

Hello Paul,

thanks for confirmation. The new false negative you are seeing has the same root cause, which is limited support for more complex control flows inside loops. Basically inside the loop we visit branches only 2 level deep, and rule S2589 requires all branches to be fully explored to report accurately. I created following ticket to document this limitation SONARJAVA-2523 , we plan to improve the way we handle loops, but this requires bit more effort.

Best regards

Tibor

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/4f254d30-6950-459e-8edc-44b296bb3cd8%40googlegroups.com.

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

[Paul Wagland]

Hi Tibor,

Thanks for the update. Do you have an issue for the "we plan to improve the way we handle loops" that I can follow?

Thanks in advance,

Paul

[Tibor Blenessy]

The plan is still in the making, we have this ticket MMF-902,  but as you can see it's just a placeholder.

Tibor