Fix S2583 FP: Change this condition so that it does not always evaluate to 'True'

[marco-carvalho]

This code is giving me Change this condition so that it does not always evaluate to 'True'. (S2589) at if (t.Any()):

using System;
using System.Collections.Generic;
using System.Linq;

List<int> t = new();
t.AddRange([]);
if (t.Any()) // Change this condition so that it does not always evaluate to 'True'.[S2589]
{
    Console.WriteLine(1);
}

But when I run it, it doesn't print 1, but Sonar thinks this code should change to not always evaluate to 'True'.

[jilles-sg]

The condition always evaluates to false, so this is not really an FP but an incorrect explanation.

This kind of report is highly dangerous if developers apply it without checking, though.

[marco-carvalho]

I believe this actually is a false positive. The issue comes from Sonar's static analysis assuming that the if (t.Any()) condition will always evaluate to true. However, since we're adding an empty array to the list with t.AddRange([]), at runtime, the list remains empty, and t.Any() evaluates to false.

So, the runtime behaviour is correct, but Sonar's flagging it as though the condition is always true, which is why I’m classifying this as a false positive. It seems like the static analysis isn’t catching the fact that nothing’s being added to the list.

[marco-carvalho]

This also happens if I use a method:

using System;
using System.Collections.Generic;
using System.Linq;

List<int> t = new();
t.AddRange(ReturnsEmpty());
if (t.Any()) // Change this condition so that it does not always evaluate to 'True'.[S2589]
{
    Console.WriteLine(1);
}

List<int> ReturnsEmpty()
{
    return [];
}

[pavel-mikula-sonarsource]

Hi,

Thank you for reporting this, I confirm it is unexpected behavior.

It's hard to tell if it should be called "False Positive" or not, because this S2583 will be replaced with S2583 with a different message, as the condition is always False and the subsequent code is never executed :)

ToDo:

Check if the engine supports [] collection expressions in all scenarios:

• [] => empty
• [ 42 ] => non-empty
• [ ... values ] => propagate (by aggregating non-empty)
• [ ... a, ... b] => aggregate non-empty We need to check this for arrays and lists separately, as the CFG they might differ.

When processing AddRange, take a look at the collection constraint for the argument. When the argument is

• Known to be empty => Do not modify collection constraint
• Known to be not-empty => Set non-empty constraint on the collection
• Be unknown => Remove collection constraint

[pavel-mikula-sonarsource]

There are going to be FNs in S4158 due to this too. The root cause is #8539

[marco-carvalho]

This also happens without collection expressions:

using System;
using System.Collections.Generic;
using System.Linq;

List<int> t = new();
t.AddRange(ReturnsEmpty());
if (t.Any()) // Change this condition so that it does not always evaluate to 'True'.[S2589]
{
    Console.WriteLine(1);
}

List<int> ReturnsEmpty()
{
    return new List<int>();
}

[pavel-mikula-sonarsource]

We don't support cross-procedure analysis yet with these rules

[Tim-Pohlmann]

The root cause is not so much the collection expression (though, that might cause some issues as well) but mainly how the engine treats AddRange. See #8570. This is fixed but not released yet.

[marco-carvalho]

There is a expected date for a new release? Right now this rule triggers "bug" reports, and we're having to manually ignore new cases like this.

[Tim-Pohlmann]

Unfortunately, there is no expected date for the next release.