vs-threading icon indicating copy to clipboard operation
vs-threading copied to clipboard

Refactoring: Make method async and await call-sites

Open alrz opened this issue 3 years ago • 6 comments

Is your feature request related to a problem? Please describe.

Migrating a codebase to use async overloads where available and make the enclosing methods async as well.

Describe the solution you'd like

Whenever an async overload is available in a non-async method, suggest to use that, make method async and update call-sites.

If this is done as a fix-all, this can be recursively applied to the call graph.

Describe alternatives you've considered

Currently this can be done in a few repetitive steps until the whole thing is migrated, but it is extremely cumbersome:

  1. Change some method call to Async overload to get VSTHRD110, or Change method return type to Task to get VSTHRD103 (note: 'change signature' doesn't support this)
  2. Use 'Add await' codefix to get CS4033
  3. Use 'Make method async' to get VSTHRD110 or CS4014 on call-sites
  4. Repeat

alrz avatar Jan 29 '21 08:01 alrz

There is an open request for this here: https://github.com/dotnet/roslyn/issues/36737 but I figured all the necessary infra is implemented in this repo.

cc @sharwell

alrz avatar Jan 29 '21 08:01 alrz

My memory is unclear, but at least one of our code fixes may already do this: https://github.com/microsoft/vs-threading/blob/d1dd01020458585e5f7f65aa044ca5f27f5982f8/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/FixUtils.cs#L78

That method will not only make a method async but also update call sites to await it. It may do it recursively but if not, at least it may reduce the steps you listed above. I just can't remember the conditions that lead to offering that code fix.

AArnott avatar Jan 29 '21 13:01 AArnott

Yes it certainly can but it's not offered when the method is not returning Task, so the steps above need to be done to trigger it.

It's fine if I have to do this to get it started, but at least call-sites should be awaited - instead you get another hint to make it so, and then another hint to make that method async, and so on.

alrz avatar Jan 29 '21 15:01 alrz

I updated the title to clarify. Note that awaiting call-sites means that we still need to run "make async and await call-sites" on the containing methods and this goes on until we get to a method that is already async.

alrz avatar Jan 29 '21 15:01 alrz

Ok. Ya, that would be great. I suspect other priorities will prevent anyone around my team from satisfying this request in the foreseeable future. If you're interested in sending a PR that includes several tests to demonstrate its capabilities and limitations, I'd be willing to review and possibly merge it.

AArnott avatar Jan 29 '21 16:01 AArnott

Would love to see this.

Eli-Black-Work avatar Feb 02 '21 03:02 Eli-Black-Work