Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Oct 28, 2025

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
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

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

public override BoundNode VisitTryStatement(BoundTryStatement node)
{
var savedDisposalLabel = _currentDisposalLabel;
LabelSymbol tryEnd = null;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2025

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


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

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


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

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

Done with review pass (commit 2) #Closed

@jcouv jcouv requested a review from AlekseyTs November 6, 2025 04:08
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
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.

ILVerify Failure: TryFinally_YieldBreakInDisposeMode

3 participants