Unions: add basic IDE support by jcouv · Pull Request #82526 · dotnet/roslyn

jcouv · 2026-02-25T00:54:45Z

recommend union keyword
test completion of types within type list
test FindAllReferences
glyphs, NavigateTo
classification
completion for members in union body

Relates to test plan #81074

Tested the classification in red:

CyrusNajmabadi · 2026-02-25T07:15:16Z


 struct_declaration
-  : attribute_list* modifier* 'struct' identifier_token type_parameter_list? parameter_list? base_list? type_parameter_constraint_clause* '{'? member_declaration* '}'? ';'?
+  : attribute_list* modifier* ('struct' | 'union') identifier_token type_parameter_list? parameter_list? base_list? type_parameter_constraint_clause* '{'? member_declaration* '}'? ';'?


This is very weird that this is part of struct_declaration when it changes that keyword. I would expect a different type-decl here.

My PR is still a draft, it's focused on IDE changes, but is stacked on top of the PR that adds declaration/parsing support. You probably meant to comment there: #82500

Fwiw, I only noticed this in julien' ide pr, but reusing StructDecl here with a different keyword/kind doesn't fit how I think TypeDecl is intended to be used. Like with RecordDecl, I think this should be it's own TypeDecl subclass. So keep the new UnionDecl syntaxkind, but pair withe a UnionDecl subclass.

If I can be included in the API review, that would be appreciated. Thanks.

@333fred fyi, tnx.

CyrusNajmabadi · 2026-02-25T07:17:03Z

            // For extension declarations, there must be a parameter list
            var paramList = CurrentToken.Kind == SyntaxKind.OpenParenToken || isExtension
-                ? ParseParenthesizedParameterList(forExtension: isExtension) : null;
+                ? ParseParenthesizedParameterList(forExtensionOrUnion: isExtension || isUnion) : null;


Imo,this is very difficult to understand precedence here. I would parenthesize for clarity.

CyrusNajmabadi · 2026-02-25T07:19:56Z

Weird. I couldn't find the place where union keyword was recommended.

jcouv · 2026-02-26T00:07:58Z

@dotnet/roslyn-ide for review. Thanks

AlekseyTs · 2026-02-27T04:02:17Z

Consider also adding tests to SyntaxNormalizerTests #Closed

AlekseyTs · 2026-02-27T04:11:47Z

+            SyntaxKind.PrivateKeyword,
+            SyntaxKind.ProtectedKeyword,
+            SyntaxKind.UnsafeKeyword,
+            SyntaxKind.FileKeyword,


SyntaxKind.FileKeyword

Is SyntaxKind.ReadOnlyKeyword excluded intentionally? #Closed

Will add. Wasn't sure.
What about ref modifier?

What about ref modifier?

This is open to interpretation. Depending whether it is meant to be one of the struct_modifier modifiers. It looks like in the current standard spec ref modifier is listed explicitly. I guess, assuming that it is not allowed should be good for now.

AlekseyTs · 2026-02-27T04:12:27Z

+            context.IsTypeDeclarationContext(
+                validModifiers: s_validModifiers,
+                validTypeDeclarations: SyntaxKindSet.NonEnumTypeDeclarations,
+                canBePartial: false,


false

true? #Closed

AlekseyTs · 2026-02-27T04:16:45Z

Done with review pass (commit 7) #Closed

AlekseyTs

LGTM (commit 8)

CyrusNajmabadi · 2026-02-27T23:21:28Z

+        await TestAsync(testHost, composition, content, async w =>
+        {
+            var item = (await _aggregator.GetItemsAsync("Goo")).Single(x => x.Kind != "Method");
+            VerifyNavigateToResultItem(item, "Goo", "[|Goo|]", PatternMatchKind.Exact, NavigateToItemKind.Structure, Glyph.StructureInternal);


This is highly likely worth using another glyph. I believe c++ has a union glyph. We should incorporate into Roslyn as well and align on that. #Resolved

Added a new glyph using existing union image (see pic in OP)

CyrusNajmabadi · 2026-02-27T23:22:40Z

+public sealed class UnionKeywordRecommenderTests : KeywordRecommenderTests
+{
+    private static readonly CSharpParseOptions s_options = CSharpNextParseOptions;
+    private static readonly CSharpParseOptions s_scriptOptions = CSharpNextScriptParseOptions;


I'm very confused by the need for all the different options. I would not expect that to change anything about the recommending here. #Resolved

The parsing for union declarations is conditional on LangVer

I do not expect anything related to script handling. :-)

In a case like this, I would just recommend that either the base class ask which langversion to use, and an override sets it, or have a helper method here that passes that value down, and all tests in this file use that.

We try to avoid repetition in these test methods. So having all/most tests have to pass these values explicitly is undesirable. Tnx

I changed the implicit options in RecommenderTests (base type for these tests) to use "preview". That's the strategy used by compiler tests (all tests use "preview" unless otherwise specified)

CyrusNajmabadi · 2026-02-27T23:24:15Z

                return NavigateToItemKind.Property;
            case DeclaredSymbolInfoKind.Struct:
+            case DeclaredSymbolInfoKind.Union:
                return NavigateToItemKind.Structure;


We should likely have a different kind here. Users are going to consider unions their own thing. #WontFix

I added a new NavigateToItemKind, but I'm not really sure what's the expected impact. How do we observe this in the editor?
It looks like we would need some changes on the VS or VS SDK side to keep this separation the whole way through. I'm still mapping the union case to CodeSearchResultType.Structure in RoslynNavigateToSearchCallback.
Similarly, I didn't fully understand how this NavigateToItemKind should interact with LSP. I'm still mapping the union case to LSP.SymbolKind.Struct in ProtocolConversions.NavigateToKindToSymbolKind

I ended up reverting the NavigateToItemKind for now. Left some PROTOTYPE comments to follow-up.

so if NavigateToItemKind is on the roslyn side, we'll want to ahve a new .Union prop on it. If it's the point that we're marshal'ing that to a VS value, we can move to .Structure if they haven't exposed .Union yet. But then an internal convo should happen on them adding that. :)

CyrusNajmabadi · 2026-02-27T23:24:49Z


  <PropertyGroup Condition="'$(DotNetBuildSourceOnly)' != 'true'">
-    <DefineConstants>$(DefineConstants),ROSLYN_4_12_OR_LOWER</DefineConstants>
+    <DefineConstants>$(DefineConstants),ROSLYN_4_12_OR_LOWER,ROSLYN_5_5_OR_LOWER</DefineConstants>


Why both constants here? #Resolved

CyrusNajmabadi · 2026-02-27T23:25:11Z

            SyntaxKind.RecordDeclaration => ClassificationTypeNames.RecordClassName,
            SyntaxKind.RecordStructDeclaration => ClassificationTypeNames.RecordStructName,
            SyntaxKind.StructDeclaration => ClassificationTypeNames.StructName,
+            SyntaxKind.UnionDeclaration => ClassificationTypeNames.StructName,


I think this should have it's own name. Users will want to color these differently. #Resolved

Added a new classification

CyrusNajmabadi · 2026-02-27T23:26:04Z

@@ -103,6 +111,10 @@ public static int GetArity(this MemberDeclarationSyntax member)
                case SyntaxKind.ClassDeclaration:
 #if !ROSLYN_4_12_OR_LOWER


Are we not beyond this yet? #Resolved

jcouv · 2026-03-01T18:44:25Z

@AlekseyTs I made some changes suggested by Cyrus to support a better IDE experience. But that required some compiler-side changes. If you feel this is too much snowballing/risk for the upcoming deadline, we can truncate this PR just before the commit that adds IsUnion

This reverts commit 897d267.

Made further changes

CyrusNajmabadi · 2026-03-01T20:09:07Z

The vs side will likely have to expose the right kind. Oleg may be a good starting point. :-)

You can map to that once exposed.

AlekseyTs · 2026-03-02T16:14:21Z

                    return SymbolDisplayPartKind.RecordClassName;
                case TypeKind.Struct when symbol.IsRecord:
                    return SymbolDisplayPartKind.RecordStructName;
+                case TypeKind.Struct when symbol.IsUnion:


TypeKind.Struct when symbol.IsUnion:

I am not sure a usage like that is appropriate. If type is union, it doesn't necessarily mean that it is declared with union declaration.

So, this matches what we did with records. We use record in symbol display even if it wasn't originally declared with record syntax. Symbol-display takes a semantics-forward approach here :)

So, this matches what we did with records.

I do not think it does. There is no way to declare a record struct without using a "record struct declaration"

Thera are also class unions

I would have thought that anything from metadata wouldn't be something whose originating declaration we would know. It would just have some shape that makes it a 'record', regardless of the originating language or originating syntax.

For records, i thought it was just the presence of a proper cloning method. Once we find that, things like symbol-display use record. That seems like a completely reasonable (and expected) correspondence here with unions.

I'm saying that the symbol display reflects semantically what it is. It's not syntax display. If the type had the right data to be considered a union, then displaying it that way is desirable :-)

Here's what I'd propose: 1. split the PR into most basic support for purpose of this week's preview (I opened #82604 for that, it cuts off from this PR just before the IsUnion addition), 2. discuss and implement more advanced handling implemented in this PR as a follow-up next week
Personally, I'm leaning towards displaying all union types with union (we might even display the list of cases) but this needs broader unions WG and IDE discussion.

That works for me. Note: more ammunition for the symbolic view of things. For things like ValueTuple<int, int> or Nullable<int> we show these in the IDE as (int, int) and int?. It's the normal view that if symbolically/semantically these are the same to the language that we then show the symbolic view of them.

Another way to think about it. If we're reading it from metadata and would treat it the same as the source-decl form, then we present it that way.

Happy to discuss this in WG/IDE though. But having written lots of SymbolDisplay (and many/most of the consumption points for it), I'm confident we'll just end up here :)

None of the arguments resonate with me. And frankly I don't find them applicable. For me, this is not about metadata vs, source view.

I'm not saying it is exactly these things. I'm stating we have a lot of precedence for "symbol display" to present a *symbolic/semantic perspective" over a particular ISymbol. We aim to cater to that and have ample precedence since this aspect was developed in roslyn in the first place. Furthermore, a huge amount of the design of SymbolDisplay came exactly from the needs and perspective of the IDE, since we present symbols in so many circumstances and have strong opinions on how a symbol should be presented. This is why the api surface area of SymbolDisplay is so rich in the first place. Including going beyond just text, but also to be a typed description of pieces, intended for rich presentation in things like quick info.

I'm stating that given our history here, and how we've treated pretty much all other symbols, we will likely land on the perspective that we should present anything the language would consider "a union" in a consistent fashion.

AlekseyTs · 2026-03-02T16:16:25Z

            var comp = CreateCompilation([src, UnionAttributeSource]);
            comp.VerifyEmitDiagnostics();

-            Assert.True(comp.GetTypeByMetadataName("S1").IsUnionType);


Assert.True(comp.GetTypeByMetadataName("S1").IsUnionType);

Please keep original asserts. It is fine to add new asserts

This comment is applicable to many other similar changes in this file

AlekseyTs · 2026-03-02T16:24:53Z

I made some changes suggested by Cyrus to support a better IDE experience. But that required some compiler-side changes. If you feel this is too much snowballing/risk for the upcoming deadline, we can truncate this PR just before the commit that adds IsUnion

In general I am Ok with adding IsUnion property, but I am not comfortable with how it is used. It appears that an equivalence is made between this property returning true and a "union declaration". There is no an equivalence like that. The property is true for a "union declaration", but it also can be true for a non-union declaration. Therefore, I think it would be better to revert compiler changes to the state they were at commit 8

CyrusNajmabadi · 2026-03-02T18:06:10Z

but it also can be true for a non-union declaration

I think that's fine (if i'm understanding you correctly). Specifically, it sounds like this returns if the compiler semantically thinks of something as a union, even if it wasn't syntactically declared using the new syntax. I think it's fine for these features to drive off the semantic side.

AlekseyTs · 2026-03-02T18:10:38Z

I think that's fine

And I don't think this.

CyrusNajmabadi · 2026-03-02T18:11:15Z

+            context.IsGlobalStatementContext ||
+            context.IsTypeDeclarationContext(
+                validModifiers: s_validModifiers,
+                validTypeDeclarations: SyntaxKindSet.NonEnumTypeDeclarations,


just making sure that this is intentional (the 'enum' part) :)

CyrusNajmabadi · 2026-03-02T18:13:49Z

            visitor.WriteString(symbol.MetadataName);
            visitor.WriteSymbolKey(symbol.ContainingType);
-#if !ROSLYN_4_12_OR_LOWER
+#if !OLDER_ROSLYN


CyrusNajmabadi

IDE side lgtm. Thanks!

jcouv self-assigned this Feb 25, 2026

dotnet-policy-service Bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Feb 25, 2026

github-actions Bot added the Area-Compilers label Feb 25, 2026

CyrusNajmabadi reviewed Feb 25, 2026

View reviewed changes

jcouv mentioned this pull request Feb 25, 2026

Initial support for union declarations #82500

Merged

Julien Couvreur added 7 commits February 25, 2026 11:41

UnionKeywordRecommender

bb3701a

SymbolCompletionProvider

dbe44b5

FAR

06ee909

Add DeclaredSymbolInfoKind.Union for FAR, NavigateTo, and glyph support

12c7277

Classification

5b6a61e

SyntaxKindSet

5bada26

Formatting

be66439

jcouv force-pushed the unions-ide branch from 954279b to be66439 Compare February 25, 2026 19:42

jcouv marked this pull request as ready for review February 25, 2026 19:43

jcouv requested a review from a team as a code owner February 25, 2026 19:43

jcouv added Feature - Unions and removed Needs API Review Needs to be reviewed by the API review council labels Feb 25, 2026

AlekseyTs reviewed Feb 27, 2026

View reviewed changes

Address feedback

75b6e8d

jcouv requested a review from a team as a code owner February 27, 2026 06:03

jcouv requested a review from AlekseyTs February 27, 2026 07:23

AlekseyTs previously approved these changes Feb 27, 2026

View reviewed changes

CyrusNajmabadi suggested changes Feb 27, 2026

View reviewed changes

Pre-processor directives

5243496

Julien Couvreur added 7 commits March 1, 2026 08:24

Clean up recommender tests

0e6f5aa

Comments

ee738d9

Add IsUnion public API

6f53d95

New glyph

6a2bda7

SymbolDisplay

38b86cc

Use new classification

fab7ff4

Add NavigateToItemKind.Union

897d267

jcouv requested a review from a team as a code owner March 1, 2026 18:35

dotnet-policy-service Bot added the Needs API Review Needs to be reviewed by the API review council label Mar 1, 2026

Julien Couvreur added 4 commits March 1, 2026 11:04

Revert "Add NavigateToItemKind.Union"

92df29c

This reverts commit 897d267.

Tweaks

9e8db24

Merge remote-tracking branch 'dotnet/features/Unions' into unions-ide

1f6e32a

Test fixups

718ea0d

Fix GlyphExtensionsTests

bc3bb6e

AlekseyTs reviewed Mar 2, 2026

View reviewed changes

CyrusNajmabadi reviewed Mar 2, 2026

View reviewed changes

CyrusNajmabadi approved these changes Mar 2, 2026

View reviewed changes

This was referenced Mar 3, 2026

Unions: add most basic IDE support #82604

Merged

Unions: improve IDE handling #82607

Open

jcouv marked this pull request as draft March 3, 2026 09:05

phil-allen-msft requested a review from akhera99 March 6, 2026 00:19

jcouv closed this Mar 6, 2026

		@@ -103,6 +111,10 @@ public static int GetArity(this MemberDeclarationSyntax member)
		case SyntaxKind.ClassDeclaration:
		#if !ROSLYN_4_12_OR_LOWER

Conversation

jcouv commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

CyrusNajmabadi commented Feb 25, 2026

Uh oh!

jcouv commented Feb 26, 2026

Uh oh!

AlekseyTs commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Feb 27, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 27, 2026 • 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 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Feb 27, 2026 • 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!

jcouv Mar 1, 2026 • 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!

CyrusNajmabadi Feb 27, 2026 • 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!

CyrusNajmabadi Feb 28, 2026 • 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!

CyrusNajmabadi Feb 27, 2026 • 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!

CyrusNajmabadi Feb 27, 2026 • 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!

CyrusNajmabadi Feb 27, 2026 • 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!

CyrusNajmabadi Feb 27, 2026 • 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

jcouv commented Feb 25, 2026 •

edited

Loading

AlekseyTs commented Feb 27, 2026 •

edited

Loading

AlekseyTs Feb 27, 2026 •

edited

Loading

AlekseyTs Feb 27, 2026 •

edited

Loading

AlekseyTs commented Feb 27, 2026 •

edited

Loading

CyrusNajmabadi Feb 27, 2026 •

edited by jcouv

Loading

jcouv Mar 1, 2026 •

edited

Loading

CyrusNajmabadi Feb 27, 2026 •

edited by jcouv

Loading

CyrusNajmabadi Feb 28, 2026 •

edited

Loading

CyrusNajmabadi Feb 27, 2026 •

edited by jcouv

Loading

CyrusNajmabadi Feb 27, 2026 •

edited by jcouv

Loading

CyrusNajmabadi Feb 27, 2026 •

edited by jcouv

Loading

CyrusNajmabadi Feb 27, 2026 •

edited by jcouv

Loading

AlekseyTs Mar 2, 2026 •

edited

Loading

AlekseyTs Mar 2, 2026 •

edited

Loading

AlekseyTs Mar 3, 2026 •

edited

Loading

AlekseyTs commented Mar 2, 2026 •

edited

Loading

AlekseyTs commented Mar 2, 2026 •

edited

Loading