Roslyn: Make method async code fix - method name changes to MethodNameAsync

[davidroth]

Version Used:
VS 2017 (15.2)

Steps to Reproduce:

1. Add _await_ inside method body of _Task_ method
2. Invoke code fix "Make method async"
3. Method is refactored to _OriginalMethodName_Async

Expected Behavior:

_async_ keyword is added to method, but method name is untouched.

Actual Behavior:

_async_ keyword is added to method, _and_ method signature is refactored (_Async_ suffix added). This is super annoying, especially if you have a virtual base method, which does not have the async suffix applied. If you have lots of usages this may lead to a huge (and slow) refactoring because all occurrences of this method will be renamed.

This is a regression from VS 2015 and makes this refactoring essentially useless in this circumstances.

[davkean]

In contrast, this is exactly what I would like. :) Note, I don't work on the C# IDE team - so I'll let them chime in.

Though, I wish it would do a refactoring of all interface declarations, base methods, overrides, etc...

[Pilchie]

Hmm, I don't remember making a deliberate change here. @kuhlenh or @CyrusNajmabadi do you? I agree that the rename is what I want in general, but I can see a perf argument if it isn't what you want.

[kuhlenh]

@davidroth do you have a code style setting for naming async methods? I don't remember us make a change here unless that style is set?

[davidroth]

@kuhlenh No I don't have a specific code style setting for naming async methods. Just a clean VS 2017 installation with default settings.
So its not cool that VS refactors my method name when I just want to add the _await_ keyword ;)
We have some commonly used based types with _virtual Task_ methods without the async suffix applied. I dont want VS to refactor my whole solution when I just wanted to use the await keyword in a derived type.

[sharwell]

I agree the method should end up with an Async suffix, but have no objection to it occurring in two distinct steps.

[CyrusNajmabadi]

This was an intentional decision. We could consider offering to also only add 'async'. However, i would definitely want the existing code fix (and it's something i use a lot) that properly renames the method to match the .net naming pattern here.

[davidroth]

A new _add async_ refactoring would make sense. So one could choose based on desire ;)
One more question: Is it possible for the current refactoring to configure a code style which omits the _Async_ suffix. Or is this hard coded?

[sharwell]

@davidroth It will likely be updated to adhere to coding style with the ability to not require that suffix. However, I would strongly urge you to use the style of adding the suffix, as I've seen it prevent real bugs on several occasions in practice.

[davidroth]

@sharwell Well that's a rather opinionated topic. I thinks if a library or app is inherently async , the suffix is just noise and adds no value. Several popular libraries and frameworks already ditched the suffix in new versions. Of course, if you need to provide both sync+async methods, its required.
Please also keep in mind that for example async asp.net core controller methods are usually written _without_ the _async_ suffix.

[CyrusNajmabadi]

Tagging @DustinCampbell @kuhlenh @Pilchie for their thoughts here.

--

Here are a few options we can have:

1. Make it so that 'make method async' doesn't add the 'Async' modifier. I personally don't like this. 'Async' naming is part of the general .Net recommendation here and is a core way for people to realize and distinguish async methods from non-async methods. If we did this, users would have to manually add 'Async' after the fact. I've personally used this feature a lot and having to manually all the methods i'm changing would be aggravating.
2. Keep things as is. But it will be annoying to people like @davidroth
3. Add more code-fixes that allow the user to say things like "Make method async" and "Make method async (and add 'Async' suffix)". Note that this means a user might get up to 4 fixes as we already have options if the method shoudl stay 'void' or not.
4. Add a user visible option if they want this feature to add 'Async' when they apply it.

I'm amenable to 2, 3, and 4. Curious what others think we should do here.

[DustinCampbell]

I don't like 3. I don't think the explosion is worth it since the user is more likely to only pick one all the time.

I like 4. Adding the 'Async' suffix is our recommendation, but not a requirement. So, I could see it as a style option.

Another option would be to do #1 but make sure there's another refactoring available to add the Async modifier. I'm not sure I like that, but it's certainly an option.

[CyrusNajmabadi]

I think i like #4 the best. I agree that #3 is unpalatable. And #1 is unfortunate as the common case (at least for me) becomes two steps. There are times when i've been changing a lot of methods in a type, and needing all of these to be two steps would be unpleasant.

[Pilchie]

Tagging @dpoeschl - I wonder if we could tie this to naming styles, so that if the user has an "async methods must end with Async" naming rule, we do this, but otherwise don't.

[sharwell]

I agree with @Pilchie. Our other code fixes follow the code style rules; this one should be no different. If we want the default behavior to add the suffix, then we need to make that part of the default configuration for the naming rules.

@davidroth Discussions of the subject generally involve opinions, but my objective experience with this rule in practice is I've seen it catch real bugs that would otherwise have likely made it through the code review process. For example, during a review someone would point out that the only issue they have was a naming violation, but as soon as the method was renamed a substantial usage bug was uncovered.

[davidroth]

@Pilchie The proposed change, which respects the code style settings, is good.

One question though:

If I have configured my code style settings to _not_ add the async suffix, will the suffix be removed then (kinda the inverted behavior?)

[sharwell]

If I have configured my code style settings to not add the async suffix, will the suffix be removed then

We don't currently have a way to specify things like "do not allow [prefix|suffix]". Until such a feature was added, the suffix would not be removed. (I already had an issue in #18354 for this.)

[davidroth]

@sharwell On more note: I experienced that the refactoring also takes place if the overwritten virtual async method is declared in a referenced assembly.

Regardless of code style setting - the _Async_ suffix must not be added/removed automatically if the method is a virtual override method, and the declaration exists outside of the current solution. Otherwise the code immediately fails.

Just wanted to add this information so that it is considered when implementing this.

[sharwell]

@davidroth This refactoring changes the return type of the method, so in that scenario it will fail whether or not the method name is changed.

[davidroth]

@sharwell I was talking about the scenario, where the _virtual method_ defined in an external assembly already returns _Task_ or _Task< T >_ but does not have the _Async_ suffix. In that case the return type does not change but the additional suffix breaks code.

[sharwell]

@davidroth Feel free to file a separate issue for that, to make sure it doesn't get overlooked and has a dedicated unit test to prevent regressions. It's definitely a case we want to work consistently. :smile:

[davidroth]

This has been fixed in https://github.com/dotnet/roslyn/pull/36054 (I have successfully tested the new behavior in VS 2019 16.2)

[johnknoop]

@davidroth I don't know if it's correct to say that this has been fixed. #36054 is about not renaming unless is changes return type. But this issue is about not renaming in general.

It would be nice if we could customize this using .editorconfig settings.

In some of my projects that are libraries, suffixing with Async is part of the code style because it conforms to the convention, but in other projects I don't really want it at all because I personally only thinks it adds noise.

Especially annoying when I write unit tests, which tend to follow this process:

1. Scaffold a new test method using snippet. Results in this:

[Fact]
public void FooShouldBar()
{

}

Then I start adding some code. If the system under test is asynchronous then I get the CS4032 warning, which beautifully offers my a fix to change the test method to async Task, which I absolutely loves. But when I'm not noticing is that the test is also renamed FooShouldBarAsync which is especially hard to notice with longer test names. It's only later when I run my whole test suite that I see that half of them are suffixed "Async".

[davidroth]

@johnknoop I agree that this should be configurable. The unit test example is a good example which annoys me to.

[jorisvergeer]

I would like this to be configurable also. I am porting a legacy application to adopt async code, that does a lot of networking and IO, Having Async at the end of almost all methods starts to look more like noise than some actual useful information.

[sharwell]

@davidroth @jorisvergeer The issue you encountered should be resolved in a different way by #45368, which is currently in code review

[davidroth]

@davidroth @jorisvergeer The issue you encountered should be resolved in a different way by #45368, which is currently in code review

@sharwell I am not sure #45368 does fix the "Async" suffix issue for example in unit-tests or aspnet controllers?

Example:

1. Lets assume I have following aspnet core controller action method:

public IActionResult Index()
{
    return View();
}

1. Now i realize that I have to invoke an async method:

public IActionResult Index()
{
    await Task.Yield(); // Invoke some async method
    return View();
}

1. VS provides me this nice refactoring:

1. Unfortunately, the result is that the action method is named with an Async suffix, which I didn`t want.

Same issue when refactoring unit tests methods. I don't think anyone wants them to have an "async" suffix too.

[CyrusNajmabadi]

This is a different case. HEre, the return type changed. So we update the name to match the expected naming convention for task-returning methods.

[davidroth]

This is a different case. HEre, the return type changed. So we update the name to match the expected naming convention for task-returning methods.

Yeah. But nobody wants XUnit-Test methods or Aspnet Controller-Action methods to be suffixed with Async . Even roslyns unit tests dont append Async on XUnit-Test methods. So there exist multiple expectations on the naming convention for task-returning methods.

[CyrusNajmabadi]

But nobody wants

I do :)

So there exist multiple expectations on the naming convention for task-returning methods.

Sure. but our refactorings don't have it as a goal to meet all expectations. For every refactoring we offer there are all sorts of knobs and tweaks everyone wants for their particular scenarios. Nearly all of those tweaks just ends up not happening.

[jorisvergeer]

@CyrusNajmabadi
We have a beautiful .editorconfig file where we can define that each private static field should (hypothetically) begin with pizza_

What is wrong with the idea that there could be a setting like

dotnet_naming_style.instance_async_method_style.require_suffix = true
dotnet_naming_style.instance_async_method_style.suffix = Async

I agree that in public packages the Async suffix is preffered. (Altough, my IDE warns me for unawaited async tasks anyway, with an analyzer I wrote myself because de default does not catch all cases).

But I am quite annoyed when I am working on an internal project where I do not want that prefix. There are use cases where the prefix is not preferred. The unit test named above is one (though, not for you). And in my case where practically all code is async and Async suffixes are nothing more than noise.

[CyrusNajmabadi]

It's more complexity and effort to support. The goal of roslyn is not to support total configurability of every option that everyone wants. We have to draw lines in terms of waht we implement (and thus what we will have to support forever :)). This commonly means making strict decisions of where to draw lines. If you fall on the other side of where a line is drawn, it will certainly not be very satisfying.

[jorisvergeer]

@CyrusNajmabadi That's a fair point.

[johnknoop]

@CyrusNajmabadi But isn't it also a fair point to say that you shouldn't rename stuff in the user code without providing a way to opt out? It would be one thing if it was reversed, meaning you _didn't_ apply any renaming, and people reached out to you and asked that you automatically put an "Async" suffix to their methods. But in this case the editor is actively changing the user code, and the light bulb menu item doesn't even give a hint that it will.

[CyrusNajmabadi]

@CyrusNajmabadi But isn't it also a fair point to say that you shouldn't rename stuff in the user code without providing a way to opt out?

It's an good perspective you bring there for sure. I will noodle on that a bit. I will tell you my current position is: it's absolutely fine to do that. But i will think on this and i see if there's some other sort of mental model i could get behind here on this.

and the light bulb menu item doesn't even give a hint that it will.

Sure it does:

We show, right there, what we're going to do with the code. We're going 'make it async'. Which means picking an appropriate type (in this case Task) as well as follow the other recommendations on how async code should be named.

--

Look, i 100% get your request :) I don't think it's unreasonable or bad in any way shape or form. My position is simply that i don't know if it rises to the level of warranting space in an area where we are extremely judicious about cluttering things up. We've gone through many many many passes where we're trimmed down these lists to give a core set of functionality that we think hits a sweet spot of meeting the needs of the vast majority of hte user base, while not adding tons of clutter and confusion.

To give an idea, these lists were at one time 3x larger, with far more options and knobs all over the place. It was an active problem for users as just trying to scan through to find the item they wanted was super painful, and it turned out that in most case, the extra knobs just aren't significantly used. So, while i get the desire you have here, it's in tension with many design goals we have in this space.

However, as i said above, i can see the idea that 'rename' is a rather important aspect of a fix, and that that's not always going to be ok. I'm going to think on that to see if it changes my conclusion on this matter. Thanks! :)