Assert shape of the bound tree #81486

jcouv · 2025-11-29T22:44:30Z

This PR generalizes the LocalRewritingValidator enforcement and allows declaring how far bound nodes are supposed to survive in the lowering pipeline.
Addresses part of #16057

CyrusNajmabadi · 2025-11-29T23:11:44Z

Cool idea!

jjonescz · 2025-12-02T10:05:19Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+    LocalRewriting,
+    ClosureConversion,
+    StateMachineRewriting,
+    None


Perhaps this should be called "All" instead of "None"? Since "does not survive none" sounds wrong. #Resolved

jjonescz · 2025-12-02T10:06:51Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

-  <!--
-  This node will not survive the local rewriting,
-  except in some scenarios in a Linq Expression Tree when the containing node
-  survives.


except in some scenarios in a Linq Expression Tree

Are these cases tested? Why doesn't the validator fail for them? Are the bound nodes flagged as erroneous? #WontFix

I was not able to hit that, so I don't think the comment was correct. I'll revisit if we do end up hitting the assertion

jjonescz · 2025-12-02T10:10:24Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+}
+
+/// <summary>
+/// Note: do not use a static/singleton instance of this type, as it holds state.


nit: this comment looks unnecessary since the class has a private constructor #Resolved

jjonescz · 2025-12-02T10:10:46Z

src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/BoundNodeClassWriter.cs

@@ -6,6 +6,7 @@

 using System;
 using System.Collections.Generic;
+using System.Diagnostics;


nit: this added using looks unnecessary #Resolved

jjonescz · 2025-12-02T10:12:12Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+    public static void AssertAfterInitialBinding(BoundNode node)
+    {
+#if DEBUG
+        Assert(node, PipelinePhase.InitialBinding);


Consider exposing just the single Assert method (and yes, having the enum outside #if DEBUG) to avoid this boilerplate #ByDesign

I had that initially, but that means shipping the enum and adding #if DEBUG at the call-sites.
I prefer this way, where we limit what is in the release assembly and we don't need #if DEBUG at the call-sites (we hide the boilerplate here)

I think the callsites wouldn't need #if DEBUG since the method is marked as [Conditional]

Let me try this again, but I think callsites do need #if DEBUG if a PipelinePhase is given as argument and that type is DEBUG-only.

Right, I meant that the enum wouldn't be DEBUG-only.

The current state seems fine to me though, thanks.

jjonescz · 2025-12-02T10:13:16Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+    }
+}
+
+#if DEBUG


Outside DEBUG, consider having a private constructor of PipelinePhaseValidator too (perhaps a one that throws unreachable exception). #Resolved

jjonescz · 2025-12-02T10:14:51Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

-            {
-                if (node is BoundIfStatement)
-                {
-                    Fail(node);


Looks like originally we didn't expect BoundIfStatement to survive local rewriting even if it had errors but now the check is relaxed. Perhaps we can preserve that by adding another knob to BoundNodes.xml ("doesn't survive even with errors"). #WontFix

That's true. However, I'm fine with this minor relaxation given that we're overall tightening and uniformizing the checks. Seems fine to lose this special handling.

AlekseyTs · 2025-12-03T17:50:59Z

DoesNotSurvive="InitialBinding"?

BTW, it looks like content of this file is out of date. #Closed

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:305 in b5ae4aa. [](commit_id = b5ae4aa, deletion_comment = False)

AlekseyTs · 2025-12-03T17:53:32Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+    InitialBinding,
+    LocalRewriting,
+    ClosureConversion,
+    StateMachineRewriting,


StateMachineRewriting

Would it make sense to make things more granular and split this one into "IteratorRewriting" and "AsyncRewriting"? #Closed

I don't think we can. There can be yields remaining after we passed the stage of doing iterator rewriting. Such yields are handled by async/async-iterator rewriter.

AlekseyTs · 2025-12-03T18:09:44Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+    LocalRewriting,
+    ClosureConversion,
+    StateMachineRewriting,
+    All


All

"All" feels somewhat strange. This enum is not a [Flags] enum. And returning All from DoesNotSurvive also feels strange. It can be interpreted as "node cannot survive any phase". Only after examining enum underlying values and DoesNotSurvive call sites it becomes clear that the meaning is the opposite. Perhaps "Emit" name would be clearer. It is true that emit phase is not a rewrite phase, but we could pretend that result of "Emit" is an empty tree. #Closed

AlekseyTs · 2025-12-03T18:10:37Z

src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs

        {

-            RoslynDebug.Assert(binder is object, "Field 'binder' cannot be null (make the type nullable in BoundNodes.xml to remove this check)");
-            RoslynDebug.Assert(valueSymbol is object, "Field 'valueSymbol' cannot be null (make the type nullable in BoundNodes.xml to remove this check)");


ug.Assert(valueSy

This feels strange, see a comment for the enum member declaration. #Closed

AlekseyTs · 2025-12-03T18:15:09Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

                }

                codeCoverageSpans = codeCoverageInstrumenter?.DynamicAnalysisSpans ?? ImmutableArray<SourceSpan>.Empty;
+                PipelinePhaseValidator.AssertAfterLocalRewriting(loweredStatement);


PipelinePhaseValidator.AssertAfterLocalRewriting(loweredStatement);

It feels like this should be done right after var loweredStatement = localRewriter.VisitStatement(statement); and SpillSequenceSpiller.Rewrite should have its own phase
#Closed

Similarly, it feels like localRewriter.AssertNoPlaceholderReplacements(); should be done earlier too.

AlekseyTs · 2025-12-03T18:21:02Z

src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/Model.cs

+        /// See PipelinePhase enum
+        /// </summary>
+        [XmlAttribute]
+        public string DoesNotSurvive;
    }

    public class Kind


Kind

Is this type used? #Closed

AlekseyTs · 2025-12-03T18:23:26Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+
+internal sealed partial class PipelinePhaseValidator
+{
+    private PipelinePhaseValidator()


private PipelinePhaseValidator()

Would it make sense to make the type static? #Closed

I see below that we are actually creating instances of this type. Then why do we need this constructor?

AlekseyTs · 2025-12-03T18:27:00Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+    All
+}
+
+internal sealed partial class PipelinePhaseValidator : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator


class PipelinePhaseValidator

Splitting declaration in the same file feels unexpected. Consider defining enum at the top and merging declarations #Closed

Consider also following a general pattern for member declarations: first fields, then constructors, then the rest.

AlekseyTs · 2025-12-03T18:34:06Z

Done with review pass (commit 2) #Closed

AlekseyTs · 2025-12-03T22:42:15Z

I didn't make a mistake regarding "initial binding" in my original comment.

In reply to: 3608074660

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:305 in b5ae4aa. [](commit_id = b5ae4aa, deletion_comment = False)

jcouv · 2025-12-03T22:55:42Z

My mistake

In reply to: 3609153340

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:305 in b5ae4aa. [](commit_id = b5ae4aa, deletion_comment = False)

AlekseyTs · 2025-12-03T23:03:59Z

Done with review pass (commit 4) #Closed

AlekseyTs

LGTM (commit 5)

jjonescz · 2025-12-04T20:12:37Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/PipelinePhaseValidator.cs

+#endif
+
+#if DEBUG
+    private PipelinePhaseValidator(PipelinePhase completedPhase)


Should we have a private constructor outside DEBUG too, perhaps a throwing one? (I think you've added it previously, but it seems removed now.)

Aleksey asked why have the constructor.
It's not necessary, but the benefit is it would help avoid misuse of the type. But I'm not too worried about that.

jcouv self-assigned this Nov 29, 2025

github-actions bot added the Area-Compilers label Nov 29, 2025

jcouv force-pushed the assert-lowering branch 5 times, most recently from 142ac09 to 70574bb Compare December 1, 2025 16:40

Assert shape of the bound tree

70a0b95

jcouv force-pushed the assert-lowering branch from 70574bb to 70a0b95 Compare December 1, 2025 18:47

jcouv marked this pull request as ready for review December 1, 2025 22:09

jcouv requested a review from a team as a code owner December 1, 2025 22:09

jjonescz reviewed Dec 2, 2025

View reviewed changes

Address feedback

b5ae4aa

jcouv requested review from a team and jjonescz December 3, 2025 15:15

jjonescz approved these changes Dec 3, 2025

View reviewed changes

AlekseyTs reviewed Dec 3, 2025

View reviewed changes

Julien Couvreur added 2 commits December 3, 2025 14:24

Address feedback

6a11cfe

Merge remote-tracking branch 'dotnet/main' into assert-lowering

7a343f0

Fix TypeOrValue to InitialBinding

ab50e39

jcouv requested a review from AlekseyTs December 4, 2025 17:20

AlekseyTs approved these changes Dec 4, 2025

View reviewed changes

jjonescz reviewed Dec 4, 2025

View reviewed changes

jjonescz approved these changes Dec 4, 2025

View reviewed changes

jcouv merged commit ca43b6b into dotnet:main Dec 4, 2025
29 checks passed

jcouv deleted the assert-lowering branch December 4, 2025 21:04

dotnet-policy-service bot added this to the Next milestone Dec 4, 2025

jcouv mentioned this pull request Dec 4, 2025

We should have a way to assert the shape of the bound tree before and after lowering #16057

Closed

dotnet-maestro bot mentioned this pull request Dec 5, 2025

[release/10.0.2xx] Source code updates from dotnet/roslyn dotnet/dotnet#3690

Merged

This was referenced Dec 6, 2025

[Automated] PRs inserted in VS build main-11305.107 #81568

Closed

[Automated] PRs inserted in VS build feature.debugger.stackAnalysis-11308.44 #81588

Closed

Assert shape of the bound tree #81486

Assert shape of the bound tree #81486

Uh oh!

Conversation

jcouv commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 29, 2025

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

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!

jjonescz Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jcouv commented Nov 29, 2025 •

edited

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

jjonescz Dec 3, 2025 •

edited

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

jjonescz Dec 2, 2025 •

edited by jcouv

Loading

AlekseyTs commented Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs Dec 3, 2025 •

edited

Loading

AlekseyTs commented Dec 3, 2025 •

edited

Loading

AlekseyTs commented Dec 3, 2025 •

edited

Loading