search

microsoft / vs-threading

The Microsoft.VisualStudio.Threading is a xplat library that provides many threading and synchronization primitives used in Visual Studio and other applications.
Other
985 stars 146 forks source link

Refactoring: Make method async and await call-sites #746

Open alrz opened 3 years ago

alrz commented 3 years ago

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 commented 3 years ago

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

AArnott commented 3 years ago

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.

alrz commented 3 years ago

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 commented 3 years ago

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.

AArnott commented 3 years ago

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.

Eli-Black-Work commented 3 years ago

Would love to see this.