False positive on S2583 in C#

SonarQube for IDE Visual Studio
1

Operating system: Windows 11

  • Visual Studio version: 17.7
  • SonarLint plugin version: 7.3.0.77872
  • Programming language you’re coding in: C#
  • Is connected mode used: Yes
    • Connected to SonarCloud or SonarQube (and which version): SonarQube 10.2.1

In some cases , the rule “S2583 Conditionally executed code should be reachable” gives false positives in the case the if statement is on a variable that could have been changed in a closure.

Example code:

var captured = 0;

Bar(() => captured++);

if (captured == 1)
{
    return 1;
}

static void Bar(Action action)
{
    action();
}

return 0;

In this code, the variable captured get changed, but the rule doe not recognize that, and gives the warning: Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable. It thinks the if statement will evaluate to false, but in reality, it will evaluate to true.

See also the screenshot:

image
image894×338 8.72 KB

Expected: No warning
Actual: Warning S5283

3

Hello @wilfriedb,

Thanks a lot for reporting this, it’s indeed an FP.
Our symbolic execution engine does not explore lambdas so it does not detect that captured can be also not 0.

Here is a GitHub issue for you to follow regarding this.

It’s in our future objectives to plan supporting lambdas for the symbolic execution.

7

I think it is more basic than just about exploring lambdas. This code also gives the S5283.

private bool test()
{
    var exists = true;
    var count = 0;
    while (exists)
    {
        exists = someRoutine();
        count++;
        if (count <= 10) continue;
        return false;
    }

    return true;
}
9

Hello @pvk,

I guess you mean in this line → if (count <= 10) continue;. Am I right? (if not please let me know - at least for me it only raises S2583 there).

If yes, this is a false positive but for another reason.
This stems from how the symbolic execution explores loops.

Our engine explores loops only two times, and it cannot “learn” that count >= 2 can be true.

We already have similar issues to our backlog (for example see here, here and here), where you can read more information.

10

Yes, thanks, that is the issue. In the actual code it runs for 1000 times, so making more iterations exploring the loop is not likely going to be the solution.

11

Unfortunately yes, as having the symbolic execution engine explore more times a loop will result in a huge performance hit in the analysis.

However, we have a scheduled sprint to look into the for-loop exploration with the goal of minimizing the false positives.