Record-structs: validate and fix a number of IDE tests #52261
Conversation
| { | ||
| ~$$ | ||
| }"; | ||
| await VerifyItemExistsAsync(markup, "C"); |
There was a problem hiding this comment.
interesting :) #Resolved
| await TestAsync(content, body, testHost, w => new FirstDocIsActiveAndVisibleDocumentTrackingService(w.Workspace)); | ||
|
|
||
| async Task TestAsync( |
There was a problem hiding this comment.
| await TestAsync(content, body, testHost, w => new FirstDocIsActiveAndVisibleDocumentTrackingService(w.Workspace)); | |
| async Task TestAsync( | |
| await TestAsync(content, body, testHost, w => new FirstDocIsActiveAndVisibleDocumentTrackingService(w.Workspace)); | |
| return; | |
| async Task TestAsync( |
IDE style is to have an explicit exit point before the local funtions to make it clear that no actual method code follows htem. #Resolved
| @@ -105,42 +106,42 @@ internal enum DeclaredSymbolInfoKind : byte | |||
| Span = span; | |||
| InheritanceNames = inheritanceNames; | |||
|
|
|||
| const uint MaxFlagValue = 0b1111; | |||
| const uint MaxFlagValue = 0b10000; | |||
There was a problem hiding this comment.
because you're chaning the format here, need to rev teh version number in SyntaxTreeIndex.SerializationFormatChecksum. That way we don't read out persisted data from a previous version that odesn't match our current format. #Resolved
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Completion)] | ||
| public async Task RecordStructDestructor() |
There was a problem hiding this comment.
💡 This set of 3 tests should be combined into a single [Theory] with the following data:
[InlineData("record")]
[InlineData("record class")]
[InlineData("record struct")]
``` #Resolved
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Completion)] | ||
| public async Task TreatRecordClassPositionalParameterAsProperty() |
There was a problem hiding this comment.
💡 This set of three tests should be combined into a single [Theory] to ensure all three are covered in a consistent and relatable manner. #Resolved
|
@sharwell @CyrusNajmabadi Addressed your feedback. Thanks |
| // VisualStudio.Language.NavigateTo.Interfaces.NavigateToItemKind doesn't have a record struct, fall back to struct. | ||
| // This should be updated whenever NavigateToItemKind has a record struct. |
There was a problem hiding this comment.
Do we need to route this to the appropriate team?
Not sure if it's the editor team who owns this?
cc: @jinujoseph @olegtk
| @@ -201,7 +201,7 @@ private static CSharpSyntaxContext CreateContextWorker(Workspace? workspace, Sem | |||
|
|
|||
| var isDestructorTypeContext = targetToken.IsKind(SyntaxKind.TildeToken) && | |||
| targetToken.Parent.IsKind(SyntaxKind.DestructorDeclaration) && | |||
| targetToken.Parent.Parent.IsKind(SyntaxKind.ClassDeclaration, SyntaxKind.StructDeclaration, SyntaxKind.RecordDeclaration); | |||
| targetToken.Parent.Parent.IsKind(SyntaxKind.ClassDeclaration, SyntaxKind.StructDeclaration, SyntaxKind.RecordDeclaration, SyntaxKind.RecordStructDeclaration); | |||
There was a problem hiding this comment.
I don't understand why StructDeclaration existed here. Both RecordStructDeclaration and StructDelaration seems wrong here. They cannot contain a destructor. Anything I'm missing @jcouv?
There was a problem hiding this comment.
Good catch. Destructors are not allowed in structs or record structs. I'll revert this.
There was a problem hiding this comment.
PR for fix: #52408
Will have to confirm with IDE folks on expected behavior since the previous behavior for structs seems intentional (had a bug attached)
Test plan #51199