Roslyn: 'Make method async' breaks overridden methods

[miloush]

Version Used: VS 2019 Preview 2.1

Steps to Reproduce:

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

Actual Behavior:

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

Expected Behavior:

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

20348

[CyrusNajmabadi]

Should not be offered as it is not really an option for an overridden method.

We don't like this sort of approach, as it just confuses users as to why something isn't offered. Furthermore, there are many times when i'll still go make such a change, but then go back and add some sort of bridge method between a new override and the new method i'm creating.

Refactorings are ways to speed things up. In this case, speeding up having to add the 'async' modifier and do the 'rename' for you. If you don't want that, and just want to add 'async' and change nothing else, the recommendation would be to just add that modifier yourself.

[miloush]

We don't like this sort of approach, as it just confuses users as to why something isn't offered

What is confusing is that it says 'make method async', nothing about renaming it.

there are many times when i'll still go make such a change, but then go back and add some sort of bridge method between a new override and the new method i'm creating

Fair enough. The refactoring could offer that solution instead.

[CyrusNajmabadi]

What is confusing is that it says 'make method async', nothing about renaming it.

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/async/#BKMK_NamingConvention

By convention, you append "Async" to the names of methods that have an async modifier.

This is felt to be the approach that most people would want here. So we follow that since it would just be annoying to add the 'async' keyword ("that's it??") then forcing people to have to take the extra step of renaming the methods.

The point is to make this an idiomatically 'async' method. That means updating the name accordingly, given that it's how .Net has pitched async-methods from the beginning. We want to follow those conventions and push people toward the most commonly accepted form out there. Refactorings follow the needs and use cases of hte greater community.

Fair enough. The refactoring could offer that solution instead.

It could. PRs welcome :)

[miloush]

I am not against the rename in general (or following conventions), if it wasn't an override that has to match the base method, or an event handler that needs to match a signature. I didn't see this case mentioned in #20348. It is fixing a compile error by introducing different one or more (hopefully). I would argue adding async modifier only is still help because you invoke it from where you are working rather then going to the method definition and back, and figuring out where the keyword needs to be. Note also that this is a fix of a compile error rather than a refactoring. I am totally up for refactoring to adhere to conventions, I would be less aggressive for error fixes.

I am also tempted to suggest that following the guidelines/conventions is not a valid argument for the stay void case, which if I am correct is only legal for exactly these purposes where signature needs to match, so it doesn't make much sense to turn it into the "correct" method but keep it void. Maybe the stay void refactoring would be more useful as preserve signature. What is the intention of the separate stay void refactoring if not to preserve the signature?

[CyrusNajmabadi]

which if I am correct is only legal for exactly these purposes where signature needs to match,

That's not correct. It's always legal. You can write your own:

async void Foo() method that doesn't need to match some other signature.

Maybe the stay void refactoring would be more useful as preserve signature

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.

[sharwell]

Design review conclusion: We believe the guidance page incorrectly states the guidance for the Async suffix. Here is the current text:

By convention, you append "Async" to the names of methods that have an async modifier.

Our initial proposal for updated text is the following (@AArnott is likely able to suggest clarifying improvements):

By convention, you append "Async" to the names of methods that have an asynchronous signature (i.e. the result is awaitable).

➡️ In addition to the above documentation updates, we should update the "Make method async (stay void)" refactoring to not rename the method.

➡️ Updated 28 May 2019: In addition to the previous conclusion, this refactoring should not rename the method if the return type did not change as part of the refactoring. This expansion of the rule from the previous design meeting accounts for cases like were presented in https://github.com/dotnet/roslyn/issues/33082#issuecomment-488811799.

Should not be offered as it is not really an option for an overridden method.

For this specific item, we believe the current behavior is by design.

[AArnott]

All that makes sense. For the guidance text wording, I think I'd use:

By convention, methods that return commonly awaitable types (e.g. Task, Task<T>, ValueTask, ValueTask<T>) _should_ have names that end with "Async". Methods that start an asynchronous operation but do not return an awaitable type should not have names that end with "Async", but may start with "Begin", "Start" or some other verb to suggest this method does not return or throw the result of the operation.

I'm trying to strike a balance here, considering that a type may or may not be awaitable based on context of the caller. E.g. string is awaitable in some contexts, yet a method that returns a string should not end with 'Async'. But ya, if you return Task yet you aren't implemented asynchronously, you should still have the Async suffix.

[CyrusNajmabadi]

@AArnott That makes sense to me. Do you know who would be best to take care of updating this text? Is this something you can do?

[AArnott]

Sure, @CyrusNajmabadi. In progress.

[davidroth]

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?

• Mediatr
• NServiceBus
• ...

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.

[CyrusNajmabadi]

I completely disagree with this. It is indeed valuable

I didn't say it wasn't valuable :) I said it wasn't "valuable enough" :)

In other words, this space is precious in the lightbulb and we don't want to overload it with more and more options that don't hold enough weight. Indeed, we had to go through many waves of cutting and culling items from this list because there were just so many of them that users could not find the high value pieces.

So, in that regard, in my personal opinion, i would not add this as there's just not enough value over having the user just make the edit manually themselves.

[CyrusNajmabadi]

Note: as mentioned here:

https://github.com/dotnet/roslyn/issues/33082#issuecomment-460096166

Fair enough. The refactoring could offer that solution instead.

It could. PRs welcome :)

Basically, that means: no one is likely to do this for you. But you're welcome to provide a PR if you really want here @davidroth .

For example, i think it would be fine to have add async (and do not rename) offered solely in the cases where tehse were overrides or implementations of metadata symbols.

That would be a best of all worlds approach where "the right thing" would happen for source-libs, but if you are using an "external nuget" you have the options you need. It also means in the common case we don't clutter the lightbulb.

But, as i mentioned, this would likely need to come as a community PR. I'd be happy to walk you through this. I'm personally not willing to invest the time myself as i'd prefer to use my free time on other things that impact me more directly. But if this is impactful to you i can help you drive the fix in this OSS project :)

[CyrusNajmabadi]

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

If this is something you are really passionate about, then this is definitely a great way to start contributing to Roslyn. Making this sort of change would not be that hard. The biggest cost is usually on your first contribution to roslyn (because you need to figure out how to get everything setup locally). But once you do that, you can then easily find and fix issues that bother you (but aren't deemed enough of a concern to warrant high prioritization from the team).

To get started, there are good docs here: https://github.com/dotnet/roslyn/wiki/Contributing-Code and here https://github.com/dotnet/roslyn/wiki/Building-Testing-and-Debugging

I can help walk you through this over on gitter.im/dotnet/roslyn. It can be a bit daunting at first, but it's really not that hard to do once you're actually setup :)

[davidroth]

I didn't say it wasn't valuable :) I said it wasn't "valuable enough" :)

Fair point ;-)

I understand that adding another option to the lightbulb might not be a good idea.

But in my option, there are solid arguments for supporting the non suffix use case. As demonstrated in my previous comment, the current refactoring breaks code when used with external libraries which makes it useless and leaves a big productivity gab in this area.

I see several possibilities:
1) Detect if adding the suffix would lead to new errors and if so, only apply the keyword without the suffix
2) provide a code style setting (prefer Async suffix for async methods
3) provide a user setting (not preferable imo)

[davidroth]

For example, i think it would be fine to have add async (and do not rename) offered solely in the cases where tehse were overrides or implementations of metadata symbols.

If this is something you are really passionate about, then this is definitely a great way to start contributing to Roslyn.I can help walk you through this over on gitter.im/dotnet/roslyn. It can be a bit daunting at first, but it's really not that hard to do once you're actually setup :)

Good to know. I'd love to fix this, maybe I can find some time to dig into this ;-)

[CyrusNajmabadi]

Awesome. I'm going on vacation for the next couple of weeks. But i still usually check in every so often. If/when you do get time, feel free to reach out to me on gitter and i'd be def happy to help you out here :)

to start with, try following those instructions to just get roslyn sync'ed and building.

[oliverjanik]

I just run into this, I thought it was Roslynator refactoring, but eventually found this.

1) I find it completely unacceptable that a refactoring breaks compilation

2) Async suffix is debatable, it makes perfect sense when you provide both sync and async methods as a library author, however, the trend I'm seeing (externally and internally) is that sync methods are a dying breed and many interfaces provide only asynchronous API. At that point async suffix is just noise because everything is async.

3) This refactoring does 3 things as far as I can observe:

• adds async keyword
• turns result type from T into Task
• adds the suffix

That last bit is very surprising and unexpected. I believe it violates the principle of least surprise. There is huge value to the refactoring even if the rename bit got removed.

[sharwell]

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

1. 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
2. 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.

[miloush]

@sharwell I would be happy with the design review conclusions as presented.

@oliverjanik

1. Many refactorings can break compilation and I believe it has been established that the developer is trusted with "they know what they are doing".
2. I tend to disagree, it might be the case in the area you are working in but I find sync methods still very relevant and desirable - on the contrary, nothing is more annoying than getting into all the threading business in a straightforward console application just because no sync methods were provided. I also find removing the Async suffix makes the code more difficult to read and find bugs in it, as it looks like nothing much is going on and that the return type does not matter. While this might be a purely personal preference, the Async suffix is part of the official guidelines and I imagine it might be a bit difficult to change these now and render most of the existing async code in violation of the guidelines.
3. Note that there are two refactorings, one changes the return type and one does not. Do I understand it correctly that you are talking about the one that changes the return type (i.e. already a breaking change on its own), and want it to not add the suffix?

[CyrusNajmabadi]

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".

[sharwell]

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

[oliverjanik]

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".

Citation needed for your majority claim.

I can't believe we're even debating this. Resharper and other tools got it right so many years ago. If the refactoring does not have "Rename" in its description, renaming is very surprising. Doubly so when it breaks the compilation.

This refactoring is very useful without the rename. It fixes up the return time into Task<T> and adds async keyword. It could stop right there I'd use it 30 times a day. Too often I start with a sync method after "Extract Method" then later need it turned into async. This would be a godsend. Instead it has go and do this annoying thing where it adds a suffix that it did not convey to me. As a bonus without the rename this refactoring would be easier to implement.

How about we spilt this refactoring into: "Turn into async method" and "Apply Hungarian notation to my async method" ?

On a philosophical note Async suffix is completely redundant. It only exists because C# does not support overloads based on different return type. It's a form of Hungarian notation and nowhere else in the framework you suffix your method names with return type.

When your interface looks like this:

public interface IMyRepository
{
    public Task<MyEntity> GetAsync(string id);
    public Task UpdateAsync(MyEntity entity);
    public Task DeleteAsync(MyEntity entity);
    public Task<IReadOnlyList<MyEntity>> QueryAsync(someArgs);
}

At this point it's just repeated noise, especially when the context is database access where IO is given. I don't see Javascript or Typescript adding Async to methods that return promises.

I'd argue that refactorings should not enforce a convention. Refactorings should strive to be useful no matter what convention the team has agreed to.

[miloush]

Well looks like an easy way out would be to have 3 refactorings instead of two:

1. for void methods only: add async keyword, keep void, don't rename
2. add async keyword, change return type, don't rename
3. add async keyword, change return type, add Async suffix

[CyrusNajmabadi]

add async keyword, change return type, don't rename

I don't think this should be offered. Not as long as .Net naming guidance is to end Tasklike-returning methods with Async.

I'm ok not renaming in the case where this overrides/implements a member from metadata. Though, in that case, it's unclear what the "right thing" is since changing the return type will also break things. (outside of the 'void' case).

[oliverjanik]

I think it should be offered. It's not refactorings job to enforce code conventions. You have analysers and formatters for that. Why restrict the usefulness?

From the answers here: https://stackoverflow.com/questions/15951774/does-the-use-of-the-async-suffix-in-a-method-name-depend-on-whether-the-async

Of course, there are always exceptions to a guideline. The most notable one in the case of naming would be cases where an entire type’s raison d’etre is to provide async-focused functionality, in which case having Async on every method would be overkill, e.g. the methods on Task itself that produce other Tasks.

So it's a guideline for a lot of cases, but not always. Not sure why there are so many zealots here.

[CyrusNajmabadi]

Why restrict the usefulness?

Because the more that is offered, the more cluttered and confusing hte experience gets. We have direct feedback over hte years about there being too many options offered and that leading people to not be able to find or care to look at the set of fixes/refactorings offered. There's good data out there that indicates taht once your lists even get past 4-5 items you've gone too far. So it's critical that we don't include items unless absolutely certain it will be relevant much of the time fo rusers.

So it's a guideline for a lot of cases, but not always.

The exception literally is for 'Task' itself :) If it's a good guideline 99% of the time, and should be followed in nearly all cases, then the IDE is going to try to follow that. That's what we do all over the place. For example, if you name something Foo we're going to presume it's a property and not a field (even though sometimes people might have capitalized fields). Similarly, if you name a type IBlah we're just going to assume it's an interface and not offer to generate class even though maybe one person out there would want that :).

Not sure why there are so many zealots here.

Because this is an area that gets a lot of attention and needs a careful touch. I've personally probably spent dozens of hours whittling away at things and trying to reign the lightbulbs under control. There's a lot of history and experience that goes into this (and has gone into the explanations). It's not zealotry to try to follow the rules and lessons we've learned over the years, it's experience. And, more importantly, if we took the attitude of "just add it, what's the big deal?" we'd quickly return back to where teh lightbulbs were 5 years ago, with most people saying they couldn't find anything in them.

[oliverjanik]

I think you completely misread my quoted text. Read again:

The most notable one in the case of naming would be cases where an entire type’s raison d’etre is to provide async-focused functionality

Which is about 80% of classes in projects I work on everything is async by default. No sync variations, needed.

It's not zealotry to try to follow the rules

You mean guidelines. It is zealotry to enforce guidelines at all costs. You're proving my point.

[sharwell]

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

[CyrusNajmabadi]

Which is about 80% of classes in projects I work on everything is async by default. No sync variations, needed.

That's not the type of case that was being referred to. You snipped off the relevant bit:

e.g. the methods on Task itself that produce other Tasks.

It's not that everything is async. It's that the entire reason for the type is to provide functionality around the async area. So, for example, the aforementioned Task case, or VS's JoinableTaskFactory. These packages exist at a metalevel for interacting with tasks and providing funcitonality specifically around managing tasks/sync-contexts/concurrency/threading etc. In that case it's not necessary. It's not saying that if you have a domain-specific API, and you happened to make all hte method async, that 'Async' isn't necessary.

You mean guidelines. It is zealotry to enforce guidelines at all costs. You're proving my point.

You completely ignored all the other things i said in my post. It is not 'free' to add more lightbulb items. Indeed, it can actively serve as a negative. So, like in langauge design, adding something here starts at a net negative to begin with and needs to provide enough value to offset that. That isn't the case here. The extra item would not be recommended most of the time, and it would clutter a very critical space that needs to not be cluttered.

It is zealotry to enforce guidelines at all costs.

It's not "at all costs". It's "at the costs that have been enumerated so far". Again, as i mentioned, consider if we took this approach with all the lightbulbs (and trust me, there are requests for tons of lightbulb features to just add a single extra entry for some case some user wants badly). We need to be conscientious and careful here because going down that path just gets us back where we were several years back, which was a very bad place.

Note: if you want to discuss this more, i'm happy to. But please take it to gitter.im/dotnet/roslyn. This issue is not an appropriate forum to discuss the meta-level concerns about complexity/clutter and the other design decisions that go into surfacing a new top-level light bulb action.