SonarSource
Fix S3966 FN: Preserve captures in Finally to support using on existing symbol
The Finally block transition loses captures, because it's leaving that block.
We need to preserve them and release them after we leave the finally.
This should fix S3966 FN on the new engine.
class Program
{
public void Disposed_Using_WithExpressions()
{
var d = new Disposable();
using (d) // FN
{
d.Dispose();
}
}
}
public class Disposable : IDisposable
{
public void Dispose() { }
}
As part of this, we also need to make sure that FlowReferenceCaptureOperation can track the state from the operation AND the symbol, if present.
If we'd make FlowCaptureReference and update the symbol releasing as it was in #7351, this could be fixed to support
using (d) // FN
and
using (s ??= new Disposable()) // FN
{
s.Dispose();
}
The problem is that changes in releasing state break too many UTs and desired behaviors, because LVA is not smart enough around flow reference captures and try/catch/finally situations.
@pavel-mikula-sonarsource will this be easier now that LVA supports FlowCapture operations?
This behavior has other unpleasant side-effects like unnecessary branching in foreach loops:
try
{
foreach (string element in collection) // can throw if the collection is null or if it gets modified
{
}
}
catch
{
}
The above gets lowered to
try
{
IEnumerator<string> enumerator = collection.GetEnumerator();
try
{
while (enumerator.MoveNext())
{
string current = enumerator.Current;
}
}
finally
{
if (enumerator != null)
{
enumerator.Dispose();
}
}
}
catch
{
}
When leaving the try region (without an exception) the engine learns that the FlowCapture capturing enumerator is NotNull but it loses that information when entering the finally region which causes unnecessary branching on the if condition.
Isn't this issue about not loosing that information on finally?
Yes, we have unnecessary branching, because we lose information that we should persist.
Yes, that's exactly what the original issue says that we should fix.