Fix FP S2589: Assignment for captures are ignored
Opened this issue · 1 comments
martin-strecker-sonarsource commented
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 commented
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;
}