Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix S2589 FP: When local is assigned in for loop #7088

Closed
drieseng opened this issue Apr 15, 2023 · 4 comments · Fixed by #8825
Closed

Fix S2589 FP: When local is assigned in for loop #7088

drieseng opened this issue Apr 15, 2023 · 4 comments · Fixed by #8825
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Type: CFG/SE FPs Rule IS triggered when it shouldn't be for CFG and SE rules.
Milestone

Comments

@drieseng
Copy link

Description

When a local variable is initialized in a for loop, then S2589 reports a false positive if you check whether the local variable has a value.

Repro steps

Compile the following code:

#nullable enable

using System;
using System.Text;

public sealed class Program
{
    private static void Main(string[] args)
    {
        int? previous = null;
        StringBuilder? sb = null;

        for (var i = 0; i < 5; i++)
        {
            if (previous == null)
            {
                previous = 667;
                continue;
            }

            if (sb is null)
            {
                sb = new StringBuilder();
            }

            sb.Append(i);
        }

        if (sb is null)
        {
            Console.WriteLine("NULL");
        }
    }
}

Expected behavior

S2589 is not reported.

Actual behavior

S2589 is reported for both line 21 and 29.

Related information

  • SonarAnalyzer.CSharp version: 8.55.0.65544
  • Visual Studio version: 17.5.3
@Tim-Pohlmann
Copy link
Contributor

Hello, I confirm this as an FP. Thank you for the report!

@Tim-Pohlmann Tim-Pohlmann added Type: False Positive Rule IS triggered when it shouldn't be. Area: CFG/SE CFG and SE related issues. Area: C# C# rules related issues. labels Apr 17, 2023
Tim-Pohlmann added a commit that referenced this issue Apr 19, 2023
@Tim-Pohlmann Tim-Pohlmann changed the title S2589 FP when local is assigned in for loop Fix S2589 FP: When local is assigned in for loop Apr 19, 2023
Tim-Pohlmann added a commit that referenced this issue Apr 19, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.7 milestone Jul 24, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource modified the milestones: 9.7, 9.8 Aug 4, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource added Type: CFG/SE FPs Rule IS triggered when it shouldn't be for CFG and SE rules. and removed Type: False Positive Rule IS triggered when it shouldn't be. labels Aug 15, 2023
@Tim-Pohlmann Tim-Pohlmann removed this from the 9.8 milestone Aug 21, 2023
@Tim-Pohlmann
Copy link
Contributor

For future reference: To fix this, our SE engine would need to iterate the loop at least three times. Right now it is only doing it twice.

@Norvand
Copy link

Norvand commented Feb 21, 2024

@Tim-Pohlmann I have a similar issue. Is this the same case?

static void Main(string[] args)
{
    int first = 0;
    int last;

    int row = 0;
    while (true)
    {
        if (row > 5 && first == 0) // first == 0: Warning S2589
        {
            first = row;
        }

        if (row > 10)
        {
            last = row;
            break;
        }

        row++;
    }

    Console.WriteLine($"{first} : {last}");
    Console.ReadKey();
}

Related information

  • SonarAnalyzer.CSharp version: 9.20.0.85982
  • Visual Studio version: 17.9.0
  • .Net Framework 4.7

@pavel-mikula-sonarsource
Copy link
Contributor

pavel-mikula-sonarsource commented Feb 22, 2024

@Norvand Yes, it looks like the same root cause. Thank you for providing one more reproducer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Type: CFG/SE FPs Rule IS triggered when it shouldn't be for CFG and SE rules.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants