Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add new SetAction overload to avoid fire & forget silent error #2565

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

Merged
merged 3 commits into from
Jun 6, 2025

Conversation

adamsitnik
Copy link
Member

fixes #2562

cc @ericstj

hide it from intellisense, so the users are more likely to use the overload that takes CancellationToken
@adamsitnik adamsitnik requested a review from jonsequitur June 3, 2025 13:43
src/System.CommandLine/Command.cs Show resolved Hide resolved
src/System.CommandLine.Tests/Invocation/InvocationTests.cs Outdated Show resolved Hide resolved
/// </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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be simpler to differentiate the methods by name rather than using overloads, e.g. SetAction and SetAsyncAction?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be simpler to differentiate the methods by name rather than using overloads, e.g. SetAction and SetAsyncAction?

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this 🙏

@adamsitnik adamsitnik requested a review from jonsequitur June 4, 2025 12:40
@KalleOlaviNiemitalo
Copy link

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 void is explicit.

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.

@adamsitnik
Copy link
Member Author

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 void is explicit.

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?

@adamsitnik adamsitnik merged commit 806a6d9 into dotnet:main Jun 6, 2025
10 checks passed
@KalleOlaviNiemitalo
Copy link

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.

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

Successfully merging this pull request may close these issues.

SetAction overloads should have Task options without CancellationToken
4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.