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 FP S2589: Assignment for captures are ignored #9204

Open
martin-strecker-sonarsource opened this issue Apr 25, 2024 · 1 comment
Open
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Type: False Positive Rule IS triggered when it shouldn't be.

Comments

@martin-strecker-sonarsource
Copy link
Contributor

Originally reported at https://community.sonarsource.com/t/false-positive-for-s2589/114185

Description

Any mutation of a captured variable is ignored. This applies to captures by delegates and by local functions. See also #8885

Repro steps

public bool ForEachTest(List<string> licenseData)
{
    var found = false;
    licenseData.ForEach(license => found = true); // Assignment in "ForEach"
    if (!found) // Noncompliant FP
    {
        Console.WriteLine("No License for artifact type");
    }
    return found;
}

public bool SelectTest(List<string> licenseData)
{
    var found = false;
    licenseData.Select(license => found = true).Any(); // Assignment in "Select"
    if (!found) // Noncompliant FP
    {
        Console.WriteLine("No License for artifact type");
    }
    return found;
}

public bool ActionTest()
{
    var found = false;
    Action assign = () => found = true; // Assignment in some delegate
    assign();
    if (!found) // Noncompliant FP
    {
        Console.WriteLine("No License for artifact type");
    }
    return found;
}

public bool LocalFunctionTest()
{
    var found = false;
    Assign();
    if (!found) // Noncompliant FP
    {
        Console.WriteLine("No License for artifact type");
    }
    return found;

    void Assign() => found = true; // Assignment in local function
}

Expected behavior

Do not raise on mutated captured variables or learn inside the delegate/local function body.

Actual behavior

FP for S2589

Known workarounds

Do not mutate capture variables.

@martin-strecker-sonarsource martin-strecker-sonarsource 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 25, 2024
@martin-strecker-sonarsource
Copy link
Contributor Author

Possible solutions

Invocation unrolling

See https://trello.com/c/dELDhBru

Ignoring captured and mutated symbols after their capturing

  • Handle FlowAnonymousFunctionOIperation and get the CFG (including nested)
  • Find assignments and mutations of captured symbols (symbols we know about in ProgramState)
  • Then do either
    • Unlearn all constraints (see case below where this is problematic)
    • Remember these symbols as being problematic. The engine stops tracking those. Single rules may need to take care of these "untrustworthy" symbols before reporting because they may use states from before we learned about the capture being problematic.

Ignoring captured and mutated symbols completely

  • Like before but as a pre-processing step before the engine starts for the outer method
public bool SelectTest(List<string> licenseData)
{
    bool found;
    var qry = licenseData.Select(license => found = true)
    found = false;
    qry.Any();  // This evaluates the lambda and maybe sets true. This is hard to predict because the lambda cfg is visited before the found = false
    if (!found) // Noncompliant FP
    {
        Console.WriteLine("No License for artifact type");
    }
    return found;
}

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: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

No branches or pull requests

1 participant