Record structs: add properties and primary ctor #51720

jcouv · 2021-03-07T01:15:33Z

... and Deconstruct method.

Test plan: #51199
Relevant sections of the spec:

Members of a record struct

In addition to the members declared in the record struct body, a record struct type has additional synthesized members.
Members are synthesized unless a member with a "matching" signature is declared in the record struct body or
an accessible concrete non-virtual member with a "matching" signature is inherited.
Two members are considered matching if they have the same
signature or would be considered "hiding" in an inheritance scenario.
See https://github.com/dotnet/csharplang/blob/master/spec/basic-concepts.md#signatures-and-overloading

It is an error for a member of a record struct to be named "Clone".

It is an error for an instance field of a record struct to have an unsafe type.

A record struct is not permitted to declare a destructor.

...

Positional record struct members

In addition to the above members, record structs with a parameter list ("positional records") synthesize
additional members with the same conditions as the members above.

Primary Constructor

A record struct has a public constructor whose signature corresponds to the value parameters of the
type declaration. This is called the primary constructor for the type. It is an error to have a primary
constructor and a constructor with the same signature already present in the struct.
A record struct is not permitted to declare a parameterless primary constructor.

Instance field declarations for a record struct are permitted to include variable initializers.
If there is no primary constructor, the instance initializers execute as part of the parameterless constructor.
Otherwise, at runtime the primary constructor executes the instance initializers appearing in the record-struct-body.

If a record struct has a primary constructor, any user-defined constructor must have an
explicit this constructor initializer.

Parameters of the primary constructor as well as members of the record struct are in scope within initializers of instance fields or properties.
Instance members would be an error in these locations (similar to how instance members are in scope in regular constructor initializers
today, but an error to use), but the parameters of the primary constructor would be in scope and useable and
would shadow members. Static members would also be useable.

A warning is produced if a parameter of the primary constructor is not read.

The definite assigment rules for struct instance constructors apply to the primary constructor of record structs. For instance, the following
is an error:
record struct Pos(int X) // definite assignment error in primary constructor
{
    private int x;
    public int X { get { return x; } set { x = value; } } = X;
}
Properties

For each record struct parameter of a record struct declaration there is a corresponding public property
member whose name and type are taken from the value parameter declaration.

For a record struct:

A public get and init auto-property is created if the record struct has readonly modifier, get and set otherwise.
Both kinds of set accessors (set and init) are considered "matching". So the user may declare an init-only property
in place of a synthesized mutable one.
An inherited abstract property with matching type is overridden.
It is an error if the inherited property does not have public get and set/init accessors.
The auto-property is initialized to the value of the corresponding primary constructor parameter.
Attributes can be applied to the synthesized auto-property and its backing field by using property: or field:
targets for attributes syntactically applied to the corresponding record struct parameter.

Deconstruct

A positional record struct with at least one parameter synthesizes a public void-returning instance method called Deconstruct with an out
parameter declaration for each parameter of the primary constructor declaration. Each parameter
of the Deconstruct method has the same type as the corresponding parameter of the primary
constructor declaration. The body of the method assigns each parameter of the Deconstruct method
to the value from an instance member access to a member of the same name.
The method can be declared explicitly. It is an error if the explicit declaration does not match
the expected signature or accessibility, or is static.

RikkiGibson · 2021-03-09T05:15:17Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

-            ParameterListSyntax? paramList = declaredMembersAndInitializers.RecordDeclarationWithParameters?.ParameterList;
+            ParameterListSyntax? paramList = declaredMembersAndInitializers.RecordDeclarationWithParameters switch
+            {
+                RecordDeclarationSyntax recordDecl => recordDecl.ParameterList,


It feels like this could be a useful extension method on TypeDeclarationSyntax #ByDesign

RikkiGibson · 2021-03-09T05:32:04Z

src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs

@@ -795,6 +795,7 @@ public static SyntaxKind GetTypeDeclarationKind(SyntaxKind kind)
                case SyntaxKind.InterfaceKeyword:
                    return SyntaxKind.InterfaceDeclaration;
                case SyntaxKind.RecordKeyword:
+                    // PROTOTYPE(record-structs): anything we can do? 


Might need an additional parameter for the record keyword. #WontFix

RikkiGibson · 2021-03-09T05:36:59Z

src/Compilers/CSharp/Portable/Compilation/MethodBodySemanticModel.cs

@@ -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 &&


Is it expected that we can't get a speculative model for a record struct here? #Resolved

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)

RikkiGibson · 2021-03-09T05:41:07Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

@@ -796,6 +796,7 @@ partial class C
        [Fact]
        public void PartialRecord_ParametersInScopeOfBothParts()
        {
+            // PROTOTYPE(record-structs): ported


What's the significance of these comments? #Resolved

Not much. Just helps keep track of which tests have been duplicated/ported for record-structs. I think this will help with feature review at the end (are there some more tests that we want to move over?). I'll bulk remove those comments once the feature review is done.

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

AlekseyTs · 2021-03-09T21:18:30Z

@jcouv Could you please clarify the following:

Instance field declarations for a record struct are permitted to include variable initializers.
If there is no primary constructor, the instance initializers execute as part of the parameterless constructor.
Otherwise, at runtime the primary constructor executes the instance initializers appearing in the record-struct-body.

If I remember correctly, structures in C# cannot declare parameter-less constructors and instance initializers are not permitted in structures for this reason, i.e. no constructor to perform the initialization. #Closed

AlekseyTs · 2021-03-09T21:21:53Z

If a record struct has a primary constructor, any user-defined constructor, except "copy constructor" must have an
explicit this constructor initializer.

It looks like the spec doesn't define a "copy constructor". #Closed

AlekseyTs · 2021-03-09T21:26:31Z

record struct Pos(int X) // definite assignment error in primary constructor

Consider clarifying what is the error to eliminate any guess work. #Closed

AlekseyTs · 2021-03-09T21:41:45Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -3414,6 +3427,11 @@ private void CheckForStructBadInitializers(DeclaredMembersAndInitializersBuilder
        {
            Debug.Assert(TypeKind == TypeKind.Struct);

+            if (builder.RecordDeclarationWithParameters is RecordStructDeclarationSyntax { ParameterList: { ParameterCount: >= 0 } })


ParameterCount: >= 0 [](start = 108, length = 20)

What is the point of this check? #Closed

AlekseyTs · 2021-03-09T21:48:06Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

                var ctor = declaredMembersAndInitializers.RecordPrimaryConstructor;
                Debug.Assert(ctor is object);
                members.Add(ctor);

                if (ctor.ParameterCount != 0)
                {
+                    // properties and Deconstruct
                    var existingOrAddedMembers = addProperties(ctor.Parameters);
                    addDeconstruct(ctor, existingOrAddedMembers);
                }

                primaryAndCopyCtorAmbiguity = ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this, TypeCompareKind.AllIgnoreOptions);


primaryAndCopyCtorAmbiguity = ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this, TypeCompareKind.AllIgnoreOptions); [](start = 16, length = 129)

A concept of a copy constructor is not defined in the spec for a record struct. This calculation doesn't make sense then. #Closed

AlekseyTs · 2021-03-09T21:55:08Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

                            // Ignore the record copy constructor
-                            if (!IsRecord ||
-                                !(SynthesizedRecordCopyCtor.HasCopyConstructorSignature(method) && method is not SynthesizedRecordConstructor))
+                            if (!(IsRecord && SynthesizedRecordCopyCtor.HasCopyConstructorSignature(method) && method is not SynthesizedRecordConstructor))


!(IsRecord && SynthesizedRecordCopyCtor.HasCopyConstructorSignature(method) && method is not SynthesizedRecordConstructor)) [](start = 32, length = 123)

It is not clear what is the motivation for this change. If it is just a refactoring, please revert it because it doesn't look semantically equivalent. #Closed

This change is not necessary. It is semantically equivalent though (DeMorgan law). I even checked with the built-in refactoring:

I think it's easier to understand this way, but if you feel strongly I could revert. #Closed

I think it's easier to understand this way, but if you feel strongly I could revert.

If it is not broken, don't fix it

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

AlekseyTs · 2021-03-09T22:16:00Z

src/Compilers/CSharp/Portable/Compilation/MethodBodySemanticModel.cs

@@ -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 &&


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

Alternatively we could check if the containing type is a class. #Closed

I think the current approach is okay and relatively compact.

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

AlekseyTs · 2021-03-09T22:26:41Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+        {
+            Debug.Assert(recordStructDecl.ParameterList is object);
+
+            Binder bodyBinder = this.GetBinder(recordStructDecl);


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

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

jcouv · 2021-03-09T22:28:27Z

Could you clarify [...] If I remember correctly, structures in C# cannot declare parameter-less constructors and instance initializers are not permitted in structures for this reason, i.e. no constructor to perform the initialization.

As far as the meaning of the spec, there is context in the intro: "Record structs will also follow the same rules as structs for parameterless instance constructors and field initializers, but this document assumes that we will lift those restrictions for structs generally."

So the intention was that instance initializers would be allowed in record-structs.
The scenario without parameter list: record struct S { int field = 1; } is not yet allowed (marked with PROTOTYPE in TypeDeclaration_NoInstanceInitializers).
The scenario with parameter list is allowed already (see RecordProperties_01 for non-empty case; I'll add a test for empty parameter list). I initially had disallowed the scenario with empty parameter list, but then relaxed it (it works). Let me know what you think.

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

jcouv · 2021-03-09T22:33:23Z

PR for fixing spec: dotnet/csharplang#4515

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

AlekseyTs · 2021-03-09T22:43:15Z

I initially had disallowed the scenario with empty parameter list, but then relaxed it (it works). Let me know what you think.

I think "it works" isn't clearly defined at this point. This looks like a violation of the spec: "A record struct is not permitted to declare a parameterless primary constructor." I think an empty parameter list should be disallowed. That should be designed, implemented and tested separately when the other feature is in, or the other feature should take care of this, if record structs are in first.
Also, it is not clear why member initializers wouldn't synthesize a parameter-less constructor in a record struct with a primary constructor, once those are supported. It feels like all this needs additional design work. I think the spec should be cleaned up, the non-primary constructor scenarios removed, or moved to an open issues section.

In reply to: 794541167 [](ancestors = 794541167,794467406)

src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs

src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeExtensions.cs

AlekseyTs · 2021-03-09T23:00:14Z

PR for fixing spec: dotnet/csharplang#4515

Please make sure to adjust PR description accordingly and reflect the change in the commit comment when this PR will be merged.

In reply to: 794545994 [](ancestors = 794545994,794471038)

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

src/Compilers/CSharp/Portable/Compilation/MethodBodySemanticModel.cs

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

jcouv · 2021-03-09T23:39:42Z

No, I haven't been including quotes from the spec into the commit comment. So that won't be a problem.
I only included sections of the spec in PR description to clarify the scope and for convenience.

In reply to: 794600331 [](ancestors = 794600331,794590011,794574016,794545994,794471038)

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

AlekseyTs · 2021-03-10T00:10:51Z

Done with review pass (commit 3) #Closed

jcouv · 2021-03-10T01:17:02Z

Added a test to illustrate. I'll update the spec

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

AlekseyTs · 2021-03-10T16:20:17Z

    internal override ExecutableCodeBinder TryGetBodyBinder(BinderFactory? binderFactoryOpt = null, bool ignoreAccessibility = false)

Consider asserting that we are not getting here for a record struct. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs:69 in 244a64a. [](commit_id = 244a64a, deletion_comment = False)

AlekseyTs · 2021-03-10T16:21:49Z

src/Compilers/CSharp/Portable/Binder/BinderFactory.cs

@@ -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();


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

Consider asserting that this is not a record struct #Closed

AlekseyTs · 2021-03-10T16:29:18Z

src/Compilers/CSharp/Portable/Binder/BinderFactory.cs

        {
+            Debug.Assert(typeDecl.Kind() is SyntaxKind.RecordDeclaration or SyntaxKind.RecordStructDeclaration);


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

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

AlekseyTs · 2021-03-10T16:32:58Z

        if (constructor is SynthesizedRecordCopyCtor copyCtor)

It feels like we should return null for a record struct primary constructor here. #Closed

Refers to: src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs:1942 in 244a64a. [](commit_id = 244a64a, deletion_comment = False)

AlekseyTs · 2021-03-10T16:35:36Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -1963,9 +1967,9 @@ private static void ReportCtorInitializerCycles(MethodSymbol method, BoundExpres
                CSharpSyntaxNode containerNode = constructor.GetNonNullSyntaxNode();
                BinderFactory binderFactory = compilation.GetBinderFactory(containerNode.SyntaxTree);

-                if (containerNode is RecordDeclarationSyntax recordDecl)
+                if (containerNode is RecordDeclarationSyntax or RecordStructDeclarationSyntax)


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

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

AlekseyTs · 2021-03-10T16:36:10Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -1991,6 +1995,10 @@ private static void ReportCtorInitializerCycles(MethodSymbol method, BoundExpres
                        outerBinder = binderFactory.GetInRecordBodyBinder(recordDecl);
                        break;

+                    case RecordStructDeclarationSyntax recordStructDecl:


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

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

AlekseyTs · 2021-03-10T16:37:07Z

        if (constructor is SynthesizedRecordCopyCtor copyCtor)

In fact, we probably can do this for any constructor in a struct.

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

Refers to: src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs:1942 in 244a64a. [](commit_id = 244a64a, deletion_comment = False)

AlekseyTs · 2021-03-10T16:40:51Z

        if (constructor is SynthesizedRecordCopyCtor copyCtor)

Or in an enum

In reply to: 795704469 [](ancestors = 795704469,795698328)

Refers to: src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs:1942 in 244a64a. [](commit_id = 244a64a, deletion_comment = False)

AlekseyTs · 2021-03-10T16:45:12Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

                case RecordDeclarationSyntax recordDecl:
                    return recordDecl;
+                case RecordStructDeclarationSyntax recordStructDecl:


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

It feels like this case is not needed, we should never need the binder. #Closed

AlekseyTs · 2021-03-10T16:52:34Z

Done with review pass (commit 5) #Closed

AlekseyTs · 2021-03-10T16:54:20Z

It looks like there are legitimate test failures. #Closed

AlekseyTs · 2021-03-10T22:08:22Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -1935,7 +1939,11 @@ private static void ReportCtorInitializerCycles(MethodSymbol method, BoundExpres
                }
            }

-            if (constructor is SynthesizedRecordCopyCtor copyCtor)
+            if (constructor is SynthesizedRecordConstructor && constructor.ContainingType.IsStructType())


constructor is SynthesizedRecordConstructor && constructor.ContainingType.IsStructType() [](start = 16, length = 88)

This is not blocking, but I would simply change this condition to check if the containing type is a structure or an enum. #Closed

AlekseyTs

LGTM (commit 7)

jcouv added Area-Compilers Feature - Records Records labels Mar 7, 2021

jcouv added this to the 16.10 milestone Mar 7, 2021

jcouv self-assigned this Mar 7, 2021

jcouv force-pushed the rs-symbol2 branch from 012500c to 384d41d Compare March 8, 2021 18:59

Record structs: add properties and primary ctor

07f6466

jcouv force-pushed the rs-symbol2 branch from 384d41d to 07f6466 Compare March 8, 2021 19:36

jcouv marked this pull request as ready for review March 8, 2021 20:56

jcouv requested a review from a team as a code owner March 8, 2021 20:56

RikkiGibson self-assigned this Mar 8, 2021

RikkiGibson approved these changes Mar 9, 2021

View reviewed changes