SonarSource/sonar-dotnet

Fix FP S2589: Assignment for captures are ignored

Opened this issue · 1 comments

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.

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;
}