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.

Sign up for GitHub

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: add properties and primary ctor #51720

Merged
merged 8 commits into from
Mar 12, 2021
Merged

Record structs: add properties and primary ctor #51720

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
07f6466
Record structs: add properties and primary ctor
jcouv Feb 20, 2021
d0789ad
Address feedback
jcouv Mar 9, 2021
dd9069b
Address feedback
jcouv Mar 9, 2021
9bec338
Address feedback
jcouv Mar 10, 2021
244a64a
Address feedback
jcouv Mar 10, 2021
caccbb3
Address feedback
jcouv Mar 10, 2021
04b39e9
tweak
jcouv Mar 10, 2021
f2b8433
Check struct or enum type
jcouv Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,15 +1142,15 @@ private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memb
return new WithParametersBinder(method.Parameters, nextBinder);
}

// PROTOTYPE(record-structs): update for record structs
if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } recordDeclSyntax)
if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } }
or RecordStructDeclarationSyntax { ParameterList: { ParameterCount: > 0 } })
{
Binder outerBinder = VisitCore(memberSyntax);
SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember(recordDeclSyntax);
SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember((TypeDeclarationSyntax)memberSyntax);
var primaryConstructor = recordType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault();

if (primaryConstructor.SyntaxRef.SyntaxTree == recordDeclSyntax.SyntaxTree &&
primaryConstructor.GetSyntax() == recordDeclSyntax)
if (primaryConstructor.SyntaxRef.SyntaxTree == memberSyntax.SyntaxTree &&
primaryConstructor.GetSyntax() == memberSyntax)
{
return new WithParametersBinder(primaryConstructor.Parameters, nextBinder);
}
Expand Down
7 changes: 4 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/BinderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,7 @@ internal Binder GetBinder(SyntaxNode node, int position, CSharpSyntaxNode member

internal InMethodBinder GetRecordConstructorInMethodBinder(SynthesizedRecordConstructor constructor)
{
// PROTOTYPE(record-structs): update for record structs
RecordDeclarationSyntax typeDecl = constructor.GetSyntax();
TypeDeclarationSyntax typeDecl = constructor.GetSyntax();
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

typeDecl [](start = 34, length = 8)

Consider asserting that this is not a record struct #Closed


var extraInfo = NodeUsage.ConstructorBodyOrInitializer;
var key = BinderFactoryVisitor.CreateBinderCacheKey(typeDecl, extraInfo);
Expand All @@ -158,8 +157,10 @@ internal InMethodBinder GetRecordConstructorInMethodBinder(SynthesizedRecordCons
return (InMethodBinder)resultBinder;
}

internal Binder GetInRecordBodyBinder(RecordDeclarationSyntax typeDecl)
internal Binder GetInRecordBodyBinder(TypeDeclarationSyntax typeDecl)
{
Debug.Assert(typeDecl.Kind() is SyntaxKind.RecordDeclaration or SyntaxKind.RecordStructDeclaration);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

or SyntaxKind.RecordStructDeclaration [](start = 73, length = 37)

It feels like we should never call this method for a record struct. #Closed


BinderFactoryVisitor visitor = _binderFactoryVisitorPool.Allocate();
visitor.Initialize(position: typeDecl.SpanStart, memberDeclarationOpt: null, memberOpt: null);
Binder resultBinder = visitor.VisitTypeDeclarationCore(typeDecl, NodeUsage.NamedTypeBodyOrTypeParameters);
Expand Down
18 changes: 17 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3286,10 +3286,12 @@ public virtual BoundNode BindMethodBody(CSharpSyntaxNode syntax, BindingDiagnost
{
switch (syntax)
{
// PROTOTYPE(record-structs): update for record structs
case RecordDeclarationSyntax recordDecl:
return BindRecordConstructorBody(recordDecl, diagnostics);

case RecordStructDeclarationSyntax recordDecl:
return BindRecordStructConstructorBody(recordDecl);

case BaseMethodDeclarationSyntax method:
if (method.Kind() == SyntaxKind.ConstructorDeclaration)
{
Expand Down Expand Up @@ -3356,6 +3358,20 @@ private BoundNode BindRecordConstructorBody(RecordDeclarationSyntax recordDecl,
expressionBody: null);
}

private BoundNode BindRecordStructConstructorBody(RecordStructDeclarationSyntax recordStructDecl)
{
Debug.Assert(recordStructDecl.ParameterList is object);

Binder bodyBinder = this.GetBinder(recordStructDecl);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Binder bodyBinder = this.GetBinder(recordStructDecl) [](start = 12, length = 52)

It doesn't look like the binder is needed #Closed

Debug.Assert(bodyBinder != null);

return new BoundConstructorMethodBody(recordStructDecl,
bodyBinder.GetDeclaredLocalsForScope(recordStructDecl),
initializer: null,
blockBody: new BoundBlock(recordStructDecl, ImmutableArray<LocalSymbol>.Empty, ImmutableArray<BoundStatement>.Empty).MakeCompilerGenerated(),
expressionBody: null);
}

internal virtual BoundExpressionStatement BindConstructorInitializer(PrimaryConstructorBaseTypeSyntax initializer, BindingDiagnosticBag diagnostics)
{
BoundExpression initializerInvocation = GetBinder(initializer).BindConstructorInitializer(initializer.ArgumentList, (MethodSymbol)this.ContainingMember(), diagnostics);
Expand Down
10 changes: 8 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,11 @@ protected void FindExpressionVariables(
Debug.Assert(node.Parent is ConstructorInitializerSyntax || node.Parent is PrimaryConstructorBaseTypeSyntax);
break;
case SyntaxKind.RecordDeclaration:
// PROTOTYPE(record-structs): update for record structs
Debug.Assert(((RecordDeclarationSyntax)node).ParameterList is object);
break;
case SyntaxKind.RecordStructDeclaration:
Debug.Assert(((RecordStructDeclarationSyntax)node).ParameterList is object);
break;
default:
Debug.Assert(node is ExpressionSyntax);
break;
Expand Down Expand Up @@ -396,7 +398,6 @@ public override void VisitConstructorDeclaration(ConstructorDeclarationSyntax no
}
}

// PROTOTYPE(record-structs): update for record structs
public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
{
Debug.Assert(node.ParameterList is object);
Expand All @@ -407,6 +408,11 @@ public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
}
}

public override void VisitRecordStructDeclaration(RecordStructDeclarationSyntax node)
{
Debug.Assert(node.ParameterList is object);
}

private void CollectVariablesFromDeconstruction(
ExpressionSyntax possibleTupleDeclaration,
AssignmentExpressionSyntax deconstruction)
Expand Down
9 changes: 8 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ public override void VisitConstructorDeclaration(ConstructorDeclarationSyntax no
Visit(node.ExpressionBody, enclosing);
}

// PROTOTYPE(record-structs): update for record structs
public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
{
Debug.Assert(node.ParameterList is object);
Expand All @@ -168,6 +167,14 @@ public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
Visit(node.PrimaryConstructorBaseType, enclosing);
}

public override void VisitRecordStructDeclaration(RecordStructDeclarationSyntax node)
{
Debug.Assert(node.ParameterList is object);

Binder enclosing = new ExpressionVariableBinder(node, _enclosing);
AddToMap(node, enclosing);
}

public override void VisitPrimaryConstructorBaseType(PrimaryConstructorBaseTypeSyntax node)
{
Binder enclosing = _enclosing.WithAdditionalFlags(BinderFlags.ConstructorInitializer);
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Compilation/MethodBodySemanticModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ internal override BoundNode Bind(Binder binder, CSharpSyntaxNode node, BindingDi
case SyntaxKind.RemoveAccessorDeclaration:
case SyntaxKind.CompilationUnit:
case SyntaxKind.RecordDeclaration:
case SyntaxKind.RecordStructDeclaration:
return binder.BindMethodBody(node, diagnostics);
}

Expand Down Expand Up @@ -270,7 +271,8 @@ internal override bool TryGetSpeculativeSemanticModelCore(SyntaxTreeSemanticMode
internal override bool TryGetSpeculativeSemanticModelCore(SyntaxTreeSemanticModel parentModel, int position, PrimaryConstructorBaseTypeSyntax constructorInitializer, out SemanticModel speculativeModel)
{
if (MemberSymbol is SynthesizedRecordConstructor primaryCtor &&
Root.FindToken(position).Parent?.AncestorsAndSelf().OfType<PrimaryConstructorBaseTypeSyntax>().FirstOrDefault() == primaryCtor.GetSyntax().PrimaryConstructorBaseType)
primaryCtor.GetSyntax() is RecordDeclarationSyntax recordDecl &&
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Is it expected that we can't get a speculative model for a record struct here? #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.

The logic here relates to the primary constructor base type, which doesn't exist for record structs. That's why we only need to handle record classes here.

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

Copy link
Contributor

@AlekseyTs AlekseyTs Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

primaryCtor.GetSyntax() is RecordDeclarationSyntax recordDecl && [](start = 16, length = 64)

Alternatively we could check if the containing type is a class. #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 think the current approach is okay and relatively compact.

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

Root.FindToken(position).Parent?.AncestorsAndSelf().OfType<PrimaryConstructorBaseTypeSyntax>().FirstOrDefault() == recordDecl.PrimaryConstructorBaseType)
{
var binder = this.GetEnclosingBinder(position);
if (binder != null)
Expand Down
12 changes: 8 additions & 4 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1912,8 +1912,8 @@ internal static BoundExpression BindImplicitConstructorInitializer(
NamedTypeSymbol baseType = constructor.ContainingType.BaseTypeNoUseSiteDiagnostics;

SourceMemberMethodSymbol sourceConstructor = constructor as SourceMemberMethodSymbol;
// PROTOTYPE(record-structs): update for record structs
Debug.Assert(sourceConstructor?.SyntaxNode is RecordDeclarationSyntax || ((ConstructorDeclarationSyntax)sourceConstructor?.SyntaxNode)?.Initializer == null);
Debug.Assert(sourceConstructor?.SyntaxNode is RecordDeclarationSyntax or RecordStructDeclarationSyntax
|| ((ConstructorDeclarationSyntax)sourceConstructor?.SyntaxNode)?.Initializer == null);

// The common case is that the type inherits directly from object.
// Also, we might be trying to generate a constructor for an entirely compiler-generated class such
Expand Down Expand Up @@ -1963,9 +1963,9 @@ internal static BoundExpression BindImplicitConstructorInitializer(
CSharpSyntaxNode containerNode = constructor.GetNonNullSyntaxNode();
BinderFactory binderFactory = compilation.GetBinderFactory(containerNode.SyntaxTree);

if (containerNode is RecordDeclarationSyntax recordDecl)
if (containerNode is RecordDeclarationSyntax or RecordStructDeclarationSyntax)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if (containerNode is RecordDeclarationSyntax or RecordStructDeclarationSyntax) [](start = 16, length = 78)

It feels like the change is not necessary, assuming we return null above. #Closed

{
outerBinder = binderFactory.GetInRecordBodyBinder(recordDecl);
outerBinder = binderFactory.GetInRecordBodyBinder((TypeDeclarationSyntax)containerNode);
}
else
{
Expand All @@ -1991,6 +1991,10 @@ internal static BoundExpression BindImplicitConstructorInitializer(
outerBinder = binderFactory.GetInRecordBodyBinder(recordDecl);
break;

case RecordStructDeclarationSyntax recordStructDecl:
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

case RecordStructDeclarationSyntax recordStructDecl: [](start = 16, length = 56)

It feels like the change is not necessary, assuming we return null above. #Closed

outerBinder = binderFactory.GetInRecordBodyBinder(recordStructDecl);
break;

default:
throw ExceptionUtilities.Unreachable;
}
Expand Down
5 changes: 2 additions & 3 deletions src/Compilers/CSharp/Portable/Declarations/DeclarationTreeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,10 +405,9 @@ private SingleNamespaceOrTypeDeclaration VisitTypeDeclaration(TypeDeclarationSyn
var memberNames = GetNonTypeMemberNames(((Syntax.InternalSyntax.TypeDeclarationSyntax)(node.Green)).Members,
ref declFlags);

// PROTOTYPE(record-structs): update for record structs
// A record with parameters at least has a primary constructor
if (((declFlags & SingleTypeDeclaration.TypeDeclarationFlags.HasAnyNontypeMembers) == 0) &&
node is RecordDeclarationSyntax { ParameterList: { } })
node is RecordDeclarationSyntax { ParameterList: { } } or RecordStructDeclarationSyntax { ParameterList: { } })
{
declFlags |= SingleTypeDeclaration.TypeDeclarationFlags.HasAnyNontypeMembers;
}
Expand Down Expand Up @@ -631,7 +630,7 @@ private static bool CheckMemberForAttributes(Syntax.InternalSyntax.CSharpSyntaxN
case SyntaxKind.InterfaceDeclaration:
case SyntaxKind.EnumDeclaration:
case SyntaxKind.RecordDeclaration:
// PROTOTYPE(record-structs): update for record structs
case SyntaxKind.RecordStructDeclaration:
return (((Syntax.InternalSyntax.BaseTypeDeclarationSyntax)member).AttributeLists).Any();

case SyntaxKind.DelegateDeclaration:
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ protected SourceConstructorSymbolBase(
{
Debug.Assert(
syntax.IsKind(SyntaxKind.ConstructorDeclaration) ||
syntax.IsKind(SyntaxKind.RecordDeclaration));
syntax.IsKind(SyntaxKind.RecordDeclaration) ||
syntax.IsKind(SyntaxKind.RecordStructDeclaration));
}

protected sealed override void MethodChecks(BindingDiagnosticBag diagnostics)
Expand Down
Loading