Bind base arguments fo records #44876

AlekseyTs · 2020-06-05T02:49:59Z

Relevant LDM notes - https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-06-01.md

AlekseyTs · 2020-06-05T08:02:09Z

@agocke, @RikkiGibson, @cston, @jcouv, @dotnet/roslyn-compiler Please review #Closed

AlekseyTs · 2020-06-05T16:39:32Z

@agocke, @RikkiGibson, @cston, @jcouv, @dotnet/roslyn-compiler Please review #Closed

RikkiGibson · 2020-06-05T16:40:38Z

Taking a look #Resolved

gafter · 2020-06-05T16:58:51Z

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

+    public Base() {}
+}
+
+record C(int X, int Y) : Base(X, Y)


Base [](start = 25, length = 4)

Please test (for semantic errors) an argument list in the base with a class and struct declaration (rather than a record declaration). #Resolved

Please test (for semantic errors) an argument list in the base with a class and struct declaration (rather than a record declaration).

There are existing tests for parsing errors in these scenarios. However, I added some tests here as well.

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

gafter · 2020-06-05T17:03:44Z

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

+
+            if (node.BaseWithArguments is SimpleBaseTypeSyntax baseWithArguments)
+            {
+                VisitNodeToBind(baseWithArguments);


VisitNodeToBind [](start = 16, length = 15)

The scope rules implemented here are not supported by the specification at https://github.com/dotnet/csharplang/blob/master/proposals/records.md #WontFix

The scope rules implemented here are not supported by the specification at https://github.com/dotnet/csharplang/blob/master/proposals/records.md

It is a well-known fact that the spec is behind. Please see https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-06-01.md

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

Please update the specification so this code can be checked against that.

In reply to: 436051230 [](ancestors = 436051230,436049242)

RikkiGibson

LGTM with some minor comments/questions.

RikkiGibson · 2020-06-05T17:17:02Z

src/Compilers/CSharp/Portable/Symbols/LexicalSortKey.cs

@@ -53,8 +53,7 @@ public int Position
        // until later when it is known if they can be optimized or not.
        // As a result the last emitted method tokens are synthetic ctor and then synthetic cctor (if not optimized)
        // Since it is not too hard, we will try keeping the same order just to be easy on metadata diffing tools and such.
-        public static readonly LexicalSortKey SynthesizedRecordCopyCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 3 };
-        public static readonly LexicalSortKey SynthesizedRecordCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 2 };
+        public static readonly LexicalSortKey SynthesizedRecordCopyCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 2 };


should the above SortKey positions be adjusted? #Resolved

I've replaced this code in #44882. #Resolved

should the above SortKey positions be adjusted?

I am not following. Could you elaborate please?

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

For instance SynthesizedRecordObjEquals still has _position = int.MaxValue - 4 while there is no symbol with position int.MaxValue - 3. I doubt this negatively affects anything so perhaps we can just leave it as is, especially if changing it may introduce more conflicts with other PRs. #Resolved

I've replaced this code in #44882.

Reverted to avoid a conflict

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

For instance SynthesizedRecordObjEquals still has _position = int.MaxValue - 4 while there is no symbol with position int.MaxValue - 3. I doubt this negatively affects anything so perhaps we can just leave it as is, especially if changing it may introduce more conflicts with other PRs.

I reverted the change in favor of in #44882.

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

RikkiGibson · 2020-06-05T17:37:03Z

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

+
+            var comp = CreateCompilation(src);
+            comp.VerifyEmitDiagnostics(
+                // (13,16): error CS8861: Unexpected argument list.


Ideally this diagnostic would say something like "only a record with positional parameters may have a constructor initializer." We can follow up in #44826 #WontFix

Ideally this diagnostic would say something like "only a record with positional parameters may have a constructor initializer." We can follow up in #44826

This is preexisting condition.

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

RikkiGibson · 2020-06-05T17:43:27Z

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

+                // record C(dynamic X) : Base(X)
+                Diagnostic(ErrorCode.ERR_NoDynamicPhantomOnBaseCtor, "(X)").WithLocation(11, 27)
+                );
+        }


Do we have a test which demonstrates whether a partial record is allowed to reiterate its base class without arguments, e.g.:

record A(int X) { } partial record B(int X, int Y) : A(X) { } partial record B : A { }

#Resolved

Do we have a test which demonstrates whether a partial record is allowed to reiterate its base class without arguments, e.g.:

I do not know. I think this is outside of scope of this PR, feel free to add a note to the Test Plan.

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

RikkiGibson · 2020-06-05T19:11:56Z

Aleksey has suggested that if the PR receives a second sign-off that we can squash merge the PR even if he is offline at the time. #Resolved

jcouv · 2020-06-05T20:54:54Z

Looking now #Resolved

jcouv · 2020-06-05T21:28:31Z

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

@@ -390,6 +393,16 @@ public override void VisitConstructorDeclaration(ConstructorDeclarationSyntax no
            }
        }

+        public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
+        {
+            Debug.Assert(((RecordDeclarationSyntax)node).ParameterList is object);


RecordDeclarationSyntax [](start = 27, length = 23)

nit: redundant cast #Resolved

nit: redundant cast

Removed in #44906

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

jcouv · 2020-06-05T21:35:37Z

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

@@ -3864,7 +3864,7 @@ private static bool IsNegativeConstantForArraySize(BoundExpression expression)
                NamedTypeSymbol initializerType = containingType;

                bool isBaseConstructorInitializer = initializerArgumentListOpt == null ||
-                                                    initializerArgumentListOpt.Parent.Kind() == SyntaxKind.BaseConstructorInitializer;
+                                                    initializerArgumentListOpt.Parent.Kind() != SyntaxKind.ThisConstructorInitializer;


nit: wouldn't it be clearer to keep the check as "parent is BaseConstructorInitializer OR parent is SimpleBaseTypeSyntax"? #Resolved

nit: wouldn't it be clearer to keep the check as "parent is BaseConstructorInitializer OR parent is SimpleBaseTypeSyntax"?

I think it is clearer this way, i.e. either we are dealing with "this" initializer syntax, or it is a base initializer swmantically.

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

jcouv

LGTM Thanks (iteration 3)

jcouv · 2020-06-05T21:55:34Z

@AlekseyTs I'm testing on top of this PR, and encountered an unexpected error:

Can you add a test like this? #Resolved

agocke

LGTM. Test suggestion that can be added later.

agocke · 2020-06-05T22:07:31Z

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

+    public Base() {}
+}
+
+partial record C(int X, int Y) : Base(X, Y)


I assume the following is legal:

record Base(int X); partial record C(int X) : Base(X); partial record C { }

Could you write a test that shows it succeeds/emits correctly? #Resolved

I assume the following is legal:

Yes, this works, adding test in the next PR

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

adding test in the next PR

Added in #44906

In reply to: 436197211 [](ancestors = 436197211,436186239)

jcouv · 2020-06-05T22:13:46Z

Confirmed the problem with a unittest:

        [Fact]
        public void TODO()
        {
            var src = @"

public record Human(int age) { }
public record Person(string First, string Last) : Human(0) { }
";

            var comp = CreateCompilation(src);
            comp.VerifyDiagnostics(
                // (4,15): error CS1729: 'Human' does not contain a constructor that takes 0 arguments
                // public record Person(string First, string Last) : Human(0) 
                Diagnostic(ErrorCode.ERR_BadCtorArgCount, "Person").WithArguments("Human", "0").WithLocation(4, 15)
                );
        }
``` #Resolved

AlekseyTs · 2020-06-05T22:28:51Z

I'm testing on top of this PR, and encountered an unexpected error:

The error is coming from generation of the copy constructor, it is not related to arguments

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

AlekseyTs · 2020-06-05T22:37:03Z

See

    .method public hidebysig specialname rtspecialname 
        instance void .ctor (
            class Person ''
        ) cil managed 
    {
        // Method begins at RVA 0x214d
        // Code size 32 (0x20)
        .maxstack 8

        IL_0000: ldarg.0
        IL_0001: call instance void Human::.ctor()
        IL_0006: nop
        IL_0007: ldarg.0
        IL_0008: ldarg.1
        IL_0009: ldfld string Person::'<First>k__BackingField'
        IL_000e: stfld string Person::'<First>k__BackingField'
        IL_0013: ldarg.0
        IL_0014: ldarg.1
        IL_0015: ldfld string Person::'<Last>k__BackingField'
        IL_001a: stfld string Person::'<Last>k__BackingField'
        IL_001f: ret
    } // end of method Person::.ctor

in https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLRIMYHsEBNkA0MIAlgDYG4gDUAPpjrgAQAScAtlAHYAUJXMJiQCUAWABQAbwlNZTAAIBmVh249hTEExgALEsh6oAjMMkBfCRfESGeJgAUICZFl7yjABiYAxEs5gECp5MADJQyDAaWmycvB7CANwSElxQ7CgADlAYEEEAdABKcAIk6XkAwljsGeROAMpOAG4kOcgS0uJyCsrI0GQQzPIATEwAksgAogAeME6pZKNcJDAych1dVlZAA

In reply to: 639867910 [](ancestors = 639867910,639844282)

AlekseyTs · 2020-06-05T22:37:49Z

Confirmed the problem with a unittest:

Responded above, doesn't look related to the primary constructor

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

jcouv

LGTM Thanks (iteration 3)
From discussion with Aleksey, I'll file an issue with copy constructor (it should probably leverage copy constructor from the base, instead of no-args constructor).

Bind base arguments fo records

db6d625

AlekseyTs added Area-Compilers New Language Feature - Records Records labels Jun 5, 2020

AlekseyTs requested review from agocke, RikkiGibson, cston, jcouv and a team June 5, 2020 02:49

gafter reviewed Jun 5, 2020

View reviewed changes

AlekseyTs mentioned this pull request Jun 5, 2020

Change name of record Clone method to unspeakable #44852

Merged

Added tests

88ec958

RikkiGibson approved these changes Jun 5, 2020

View reviewed changes

Undo some chnages to avoid conflicts with a different PR

730c232

jcouv mentioned this pull request Jun 5, 2020

Test plan for "records" #40726

Closed

jcouv self-assigned this Jun 5, 2020

jcouv reviewed Jun 5, 2020

View reviewed changes

jcouv approved these changes Jun 5, 2020

View reviewed changes

jcouv self-requested a review June 5, 2020 21:53

agocke approved these changes Jun 5, 2020

View reviewed changes

jcouv approved these changes Jun 5, 2020

View reviewed changes

AlekseyTs merged commit 062cfa5 into dotnet:features/records Jun 5, 2020

Bind base arguments fo records #44876

Bind base arguments fo records #44876

Conversation

AlekseyTs commented Jun 5, 2020 • edited Loading

AlekseyTs commented Jun 5, 2020 • edited Loading

AlekseyTs commented Jun 5, 2020 • edited Loading

RikkiGibson commented Jun 5, 2020 • edited by AlekseyTs Loading

gafter Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Jun 5, 2020 • edited by AlekseyTs Loading

jcouv commented Jun 5, 2020 • edited by AlekseyTs Loading

jcouv Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Jun 5, 2020 • edited by AlekseyTs Loading

agocke left a comment

Choose a reason for hiding this comment

agocke Jun 5, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 5, 2020 • edited by AlekseyTs Loading

AlekseyTs commented Jun 5, 2020

AlekseyTs commented Jun 5, 2020

AlekseyTs commented Jun 5, 2020

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 5, 2020 •

edited

Loading

AlekseyTs commented Jun 5, 2020 •

edited

Loading

AlekseyTs commented Jun 5, 2020 •

edited

Loading

RikkiGibson commented Jun 5, 2020 •

edited by AlekseyTs

Loading

gafter Jun 5, 2020 •

edited by AlekseyTs

Loading

gafter Jun 5, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jun 5, 2020 •

edited by AlekseyTs

Loading

cston Jun 5, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jun 5, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jun 5, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jun 5, 2020 •

edited by AlekseyTs

Loading

RikkiGibson commented Jun 5, 2020 •

edited by AlekseyTs

Loading

jcouv commented Jun 5, 2020 •

edited by AlekseyTs

Loading

jcouv Jun 5, 2020 •

edited by AlekseyTs

Loading

jcouv Jun 5, 2020 •

edited by AlekseyTs

Loading

jcouv commented Jun 5, 2020 •

edited by AlekseyTs

Loading

agocke Jun 5, 2020 •

edited by AlekseyTs

Loading

jcouv commented Jun 5, 2020 •

edited by AlekseyTs

Loading