Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA1849 ("Call async methods when in an async method") needs to consider related differences in signatures #6267

Open
stephentoub opened this issue Nov 15, 2022 · 4 comments

Comments

@stephentoub
Copy link
Member

Analyzer

Diagnostic ID: CA1849

Describe the improvement

Currently, CA1849 only considers an async method to be a counterpart to a sync method if it has exactly the same arguments as the sync method. That's rare, however. Typically at a minimum the async counterpart will at least have an additional CancellationToken argument, and it's relatively common for a T[] or {ReadOnly}Span<T> in the sync method to be a {ReadOnly}Memory<T> in the async method. The analyzer should allow such differences when determining whether an async method is a counterpart to a sync method.

@cremor
Copy link

cremor commented Jul 27, 2023

This limitation is extremely annoying and makes this analyzer nearly useless because the (optional) CancellationToken parameter is on nearly all async methods.

Also: I'm not sure if this would already work if CancellationToken parameters would be supported, but this analyzer should also find and suggest Async overloads from extension methods which are in different assemblies/namespaces (e.g. IEnumerable<T>.ToList() and IQueryable<T>.ToListAsync() from EF Core).

@mahdiva @Youssef1313 Any chance to get this fixed soon, maybe in the context of dotnet/runtime#78442?

@stephentoub
Copy link
Member Author

stephentoub commented Jul 27, 2023

but this analyzer should also find and suggest Async overloads from extension methods which are in different assemblies/namespaces (e.g. IEnumerable.ToList() and IQueryable.ToListAsync() from EF Core).

I don't think that's generally desirable. While it's not guaranteed that two overloads on the same type provide the same semantics, there's one owner of that type and it's a general design guideline that they behave equivalently. The same can't be said of two methods on two different types in two different assemblies that happen to be named the same.

@cremor
Copy link

cremor commented Jul 27, 2023

Ok, this might be true in general. But since there are no async overloads for all that methods that execute queryables in the base framework, each ER mapping library has to provide its own extension methods. And for users it's important that they use those async extension methods to stay fully async.

How about whitelisting IEnumerable/IQueryable extension methods, so that those can find the async overloads in the ER mapping libraries?

@cremor
Copy link

cremor commented Jul 28, 2023

About the core issue: It looks like this change from #6719 might have already fixed part of this by allowing all parameters that have a default value. That would be a very important improvement.

But of course the Array/Span/Memory improvements would still be good to have.
Also maybe CancellationToken could be allowed even if it isn't a default parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants