Roslyn: "'using' statement can be simplified" breaks code if variable name has been used before

[RayKoopa]

Version Used:
16.2.0 Preview 1.0

Steps to Reproduce:

1. Create a new C# 8 project with the following, compilable code:

using System.IO;

namespace ConsoleApp1
{
    class Program
    {
        static void Main()
        {
            using (Stream stream = File.OpenRead("test"))
            {
            }
            using (Stream stream = File.OpenRead("test"))
            {
            }
        }
    }
}

1. Accept VS' suggestion to simplify the second using by turning it into a using declaration (IDE0063).

Expected Behavior:
I expected the refactor to create compilable code like this:

        static void Main()
        {
            using (Stream stream = File.OpenRead("test"))
            {
            }
            using Stream stream2 = File.OpenRead("test"); // note the required unique name
        }

Actual Behavior:
The suggestion however does not check if a variable in the same scope has already been declared and generates the following which does not compile:

        static void Main()
        {
            using (Stream stream = File.OpenRead("test"))
            {
            }
            // CS0136 "A local or parameter named 'stream' cannot be declared in this scope
            // because that name is used in an enclosing local scope to define a local or parameter.
            using Stream stream = File.OpenRead("test");
        }

I couldn't find a similar issue about exactly this refactor, please close as duplicate if I am mistaken.

[ericwj]

I think this fix suggestion should be proposed very conservatively.

In small methods or if the using scope encompasses the whole scope of whatever block it is in, the fix suggestion is fine.

In larger methods it is easily and often very undesirable to replace the scope with the simplified using declaration because it changes the scope of the declaration and punts the disposal to the end of a potentially much larger block.

I didn't investigate which knobs exist in VS or .editorconfig to configure this warning, but I would say a simple on/off isn't enough.

[CyrusNajmabadi]

In larger methods it is easily and often very undesirable to replace the scope with the simplified using declaration because it changes the scope of the declaration and punts the disposal to the end of a potentially much larger block.

That is not how the feature works. It is already conservative, and only offers the feature when the .Dispose would still happen at teh same time as before (thus not affecting semantics).

You can see the logic for this here:
https://github.com/dotnet/roslyn/blob/8911c9fe424f153b308414356c61d7a87bca111c/src/Features/CSharp/Portable/UseSimpleUsingStatement/UseSimpleUsingStatementDiagnosticAnalyzer.cs#L173

[CyrusNajmabadi]

Note: in terms of fixing this, we'll need to check both any variables declared in the using itself, as well as any nested variables which are now being 'lifted' to the containing scope.

[CyrusNajmabadi]

I'm working on a fix for this.