Async-streams: disposal should continue without jump within a finally by jcouv · Pull Request #46188 · dotnet/roslyn

jcouv · 2020-07-21T19:32:21Z

When we lower a finally in an async-iterator method, we follow it with a if (disposeMode) goto <nextDisposalLabel>;.
But when we're inside an outer finally, then we should have no jump. We just want to execute the remainder of the outer finally.

Fixes #43936

jcouv · 2020-07-21T19:36:37Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs

📝 this additional condition is the core of the change.
We don't want to generate a jump when inside a finally. We achieve that by stacking a null disposal label when visiting finally blocks (both real and extracted ones).

That last line "We don't want... We achieve..." would be a great comment here.

In reply to: 458340690 [](ancestors = 458340690)

RikkiGibson · 2020-07-23T17:07:56Z

Taking a look this morning.

RikkiGibson

Welp, forgot to submit my pending review yesterday. LGTM.

RikkiGibson · 2020-07-23T17:33:26Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs

                node = node.Update(
-                    tryBlock: F.Block(node.TryBlock, F.Label(finallyEntry)),
-                    node.CatchBlocks, F.Block(node.FinallyBlockOpt, F.Label(finallyExit)), node.FinallyLabelOpt, node.PreferFaultHandler);
+                     tryBlock: F.Block(node.TryBlock, F.Label(finallyEntry)),


nit: indentation is off here

RikkiGibson · 2020-07-23T18:26:07Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+        finally
+        {
+            Console.Write(""BEFORE "");
+            try { Console.Write(""INSIDE ""); } finally { Console.Write(""INSIDE2 ""); }


Consider formatting this in a conventional style so it is easier to scan.

jcouv · 2020-07-24T21:37:04Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs

-            //  goto _enclosingFinallyOrExitLabel;
+            //  goto currentDisposalLabel;

            return F.Block(


put a similar assertion: Debug.Assert(CurrentDisposalLabel is object); // no yield return allowed inside a finally

jcouv · 2020-07-24T21:38:54Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+        finally
+        {
+            Console.Write(""BEFORE "");
+            try { Console.Write(""INSIDE ""); } finally { Console.Write(""INSIDE2 ""); await Task.Yield(); }


format please

gafter · 2020-07-24T21:45:58Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs

        private readonly AsyncIteratorInfo _asyncIteratorInfo;

        /// <summary>
+        /// Where should we jump to to continue the execution of disposal path.


Where [](start = 12, length = 5)

We usually implement a "stack" using the compiler's execution stack (i.e. "previous=current; current=newvalue; dostuff; current=previous;" and a simple variable rather than a stack data structure.
I have no problem using an explicit stack data structure - it is certainly more clear. But please say that it is a stack in the doc here.

Now that you mention it, I think I'll change to use a simple variable (typical pattern) rather than an explicit stack data structure.

gafter

😷 Looks good with minor comments about comments.

gafter · 2020-07-24T23:10:21Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs

-
-            // When exiting the try statement, we restore the previous disposal label.
-            _enclosingFinallyEntryOrFinallyExitOrExitLabel = _previousDisposalLabels.Pop();
+            if (changedDisposalLabel)


changedDisposalLabel [](start = 16, length = 20)

You don't need to track whether or not it changed. You can just unconditionally set it to what it used to be.

Will do. Thanks

gafter

😷 Looks good, with a small suggestion to further simplify.

…-pointers * upstream/master: (207 commits) Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) Skip a flaky test Relates to dotnet#46304 ...

…to function-pointer-type-lookup * upstream/features/function-pointers: (212 commits) Correct public API number. Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) ...

jcouv added Area-Compilers Feature - Async Streams Async Streams labels Jul 21, 2020

jcouv added this to the 16.8 milestone Jul 21, 2020

jcouv self-assigned this Jul 21, 2020

jcouv commented Jul 21, 2020

View reviewed changes

Async-streams: disposal should continue without jump within a finally

076f05c

jcouv force-pushed the async-finally branch from e75af56 to 076f05c Compare July 21, 2020 20:41

jcouv marked this pull request as ready for review July 21, 2020 23:02

jcouv requested a review from a team as a code owner July 21, 2020 23:02

RikkiGibson approved these changes Jul 24, 2020

View reviewed changes

Adjust format

19bf518

gafter self-requested a review July 24, 2020 20:07

jcouv commented Jul 24, 2020

View reviewed changes

gafter reviewed Jul 24, 2020

View reviewed changes

gafter approved these changes Jul 24, 2020

View reviewed changes

Julien Couvreur added 2 commits July 24, 2020 14:53

Tweaks

d4a7ebf

Don't allocate a stack structure

08b6922

gafter reviewed Jul 24, 2020

View reviewed changes

Simplify

76b5bbc

jcouv merged commit a1fcf0a into dotnet:master Jul 25, 2020

ghost modified the milestones: 16.8, Next Jul 25, 2020

jcouv deleted the async-finally branch July 25, 2020 05:17

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

Conversation

jcouv commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Jul 23, 2020

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcouv commented Jul 21, 2020 •

edited

Loading