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

Caller of an async local function cannot expect any of the callee’s code to execute #64482

Merged
merged 6 commits into from
Oct 27, 2022

Conversation

AlekseyTs
Copy link
Contributor

Fixes #43697.

@AlekseyTs AlekseyTs requested a review from a team as a code owner October 4, 2022 20:56
@AlekseyTs
Copy link
Contributor Author

@RikkiGibson, @dotnet/roslyn-compiler Please review

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@RikkiGibson, @dotnet/roslyn-compiler Please review

@RikkiGibson RikkiGibson self-assigned this Oct 6, 2022
@RikkiGibson
Copy link
Contributor

Should we update the breaking change doc in this PR?
Will we perform any test insertions to VS or runtime before merging to gauge the impact of the breaking change?


if (!symbol.IsAsync)
{
Meet(ref State, ref localFunctionState.StateFromTop);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a follow-up issue to track the decision to also perform this Meet when the call is "immediately awaited"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have a follow-up issue to track the decision to also perform this Meet when the call is "immediately awaited"?

That is a language design change that I am not planning to champion at the moment.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@jcouv jcouv self-assigned this Oct 7, 2022
@AlekseyTs
Copy link
Contributor Author

@RikkiGibson

Will we perform any test insertions to VS or runtime before merging to gauge the impact of the breaking change?

It looks like test insertion succeeded - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6814361&view=results

@RikkiGibson
Copy link
Contributor

That indicates success of creating the insertion, located at https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/429093. We should see the results for that by this afternoon.

@@ -1,5 +1,32 @@
# This document lists known breaking changes in Roslyn after .NET 6 all the way to .NET 7.

## Caller of an async local function cannot expect any of the callee�s code to execute.
Copy link
Member

@jcouv jcouv Oct 13, 2022

Choose a reason for hiding this comment

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

nit: GitHub diff shows some unicode character as the apostrophe for "callee's". Is that intentional? #Closed


***Introduced in Visual Studio 2022 version 17.5***

See https://github.com/dotnet/roslyn/issues/43697 for the rational.
Copy link
Member

@jcouv jcouv Oct 13, 2022

Choose a reason for hiding this comment

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

typo: rationale #Closed


***Introduced in Visual Studio 2022 version 17.5***

See https://github.com/dotnet/roslyn/issues/43697 for the rational.
Copy link
Member

@jcouv jcouv Oct 13, 2022

Choose a reason for hiding this comment

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

Consider adding a one sentence summary, maybe something like:
For definite assignment analysis, invocations of async local functions are no longer treated as being awaited and therefore the local function is not considered executed.

Let's send an email as FYI to compat council for new compat break.

It would be good to add a note to the spec even though it's an older/incomplete document, and tag Rex on the edit (so that definite assignment rules in standard can reflect this change as appropriate). #Closed

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 3)

@jcouv jcouv self-requested a review October 13, 2022 08:09
@AlekseyTs
Copy link
Contributor Author

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5)

@AlekseyTs AlekseyTs enabled auto-merge (squash) October 27, 2022 13:13
@AlekseyTs AlekseyTs merged commit d2f6077 into dotnet:main Oct 27, 2022
@ghost ghost added this to the Next milestone Oct 27, 2022
@allisonchou allisonchou removed this from the Next milestone Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Definite assignment bug when not awaiting an async closure
4 participants