Use Async Methods when in Async Context Analyzer

[mahdiva]

This PR adds the analyzer for preferring async methods when inside a Task-returning async method.

Fixes dotnet/runtime#45661

This analyzer was already implemented in VS-Threading VSTHRD103, but was based on C# specific APIs.
This PR is implemented using the IOperation based APIs instead with a similar functionality, supporting both C# and VB tests.

Old analyzer implementation for reference

cc @pgovind

[stephentoub]

This is a fine way to start for now. Subsequently, though, we might want to consider augmenting it to handle common patterns between sync and async variants. For example, a sync method might take byte[] array, int offset, int count, but the newer async variant might take Memory<byte>.

[buyaa-n]

@jeffhandley I am comfortable with this if it is shipped as off by default and we experiment with enabling it in the .NET 7.0 timeframe

@jmarolf the severity updated to disabled and all other comments are addressed with some refactoring, the PR is ready for final review, as you know we are still planning to add it in RC2 (The analyzer will have more improvement in 7.0)

CC @carlossanlop @jeffhandley @pgovind @stephentoub

[Youssef1313]

I'm not sure if a struct will be better or not (I think the object size is small enough?), But was it considered?

[buyaa-n]

i am not sure either, so leaving as is

[jmarolf]

To be honest I am skeptical that the general approach that this analyzer is taking can work. There are just so many places where just because two methods are named XYZAsync() and XYZ() does not mean they are semantically the same except for one being an async implementation. Like the Dispose() pattern there has been 10+ years of people creating async methods with no enforcement that the semantics make sense.

While I can understand the argument of "Well they should make sense" this just isn't the reality in any large codebase (including Visual Studio).

imho a better approach here would be to have this analyzer only operate on a known set of blessed framework methods. File.ReadAllText and File.ReadAllTextAsync are obviously equivalent to each other and we can know this. We cannot know this for XYZAsync() and XYZ().

this analyzer in its current form is something I am always going to recommend be off by default because of the number of false positives this approach will find. A more targeted analyzer that operates on a set of methods that we know the semantics for I can vote to turn on by default.

[jeffhandley]

Thanks for the feedback and input, @jmarolf. I'll plan for us to spend more time iterating on this, getting usage feedback, and determining how to move the analyzer forward.

[carlossanlop]

LGTM.

@jmarolf the analyzer is currently Disabled. Can we merge it as is? We can continue improving it afterwards if needed.

[buyaa-n]

To be honest I am skeptical that the general approach that this analyzer is taking can work. There are just so many places where just because two methods are named XYZAsync() and XYZ() does not mean they are semantically the same except for one being an async implementation. Like the Dispose() pattern there has been 10+ years of people creating async methods with no enforcement that the semantics make sense.

While I can understand the argument of "Well they should make sense" this just isn't the reality in any large codebase (including Visual Studio).

As you mentioned this exists in VS and was requested by internal partners to be added into SDK which I think shows it was useful. We all agree that it will be noisy and we are planning to make improvements in 7.0, Dispose() pattern example you mentioned could be covered there

imho a better approach here would be to have this analyzer only operate on a known set of blessed framework methods.

For me, its might be better to exclude known exceptions from the diagnostics, but i have no strong opinion here

this analyzer in its current form is something I am always going to recommend be off by default because of the number of false positives this approach will find. A more targeted analyzer that operates on a set of methods that we know the semantics for I can vote to turn on by default.

Yes, we disabled the analyzer as you suggested, please take another look @jmarolf

[jeffhandley]

Thanks for the feedback and input, @jmarolf. I'll plan for us to spend more time iterating on this, getting usage feedback, and determining how to move the analyzer forward.

I realized this comment might have been unclear on the intent. I would still like to include this analyzer in RC2, and then proactively seek early adopters of it who can provide feedback to us to measure the true-positive vs. false-positive ratio with the current design. We can then iterate on the implementation during the 7.0 cycle based on feedback.