dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

Support BoundUsingLocalDeclarations in IOperation and CFG #32100

Closed chsienki closed 5 years ago

chsienki commented 5 years ago

BoundUsingLocalDeclarations are currently implemented as OperationKind.None, and return the List of local declarations as its children. We should implement this correctly with an Operation node.

We should also support CFG correctly with using declarations. Currently we report the using declaration as OperationKind.None in a single block, and thus don't report the actual underlying block structure generated by lowering.

[jcouv update:] When fixing this, let's also verify IDE behavior got fixed. For example https://github.com/dotnet/roslyn/issues/36502

jcouv commented 5 years ago

Once fixed, please verify this scenario (copied from https://github.com/dotnet/roslyn/issues/33464):

Using-declaration should not trigger RemoveUnusedValues.

I suspect this is a compiler issue, as the AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer relies on the control flow graph to determine how variables are used.

image

image


We should also test ExtractMethod on a using-declaration (tracked by this issue)

yyjdelete commented 5 years ago

Maybe the same as #33464. An quick fix Inline temporary variable should not be provided for the first x1.

using var x1 = File.OpenText("test");
var x2 = x1.ReadToEnd();

Expect: Inline temporary variable should not be provided. Actual:

var x2 = File.OpenText("test").ReadToEnd();

[jcouv update:] Yes. Also we can close this issue when we've fixed/verified the InlineVariable scenario.

mavasani commented 5 years ago

https://github.com/dotnet/roslyn/issues/36502 - another report where IOperation/CFG based analyses in IDE is falling over due to this issue. There are now at least 4 known IDE features that give false results due to this missing support.

jinujoseph commented 5 years ago

also reported here

mavasani commented 5 years ago

Adding more context to @jinujoseph's comment: We have added new CFG/dataflow analysis based dispose analyzers in the IDE in 16.2, and they generate false positives due to this missing support:

using System.Diagnostics;

public class Class1
{
    public void M()
    {
        using Process _ = new Process  // Dispose analyzer flags this object creation as not-disposed.
        {
            StartInfo = new ProcessStartInfo()
        };
    }
}
mavasani commented 5 years ago

I have created https://github.com/dotnet/roslyn/pull/36734 to workaround this issue in couple of IDE analyzers. When the IOperation/CFG support has been added for using declarations, kindly revert the product workarounds in that PR and ensure the added unit tests still pass.

chsienki commented 5 years ago

Fixed with #39125