SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
771 stars 226 forks source link

Fix S2583 FP: Warns about captured variable changed in local function #8885

Open kevinoid opened 6 months ago

kevinoid commented 6 months ago

Description

S2589 warns that a variable which is only assigned in a local function always evaluates to the same value.

Repro steps

using System;
using System.Timers;

public static class Program
{
    public static int Main(string[] args)
    {
        bool elapsed = false;
        void OnElapsed(object? source, ElapsedEventArgs e)
        {
            elapsed = true;
        }

        var timer = new Timer(2000);
        timer.Elapsed += OnElapsed;
        timer.Enabled = true;

        while (!elapsed)
        {
            Console.WriteLine("Timer not elapsed.");
            System.Threading.Thread.Sleep(500);
        }

        Console.WriteLine("Timer elapsed!");
        return 0;
    }
}

Expected behavior

S2589 is not produced.

Actual behavior

/path/to/Program.cs(18,17): warning S2583: Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.

Known workarounds

The warning can be suppressed.

Related information

cristian-ambrosini-sonarsource commented 6 months ago

Hi @kevinoid! I confirm this as a FP. At this stage, our Symbolic Execution engine is not tracking events nor delegates, resulting in this kind of false positives. We don't have a specific timeline for this issue to be fixed but I'll add it to our backlog. Thanks!

jilles-sg commented 5 months ago

@cristian-ambrosini-sonarsource I have disabled this rule in our configuration because I think the risk is too high that someone removes a condition that the rule says is redundant but is actually required. It would be better if the rule does not report anything if the affected code is too hard to analyze properly.

It's sad because this kind of rule would be very useful if it did not cause false positives (some false negatives are acceptable).

By the way, I consider the reproducer to have a threading bug (unsynchronized access to elapsed).