Support struct records #43133

agocke · 2020-04-07T00:11:25Z

I believe this should work according to the spec and am just cleaning
up some implementation gaps.

Relates to #40726 (test plan for records)

I believe this should work according to the spec and am just cleaning up some implementation gaps.

gafter · 2020-04-07T22:33:03Z

I see there were some recent changes to the spec which have issues (i.e. don't make sense). I cannot figure out how to add comments on those changes as there was never a PR that I could view. I don't think it is implementable as written (e.g. there is no way to "suppress" the struct default constructor on the CLR). So consider this a comment that the implementation here doesn't do that as specified. #Resolved

agocke · 2020-04-07T22:54:18Z

@gafter I don't follow. There's no way to suppress struct default construction but we can certainly prevent surfacing a default constructor. The spec change is that the struct parameterless constructor for a record is no longer visible. #Resolved

gafter · 2020-04-07T22:57:24Z

The C# compiler currently never produces a default constructor for a struct, so I don't know what there is to suppress (or make not visible). #Resolved

agocke · 2020-04-07T22:59:31Z

Ah, I see, on metadata import there's no way to tell that the struct is a record, and therefore no way to avoid creating the symbol. Good point. I'll add a test for this and adjust the spec. #Resolved

gafter · 2020-04-07T23:03:40Z

As far as I know we don't create a symbol for a struct's default constructor, but yes, you have the idea. But the problem occurs purely in source too. A record struct would satisfy a type parameter's struct constraint, producing a type parameter that would satisfy another type parameter's new() constraint. #Resolved

AlekseyTs · 2020-04-07T23:07:21Z

As far as I know we don't create a symbol for a struct's default constructor

I think we do:

                        // Structs have an implicit parameterless constructor, even if it
                        // does not appear in metadata (11.3.8)
                        if (!haveParameterlessConstructor)
                        {
                            nonFieldMembers.Insert(0, new SynthesizedInstanceConstructor(this));
                        }
``` #Resolved

agocke · 2020-04-09T22:34:33Z

I've adjusted the spec and implementation to preserve default struct parameterless constructors #Resolved

gafter · 2020-04-10T20:50:10Z

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

@@ -1153,7 +1153,8 @@ public override object VisitField(FieldSymbol symbol, TypeCompilationState argum
                    // don't emit if the resulting method would contain initializers with errors
                    if (!hasErrors && (hasBody || includeInitializersInBody))
                    {
-                        Debug.Assert(!methodSymbol.IsImplicitInstanceConstructor || !methodSymbol.ContainingType.IsStructType());
+                        Debug.Assert(!(methodSymbol.IsImplicitInstanceConstructor && methodSymbol.ParameterCount == 0) ||


IsImplicitInstanceConstructor [](start = 52, length = 29)

I think the condition IsImplicitInstanceConstructor generally implies methodSymbol.ParameterCount == 0, so you've weakened the assertion in case there is a mismatch there. #Resolved

Not as far as I can tell: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/MethodSymbol.cs,fb6aebe573e152e2,references

That is satisfied for every record constructor as well, so all record constructors are now implicit instance constructors #Resolved

Are there any other implicit instance constructors though? AFAIK, there's only the default parameterless one.

Record constructors!

Fair enough.

Thankfully there's only one use of this property (in this assert), so we don't have to worry about that contract being violated elsewhere.

gafter · 2020-04-10T21:04:45Z

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

@@ -143,6 +143,9 @@ public bool SetNullableContext(byte? value)
            }
        }

+        private static readonly ObjectPool<PooledDictionary<Symbol, Symbol>> s_duplicateMemberSignatureDictionary =


s_duplicateMemberSignatureDictionary [](start = 77, length = 36)

Why do you need a custom pool instead of using PooledDictionary<Symbol, Symbol>.GetInstance()? #Resolved

Need a custom comparer #Resolved

gafter · 2020-04-10T21:14:48Z

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

+        public void StructRecord1()
+        {
+            var src = @"
+data struct Point(int X, int Y);";


data struct [](start = 0, length = 11)

I took a "data structures" class once. 😉

gafter

alrz · 2020-04-11T10:15:28Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordObjEquals.cs

+            {
+                // For structs:
+                //
+                //      return param is ContainingType i ? this.Equals(in i) : false;


param is ContainingType i && this.Equals(in i)?

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

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordObjEquals.cs

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

333fred · 2020-04-22T21:12:57Z

Done review pass (commit 3). Just a couple of small points.

agocke added the Area-Compilers label Apr 7, 2020

Support struct records

2a0e104

I believe this should work according to the spec and am just cleaning up some implementation gaps.

agocke force-pushed the record-struct branch from f6e6760 to 2a0e104 Compare April 7, 2020 00:12

agocke changed the title ~~Record struct~~ Support struct records Apr 7, 2020

agocke marked this pull request as ready for review April 7, 2020 18:39

agocke requested a review from a team as a code owner April 7, 2020 18:39

Keep parameterless constructor for struct records

b73b5b1

gafter self-requested a review April 8, 2020 01:15

Adjust formatting

7955a39

gafter reviewed Apr 10, 2020

View reviewed changes

gafter approved these changes Apr 10, 2020

View reviewed changes

alrz reviewed Apr 11, 2020

View reviewed changes

333fred reviewed Apr 22, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Show resolved Hide resolved

333fred reviewed Apr 22, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs Show resolved Hide resolved

333fred reviewed Apr 22, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordObjEquals.cs Show resolved Hide resolved

333fred reviewed Apr 22, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs Show resolved Hide resolved

agocke added 2 commits April 22, 2020 15:08

Respond to PR comments

9ddd3c1

Fix formatting

df4fc5f

333fred approved these changes Apr 23, 2020

View reviewed changes

agocke merged commit e232f50 into dotnet:features/records Apr 23, 2020

agocke deleted the record-struct branch April 23, 2020 00:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support struct records #43133

Support struct records #43133

agocke commented Apr 7, 2020 •

edited by jcouv

Loading

gafter commented Apr 7, 2020 •

edited

Loading

agocke commented Apr 7, 2020 •

edited by gafter

Loading

gafter commented Apr 7, 2020 •

edited

Loading

agocke commented Apr 7, 2020 •

edited by gafter

Loading

gafter commented Apr 7, 2020 •

edited

Loading

AlekseyTs commented Apr 7, 2020 •

edited by gafter

Loading

agocke commented Apr 9, 2020 •

edited by gafter

Loading

gafter Apr 10, 2020 •

edited

Loading

agocke Apr 10, 2020 •

edited by gafter

Loading

333fred Apr 22, 2020

agocke Apr 22, 2020

333fred Apr 22, 2020

333fred Apr 22, 2020

gafter Apr 10, 2020 •

edited

Loading

agocke Apr 10, 2020 •

edited by gafter

Loading

gafter Apr 10, 2020

gafter left a comment

alrz Apr 11, 2020 •

edited

Loading

333fred commented Apr 22, 2020

Support struct records #43133

Support struct records #43133

Conversation

agocke commented Apr 7, 2020 • edited by jcouv Loading

gafter commented Apr 7, 2020 • edited Loading

agocke commented Apr 7, 2020 • edited by gafter Loading

gafter commented Apr 7, 2020 • edited Loading

agocke commented Apr 7, 2020 • edited by gafter Loading

gafter commented Apr 7, 2020 • edited Loading

AlekseyTs commented Apr 7, 2020 • edited by gafter Loading

agocke commented Apr 9, 2020 • edited by gafter Loading

gafter Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

agocke Apr 10, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

333fred Apr 22, 2020

Choose a reason for hiding this comment

agocke Apr 22, 2020

Choose a reason for hiding this comment

333fred Apr 22, 2020

Choose a reason for hiding this comment

333fred Apr 22, 2020

Choose a reason for hiding this comment

gafter Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

agocke Apr 10, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Apr 10, 2020

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

alrz Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

333fred commented Apr 22, 2020

agocke commented Apr 7, 2020 •

edited by jcouv

Loading

gafter commented Apr 7, 2020 •

edited

Loading

agocke commented Apr 7, 2020 •

edited by gafter

Loading

gafter commented Apr 7, 2020 •

edited

Loading

agocke commented Apr 7, 2020 •

edited by gafter

Loading

gafter commented Apr 7, 2020 •

edited

Loading

AlekseyTs commented Apr 7, 2020 •

edited by gafter

Loading

agocke commented Apr 9, 2020 •

edited by gafter

Loading

gafter Apr 10, 2020 •

edited

Loading

agocke Apr 10, 2020 •

edited by gafter

Loading

gafter Apr 10, 2020 •

edited

Loading

agocke Apr 10, 2020 •

edited by gafter

Loading

alrz Apr 11, 2020 •

edited

Loading