Add initial support for Union matching by AlekseyTs · Pull Request #81188 · dotnet/roslyn

AlekseyTs · 2025-11-12T23:08:57Z

Relates to test plan #81074

dotnet-policy-service · 2025-11-12T23:09:29Z

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

AlekseyTs · 2025-11-13T14:02:43Z

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review

RikkiGibson

Taking a look now.

333fred · 2025-11-13T23:39:18Z

+      (LeftOfPendingConjunction is null), or a conjunction with a Union matching operation
+      on the right hand side. The left hand side of the conjunction is then
+      represented by the LeftOfPendingConjunction child, and it is not applied
+      to the ValueProperty. It represents a pattern that is matched emmidiately


Suggested change

to the ValueProperty. It represents a pattern that is matched emmidiately

to the ValueProperty. It represents a pattern that is matched immediately

``` #Resolved

333fred · 2025-11-13T23:39:42Z

+
+      A Union matching operation corresponding to the top most BoundPatternWithUnionMatching node
+      represents the last Union matching operation in evaluation order in the entire hierarchy.
+      The preseeding union matching operation in evaluation order, if any, corresponds


Suggested change

The preseeding union matching operation in evaluation order, if any, corresponds

The preceding union matching operation in evaluation order, if any, corresponds

``` #Resolved

333fred · 2025-11-14T19:19:27Z

 {
    partial class Binder
    {
+        private NamedTypeSymbol? PrepareForUnionMatching(SyntaxNode node, ref TypeSymbol inputType, BindingDiagnosticBag diagnostics)


Not sure how I feel about the naming of this method. I think TryGetUnionTypeForMatching or similar would be a better name, I'd expect a method with the name Prepare to actually return nothing. #Resolved

333fred

Done implementation review, only some minor comments. Will start reviewing tests tomorrow.

333fred · 2025-11-17T18:05:36Z


            public override BoundNode? Visit(BoundNode? node)
            {
+                Debug.Assert(node is not BoundPattern { IsUnionMatching: true });


Think inverting would make this clearer:

Suggested change

Debug.Assert(node is not BoundPattern { IsUnionMatching: true });

Debug.Assert(node is BoundPattern { IsUnionMatching: false });

``` #Resolved

Think inverting would make this clearer

I do not mean to assert that node is a pattern, which the suggestion would do. I mean to assert that it is not a pattern utilizing union matching. It is quite possible that this visitor can handle only patterns today, but that is orthogonal and I intentionally do not try to include this into the assert.

Well, the very next assertion is that the visitor only handles very specific pattern nodes, so I think it would be fine to strengthen this assertion and make it more readable at the same time.

I do not intend to mix things and find current assert simple and clear enough.

333fred · 2025-11-17T19:07:11Z

+        public override BoundNode? VisitListPattern(BoundListPattern node)
+        {
+            Symbol? variable = node.Variable;
+            ImmutableArray<BoundPattern> subpatterns = this.VisitList(node.Subpatterns).SelectAsArray(p => RewritePatternWithUnionMatchingToPropertyPattern(p));


Alternatively, it can be rewritten to just be .SelectAsArray(RewritePatternWithUnionMatchingToPropertyPattern).

Suggested change

ImmutableArray<BoundPattern> subpatterns = this.VisitList(node.Subpatterns).SelectAsArray(p => RewritePatternWithUnionMatchingToPropertyPattern(p));

ImmutableArray<BoundPattern> subpatterns = this.VisitList(node.Subpatterns).SelectAsArray(static p => RewritePatternWithUnionMatchingToPropertyPattern(p));

``` #Resolved

333fred · 2025-11-17T19:11:28Z

+                    {
+                        // Update LeftOfPendingConjunction with the conjunction of left and LeftOfPendingConjunction
+
+                        // The code below unwraps the following reqursive operation:


Suggested change

// The code below unwraps the following reqursive operation:

// The code below unwraps the following recursive operation:

``` #Resolved

333fred · 2025-11-17T20:57:04Z

+                    , narrowedTypeCandidates
+#endif
+                    );
+            } while (binaryPatternStack.TryPop(out binaryPattern));


Suggested change

} while (binaryPatternStack.TryPop(out binaryPattern));

}

while (binaryPatternStack.TryPop(out binaryPattern));

``` #Resolved

333fred · 2025-11-17T21:43:14Z

+                        inputType: leftOfPendingConjunction?.NarrowedType ?? inputType,
+                        narrowedType: leftOfPendingConjunction?.NarrowedType ?? inputType).MakeCompilerGenerated();


Nit: extract a common variable? #Resolved

333fred · 2025-11-17T21:59:43Z

+                        {
+                            Pattern: { } nestedPattern,
+                            Member:
+                            { Type.SpecialType: SpecialType.System_Object, Symbol: var symbol } and


Nit: perhaps unionValueSymbol instead of just symbol? #Closed

perhaps unionValueSymbol instead of just symbol?

Well, we don't know that it is a union value symbol. We are yet to check that.

Then possibleUnionValueSymbol? Just symbol is rather vague.

RikkiGibson

Done review pass

RikkiGibson · 2025-11-18T05:42:53Z

+                Diagnostic(ErrorCode.ERR_UseDefViolation, "y").WithArguments("y").WithLocation(29, 16),
+                // (34,22): error CS8121: An expression of type 'S1?' cannot be handled by a pattern of type 'int'.
+                //         if (u is not int z)
+                Diagnostic(ErrorCode.ERR_PatternWrongType, "int").WithArguments("S1?", "int").WithLocation(34, 22)


I am curious what a pattern like u is not (S1 and int), or u is not ({ } and int) would do, if a "step by step" type narrowing would work, essentially. #Resolved

Adding scenarios to UnionMatching_14_Negated

RikkiGibson · 2025-11-18T06:02:01Z

+}
+";
+            var comp = CreateCompilation([src, IUnionSource], targetFramework: TargetFramework.NetCoreApp);
+            // PROTOTYPE: It looks like list pattern cannot work with union types.


I found it surprising that even prefixing with a type, e.g. u is int[] [10], didn't make things work. It seems inconsistent with recursive patterns. See related scenario in .NET Lab #Resolved

I found it surprising that even prefixing with a type,

I might be missing something, but I think a list pattern doesn't include a type. Therefore, obj is string[] [] is a type check against an array of string[].

Of course. My mistake. Thanks.

Right. It would have to be a conjunction, is int[] and [10]. We debating making list patterns part of the definition of a recursive pattern, but this syntax was a major "well how do we make that work" issue.

RikkiGibson · 2025-11-18T06:18:13Z

+
+            if (unionType is { TypeKind: not TypeKind.Struct })
+            {
+                underIsPattern = false;


I didn't follow why this is being done, could you please explain? #Closed

I didn't follow why this is being done, could you please explain?

When we are not underIsPattern, we don't permitDesignations. See an assignment and a comment below. Only BindIsPatternExpression passes true for underIsPattern and only BindUnaryPattern propagates it.

Since when we are doing Union matching, we are effectively in a sub-pattern, we are not directly under "is pattern", but we can still make things work for struct types, because they do not have an implicit null check for the Union instance itself. The effect of this logic can be observed in unit-tests UnionMatching_15_Negated and UnionMatching_16_Negated, for example.

RikkiGibson · 2025-11-18T06:22:54Z

+                Debug.Assert(!inputType.IsNullableType());
+                Debug.Assert(!negated);
+
+                negated ^= nestedPattern.IsNegated(out var negatedNestedPattern);


It looks like 'IsNegated' no longer digs through all the negations when union matching is involved, do the other existing call sites of IsNegated need to be updated to account for this? #Resolved

It looks like 'IsNegated' no longer digs through all the negations when union matching is involved, do the other existing call sites of IsNegated need to be updated to account for this?

I do not think any other places need an adjustment. This place needed an adjustment due to the fact that DefiniteAssignmentPass.VisitPattern never considers pattern locals under NegatedPattern as definitely assigned. In this particular situation they should be considered definitely assigned. Therefore, we perform a semantically equivalent rewrite by pulling negation out of the sub-pattern. When applied to a struct type a pattern {Value: not (...) } is equivalent to a pattern not {Value: (...) }

333fred

Generally looking good to me. A few comments on tests, but nothing that blocks merging.

333fred · 2025-11-18T17:34:49Z

+            // PROTOTYPE: This looks strange. C5 simply inherits from C1 and because of that it is treated as a union type.
+            //            It doesn't change what IUnion.Value returns, but its constructors are treated as though they
+            //            define possible types returned by IUnion.Value.
+            VerifyCaseTypes(comp, "C5", ["System.String"]);


Definitely agreed that this should be discussed, but fwiw, my initial reaction is that redefinition of Value shouldn't drive whether constructors are considered to "add" to the list of valid cases. It feels a tad opaque and "spooky action at a distance" to me. #Resolved

I don't want to go into a discussion in context of this PR, but nothing is redefined here. We simply inherit from a Union class, that is all that is needed to mess things up.

333fred · 2025-11-18T18:13:52Z

+
+struct S2<T>
+{
+    public T Value;


Nit: Consider naming this something else, so I don't initially read it as IUnion.Value. Prop or the like. #WontFix

333fred · 2025-11-18T19:31:31Z

+}
+";
+            var comp = CreateCompilation([src, IUnionSource], targetFramework: TargetFramework.NetCoreApp);
+            // PROTOTYPE: It looks like list pattern cannot work with union types.


Right. It would have to be a conjunction, is int[] and [10]. We debating making list patterns part of the definition of a recursive pattern, but this syntax was a major "well how do we make that work" issue.

RikkiGibson

LGTM (iteration 3).

jcouv · 2025-11-19T20:44:12Z


+        internal static bool IsWellKnownTypeIUnion(this NamedTypeSymbol typeSymbol)
+            => typeSymbol.DeclaredAccessibility == Accessibility.Public &&
+               typeSymbol.IsInterface &&


Should we check arity too?

Should we check arity too?

Thanks. I think we should. This function is not fully tested. I'll add a PROTOTYPE comment for that. It looks like many similar functions in this file are missing the Arity check. I'll open an issue for that.

Add initial support for Union matching

1c0c5bf

AlekseyTs requested review from 333fred and RikkiGibson November 12, 2025 23:08

AlekseyTs added the Area-Compilers label Nov 12, 2025

AlekseyTs requested a review from a team as a code owner November 12, 2025 23:08

AlekseyTs added the Feature - Unions label Nov 12, 2025

AlekseyTs requested a review from a team as a code owner November 12, 2025 23:08

dotnet-policy-service Bot added the Needs API Review Needs to be reviewed by the API review council label Nov 12, 2025

RikkiGibson reviewed Nov 13, 2025

View reviewed changes

Adjust a test

22112c8

333fred reviewed Nov 15, 2025

View reviewed changes

333fred reviewed Nov 17, 2025

View reviewed changes

RikkiGibson reviewed Nov 18, 2025

View reviewed changes

RikkiGibson approved these changes Nov 18, 2025

View reviewed changes

333fred approved these changes Nov 18, 2025

View reviewed changes

PR feedback

fa27737

333fred approved these changes Nov 18, 2025

View reviewed changes

RikkiGibson approved these changes Nov 18, 2025

View reviewed changes

AlekseyTs enabled auto-merge (squash) November 18, 2025 22:48

AlekseyTs merged commit e154e00 into dotnet:features/Unions Nov 18, 2025
28 of 29 checks passed

jcouv reviewed Nov 19, 2025

View reviewed changes

This was referenced Mar 7, 2026

[release/11.0.1xx-preview2] Source code updates from dotnet/roslyn dotnet/dotnet#5293

Merged

[release/10.0.3xx] Source code updates from dotnet/roslyn dotnet/dotnet#5292

Merged

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

Merged

This was referenced Mar 10, 2026

[Automated] PRs inserted in VS build main-11609.20 #82668

Closed

[Automated] PRs inserted in VS build feature.debugger.wix5-11613.132 #82758

Closed

	to the ValueProperty. It represents a pattern that is matched emmidiately
	to the ValueProperty. It represents a pattern that is matched immediately
	``` #Resolved

	The preseeding union matching operation in evaluation order, if any, corresponds
	The preceding union matching operation in evaluation order, if any, corresponds
	``` #Resolved

	Debug.Assert(node is not BoundPattern { IsUnionMatching: true });
	Debug.Assert(node is BoundPattern { IsUnionMatching: false });
	``` #Resolved

	ImmutableArray<BoundPattern> subpatterns = this.VisitList(node.Subpatterns).SelectAsArray(p => RewritePatternWithUnionMatchingToPropertyPattern(p));
	ImmutableArray<BoundPattern> subpatterns = this.VisitList(node.Subpatterns).SelectAsArray(static p => RewritePatternWithUnionMatchingToPropertyPattern(p));
	``` #Resolved

	// The code below unwraps the following reqursive operation:
	// The code below unwraps the following recursive operation:
	``` #Resolved

		inputType: leftOfPendingConjunction?.NarrowedType ?? inputType,
		narrowedType: leftOfPendingConjunction?.NarrowedType ?? inputType).MakeCompilerGenerated();

Conversation

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

Uh oh!

Uh oh!

dotnet-policy-service Bot commented Nov 12, 2025

Uh oh!

AlekseyTs commented Nov 13, 2025

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Nov 13, 2025 • 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!

333fred Nov 13, 2025 • 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!

Uh oh!

Uh oh!

333fred Nov 14, 2025 • 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!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Nov 17, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

333fred Nov 17, 2025 • 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!

Uh oh!

333fred Nov 17, 2025 • 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!

333fred Nov 17, 2025 • 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!

333fred Nov 17, 2025 • 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!

Uh oh!

333fred Nov 17, 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!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Nov 18, 2025 • 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!

RikkiGibson Nov 18, 2025 • 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

AlekseyTs commented Nov 12, 2025 •

edited by jcouv

Loading

333fred Nov 13, 2025 •

edited by AlekseyTs

Loading

333fred Nov 13, 2025 •

edited by AlekseyTs

Loading

333fred Nov 14, 2025 •

edited by AlekseyTs

Loading

333fred Nov 17, 2025 •

edited by AlekseyTs

Loading

333fred Nov 17, 2025 •

edited by AlekseyTs

Loading

333fred Nov 17, 2025 •

edited by AlekseyTs

Loading

333fred Nov 17, 2025 •

edited by AlekseyTs

Loading

333fred Nov 17, 2025 •

edited by AlekseyTs

Loading

333fred Nov 17, 2025 •

edited by jcouv

Loading

RikkiGibson Nov 18, 2025 •

edited by AlekseyTs

Loading

RikkiGibson Nov 18, 2025 •

edited by AlekseyTs

Loading

RikkiGibson Nov 18, 2025 •

edited by jcouv

Loading

RikkiGibson Nov 18, 2025 •

edited by AlekseyTs

Loading

333fred Nov 18, 2025 •

edited by AlekseyTs

Loading

333fred Nov 18, 2025 •

edited by AlekseyTs

Loading