Add new SetAction overload to avoid fire & forget silent error#2565
Add new SetAction overload to avoid fire & forget silent error#2565adamsitnik merged 3 commits intodotnet:mainfrom
Conversation
hide it from intellisense, so the users are more likely to use the overload that takes CancellationToken
| /// </remarks> | ||
| // Hide from intellisense, it's public to avoid the compiler choosing a sync overload | ||
| // for an async action (and fire and forget issue described in https://github.com/dotnet/command-line-api/issues/2562). | ||
| [EditorBrowsable(EditorBrowsableState.Never)] |
There was a problem hiding this comment.
If you wanted to force folks to think about the CancellationToken you could also mark it obsolete -- not sure how hardcore you are about that.
There was a problem hiding this comment.
It was my first idea, but then I realized that we should most likely not release new public [Obsolete] methods. But I agree, it would be even better as it would allow us emit a compiler warning/error with a clear message.
@jonsequitur thoughts?
There was a problem hiding this comment.
Would it be simpler to differentiate the methods by name rather than using overloads, e.g. SetAction and SetAsyncAction?
There was a problem hiding this comment.
Would it be simpler to differentiate the methods by name rather than using overloads, e.g.
SetActionandSetAsyncAction?
I like the SetAsyncAction name idea (we never discussed actions at the API review), but users could still run into this particular bug and it would cause another wave of breaking changes in SDK/VMR etc. Let's discuss it offline, I think it's now or never.
There was a problem hiding this comment.
we should most likely not release new public [Obsolete] methods
dotnet/Open-XML-SDK#1600 added ObsoleteAttribute to several new types and methods. There though, they intend to remove the attribute eventually, unlike here.
|
I feel this deserves a more general Roslyn analyzer, which would warn whenever an async lambda expression as an argument of a call is implicitly converted to a void-returning delegate type. If the developer really wants an async void function, they can replace the lambda expression with a named local function so that the The analyzer could abstain from warning if there is no overload that would accept some sort of task-type-returning delegate instead. However, I suspect there wouldn't be too many warnings even without this check. |
It's great idea, would you like to open an issue in the analyzers repo? |
I found some pre-existing feature requests for warning about async void:
At this time then, it does not seem useful to file a new request there. But I'll be looking into whether I can start using the vs-threading analysers. |
fixes #2562
cc @ericstj