Use inherited properties for records #44705

cston · 2020-05-29T23:02:58Z

Spec: https://github.com/dotnet/csharplang/blob/master/proposals/records.md

cston · 2020-06-01T15:38:57Z

@dotnet/roslyn-compiler please review.

gafter · 2020-06-01T17:00:02Z

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

+    public new int P1 { get; }
+    public new int P2 { get; }
+}
+data class C(object P1, int P2, object P3, int P4) : B


object P3 [](start = 32, length = 9)

~~The spec has changed again. See dotnet/csharplang#3527 ... Now it appears P2 and P3 should be generated. I'll hold off reviewing more until the spec and this PR can be brought into alignment.~~

I'm now not sure about the correct status of this test. The spec says 'Members are synthesized unless an accessible concrete (non-abstract) member with a "matching" signature is
either inherited or declared in the record body.' but the underlying language specification doesn't define "signature" for a property. Either P4 should be synthesized (if signature includes return type) or nothing. #Resolved

The spec currently allows the base member to differ by signature (or member kind) if the record property would hide the base member:

Two members are considered matching if they have the same signature or would be considered "hiding" in an inheritance scenario. #ByDesign

gafter · 2020-06-02T22:50:19Z

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

+{
+}";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics(


VerifyDiagnostics [](start = 17, length = 17)

These diagnostics look incorrect. According to the latest spec at dotnet/csharplang#3527 'Each "matching" inherited abstract accessor is overridden' #Resolved

#44785

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

gafter · 2020-06-02T22:58:45Z

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

-                "System.Object B.P7 { get; init; }",
-            };
-            AssertEx.Equal(expectedMembers, actualMembers);
+            AssertEx.Equal(new string[0], actualMembers);


string [](start = 31, length = 6)

P5 should be synthesized per dotnet/csharplang#3527 #Resolved

P5 is virtual in the base class rather than abstract. #ByDesign

gafter · 2020-06-02T23:02:53Z

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

-                Diagnostic(ErrorCode.ERR_UnimplementedAbstractMethod, "B").WithArguments("B", "A.X.get").WithLocation(6, 12));
+                // (9,12): error CS0534: 'B2' does not implement inherited abstract member 'A.X.get'
+                // data class B2(int X, int Y) : A
+                Diagnostic(ErrorCode.ERR_UnimplementedAbstractMethod, "B2").WithArguments("B2", "A.X.get").WithLocation(9, 12));


ERR_UnimplementedAbstractMethod [](start = 37, length = 31)

Error is incorrect per dotnet/csharplang#3527 #Resolved

Yes, we're not synthesizing overrides for abstract properties currently. That is tracked by #44785. #Resolved

gafter · 2020-06-02T23:14:15Z

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

+    public virtual int Y { get; }
+}";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();


VerifyDiagnostics [](start = 17, length = 17)

This should be an error because C contains both a declared property X and a synthesized one, because, from dotnet/csharplang#3527

'Members are synthesized unless an accessible concrete (non-abstract) member with a "matching" signature is either inherited or declared in the record body.'

I assume you will work with @agocke to resolve the conflict (I expect by modifying the spec).

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

gafter · 2020-06-02T23:36:00Z

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

+@"abstract class A
+{
+    public abstract int X { get; }
+    public virtual int Y { get; }


Please add a test like this (X) but in which the type of the property in A and in C are incompatible. There should be an error that the override has the wrong type.

Or do it when handling #44785

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

Added #44785 (comment)

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

gafter

agocke

LGTM aside from comments

agocke · 2020-06-03T00:43:36Z

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

            void addObjectEquals(MethodSymbol thisEquals)
            {
                var objEquals = new SynthesizedRecordObjEquals(this, thisEquals);
                if (!memberSignatures.ContainsKey(objEquals))
                {
-                    // PROTOTYPE: Don't add if the overridden method is sealed
+                    // https://github.com/dotnet/roslyn/issues/44617: Don't add if the overridden method is sealed


I believe this behavior is correct now. The last LDM confirmed that it is an error if these methods are sealed, which I think is the current behavior.

Thanks. I'll address with the Equals changes in a separate PR.

agocke · 2020-06-03T00:43:54Z

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

@@ -3031,7 +3059,7 @@ void addHashCode()
                var hashCode = new SynthesizedRecordGetHashCode(this);
                if (!memberSignatures.ContainsKey(hashCode))
                {
-                    // PROTOTYPE: Don't add if the overridden method is sealed
+                    // https://github.com/dotnet/roslyn/issues/44617: Don't add if the overridden method is sealed


cston requested a review from a team as a code owner May 29, 2020 23:02

cston added Area-Compilers New Language Feature - Records Records labels May 29, 2020

cston added this to the 16.7.P3 milestone May 29, 2020

gafter self-requested a review May 30, 2020 01:13

Use inherited properties

4c0770a

cston force-pushed the 44616 branch from 7f165d6 to 4c0770a Compare May 30, 2020 03:18

cston added 3 commits May 29, 2020 22:54

Fix tests

7aac024

Misc.

7a6ff04

Handle duplicate properties

f0921cf

gafter reviewed Jun 1, 2020

View reviewed changes

cston added 3 commits June 1, 2020 23:00

Match updated spec

b6f906e

Fix formatting

454320b

Merge remote-tracking branch 'upstream/features/records' into 44616

5c34ef4

cston requested review from agocke and a team June 2, 2020 19:38

Merge remote-tracking branch 'upstream/features/records' into 44616

f8e388b

gafter reviewed Jun 2, 2020

View reviewed changes

cston mentioned this pull request Jun 2, 2020

Record should implement abstract properties #44785

Closed

agocke reviewed Jun 3, 2020

View reviewed changes

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

Misc.

46b87c4

gafter approved these changes Jun 3, 2020

View reviewed changes

agocke approved these changes Jun 3, 2020

View reviewed changes

cston closed this Jun 3, 2020

cston reopened this Jun 3, 2020

cston merged commit 56ce056 into dotnet:features/records Jun 3, 2020

cston deleted the 44616 branch June 3, 2020 13:07

RikkiGibson mentioned this pull request Jun 3, 2020

Change implementation of a SynthesizedRecordConstructor to act more like a constructor declared in source. #44803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use inherited properties for records #44705

Use inherited properties for records #44705

cston commented May 29, 2020 •

edited

Loading

cston commented Jun 1, 2020

gafter Jun 1, 2020 •

edited

Loading

cston Jun 2, 2020 •

edited by gafter

Loading

gafter Jun 2, 2020 •

edited

Loading

gafter Jun 2, 2020

gafter Jun 2, 2020 •

edited

Loading

cston Jun 2, 2020 •

edited by gafter

Loading

gafter Jun 2, 2020 •

edited

Loading

cston Jun 2, 2020 •

edited by gafter

Loading

gafter Jun 2, 2020

gafter Jun 3, 2020

gafter Jun 2, 2020

gafter Jun 2, 2020

cston Jun 2, 2020

gafter left a comment

agocke left a comment •

edited

Loading

agocke Jun 3, 2020

cston Jun 3, 2020

agocke Jun 3, 2020

Use inherited properties for records #44705

Use inherited properties for records #44705

Conversation

cston commented May 29, 2020 • edited Loading

cston commented Jun 1, 2020

gafter Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

cston Jun 2, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

cston Jun 2, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

cston Jun 2, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

agocke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented May 29, 2020 •

edited

Loading

gafter Jun 1, 2020 •

edited

Loading

cston Jun 2, 2020 •

edited by gafter

Loading

gafter Jun 2, 2020 •

edited

Loading

gafter Jun 2, 2020 •

edited

Loading

cston Jun 2, 2020 •

edited by gafter

Loading

gafter Jun 2, 2020 •

edited

Loading

cston Jun 2, 2020 •

edited by gafter

Loading

agocke left a comment •

edited

Loading