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.
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
@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.
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.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”.
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.
@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.
@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.
@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:
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.