meziantou / Meziantou.Analyzer

A Roslyn analyzer to enforce some good practices in C#.
MIT License
947 stars 49 forks source link

MA0147 clarifications #677

Closed bdovaz closed 9 months ago

bdovaz commented 9 months ago

I see that recently this rule has been added and as I have warning as errors I have started to fail the code at several points.

Does the severity make sense to be warning instead of information? I mean, in my case where I use "async () => {}" is where I care within the lambda expression itself to do await and wait for each other but only applicable to that scope.

Typical example, event aggregator like the one in Prism.

eventAggregator
    .GetEvent<OnUserCreated>()
    .Subscribe(async user => await MyAsyncCodeForCreatedUser(user));

@meziantou how do you solve this example? And not fire and forget, of course.

Thanks!

meziantou commented 9 months ago

There are many cases where it make sense. If it's not the case in your code, you can use the .editorconfig file to change the rule's severity. Or you can disable the warning locally using #pragma warning disable or an attribute.

how do you solve this example? And not fire and forget, of course.

If the library doesn't provide an overload that accept a Task, ValueTask or any type decorated with the AsyncMethodBuilder attribute, there is no way to correctly fix the issue.

Throwing an exception in an async void method is not something you want as it may crash the app. So, you should at least use a try/catch (or a fire and forget).

bdovaz commented 9 months ago

I still think that warning is too much because it is a rule that is not possible to comply with when you use API that is not yours. Typical examples are event delegates where you put async void or async () => {} and those are impossible to fix.

In the documentation you should clarify what to do in cases where you don't control the code (if anything can be done at all): https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0147.md

meziantou commented 9 months ago

The rule was not intended to report events. It should be fixed soon.