Skip to content
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: remove RecordStructDeclarationSyntax #52702

Merged
merged 13 commits into from
Apr 22, 2021

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 17, 2021

Test plan: #51199

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Apr 17, 2021

I'm curious what you thought about the change. To me it reads better. What do you think? #Resolved

@jcouv
Copy link
Member Author

jcouv commented Apr 17, 2021

@CyrusNajmabadi Got scared by last minute change and editing publicAPI is still annoying as hell at the moment, but I like the change. Thanks for the suggestion! #Resolved

break;
}
break;

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. this change alone makes me want this PR :) #Resolved

@@ -671,7 +671,7 @@ internal override bool TryGetBaseList(ExpressionSyntax expression, out TypeKindO
{
if (node is BaseListSyntax)
{
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax or RecordStructDeclarationSyntax)
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels like we need to add node.Parent.Kind() == SyntaxKind.RecordStruct #Resolved

? SyntaxFactory.RecordDeclaration(SyntaxFactory.Token(SyntaxKind.RecordKeyword), namedType.Name.ToIdentifierToken())
: SyntaxFactory.RecordStructDeclaration(SyntaxFactory.Token(SyntaxKind.RecordKeyword), namedType.Name.ToIdentifierToken());
var declarationKind = namedType.TypeKind is TypeKind.Class ? SyntaxKind.RecordDeclaration : SyntaxKind.RecordStructDeclaration;
typeDeclaration = SyntaxFactory.RecordDeclaration(declarationKind, SyntaxFactory.Token(SyntaxKind.RecordKeyword), namedType.Name.ToIdentifierToken());
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does RecordStructDeclaration cause SyntaxFactory.RecordDeclaration to create the struct token? If not, we need to do that here. #Resolved

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely like the change. A couple of subtle IDE issues potentially.

@jcouv jcouv marked this pull request as ready for review April 18, 2021 00:37
@jcouv jcouv requested review from a team as code owners April 18, 2021 00:37
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should make supporting Edit and Continue just a couple of new cases in a couple of SyntaxKind switches 😄

: SyntaxFactory.RecordStructDeclaration(SyntaxFactory.Token(SyntaxKind.RecordKeyword), namedType.Name.ToIdentifierToken());
var isRecordClass = namedType.TypeKind is TypeKind.Class;
var declarationKind = isRecordClass ? SyntaxKind.RecordDeclaration : SyntaxKind.RecordStructDeclaration;
var classOrStructKeyword = SyntaxFactory.Token(isRecordClass ? default : SyntaxKind.ClassKeyword);
Copy link
Contributor

@davidwengier davidwengier Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on my phone, so might have missed something, but shouldn't this be SyntaxKind.StructKeyword? Or does specifying the syntax kind mean it will automatically use a strict keyword? But if so, why specify the class keyword? #Resolved

Copy link
Member

@Youssef1313 Youssef1313 Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on my phone, so might have missed something, but shouldn't this be SyntaxKind.StructKeyword? Or does specifying the syntax kind mean it will automatically use a strict keyword? But if so, why specify the class keyword?

I agree, it looks like it should have been StructKeyword. Also needs tests if possible. #Resolved

Copy link
Member Author

@jcouv jcouv Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. We don't yet have any scenarios that need generation of record structs, so this will get coverage when we do. #Resolved

Comment on lines +3429 to +3430
Debug.Assert(builder.RecordDeclarationWithParameters is RecordDeclarationSyntax { ParameterList: not null } record
&& record.Kind() == SyntaxKind.RecordStructDeclaration);
Copy link
Contributor

@alrz alrz Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me realize a void Deconstruct(this SyntaxNode, out SyntaxKind) might be actually useful. Currently matching multi-kind nodes is kind of ugly..

is RecordDeclarationSyntax(SyntaxKind.RecordStructDeclaration) { ParameterList: not null }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a really interesting suggestion. I've also struggled with the fact that Kind is a method and can't be used with pattern matching.

@@ -671,7 +671,8 @@ internal override bool TryGetBaseList(ExpressionSyntax expression, out TypeKindO
{
if (node is BaseListSyntax)
{
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax or RecordStructDeclarationSyntax)
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax
|| node.Parent.Kind() == SyntaxKind.RecordStructDeclaration)
Copy link
Member

@Youssef1313 Youssef1313 Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|| node.Parent.Kind() == SyntaxKind.RecordStructDeclaration)
|| node.Parent.IsKind(SyntaxKind.RecordStructDeclaration))

@jcouv jcouv added the Area-IDE label Apr 19, 2021
@jcouv jcouv requested review from AlekseyTs and a team April 19, 2021 19:10
@@ -1142,8 +1139,7 @@ private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memb
return new WithParametersBinder(method.Parameters, nextBinder);
}

if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } }
or RecordStructDeclarationSyntax { ParameterList: { ParameterCount: > 0 } })
if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } })
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecordDeclarationSyntax

Have you confirmed that all remaining type checks for RecordDeclarationSyntax are applicable to record structs or appropriately adjusted? Same for Visit methods. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I have a list. The places that previously only handled RecordDeclarationSyntax are guarded (kind check or base type check). Some are guarded locally and are checked elsewhere (unreachable).
For the latter cases, I didn't see anything wrong with them. I could add assertions if you think that's useful.
An example is ExpressionVariableFinder.VisitRecordDeclaration.


In reply to: 616728346 [](ancestors = 616728346)

case RecordStructDeclarationSyntax recordStructDeclaration when TryGetSynthesizedRecordConstructor(recordStructDeclaration) is SynthesizedRecordConstructor ctor:
switch (declaredSymbol.Kind)
{
case SymbolKind.Method:
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case SymbolKind.Method:

I think we should preserve the difference for this case. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing why (here or below). We get the same behavior. Do you mean just as an optimization?


In reply to: 616734766 [](ancestors = 616734766)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. This would be necessary if we add assertion in PrimaryConstructorBaseType as you suggested... Let me take a look


In reply to: 616930880 [](ancestors = 616930880,616734766)

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing why (here or below). We get the same behavior. Do you mean just as an optimization?

I do not think we should look at bases for record structs even when they are present in an error scenario #Resolved

return true;
};

case SymbolKind.NamedType:
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case SymbolKind.NamedType:

I think we should preserve the difference for this case. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 20, 2021

        public TypeDeclarationSyntax? RecordDeclarationWithParameters

RecordDeclarationSyntax? #Closed


Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:2489 in 9c48dd6. [](commit_id = 9c48dd6, deletion_comment = False)

@@ -2491,7 +2491,7 @@ private sealed class DeclaredMembersAndInitializersBuilder
get { return _recordDeclarationWithParameters; }
set
{
Debug.Assert(value is RecordDeclarationSyntax or RecordStructDeclarationSyntax);
Debug.Assert(value is RecordDeclarationSyntax);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug.Assert(value is RecordDeclarationSyntax);

The assert will be unnecessary after the type change for the property. #Closed

};
ParameterListSyntax? paramList = declaredMembersAndInitializers.RecordDeclarationWithParameters is RecordDeclarationSyntax recordDecl
? recordDecl.ParameterList
: null;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declaredMembersAndInitializers.RecordDeclarationWithParameters?.ParameterList? #Closed

_ => throw ExceptionUtilities.Unreachable
};
var record = (RecordDeclarationSyntax)GetSyntax();
Debug.Assert(record.Kind() == SyntaxKind.RecordDeclaration || record.PrimaryConstructorBaseType is null);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| record.PrimaryConstructorBaseType is null

We cannot make this assumption. I think we should explicitly return null for a record struct. #Closed

@@ -671,7 +671,8 @@ internal override bool TryGetBaseList(ExpressionSyntax expression, out TypeKindO
{
if (node is BaseListSyntax)
{
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax or RecordStructDeclarationSyntax)
if (node.Parent is InterfaceDeclarationSyntax or StructDeclarationSyntax
|| node.Parent.Kind() == SyntaxKind.RecordStructDeclaration)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind

I think IDE layer has a helper that checks kind agains multiple possible values. If InterfaceDeclarationSyntax and StructDeclarationSyntax can have only one kind, it might be more efficient to that helper instead. #Closed

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Or just use pattern matching against the Kind() #Resolved

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 20, 2021

Done with review pass (commit 3)


In reply to: 823349009


In reply to: 823349009


In reply to: 823349009

{
get
{
if (ClassOrStructKeyword.Kind() is SyntaxKind.StructKeyword)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClassOrStructKeyword.Kind() is SyntaxKind.StructKeyword

Kind() == SintaxKind.RecordStructDeclaration? #Closed

@@ -271,7 +271,7 @@ internal override bool TryGetSpeculativeSemanticModelCore(SyntaxTreeSemanticMode
{
if (MemberSymbol is SynthesizedRecordConstructor primaryCtor &&
primaryCtor.GetSyntax() is RecordDeclarationSyntax recordDecl &&
Root.FindToken(position).Parent?.AncestorsAndSelf().OfType<PrimaryConstructorBaseTypeSyntax>().FirstOrDefault() == recordDecl.PrimaryConstructorBaseType)
Root.FindToken(position).Parent?.AncestorsAndSelf().OfType<PrimaryConstructorBaseTypeSyntax>().FirstOrDefault() == recordDecl.PrimaryConstructorBaseTypeIfClass)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root.FindToken(position).Parent?.AncestorsAndSelf().OfType().FirstOrDefault() == recordDecl.PrimaryConstructorBaseTypeIfClass

I am assuming this method is never called for a record struct, otherwise it should never return true, even in error scenarios. It might be worth adding an assert at the top of the method that MemberSymbol is not a primary constructor of a record struct. Also, I think we need to add a test that tries to get the speculative semantic model for a record struct at position of the first base when it has and doesn't have arguments. BTW, it looks like the is a bug, the following code compiles without any error:

public interface I1
{
   void M01();
}

record struct R(int X) : I1(X)
{
   void I1.M01() {}
}
``` #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a bug for a regular record:

record R(int X) : I1()
{
    void I1.M01() {}
}

No error. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test BaseArguments_Speculation. That test will reach the body of this conditional when using record instead of record struct. With record struct we don't even get to the TryGetSpeculativeSemanticModelCore method.
I think it's because of the logic in SyntaxTreeSemanticModel.GetMemberModel which handles SyntaxKind.RecordDeclaration but not SyntaxKind.RecordStructDeclaration


In reply to: 617049655 [](ancestors = 617049655)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That matches my expectations, that is why I am suggesting to add an assert to ensure the method (not the body of the if) is not reachable for a record struct

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 20, 2021

Done with review pass (commit 6)


In reply to: 823618084

@jcouv jcouv requested a review from AlekseyTs April 21, 2021 06:21
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 21, 2021

                if (typeKeyword.Kind != SyntaxKind.RecordKeyword || !haveParameters)

Not sure why this check was removed. #Closed


Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:1823 in 8618ce5. [](commit_id = 8618ce5, deletion_comment = True)

void checkPrimaryConstructorBaseType(BaseTypeSyntax baseTypeSyntax, TypeSymbol baseType)
{
if (baseTypeSyntax is PrimaryConstructorBaseTypeSyntax primaryConstructorBaseType &&
(!IsRecord || TypeKind == TypeKind.Struct || baseType.TypeKind == TypeKind.Interface || ((RecordDeclarationSyntax)decl.SyntaxReference.GetSyntax()).ParameterList is null))
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeKind == TypeKind.Struct || baseType.TypeKind == TypeKind.Interface

TypeKind != TypeKind.Class || baseType.TypeKind != TypeKind.Class? Only base class can have arguments. #Closed

Copy link
Member Author

@jcouv jcouv Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first part: != Class and == Struct are equivalent since we're dealing with a record (IsRecord check earlier on the line).
For the second part, the reason I checked for interface is because the other scenarios all already have errors (see below). #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first part: != Class and == Struct are equivalent since we're dealing with a record (IsRecord check earlier on the line).
For the second part, the reason I checked for interface is because the other scenarios all already have errors (see below).

It always more robust to not depend on other conditions and checking for what is required for the "good" state rather than trying to cover other possible "bad" states. The arguments are allowed for classes on base classes. I believe that the condition should be coded in these terms, rather than trying to cover what else might be there based on some previos conditions. #Resolved

@@ -500,6 +501,8 @@ private static BaseListSyntax GetBaseListOpt(SingleTypeDeclaration decl)
baseType = baseBinder.BindType(typeSyntax, diagnostics, newBasesBeingResolved).Type;
}

checkPrimaryConstructorBaseType(baseTypeSyntax, baseType);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkPrimaryConstructorBaseType

It is probably not worth running this check past the first item, or we should complain unconditionally (only the first item in the list is allowed to have arguments). #Closed

Copy link
Member Author

@jcouv jcouv Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser only produces a PrimaryConstructorBaseType for the first element, so this check bails out on subsequent items already. I'm not sure what you're proposing. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check, but seems redundant to me...

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check, but seems redundant to me...

I don't think it is redundant because we are not always dealing with trees from the parser. #Resolved

@@ -2224,11 +2224,7 @@ class C(int X, int Y)

if (!withParameters && withBaseArguments)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!withParameters && withBaseArguments)

Conditional logic is no longer necessary #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 8)

record struct R1(int X) : Error(0, 1)
{
}
record struct R2(int X) : Error()
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also try to speculate on that case without the argument list. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps on position in the middle of the base type name

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 13)

@jcouv jcouv merged commit a657570 into dotnet:features/record-structs Apr 22, 2021
@jcouv jcouv deleted the record-syntax branch April 22, 2021 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants