Assert the shape of bound tree created fo user defined conversions by AlekseyTs · Pull Request #81896 · dotnet/roslyn

AlekseyTs · 2026-01-07T01:19:47Z

No description provided.

jcouv · 2026-01-07T18:22:42Z

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

         Conversion is represented by a single BoundConversion.
         -->
    <Field Name="ConversionGroupOpt" Type="ConversionGroup?"/>
+    <Field Name="InConversionGroupFlags" Type="InConversionGroupFlags"/>


nit: Is there a way we could avoid storing this in RELEASE build (if we only read it for assertions)? #Resolved

nit: Is there a way we could avoid storing this in RELEASE build (if we only read it for assertions)?

I am planning to use this information in "production" code in upcoming changes

jcouv · 2026-01-07T18:45:31Z

src/Compilers/CSharp/Portable/BoundTree/ConversionGroup.cs

+    /// <summary>
+    /// Flags assigned to individual conversion nodes associated with
+    /// the same <see cref="ConversionGroup"/> instance.
+    /// </summary>


Consider mentioning this is only meant to drive some assertions #Resolved

Consider mentioning this is only meant to drive some assertions

That is not the intent long term

jcouv · 2026-01-07T18:47:40Z

src/Compilers/CSharp/Portable/BoundTree/BoundConversion.cs

+                        ExceptionUtilities.UnexpectedValue(InConversionGroupFlags);
+                    }
+                }
+            }


Is the goal to shrink usage of InConversionGroupFlags.Unspecified over time? Consider an assertion covering the remaining unspecified cases, hopefully that triggers when new conversion kinds are added. #Resolved

Is the goal to shrink usage of InConversionGroupFlags.Unspecified over time?

No, there is no such goal.

jcouv · 2026-01-07T18:50:05Z

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

                // Need to represent the implicit conversion as a node in order to be able to produce correct diagnostics.
-                left = new BoundConversion(left.Syntax, left, conversion, node.Operator.Kind.IsChecked(),
-                                           explicitCastInCode: false, conversionGroupOpt: null, constantValueOpt: null, type: node.Operator.LeftType);
+                left = node.LeftConversion;


Is the above comment still applicable? #Resolved

Is the above comment still applicable?

I think it is.

jcouv · 2026-01-07T18:58:09Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

-                            new BoundConversion(boundRHS.Syntax, boundRHS, Conversion.Deconstruction, @checked: false, explicitCastInCode: false, conversionGroupOpt: null,
-                                constantValueOpt: null, type: type, hasErrors: true),
+                            new BoundConversion(boundRHS.Syntax, boundRHS, Conversion.Deconstruction, @checked: false, explicitCastInCode: false,
+                                conversionGroupOpt: null, inConversionGroupFlags: InConversionGroupFlags.Unspecified, constantValueOpt: null, type: type, hasErrors: true),


inConversionGroupFlags:

nit: I'd probably not specify parameter name for InConversionGroupFlags arguments, unless required because of existing ordering, as there is no risk of accidentally mixing up arguments. Applies to a bunch of locations #Resolved

I'd probably not specify parameter name for InConversionGroupFlags arguments, unless required

Done

jcouv · 2026-01-07T19:00:35Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs

        public bool Equals(Conversion other)
        {
-            return this.Kind == other.Kind && this.Method == other.Method;
+            return this.Kind == other.Kind && Equals(this._uncommonData, other._uncommonData);


nit: just curious, how did you spot this? #Resolved

how did you spot this?

Some logic that I was adding to Validate wasn't quite working because of false positives from comparison operators. I think that logic was changed, but the problem got noticed.

jcouv

LGTM Thanks (commit 1)

AlekseyTs · 2026-01-07T19:09:07Z

@dotnet/roslyn-compiler For a second review

CyrusNajmabadi · 2026-01-07T19:36:05Z

src/Compilers/CSharp/Portable/BoundTree/ConversionGroup.cs

+    [Flags]
+    internal enum InConversionGroupFlags : ushort
+    {
+        Unspecified = 0,


Does 'unspecified' reflect in a point in time where we're just choosing to not provide more specific data? or is it a strong value that we woudl expect to stay the same in all the locations were currently passing it in.

My guess is the former, but it's not readily apparent to me. #Resolved

Put another way, will we likkely update some of the places that currently pass 'unspecified' to pass something more specific when we realize there are useful conversion checks we want?

Put another way, will we likkely update some of the places that currently pass 'unspecified' to pass something more specific when we realize there are useful conversion checks we want?

I think making a change like that in the future would be fine.

CyrusNajmabadi · 2026-01-07T19:38:10Z

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

                                @checked: conversion.Checked,
                                explicitCastInCode: conversion.ExplicitCastInCode,
-                                conversionGroupOpt: null,
+                                conversionGroupOpt: null, inConversionGroupFlags: InConversionGroupFlags.TupleBinaryOperatorPendingLowering,


could you wrap this to keep each arg on its own line like before? tnx. #Resolved

could you wrap this to keep each arg on its own line like before?

Done

CyrusNajmabadi · 2026-01-07T19:39:07Z

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

                // Need to represent the implicit conversion as a node in order to be able to produce correct diagnostics.
-                left = new BoundConversion(left.Syntax, left, conversion, node.Operator.Kind.IsChecked(),
-                                           explicitCastInCode: false, conversionGroupOpt: null, constantValueOpt: null, type: node.Operator.LeftType);
+                left = node.LeftConversion;


i'm curious why this was changed. can you provide more info on that? #Resolved

i'm curious why this was changed. can you provide more info on that?

We already have the conversion node, no need to "re-invent" it

CyrusNajmabadi · 2026-01-07T19:40:22Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs

-            Assert.Equal(expectedConversion, conversionOperation.GetConversion());
+            Conversion conversion = conversionOperation.GetConversion();
+            Assert.Equal(ConversionKind.CollectionExpression, conversion.Kind);
+            AssertEx.SequenceEqual(ImmutableArray.Create(Conversion.Boxing, Conversion.Boxing), conversion.UnderlyingConversions);


since SeqEqual is really just validating same values in same order, the above may be clearer as

Suggested change

AssertEx.SequenceEqual(ImmutableArray.Create(Conversion.Boxing, Conversion.Boxing), conversion.UnderlyingConversions);

AssertEx.SequenceEqual([Conversion.Boxing, Conversion.Boxing], conversion.UnderlyingConversions);

as it's not really needed to specify the underlying temp collection type used to group the values. #Resolved

Assert the shape of bound tree created fo user defined conversions

a0505c3

AlekseyTs requested review from 333fred and RikkiGibson January 7, 2026 01:19

AlekseyTs requested a review from a team as a code owner January 7, 2026 01:19

AlekseyTs added the Feature - Unions label Jan 7, 2026

github-actions bot added the Area-Compilers label Jan 7, 2026

jcouv reviewed Jan 7, 2026

View reviewed changes

jcouv self-assigned this Jan 7, 2026

jcouv approved these changes Jan 7, 2026

View reviewed changes

AlekseyTs requested a review from a team January 7, 2026 19:08

CyrusNajmabadi reviewed Jan 7, 2026

View reviewed changes

CyrusNajmabadi approved these changes Jan 7, 2026

View reviewed changes

PR feedback

eecc087

AlekseyTs enabled auto-merge (squash) January 7, 2026 20:38

AlekseyTs merged commit b5872bb into dotnet:main Jan 7, 2026
28 of 29 checks passed

dotnet-policy-service bot added this to the Next milestone Jan 7, 2026

dotnet-bot mentioned this pull request Jan 8, 2026

[Automated] PRs inserted in VS build main-11407.256 #81917

Closed

davidwengier mentioned this pull request Jan 12, 2026

Update Razor and Roslyn dotnet/vscode-csharp#8877

Merged

dotnet-maestro bot mentioned this pull request Jan 13, 2026

[main] Source code updates from dotnet/roslyn dotnet/dotnet#4162

Merged

davidwengier modified the milestones: Next, 18.4 Jan 27, 2026

	AssertEx.SequenceEqual(ImmutableArray.Create(Conversion.Boxing, Conversion.Boxing), conversion.UnderlyingConversions);
	AssertEx.SequenceEqual([Conversion.Boxing, Conversion.Boxing], conversion.UnderlyingConversions);

Comments

Conversation

AlekseyTs commented Jan 7, 2026

Uh oh!

jcouv Jan 7, 2026 • edited by AlekseyTs 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!

jcouv Jan 7, 2026 • edited by AlekseyTs 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!

jcouv Jan 7, 2026 • edited by AlekseyTs 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!

jcouv Jan 7, 2026 • edited by AlekseyTs 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!

jcouv Jan 7, 2026 • edited by AlekseyTs 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!

jcouv Jan 7, 2026 • edited by AlekseyTs 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 7, 2026

Uh oh!

CyrusNajmabadi Jan 7, 2026 • edited by AlekseyTs 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!

CyrusNajmabadi Jan 7, 2026 • edited by AlekseyTs 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!

CyrusNajmabadi Jan 7, 2026 • edited by AlekseyTs 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!

CyrusNajmabadi Jan 7, 2026 • edited by AlekseyTs 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv Jan 7, 2026 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2026 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2026 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2026 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2026 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2026 •

edited by AlekseyTs

Loading

CyrusNajmabadi Jan 7, 2026 •

edited by AlekseyTs

Loading

CyrusNajmabadi Jan 7, 2026 •

edited by AlekseyTs

Loading

CyrusNajmabadi Jan 7, 2026 •

edited by AlekseyTs

Loading

CyrusNajmabadi Jan 7, 2026 •

edited by AlekseyTs

Loading