Fix S2583 FP: Counter variable in foreach loop

[linkdotnet]

Description

When using pre-increment in combination with a boolean expression, S2583 shows a false positive since version 9.8.

Repro steps

Code repro:

private static void Do(IEnumerable<int> records)
{
    var count = 0;
    foreach (var record in records)
    {
        if (++count % 1000 == 0) // FP
        {
            Console.WriteLine(count);
        }
    }
}

Side note: If count++ is used, there is no alert.

Expected behavior

No alert.

Actual behavior

Alert.

Known workarounds

Use post-increment.

Related information

• C#/VB.NET Plugins version
• Visual Studio version
• MSBuild / dotnet version: net8.0
• SonarScanner for .NET version (if used): 9.8
• Operating System: macOS 13

[Tim-Pohlmann]

Thanks for the report. With 9.8, we have migrated S2583 (and S2589) to our new symbolic execution engine. The new engine has way more capabilities, which lets us find way more issues. Unfortunately, this also comes with a few new false positives. In this case, the problem is not so much the increment but the loop. The engine explores loops only two times. count will be 0 and 1, respectively. The condition is always false in both cases. Unfortunately, this is very hard to fix without fundamentally changing how we explore loops.

[linkdotnet]

Hey @Tim-Pohlmann thanks for the swift answer.

Just out of curiosity:

The condition is always false in both cases.

Why doesn't sonar detect an issue if I use the postfix aka count++ in this case?

In bUnit we saw similar behavior (like here)

    [Fact(DisplayName = "Get policy based on name not in the PolicyProvider.")]
    public async Task Test004()
    {
        // arrange
        var provider = new FakeAuthorizationPolicyProvider();
        provider.SetPolicyScheme("TestScheme");

        // act
        var policy = await provider.GetPolicyAsync("OtherPolicy");

        // assert
        Assert.NotNull(policy);
        Assert.Equal(1, policy?.AuthenticationSchemes?.Count);
        Assert.Equal("TestScheme:OtherPolicy", policy?.AuthenticationSchemes?[0]);
        Assert.Equal(1, policy?.Requirements?.Count);
        Assert.IsType<TestPolicyRequirement>(policy?.Requirements?[0]);
    }

Leads to

/home/runner/work/bUnit/bUnit/tests/bunit.web.tests/TestDoubles/Authorization/FakeAuthorizationPolicyProviderTest.cs(66,19): error S2589: Change this expression which always evaluates to the same result. (https://rules.sonarsource.com/csharp/RSPEC-2589)

Guess this is related to the issue reported initially!?

[Tim-Pohlmann]

Why doesn't sonar detect an issue if I use the postfix aka count++ in this case? For count values of 0 and 1:

• count++ evaluates to 0 and 1
• ++count evaluates to 1 and 2 For 0 the condition is actually true => No issue will be raised.

[Tim-Pohlmann]

Assert.NotNull(policy);
Assert.Equal(1, policy?.AuthenticationSchemes?.Count);  // Change this expression which always evaluates to the same result.

This is a true positive. policy cannot be null; thus, the ? is obsolete.

[linkdotnet]

Assert.NotNull(policy); Assert.Equal(1, policy?.AuthenticationSchemes?.Count)

Dahh - yes you are right :D it's obsolete.

[eXpl0it3r]

Another repro with SonarAnalyzer.CSharp 9.12.0.78982:

var i = 0;
foreach (var y in Enumerable.Range(1, 10))
{
    if (i++ != 5)
    {
        Console.WriteLine("Hello World!");
    }
}

[Tim-Pohlmann]

I close this as a duplicate of #8028 to track all issues caused by loop counters not being fully explored in one place.