Extract Method changes 'using var' semantic to 'out parameter' semantic

[vsfeedback]

This issue has been moved from a ticket on Developer Community.

---

Using 'Extract Method' in a block containing 'using var' causes the resulting method to assign the variables to out parameters and contains no using expression.

---

Original Comments

Visual Studio Feedback System on 10/6/2019, 11:35 PM:

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

Visual Studio Feedback System on 10/8/2019, 02:04 PM:

This issue is currently being investigated. Our team will get back to you if either more information is needed, a workaround is available, or the issue is resolved.

---

Original Solutions

(no solutions)

[ryzngard]

Verified in 16.4 preview 2 with the below code

Before

    public class Goo : IDisposable
    {
        void M2() { }
        void M3() { }
        string S => "S";

        void M()
        {
            using var g = new Goo();
            var s = g.S;
            g.M2();
            g.M3();
        }

        public void Dispose()
        {
            throw new NotImplementedException();
        }
    }

After

    public class Goo : IDisposable
    {
        void M2() { }
        void M3() { }
        string S => "S";

        void M()
        {
            Goo g = NewMethod();
        }

        private static Goo NewMethod()
        {
            var g = new Goo();
            var s = g.S;
            g.M2();
            g.M3();
            return g;
        }

        public void Dispose()
        {
            throw new NotImplementedException();
        }
    }

[RikkiGibson]

The expectation is that the Goo g = NewMethod(); would change to using Goo g = NewMethod(), right?

[ts2do]

Here's the expected result of extracting the entire body from method M:

    public class Goo : IDisposable
    {
        void M2() { }
        void M3() { }
        string S => "S";

        void M()
        {
            NewMethod();
        }

        private static void NewMethod()
        {
            using var g = new Goo();
            var s = g.S;
            g.M2();
            g.M3();
        }

        public void Dispose()
        {
            throw new NotImplementedException();
        }
    }

[davidwengier]

The expectation is that the Goo g = NewMethod(); would change to using Goo g = NewMethod(), right?

My expectation is that, as above, NewMethod() wouldn't have a return type at all. I've been running into this a lot in some code I've got doing a lot of SkiaSharp drawing, and the problem is clearer IMO when there are multiple things with usings:

Before:

        private void M()
        {
            using var x1 = new System.IO.MemoryStream();
            using var x2 = new System.IO.MemoryStream();
            using var x3 = new System.IO.MemoryStream();
        }

After extracting the all 3 lines to a method:

 		private void M()
        {
            MemoryStream x1, x2, x3;
            NewMethod(out x1, out x2, out x3);
        }

        private static void NewMethod(out MemoryStream x1, out MemoryStream x2, out MemoryStream x3)
        {
            x1 = new System.IO.MemoryStream();
            x2 = new System.IO.MemoryStream();
            x3 = new System.IO.MemoryStream();
        }

It's not that the usings are gone, though obviously thats not good, its that the analysis thinks the variables are used outside of the selection, even though all of the method contents were selected.

[ts2do]

After further consideration, it seems like an extremely complex construct to handle properly.

Consider this method:

int Method(IEnumerable<char> source1, string source2)
{
    using IEnumerator<char> enumerator1 = source1.GetEnumerator();
    using CharEnumerator enumerator2 = source2.GetEnumerator();

    enumerator1.MoveNext();
    enumerator2.MoveNext();

    return enumerator1.Current + enumerator2.Current;
}

If you try to extract all but the last line of the method body, you would end up with something akin to this:

int Method(IEnumerable<char> source1, string source2)
{
    IEnumerator<char> enumerator1 = null;
    CharEnumerator enumerator2 = default;
    bool disposeEnumerator2 = false;

    try
    {
        ExtractedMethod(source1, source2, out enumerator1, out enumerator2, out disposeEnumerator2);

        return enumerator1.Current + enumerator2.Current;
    }
    finally
    {
        using (enumerator1)
        using (ConditionalDispose.Create(enumerator2, disposeEnumerator2))
        {
        }
    }
}

void ExtractedMethod(IEnumerable<char> source1, string source2, out IEnumerator<char> enumerator1, out CharEnumerator enumerator2, out bool disposeEnumerator2)
{
    enumerator1 = source1.GetEnumerator();
    enumerator2 = source2.GetEnumerator();
    disposeEnumerator2 = true;

    enumerator1.MoveNext();
    enumerator2.MoveNext();
}

This would be the most accurate handling for such an operation, though it's not necessarily best practice to use out parameters when exceptions are thrown. As you can see, method extraction gets hairier when a value type is involved because the resulting code must not call Dispose on arbitrary default value types, needing an auxiliary ConditionalDispose<> type for support. It's possible to try to encapsulate the extra out bool parameter generated, but nothing would really be suitable.

I think the best solution is to either ask if the user would like to expand the code selection to the closing bracket ending the topmost using-var expression within the selection or not show the 'Extract Method' quick action at all. Anything else would clearly cause a lot of unexpected code generation.

[jnm2]

Another repro:

using System.Collections.Generic;

class C
{
    bool M(IEnumerable<int> p)
    {
        [|using var x = p.GetEnumerator();
        return x.MoveNext();|]
    }
}

🛠 Extract method:

using System.Collections.Generic;

class C
{
    bool M(IEnumerable<int> p)
    {
        IEnumerator<int> x;
        return NewMethod(p, out x);
    }

    private static bool NewMethod(IEnumerable<int> p, out IEnumerator<int> x)
    {
        x = p.GetEnumerator();
        return x.MoveNext();
    }
}

[jnm2]

The danger is that you might forget to put the using token back.