-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Record-structs: address misc other IDE scenarios #52464
Conversation
@@ -392,8 +392,12 @@ private static bool SemicolonIsMissing(SyntaxNode currentNode) | |||
return ((ExternAliasDirectiveSyntax)currentNode).SemicolonToken.IsMissing; | |||
case SyntaxKind.ClassDeclaration: | |||
return ((ClassDeclarationSyntax)currentNode).SemicolonToken.IsMissing; | |||
case SyntaxKind.RecordDeclaration: | |||
return ((RecordDeclarationSyntax)currentNode).SemicolonToken.IsMissing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 I did a search for usage of StructDeclaration
and IsRecord
and added cases for records and record structs if missing.
But some of them I was not able to cover with test. I'll call them out. This is one of them. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public async Task FieldInRecordStruct() | ||
{ | ||
var text = CreateTestSource(@" | ||
record struct S |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 The language doesn't allow record
and ref struct
together. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. i'm curious why not. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in other words, was this an intentional omission, or we just didnt' consider it. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We considered it and rejected it.
The main rationale is that ref structs can't be used as type parameter, so IEquatable<RefStruct>
and generic code we produce for equality comparison isn't possible.
In reply to: 609207949 [](ancestors = 609207949)
src/Workspaces/CSharp/Portable/CodeGeneration/CSharpDeclarationComparer.cs
Show resolved
Hide resolved
} | ||
else | ||
{ | ||
typeDeclaration = SyntaxFactory.RecordStructDeclaration(SyntaxFactory.Token(SyntaxKind.RecordKeyword), namedType.Name.ToIdentifierToken()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 not tested (we don't yet have scenarios to generate record structs, but implementation is straightforward #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we'll be fine. i'm going to write a feature to covert a tuple to a named record. So i'll test it that way. #Resolved
src/EditorFeatures/Core.Wpf/Classification/ClassificationTypeFormatDefinitions.cs
Show resolved
Hide resolved
=> node.Kind() == SyntaxKind.ClassDeclaration || node.Kind() == SyntaxKind.StructDeclaration || | ||
node.Kind() == SyntaxKind.InterfaceDeclaration || node.Kind() == SyntaxKind.RecordDeclaration; | ||
=> node.IsKind(SyntaxKind.ClassDeclaration) || node.IsKind(SyntaxKind.StructDeclaration) || | ||
node.IsKind(SyntaxKind.InterfaceDeclaration) || node.IsKind(SyntaxKind.RecordDeclaration) || node.IsKind(SyntaxKind.RecordStructDeclaration); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change intended to be included in the PR? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I made the change intentionally. #Resolved
=> context.ContainingTypeDeclaration.IsKind(SyntaxKind.StructDeclaration) && | ||
{ | ||
var type = context.ContainingTypeDeclaration; | ||
return (type.IsKind(SyntaxKind.StructDeclaration) || type.IsKind(SyntaxKind.RecordStructDeclaration)) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think there's precedent to use or
when there are multiple acceptable kinds. Alternatively I believe IsKind is overloaded with various numbers of parameters.
type.Kind() is SyntaxKind.StructDeclaration or SyntaxKind.RecordStructDeclaration
#Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. we're switching these to use or
. it's esp nice as precedence is clear. i.e. you dn't need to parenthesize as the or
and the &&
don't mix :) #Pending
@@ -38,7 +38,7 @@ public virtual ImmutableArray<INamedTypeSymbol> Interfaces | |||
public ImmutableArray<INamedTypeSymbol> AllInterfaces | |||
=> ImmutableArray.Create<INamedTypeSymbol>(); | |||
|
|||
public bool IsReferenceType => false; | |||
public bool IsReferenceType => TypeKind == TypeKind.Class; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which scenario is affected by this change?
Should this method return 'true' if TypeKind is Interface, for example? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll adjust.
Elsewhere in code generators I made a change that adds a check for IsReferenceType
. That code relied on a cast to RecordDeclarationSyntax
which would obviously break. #Resolved
@@ -196,7 +198,14 @@ private static MemberDeclarationSyntax RemoveAllMembers(MemberDeclarationSyntax | |||
TypeDeclarationSyntax typeDeclaration; | |||
if (namedType.IsRecord) | |||
{ | |||
typeDeclaration = SyntaxFactory.RecordDeclaration(SyntaxFactory.Token(SyntaxKind.RecordKeyword), namedType.Name.ToIdentifierToken()); | |||
if (namedType.IsReferenceType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider pushing this down into a conditional expression that assigns to typeDeclaration
#Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. plz do this. #Pending
@@ -171,6 +171,7 @@ private static OuterOrdering GetOuterOrdering(MemberDeclarationSyntax x) | |||
case SyntaxKind.EnumDeclaration: | |||
case SyntaxKind.DelegateDeclaration: | |||
case SyntaxKind.RecordDeclaration: | |||
case SyntaxKind.RecordStructDeclaration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 not tested. I couldn't find test impacted by some cases above and am not familiar with organizers in general. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we haven't shipped this. so no tests needed. #Resolved
{ | ||
int field; | ||
|
||
public int this[int i] => {|Rename:GetField|}(); | ||
|
||
private int GetField() | ||
private readonly int GetField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. i didn't know we did that. #Resolved
[Theory, Trait(Traits.Feature, Traits.Features.CodeLens)] | ||
[InlineData("class", "class", "class")] | ||
[InlineData("record class", "record class", "record class")] | ||
[InlineData("record struct", "record struct", "record struct")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't get the need for triplicate (since they're all the same). why not just pass in one, and reference hte paramter three times? #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I thought I'd include some mix&match combinations, but it wasn't necessary to hit the cases I wanted. I'll simplify
In reply to: 609207610 [](ancestors = 609207610)
@@ -24,6 +24,8 @@ public class OrganizeTypeDeclarationTests : AbstractOrganizerTests | |||
[Theory, Trait(Traits.Feature, Traits.Features.Organizing)] | |||
[InlineData("class")] | |||
[InlineData("record")] | |||
[InlineData("record class")] | |||
[InlineData("record struct")] | |||
public async Task TestFieldsWithoutInitializers1(string typeKind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the most tested feature we're not even shipping :D
#region User Types - Record Structs | ||
[Export] | ||
[Name(ClassificationTypeNames.RecordStructName)] | ||
[BaseDefinition(ClassificationTypeNames.StructName)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting base-definition. i would honestly expect .FormatLanguage here. but i suppose this won't hurt :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the history, for the corresponding record class case, it started as FormatLanguage, but then got changed to ClassName.
I have no opinion (first time touching this)
In reply to: 609208508 [](ancestors = 609208508)
@@ -673,7 +671,7 @@ internal override bool TryGetBaseList(ExpressionSyntax expression, out TypeKindO | |||
{ | |||
if (node is BaseListSyntax) | |||
{ | |||
if (node.Parent != null && (node.Parent is InterfaceDeclarationSyntax || node.Parent is StructDeclarationSyntax)) | |||
if (node.Parent != null && (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax or RecordStructDeclarationSyntax)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (node.Parent != null && (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax or RecordStructDeclarationSyntax)) | |
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax or RecordStructDeclarationSyntax) | |
``` #Pending |
else if (node.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax typeDeclaration) | ||
|| node.IsKind(SyntaxKind.RecordDeclaration, out typeDeclaration) | ||
|| node.IsKind(SyntaxKind.RecordStructDeclaration, out typeDeclaration) | ||
|| node.IsKind(SyntaxKind.StructDeclaration, out typeDeclaration) | ||
|| node.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if (node.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax typeDeclaration) | |
|| node.IsKind(SyntaxKind.RecordDeclaration, out typeDeclaration) | |
|| node.IsKind(SyntaxKind.RecordStructDeclaration, out typeDeclaration) | |
|| node.IsKind(SyntaxKind.StructDeclaration, out typeDeclaration) | |
|| node.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration)) | |
else if (node is TypeDeclarationSyntax typeDeclaration) |
Not sure why we have so many patterns to express things that are trivial language checks :) #Pending
@@ -373,6 +374,7 @@ static SyntaxToken GetHintTextEndToken(SyntaxNode node) | |||
} | |||
else if (node.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax typeDeclaration) | |||
|| node.IsKind(SyntaxKind.RecordDeclaration, out typeDeclaration) | |||
|| node.IsKind(SyntaxKind.RecordStructDeclaration, out typeDeclaration) | |||
|| node.IsKind(SyntaxKind.StructDeclaration, out typeDeclaration) | |||
|| node.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration)) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. #Pending
@@ -79,7 +79,7 @@ internal static class NamedTypeGenerator | |||
|
|||
var members = GetMembers(namedType).Where(s => s.Kind != SymbolKind.Property || PropertyGenerator.CanBeGenerated((IPropertySymbol)s)) | |||
.ToImmutableArray(); | |||
if (namedType.IsRecord) | |||
if (namedType.IsRecord && namedType.IsReferenceType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wouldn't make this change. looking at the code it looks like it would handle struct records jsut fine. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the change because of the cast below which would crash for record structs. Record structs use RecordStructDeclarationSyntax
which derives from TypeDeclarationSyntax
but isn't related to RecordDeclarationSyntax
.
In reply to: 609211082 [](ancestors = 609211082)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! minor nits. up to you on handling them.
This PR is waiting for #52455 to be merged first. Marking as Blocked |
@@ -38,7 +38,7 @@ public virtual ImmutableArray<INamedTypeSymbol> Interfaces | |||
public ImmutableArray<INamedTypeSymbol> AllInterfaces | |||
=> ImmutableArray.Create<INamedTypeSymbol>(); | |||
|
|||
public bool IsReferenceType => false; | |||
public bool IsReferenceType => TypeKind != TypeKind.Enum && TypeKind != TypeKind.Struct && TypeKind != TypeKind.Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there is a reason for the need of this change, I'd keep it as is, and use TypeKind.Class in the places IsReferenceType
is used in this PR. It's always used as a second condition for "IsRecord", so we know for sure it's either a class or a struct.
I also believe IsRecord is checked along with TypeKind in the compiler layer, so that will be consistent as well. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased to remove the first commit (since merged as part of another PR) |
New ClassifiedTextRun(ClassificationTypeNames.WhiteSpace, " "), | ||
New ClassifiedTextRun(ClassificationTypeNames.Text, "1"), | ||
New ClassifiedTextRun(ClassificationTypeNames.WhiteSpace, " "), | ||
New ClassifiedTextRun(ClassificationTypeNames.Text, "overload"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi @sharwell FYI this test was failing in Spanish CI leg because of "overload" in New ClassifiedTextRun(ClassificationTypeNames.Text, "overload")
.
I'ved fixed it, but noticed that other tests in this file hardcode "overloads". Does that mean we have a localization problem? (I'd expect those tests to fail in Spanish too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see a reason why the other tests don't fail.
Both "overload" and "overloads" resources are localized in Spanish.
roslyn/src/Features/Core/Portable/xlf/FeaturesResources.es.xlf
Lines 2633 to 2642 in be8f790
<trans-unit id="overload"> | |
<source>overload</source> | |
<target state="translated">sobrecarga</target> | |
<note /> | |
</trans-unit> | |
<trans-unit id="overloads_"> | |
<source>overloads</source> | |
<target state="translated">sobrecargas</target> | |
<note /> | |
</trans-unit> |
Ignore the first commit (comes from previous PR).Test plan: #51199