Records: Support EqualityContract in Equals by cston · Pull Request #44882 · dotnet/roslyn

cston · 2020-06-05T07:39:03Z

RikkiGibson · 2020-06-05T18:36:10Z

Will take a look after lunch.

gafter · 2020-06-05T19:13:15Z

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

+    /// A strongly-typed `public bool Equals(T other)` method.
+    /// There are two types of strongly-typed Equals methods:
+    /// the strongly-typed virtual method where T is the containing type; and
+    /// overrides of the strongly-typed virtual methods from base record types.


overrides of the strongly-typed virtual methods from base record types [](start = 8, length = 70)

According to the specification at https://github.com/dotnet/csharplang/blob/master/proposals/records.md no such overrides are supposed to be produced.

The spec needs to be updated to handle calls to base.Equals(Base). For instance, the code below should report False False rather than False True.

using System; data class A(int X); data class B(int X, int Y) : A(X); class Program { static void Main() { var b1 = new B(1, 2); var b2 = new B(1, 3); Console.WriteLine(b1.Equals(b2)); Console.WriteLine(((A)b1).Equals(b2)); } }

See dotnet/csharplang#3527 (comment)

gafter · 2020-06-05T19:13:44Z

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

        public override bool IsStatic => false;

-        public override bool IsVirtual => false;
+        public override bool IsVirtual => true;


IsVirtual => true [](start = 29, length = 17)

This disagrees with the specification.

RikkiGibson

LGTM modulo remaining PROTOTYPE comments.

RikkiGibson · 2020-06-05T19:32:24Z

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

+
+            static void getOtherEquals(ArrayBuilder<MethodSymbol> otherEqualsMethods, PropertySymbol equalityContract)
+            {
+                while ((equalityContract = equalityContract.OverriddenProperty) is object)


is there something specific we're trying to do by traversing up the equalityContract.OverriddenProperty chain instead of just up the BaseType chain?

RikkiGibson · 2020-06-05T19:37:41Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs

+
+        public override RefKind RefKind => RefKind.None;
+
+        public override TypeWithAnnotations TypeWithAnnotations { get; }


consider ordering auto properties before manual properties.

RikkiGibson · 2020-06-05T19:55:16Z

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

-                "System.Boolean B.Equals(System.Object? )",
                "System.Int32 B.GetHashCode()",
+                "System.Boolean B.Equals(System.Object? )",
+                "System.Boolean B.Equals(B? )",


I wonder if the IDE experience is negatively affected by these parameters being unnamed? (no need to address in this PR)

RikkiGibson · 2020-06-05T20:08:12Z

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

+  IL_0012:  ldc.i4.0
+  IL_0013:  ret
+}");
+            verifier.VerifyIL("B1.Equals(B1)",


would it be worthwhile to VerifyIL on B2.Equals(B2) or perhaps on B1.Equals(A)? (It looks like other tests have verified this but only in records with no properties.)

agocke · 2020-06-05T22:51:17Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs

+            internal override IEnumerable<SecurityAttribute> GetSecurityInformation()
+                => Array.Empty<SecurityAttribute>();
+
+            internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false;


I think this needs to be true for the first method, and then false for every override

agocke · 2020-06-05T23:03:07Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs

+
+        public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => ImmutableArray<SyntaxReference>.Empty;
+
+        public override Accessibility DeclaredAccessibility => Accessibility.Public;


protected?

agocke · 2020-06-05T23:03:36Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs

+{
+    internal sealed class SynthesizedRecordEqualityContractProperty : PropertySymbol
+    {
+        internal const string PropertyName = "EqualityContract";


May want to put this in WellKnownMemberNames

agocke · 2020-06-05T23:11:53Z

I'm not sure there's a test for overriding EqualityContract across emit reference. Can you validate there is one or add one?

jcouv · 2020-06-06T00:24:07Z

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

            }

-            static bool hidesInheritedMember(Symbol symbol, NamedTypeSymbol type)
+            static Symbol? getInheritedMember(Symbol symbol, NamedTypeSymbol type)


getInheritedMember [](start = 27, length = 18)

What's the benefit of returning a Symbol rather than a bool? It looks like we're only using the value to test for null. #Pending

Hmm, I was using the Symbol at one point, but not now. Reverted. #Pending

jcouv · 2020-06-06T00:26:27Z

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

            addCloneMethod();

-            var thisEquals = addThisEquals();
+            var equalityContract = addEqualityContract();


var [](start = 12, length = 3)

nit: having type (PropertySymbol) would be helpful for readability here

jcouv · 2020-06-06T00:27:42Z

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

+            var otherEqualsMethods = ArrayBuilder<MethodSymbol>.GetInstance();
+            getOtherEquals(otherEqualsMethods, equalityContract);
+
+            var thisEquals = addThisEquals(equalityContract, otherEqualsMethods.Count == 0 ? null : otherEqualsMethods[0]);


, [](start = 59, length = 1)

nit: consider naming the second argument

jcouv · 2020-06-06T00:35:35Z

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

+            {
+                var property = new SynthesizedRecordEqualityContractProperty(this, isOverride: getInheritedEqualityContract(this) is object);
+                // https://github.com/dotnet/roslyn/issues/44903: Check explicit member has expected signature.
+                if (!memberSignatures.ContainsKey(property))


not blocking this PR: should we check both whether the property and the get accessor are already in the list of members? Could we have the get accessor defined without property?

jcouv · 2020-06-06T01:07:17Z

Oops, I didn't realize @agocke was already mostly done with review. I'll let him finish.

jaredpar · 2020-06-06T02:51:26Z

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

+            int memberOffset)
        {
+            var compilation = containingType.DeclaringCompilation;
+            bool isStruct = parameterType.IsStructType();


Given that records are emitted as class when would this be true?

It cannot be true currently. I left the code here since the code existed before this PR but I can remove it.

Looks like we're green now with two sign offs. I'd say go for the merge now and remove this later. Easy to squeeze in as a bug fix later.

Removed in #44912.

jaredpar · 2020-06-06T02:53:00Z

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

+                {
+                    var method = new SynthesizedRecordEquals(
+                        this,
+                        parameterType: otherEqualsMethod.Parameters[0].Type,


Consider the following case

record A<T> { } record B : A<int> { }

Will otherEqualsMethod.Parameters[0].Type be the closed or open generic here?

Great test case, thanks. It looks like it's handled correctly but I'll add a test.

Added in #44912.

agocke

LGTM

RikkiGibson · 2020-06-06T17:54:33Z

It looks like EqualityContract was made protected. This makes me realize that the spec does not specify the accessibility of the synthesized members. Unless I am missing something?

Records: Support EqualityContract in Equals

0068758

cston requested a review from a team as a code owner June 5, 2020 07:39

cston added Area-Compilers Feature - Records Records labels Jun 5, 2020

cston added this to the 16.7.P3 milestone Jun 5, 2020

Fix tests

7974b1b

cston mentioned this pull request Jun 5, 2020

Bind base arguments fo records #44876

Merged

gafter reviewed Jun 5, 2020

View reviewed changes

Address some PROTOTYPE comments

64de377

RikkiGibson approved these changes Jun 5, 2020

View reviewed changes

More PROTOTYPE comments

42f1b09

agocke reviewed Jun 5, 2020

View reviewed changes

cston added 2 commits June 5, 2020 16:25

Remove remaining PROTOTYPE comments added in PR

fa59d0a

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

3bec675

RikkiGibson mentioned this pull request Jun 5, 2020

Synthesize Deconstruct on positional records #44806

Merged

jcouv reviewed Jun 6, 2020

View reviewed changes

cston added 2 commits June 5, 2020 18:03

PR feedback

be62ec1

Fix tests

233988f

jaredpar reviewed Jun 6, 2020

View reviewed changes

agocke approved these changes Jun 6, 2020

View reviewed changes

cston merged commit 9b82fcd into dotnet:features/records Jun 6, 2020

cston deleted the Equality branch June 6, 2020 03:49

cston mentioned this pull request Jun 6, 2020

Misc. changes following recent records PR #44912

Merged


		public override RefKind RefKind => RefKind.None;

		public override TypeWithAnnotations TypeWithAnnotations { get; }


		public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => ImmutableArray<SyntaxReference>.Empty;

		public override Accessibility DeclaredAccessibility => Accessibility.Public;

Conversation

cston commented Jun 5, 2020

Uh oh!

RikkiGibson commented Jun 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented Jun 5, 2020

Uh oh!

jcouv Jun 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 6, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Jun 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cston Jun 5, 2020 •

edited

Loading

jcouv Jun 6, 2020 •

edited

Loading

cston Jun 6, 2020 •

edited by jcouv

Loading

cston Jun 6, 2020 •

edited

Loading