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.08k stars 4.04k forks source link

IDE0059 "Use discard" changes semantics with "using" #36502

Closed stephentoub closed 5 years ago

stephentoub commented 5 years ago

Version Used: 3.2.0-beta3-19307-02+a87b8ee11cb50f233d288cf86b99e315a408afa4

Steps to Reproduce:

  1. Use this program
    
    using System;

class Program { static void Main() { using var write = new WriteOnDispose(); Console.WriteLine("almost done"); }

class WriteOnDispose : IDisposable
{
    public void Dispose() => Console.WriteLine("done");
}

}


2. Refactor:
![image](https://user-images.githubusercontent.com/2642209/59606591-88e00400-90df-11e9-97cf-4a80268787fc.png)

3. See the resulting program:
```C#
using System;

class Program
{
    static void Main()
    {
        _ = new WriteOnDispose();
        Console.WriteLine("almost done");
    }

    class WriteOnDispose : IDisposable
    {
        public void Dispose() => Console.WriteLine("done");
    }
}

Expected Behavior: Either the refactoring shouldn't be offered, or it should be done in a way that doesn't remove the using. After refactoring the program should still output:

almost done
done

Actual Behavior: The using gets removed, which means the disposable's Dispose method is never invoked. The refactored program only outputs:

almost done
CyrusNajmabadi commented 5 years ago

Tagging @mavasani @jasonmalinowski . Looks like incomplete testing on the new using-local-variable feature. Was the IDE checklist not updated?

mavasani commented 5 years ago

Duplicate of #32100 - @chsienki given the number of IDE analyzers that are now based on IOperation, I believe #32100 should be treated as higher priority as missing IOperation/CFG support for new language constructs will almost certainly lead to false positives from the analyzers.

mavasani commented 5 years ago

Also tagging @jcouv so #32100 can be prioritized.

@stephentoub @CyrusNajmabadi to clarify the issue here - compiler does not implement any IOperation/CFG support for using declarations, which leads to all IOperation/CFG based analyzers seeing these as regular variable declarations with no implicit Dispose invocation. I have had to special case this scenario based on syntax at bunch of places in analyzers repo as well, which is ugly for any language agnostic IOperation/CFG based analysis.

CyrusNajmabadi commented 5 years ago

@mavasani Understood. Personally, I woudn't consider the language feature 'done' unless we actually had the proper IOp information exposed.

jcouv commented 5 years ago

Closing as dupe of https://github.com/dotnet/roslyn/issues/36502 https://github.com/dotnet/roslyn/issues/32100 We're prioritizing nullable analysis hard at the moment, but IOperation support for some C# 8 features is on our radar.