Skip to content

Fix bad jump from yield break inside catch in async-iterator#80927

Merged
jcouv merged 3 commits intodotnet:mainfrom
jcouv:jump-try
Nov 6, 2025
Merged

Fix bad jump from yield break inside catch in async-iterator#80927
jcouv merged 3 commits intodotnet:mainfrom
jcouv:jump-try

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Oct 28, 2025 •
edited
Loading

Fixes #79124

The problem is that given the following code:

       try
       {
           try
           {
               Write("Break ");
               yield break;
           }
           finally
           {
               Write("Throw ");
               throw null;
           }
       }
       catch
       {
           Write("Caught ");
           yield break;
       }
       finally
       {
           Write("Finally ");
       }

We'd lower to a tree like this (output from DumpSource() with some formatting/cleanups):

       // ...
       try
       {
           // ...
           <finallyEntry-41>: ;
       }
       catch ( NULL)
       {
           Write("Caught ");
           this.<>w__disposeMode = True;
           goto <finallyEntry-41>; // Wrong place to go to
       }
       finally
       {
           if (!temp1 IntEqual -1) goto <afterif-45>;
           {
               Write("Finally ");
           }
           <afterif-45>: ;
       }

       if (!this.<>w__disposeMode) 
           goto <afterif-46>;

       goto <exprReturn-33>;
       <afterif-46>: ;

The fix is to move the label after the try (output from DumpSource()):

           try
           {
                   try
                   {
                       Write("Break ");

                       this.<>w__disposeMode = True;
                       goto <finallyEntry-36>;
                   }
                   finally
                   {
                       if (!temp1 IntEqual -1) goto <afterif-37>;

                       Write("Throw ");
                       throw null;

                       <afterif-37>: ;
                   }
                   <finallyEntry-36>: ;
                   
                   if (!this.<>w__disposeMode) goto <afterif-38>;

                   goto <finallyEntry-35>;
                   <afterif-38>: ;
           }
           catch ( NULL)
           {
               Write("Caught ");

               this.<>w__disposeMode = True;
               goto <finallyEntry-35>;
           }
           finally
           {
               if (!temp1 IntEqual -1) goto <afterif-39>;

               Write("Finally ");
               <afterif-39>: ;
           }

           <finallyEntry-35>: ; // label is moved after the `try`
       }

       if (!this.<>w__disposeMode) goto <afterif-40>;
       goto <exprReturn-27>;
       <afterif-40>: ;

@jcouv jcouv self-assigned this Oct 28, 2025
@jcouv jcouv marked this pull request as ready for review October 28, 2025 17:21
@jcouv jcouv requested a review from a team as a code owner October 28, 2025 17:21
@jcouv jcouv marked this pull request as draft October 28, 2025 17:21
@jcouv jcouv marked this pull request as ready for review October 28, 2025 18:21
@jcouv jcouv requested a review from 333fred October 29, 2025 16:45
@jcouv jcouv requested a review from a team November 5, 2025 03:06
AlekseyTs
AlekseyTs reviewed Nov 5, 2025
View reviewed changes
...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs Outdated
tryBlock: F.Block(node.TryBlock, F.Label(finallyEntry)),
node.CatchBlocks, node.FinallyBlockOpt, node.FinallyLabelOpt, node.PreferFaultHandler);
tryEnd = F.GenerateLabel("tryEnd");
_currentDisposalLabel = tryEnd;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

_currentDisposalLabel = tryEnd;

According to documentation for _currentDisposalLabel: " Inside a try or catch with a finally, we'll use the label directly preceding the finally." This is no longer accurate. Please adjust documentation accordingly, perhaps also explaining why placing the label there gives us the right control flow. #Closed

AlekseyTs
AlekseyTs reviewed Nov 5, 2025
View reviewed changes
...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs Outdated
public override BoundNode VisitTryStatement(BoundTryStatement node)
{
var savedDisposalLabel = _currentDisposalLabel;
LabelSymbol tryEnd = null;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

tryEnd

I think the name is somewhat unclear or open to interpretation. Consider renaming to "afterTryCatchFinallyStatement" #Closed

AlekseyTs
AlekseyTs reviewed Nov 5, 2025
View reviewed changes
...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs Outdated

if (tryEnd != null)
{
// Append a label at the end of the `try`, which disposal within `try` can jump to to execute the `finally` (if any):
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(if any)

It looks like we will never get here without a finally. #Closed

AlekseyTs
AlekseyTs reviewed Nov 5, 2025
View reviewed changes
...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs Outdated

if (tryEnd != null)
{
// Append a label at the end of the `try`, which disposal within `try` can jump to to execute the `finally` (if any):
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

// Append a label at the end of the try, which disposal within try can jump to to execute the finally (if any):

Consider making the comment clearer. Something like: "Append a label immediately after the try-catch-finally statement, which disposal within try/catch blocks jumps to with a goal to pass control flow to the finally block" implicitly" #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 5, 2025 •
edited
Loading

Done with review pass (commit 2) #Closed

@jcouv jcouv requested a review from AlekseyTs November 6, 2025 04:08
AlekseyTs
AlekseyTs approved these changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3)

@jcouv jcouv merged commit 3cc056b into dotnet:main Nov 6, 2025
25 checks passed
@jcouv jcouv deleted the jump-try branch November 6, 2025 16:29
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 6, 2025
This was referenced Nov 7, 2025
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request Nov 8, 2025
Closed
@dotnet-maestro dotnet-maestro bot mentioned this pull request Dec 18, 2025
Merged
@davidwengier davidwengier modified the milestones: Next, 18.3 Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Async Streams Async Streams

Projects

None yet

Milestone

18.3

Development

Successfully merging this pull request may close these issues.

ILVerify Failure: TryFinally_YieldBreakInDisposeMode

4 participants

@jcouv @AlekseyTs @333fred @davidwengier