roslyn: 'Make method async' breaks overridden methods

Version Used: VS 2019 Preview 2.1

Steps to Reproduce:

partial class MyPage : Page
{
    protected override void OnNavigatedTo(NavigationEventArgs e)
    {
        await Task.Delay(100);
    }
}

Actual Behavior:

‘Make method async’ is offered that changes the return type and renames the method.
'Make method async (stay void) is offered that renames the method.

Expected Behavior:

Should not be offered as it is not really an option for an overridden method.
That is the quick fix I was looking for, but it should not rename the method as it breaks the override.

#20348

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 1
Comments: 29 (18 by maintainers)

Commits related to this issue

Update guidance on Async suffix As discussed at https://github.com/dotnet/roslyn/issues/33082#issuecomment-467536910 — committed to AArnott/core-docs by AArnott 5 years ago
Avoid renaming methods that don't change return type Fixes #33082 — committed to sharwell/roslyn by sharwell 5 years ago

Most upvoted comments

@davidroth We took this to a design review today, and agreed that in your scenario the method should not be renamed. I will update https://github.com/dotnet/roslyn/issues/33082#issuecomment-488811799 to reflect the new design.

sharwell on May 29, 2019

Many refactorings can break compilation and I believe it has been established that the developer is trusted with “they know what they are doing”

To back this up. I’d say most (possibly all) refactorings can end up breaking code in some cases. They’re intended, for many cases, to allow people to much more quickly perform operations that would take a long time, or be extremely tedious to perform by hand.

This case was thought about, and even discussed with the .net naming people at the time. The conclusion was a strong “async methods should end with Async”. It provides a consistency that is very valuable to the .net ecosystem and means it’s very easy to see and understand if code is doing what’s expected just through easy visual inspection. This makes maintenance much simpler, and it means that things like PRs don’t easily have bugs slipping in the cracks. As long as the advice of .net is that these should end in ‘Async’, then i think Roslyn should try to follow.

That last bit is very surprising and unexpected. I believe it violates the principle of least surprise

I actually disagree with that wholeheartedly. I think it would be very surprising if it did not add the Async suffix. First, without that, the feature would be doing practically nothing. Second, for the vast majority of cases, you’d end up with people going “this isn’t helpful, now i have to go rename my methods right after i invoke you”.

CyrusNajmabadi on May 28, 2019

I am not happy with the current situation of this refactoring as already mentioned here 2 years ago: https://github.com/dotnet/roslyn/issues/20348#issuecomment-312653269

I dont want to argue about whether the Async suffix convention makes sense or not. The roslyn team has made it clear, that its pro async. Thats ok.

But please at least stop breaking code when using this refactoring.

Like it or not, lots of popular libraries don`t use the Async suffix because they have a different opinion on this topic. Examples?

It is really frustrating when VS breaks your code everytime using the make method async recatoring.

Example mediatr:

After the refactoring:

Boom: Code is broken because I cannot rename my method as its an implementation from an external nuget.

This is super annoying and really frustrating.

If you just want this, then the feature is literally only adding the ‘async’ keyword, and i don’t really see that as valuable enough. The async mod can just be added manually.

@CyrusNajmabadi

I completely disagree with this. It is indeed valuable because it allows me to quickly use the await keyword within a method, without manually navigating to the signature, adding the keyword, and navigating back to the original editing location. This is just not productive, costs time and is a painful “context switch”. It would be very valuable for the coding flow if the refactoring would just add the async keyword without renaming things I cannot rename. And since async/await is becoming more and more “normal” in most of our new code, this is really painful on a daily basis.

So please do something about this and also make the “No-Async-Suffix”-Camp more productive.

davidroth on May 2, 2019

@oliverjanik The majority of your cases should already be covered by the design we chose above and implemented in #36054. The renaming now only occurs when the refactoring changes the method type, so as long as your method already had an asynchronous signature before the refactoring it will keep the same name. It should be easy to create an analyzer (“Do not use Async suffix”) to handle any remaining cases.

sharwell on May 31, 2019

@davidroth The scenario you presented in https://github.com/dotnet/roslyn/issues/33082#issuecomment-488811799 is different from the one that previously went to design review. The previous design review conclusion had the following characteristics:

For the case we reviewed, when the return type of the method was changed as part of the refactoring, the method name was also updated
For the case we reviewed, when the return type of the method was not changed as part of the refactoring, the method name was not updated

~~Your situation is arguably closer to the second case than the first. I would encourage you to open a new issue for it so we can review it.~~ Edit: Since the complete design for this has not yet been implemented and the issue is still open, I will take this situation to design review and update this issue with the conclusion.

sharwell on May 28, 2019