Make Method Async renames wrong method if a method with the same name already exists

[chrisaut]

Version Used: VS 16.7.2

Steps to Reproduce: Create the following Methods

        static int Foo()
        {
            return 42;
        }

        static int Foo()
        {
            await Task.Delay(42);
            return 1;
        }

Place the cursor on the await and select Make method async refactoring Expected Behavior:

        static int Foo()
        {
            return 42;
        }

        static async Task<int> FooAsync()
        {
            await Task.Delay(42);
            return 1;
        }

Actual Behavior:

        static int FooAsync()
        {
            return 42;
        }

        static async Task<int> Foo()
        {
            await Task.Delay(42);
            return 1;
        }

[CyrusNajmabadi]

We likely have to go find the method again after an edit, and in the case of multiple matching methods find a differnet one. In general, this is such a corner case i don't think this needs to be fixed. If it is fixed, I would only want to accept the code if it didn't substantially increase the complexity of the feature. i.e. a couple-line fix here would be acceptable. Having to write several dozens of lines to address this likely would not be.

[chrisaut]

Is it really a corner case though? I've ran into it 5 times yesterday.

I'm adding some async functionality, and I start by copying the the sync version, call into async counterparts in its implementation, add an await there and and hit Make async. In theory this is much faster than manually updating the method signature.

If it's hard to fix then fine I guess, I would have thought it knows which method it's being executed on.

[CyrusNajmabadi]

Is it really a corner case though?

For you it sounds like no, because of how you are producing one method from another. I don't really have any reason to think this sort of approach is widespread. Put another way, you're the only one to ever report this :)

I would have thought it knows which method it's being executed on.

The problem is that these sorts of features often perform a "series of changes" to a document/tree/symbol. And after each edit they have to go "find" the "original" node they are based off of. As it happens, this sort of "finding" can be challenging (consider how edits completely change positions, parent/child relationships, meaning of code, etc. etc.). We have a general API for doing this. However, one thign it is not good at is finding nodes again that exactly match in signature. In general this isn't an issue, as that code is already in error, and most people don't seem to write that way. However, you are in that bucket, so it's hitting you disproportionately.

(Note: i coudl be wrong, and it coudl be an entirely different issue).

My recommendation would be to make use this as a way to dive into Roslyn and contribute PRs that improve the product, but scratch an itch that is especially annoying to you.

The code for this, for example is in: https://github.com/dotnet/roslyn/blob/master/src/Features/CSharp/Portable/MakeMethodAsynchronous/CSharpMakeMethodAsynchronousCodeFixProvider.cs and the base type: https://github.com/dotnet/roslyn/blob/master/src/Features/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

The issue here is likely in:

https://github.com/dotnet/roslyn/blob/31ebe4954e5455e630acb1e8445ecb742c678753/src/Features/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs#L165-L166

And tests would go here:

https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/CSharpTest/Diagnostics/MakeMethodAsynchronous/MakeMethodAsynchronousTests.cs

Cheers! :)