Unions: add most basic IDE support#82604
Conversation
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
This looks good. And it makes sense to pull out the easy/safe stuff from teh other PR. Can you make sure we have tracking bugs though? Thanks!
| case DeclaredSymbolInfoKind.Property: | ||
| return NavigateToItemKind.Property; | ||
| case DeclaredSymbolInfoKind.Struct: | ||
| // PROTOTYPE consider having a separate NavigateToItemKind category for unions |
There was a problem hiding this comment.
can you track this with an issue at least. We do need this, or the glyph will be inconsistent in these different VS experiences, which we try very hard to avoid. Thanks! :) #Pending
| context.IsGlobalStatementContext || | ||
| context.IsTypeDeclarationContext( | ||
| validModifiers: s_validModifiers, | ||
| validTypeDeclarations: SyntaxKindSet.NonEnumTypeDeclarations, |
There was a problem hiding this comment.
same question from before. is NonEnumTypeDecls right here? (just want to make sure it was loooked at. #Resolved
There was a problem hiding this comment.
I think that's correct. This is the same validTypeDeclarations as we use for StructKeywordRecommender. It means that we won't recommend union inside an enum.
The only thing that's a bit weird is that NonEnumTypeDeclarations includes extension blocks. This means that we're currently recommending struct and union inside extension blocks, even though that will end up erroring. That said, my understanding is that we'd rather recommend a bit too much than a bit too little. So seems fine.
There was a problem hiding this comment.
Yup. This is totally fine. Thanks for checking/confirming.
| <PropertyGroup Condition="'$(DotNetBuildSourceOnly)' != 'true'"> | ||
| <DefineConstants>$(DefineConstants),ROSLYN_4_12_OR_LOWER</DefineConstants> | ||
| <!-- Projects built with OLDER_ROSLYN do not have access to the latest Roslyn APIs --> | ||
| <DefineConstants>$(DefineConstants),OLDER_ROSLYN</DefineConstants> |
|
|
||
| /// <summary> | ||
| /// When adding a new value, also bump <see cref="AbstractSyntaxIndex{TIndex}.s_serializationFormatChecksum"/>. | ||
| /// </summary> |
| SyntaxKind.RecordDeclaration => ClassificationTypeNames.RecordClassName, | ||
| SyntaxKind.RecordStructDeclaration => ClassificationTypeNames.RecordStructName, | ||
| SyntaxKind.StructDeclaration => ClassificationTypeNames.StructName, | ||
| // PROTOTYPE consider using a separate classification type for unions so users can color them differently |
There was a problem hiding this comment.
can we have an issue on this. we def want this. note: this part can likely easily be part of this PR, it doesn't hit the compiler side. but it can also def come later. #Pending
There was a problem hiding this comment.
I created a follow-up issue: #82607
Will update these PROTOTYPE markers to reference the issue
|
Since #82526 is already under review. I suggest that we bring that PR to a mergeable state and continue working in its context rather than asking everybody to review from scratch. |
BTW, I would like to point out again, that classes can be unions as well. #Resolved |
Specifically, I suggest revering controversial changes rather than trying to reach concensus. #ByDesign |
It would definitely be good to have an API from teh compiler (if not already available) so we can determine if something is a union, and then present things that the lang/compiler thinks are unions in a consistent fashion. Is it just a 'boolean' "yes/no this is a union"? Or is there additional information that is needed to expose? |
|
|
||
| [Fact] | ||
| public Task TestInCompilationUnit_LangVer14() | ||
| => VerifyKeywordAsync("$$", CSharp14ParseOptions); |
There was a problem hiding this comment.
Why would union be suggested for C# 14? #Resolved
There was a problem hiding this comment.
We still recommemd, so that you can type the nifty thing you just saw a tweet about. Then we offer the fixer to upgrade your project version to use it.
You can see the commit/SHA before merge (entitled "Comments") is the same commit/SHA as the original PR (
Thanks, I'll add some tests for that |
This is forked from #82526
Given that we're trying to merge minimal support for preview this week, but there's still discussion on the desired behavior, I'm splitting off the smaller part of the PR. This most basic approach is smaller and safer for merging this week. We can expand support after initial preview.
Opened #82607 to track some follow-ups that were discussed and mostly handled in the original PR