Record-structs: Add equality members #51900

jcouv · 2021-03-16T04:36:26Z

Add IEquatable<T> interface, Equals and GetHashCode methods and equality/inequality operators.

Implements this section of the spec (copied below for convenience).

Test plan: #51199

Equality members

The synthesized equality members are similar as in a record class (Equals for this type, Equals for object type, == and != operators for this type),
except for the lack of EqualityContract, null checks or inheritance.

The record struct implements System.IEquatable<R> and includes a synthesized strongly-typed overload of Equals(R other) > where R is the record struct.
The method is public.
The method can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility.

If Equals(R other) is user-defined (not synthesized) but GetHashCode is not, a warning is produced.
public bool Equals(R other);
The synthesized Equals(R) returns true if and only if for each instance field fieldN in the record struct
the value of System.Collections.Generic.EqualityComparer<TN>.Default.Equals(fieldN, other.fieldN) where TN is the field type is true.

The record struct includes synthesized == and != operators equivalent to operators declared as follows:
public static bool operator==(R r1, R r2)
    => r1.Equals(r2);
public static bool operator!=(R r1, R r2)
    => !(r1 == r2);
The Equals method called by the == operator is the Equals(R other) method specified above. The != operator delegates to > the == operator. It is an error if the operators are declared explicitly.

The record struct includes a synthesized override equivalent to a method declared as follows:
public override bool Equals(object? obj);
It is an error if the override is declared explicitly.
The synthesized override returns other is R temp && Equals(temp) where R is the record struct.

The record struct includes a synthesized override equivalent to a method declared as follows:
public override int GetHashCode();
The method can be declared explicitly.

A warning is reported if one of Equals(R) and GetHashCode() is explicitly declared but the other method is not explicit.

The synthesized override of GetHashCode() returns an int result of combining the values of System.Collections.Generic.EqualityComparer<TN>.Default.GetHashCode(fieldN) for each instance field fieldN with TN being > the type of fieldN.

For example, consider the following record struct:
record struct R1(T1 P1, T2 P2);
For this record struct, the synthesized equality members would be something like:
[...]

jcouv · 2021-03-18T14:38:03Z

@dotnet/roslyn-compiler for review. Thanks

AlekseyTs · 2021-03-18T16:42:33Z

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

@@ -3934,7 +3945,8 @@ MethodSymbol addThisEquals(PropertySymbol equalityContract)

            void reportStaticOrNotOverridableAPIInRecord(Symbol symbol, BindingDiagnosticBag diagnostics)
            {
-                if (!IsSealed &&
+                if (symbol.ContainingType.IsReferenceType &&


symbol.ContainingType.IsReferenceType [](start = 20, length = 37)

isRecordClass? #Closed

AlekseyTs · 2021-03-18T16:48:43Z

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

-                BoundExpression objectEqual = F.ObjectEqual(r1, r2);
-                BoundExpression recordEquals = F.LogicalAnd(F.ObjectNotEqual(r1, F.Null(F.SpecialType(SpecialType.System_Object))),
+                BoundExpression expression;
+                if (ContainingType.IsReferenceType)


ContainingType.IsReferenceType [](start = 20, length = 30)

I think we should use TypeKind or the special property instead. #Closed

AlekseyTs · 2021-03-18T16:51:41Z

...mpilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityOperatorBase.cs

@@ -57,10 +64,10 @@ protected sealed override (TypeWithAnnotations ReturnType, ImmutableArray<Parame
            return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Boolean, location, diagnostics)),
                    Parameters: ImmutableArray.Create<ParameterSymbol>(
                                    new SourceSimpleParameterSymbol(owner: this,
-                                                                    TypeWithAnnotations.Create(ContainingType, NullableAnnotation.Annotated),
+                                                                    TypeWithAnnotations.Create(ContainingType, ContainingType.IsReferenceType ? NullableAnnotation.Annotated : NullableAnnotation.Oblivious),


ContainingType.IsReferenceType [](start = 111, length = 30)

I think we should use TypeKind or the special property instead. #Closed

AlekseyTs · 2021-03-18T16:51:56Z

...mpilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityOperatorBase.cs

                                                                    ordinal: 0, RefKind.None, "r1", isDiscard: false, Locations),
                                    new SourceSimpleParameterSymbol(owner: this,
-                                                                    TypeWithAnnotations.Create(ContainingType, NullableAnnotation.Annotated),
+                                                                    TypeWithAnnotations.Create(ContainingType, ContainingType.IsReferenceType ? NullableAnnotation.Annotated : NullableAnnotation.Oblivious),


ContainingType.IsReferenceType [](start = 111, length = 30)

I think we should use TypeKind or the special property instead. #Closed

AlekseyTs · 2021-03-18T16:57:25Z

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

            : base(containingType, WellKnownMemberNames.ObjectEquals, hasBody: true, memberOffset, diagnostics)
        {
+            Debug.Assert(equalityContract is null == containingType.IsStructType());


containingType.IsStructType() [](start = 53, length = 29)

I think we should be consistent in the way we check for record class vs. record struct. It feels like relying on a special property would be preferred, that way we will be able to quikly locate all the placess that perform the check with the purpose to distinguish kinds of records. #Closed

I'm planning to merge IsRecord and IsRecordStruct. So I'll align on TypeKind check for class vs. struct.

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

We could keep an IsRecordStruct => IsRecord && TypeKind == Struct, but then the IsRecord check is redundant. Is that what you were thinking?

In reply to: 597866787 [](ancestors = 597866787,597069643)

We could keep an IsRecordStruct => IsRecord && TypeKind == Struct, but then the IsRecord check is redundant. Is that what you were thinking?

I would leave implementation of IsRecordStruct as is, it is correct and efficient.

In reply to: 597888108 [](ancestors = 597888108,597866787,597069643)

AlekseyTs · 2021-03-18T16:58:00Z

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

        {
            var compilation = DeclaringCompilation;
            var location = ReturnTypeLocation;
            return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Boolean, location, diagnostics)),
                    Parameters: ImmutableArray.Create<ParameterSymbol>(
                                    new SourceSimpleParameterSymbol(owner: this,
-                                                                    TypeWithAnnotations.Create(ContainingType, NullableAnnotation.Annotated),
+                                                                    TypeWithAnnotations.Create(ContainingType, ContainingType.IsReferenceType ? NullableAnnotation.Annotated : NullableAnnotation.Oblivious),


ContainingType.IsReferenceType [](start = 111, length = 30)

I think we should use TypeKind or the special property instead. #Closed

AlekseyTs · 2021-03-18T16:59:01Z

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

@@ -62,8 +60,17 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
                // This method is the strongly-typed Equals method where the parameter type is
                // the containing type.

-                if (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType())
+                bool isRecordStruct = ContainingType.IsStructType();


ContainingType.IsStructType() [](start = 38, length = 29)

Same comment as for the constructor. #Closed

AlekseyTs · 2021-03-18T17:00:26Z

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

                {
+                    // We'll produce:
+                    // virtual bool Equals(T other) =>


virtual [](start = 23, length = 7)

Why would we produce a virtual method in a struct? #Closed

AlekseyTs · 2021-03-18T17:04:37Z

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

+                    // We'll produce:
+                    // virtual bool Equals(T other) =>
+                    //     field1 == other.field1 && ... && fieldN == other.fieldN;
+                    retExpr = F.Literal(true);


F.Literal(true); [](start = 30, length = 16)

This doesn't look usefull. I think we can use null value instead and avoid generating unnecessary nodes here and below. #Closed

We need a true one way or another for the case with no fields. I'll move it to only that case

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

AlekseyTs · 2021-03-18T17:06:17Z

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

@@ -155,7 +163,10 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
                }

                fields.Free();
-                retExpr = F.LogicalOr(F.ObjectEqual(F.This(), other), retExpr);
+                if (!isRecordStruct)


if (!isRecordStruct) [](start = 16, length = 20)

Consider adding empty lines around this if #Closed

AlekseyTs · 2021-03-18T17:08:08Z

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

            : base(containingType, WellKnownMemberNames.ObjectGetHashCode, memberOffset, diagnostics)
        {
+            Debug.Assert(containingType.IsReferenceType == equalityContract is not null);


containingType.IsReferenceType [](start = 25, length = 30)

There is a lack of consistency in terms of how we distinguish kinds of records. #Closed

AlekseyTs · 2021-03-18T17:08:39Z

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


-                if (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType())
+                if (ContainingType.IsStructType())


ContainingType.IsStructType() [](start = 20, length = 29)

There is a lack of consistency in terms of how we distinguish kinds of records. #Closed

AlekseyTs · 2021-03-18T17:16:38Z

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

                {
+                    currentHashValue = null;


currentHashValue [](start = 20, length = 16)

Should we add hash code for the type in order to add variance across different types of record structs? #Closed

I'm not sure I follow. You mean if we did struct inheritance?

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

I'm not sure I follow. You mean if we did struct inheritance?

No, inheritance isn't part of this. This is about instances of record struct S1; and instances of record struct S2; having distict hash codes. The same way we do this for record classes.

In reply to: 597882249 [](ancestors = 597882249,597084774)

Current spec doesn't include that.
I'll add an open question: dotnet/csharplang#4564

In reply to: 597887909 [](ancestors = 597887909,597882249,597084774)

AlekseyTs · 2021-03-18T17:17:51Z

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

        {
            var compilation = DeclaringCompilation;
            var location = ReturnTypeLocation;
            return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Boolean, location, diagnostics)),
                    Parameters: ImmutableArray.Create<ParameterSymbol>(
                                    new SourceSimpleParameterSymbol(owner: this,
-                                                                    TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Object, location, diagnostics), NullableAnnotation.Annotated),
+                                                                    TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Object, location, diagnostics),
+                                                                        ContainingType.IsReferenceType ? NullableAnnotation.Annotated : NullableAnnotation.Oblivious),


ContainingType.IsReferenceType [](start = 72, length = 30)

There is a lack of consistency in terms of how we distinguish kinds of records. #Closed

AlekseyTs · 2021-03-18T17:18:52Z

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

+                    F.CloseMethod(F.ThrowNull());
+                    return;
+                }
+
                var paramAccess = F.Parameter(Parameters[0]);

                BoundExpression expression;
                if (ContainingType.IsStructType())


ContainingType.IsStructType() [](start = 20, length = 29)

There is a lack of consistency in terms of how we distinguish kinds of records. #Closed

AlekseyTs · 2021-03-18T17:20:25Z

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

                var paramAccess = F.Parameter(Parameters[0]);

                BoundExpression expression;
                if (ContainingType.IsStructType())
                {
-                    throw ExceptionUtilities.Unreachable;
+                    // For record structs:
+                    //      return other is R && Equals((R)other)


other is R && Equals((R)other) [](start = 35, length = 30)

The spec suggests a different code - other is R temp && Equals(temp) #Closed

The underlying IL is probably going to be equivalent though.

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

Yes. We don't need to store the temp. I can update the spec if you feel the distinction is meaningful.

In reply to: 597090857 [](ancestors = 597090857,597087812)

AlekseyTs · 2021-03-18T17:48:26Z

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

+        [Fact]
+        public void RecordEquals_01()
+        {
+            // PROTOTYPE(record-structs): ported


// PROTOTYPE(record-structs): ported [](start = 12, length = 36)

Is this comment meaningful? #Closed

AlekseyTs · 2021-03-18T17:53:08Z

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

+  IL_0001:  ret
+}");
+
+            var recordEquals = comp.GetMembers("A.Equals").OfType<SynthesizedRecordEquals>().Single();


Single [](start = 93, length = 6)

It looks like there are two Equals methods in the type. We should check the other one too, I think. #Closed

AlekseyTs · 2021-03-18T17:55:09Z

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

+                Diagnostic(ErrorCode.WRN_NoRuntimeMetadataVersion).WithLocation(1, 1)
+                );
+
+            var recordEquals = comp.GetMembers("A.Equals").OfType<SynthesizedRecordEquals>().Single();


Single [](start = 93, length = 6)

I am confused, why Single succeeds here? #Closed

Because of .OfType<SynthesizedRecordEquals>(). There is only one "Equals(R). The other one is an Equals(object)which isSynthesizedRecordObjEquals`.

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

AlekseyTs · 2021-03-18T18:06:26Z

/// The method is `public`, and the method is `virtual` unless the record type is `sealed`.

Do we have tests for modifiers on all new methods we generate in a record struct? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEquals.cs:15 in 38c9ec2. [](commit_id = 38c9ec2, deletion_comment = False)

AlekseyTs · 2021-03-18T18:08:21Z

Done with review pass (commit 2) #Closed

jcouv · 2021-03-19T18:41:51Z

/// The method is `public`, and the method is `virtual` unless the record type is `sealed`.

Added coverage for GetHashCode

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEquals.cs:15 in 38c9ec2. [](commit_id = 38c9ec2, deletion_comment = False)

AlekseyTs

LGTM (commit 3)

jcouv · 2021-03-22T19:30:55Z

@dotnet/roslyn-compiler for second review. Thanks

RikkiGibson · 2021-03-23T17:14:22Z

...mpilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityOperatorBase.cs

@@ -54,13 +61,14 @@ protected sealed override (TypeWithAnnotations ReturnType, ImmutableArray<Parame
        {
            var compilation = DeclaringCompilation;
            var location = ReturnTypeLocation;
+            var annotation = ContainingType.IsRecordStruct ? NullableAnnotation.Oblivious : NullableAnnotation.Annotated;


What's the effect of using Oblivious here vs NotAnnotated?

There's no effect because TypeWithAnnotation already has special case for structs, but it seemed misleading to call the API with NotAnnotated unconditionally.

RikkiGibson · 2021-03-23T17:21:21Z

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

-record struct C(bool X)
+            {
+                var src = @"
+readonly record struct C(bool X)


Were adjacent tests combined here? #Resolved

Yes, they were sharing the corlib source

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

Add RecordEquals method and IEquatable interface

5535d01

jcouv added Area-Compilers New Language Feature - Records Records labels Mar 16, 2021

jcouv self-assigned this Mar 16, 2021

Add ObjectEquals and GetHashCode

38c9ec2

jcouv force-pushed the rs-symbol3 branch from 2c5de66 to 38c9ec2 Compare March 16, 2021 06:21

jcouv added this to the C# 10 milestone Mar 16, 2021

jcouv marked this pull request as ready for review March 16, 2021 15:53

jcouv requested a review from a team as a code owner March 16, 2021 15:53

RikkiGibson self-assigned this Mar 17, 2021

AlekseyTs reviewed Mar 18, 2021

View reviewed changes

Address feedback

1336ca6

jcouv requested a review from RikkiGibson March 19, 2021 21:33

AlekseyTs approved these changes Mar 19, 2021

View reviewed changes

RikkiGibson approved these changes Mar 23, 2021

View reviewed changes

jcouv merged commit 157512d into dotnet:features/record-structs Mar 23, 2021

jcouv deleted the rs-symbol3 branch March 23, 2021 18:11


		if (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType())
		if (ContainingType.IsStructType())

Record-structs: Add equality members #51900

Record-structs: Add equality members #51900

Conversation

jcouv commented Mar 16, 2021

Equality members

jcouv commented Mar 18, 2021

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • 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

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • 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

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 18, 2021 • edited Loading

AlekseyTs commented Mar 18, 2021 • edited Loading

jcouv commented Mar 19, 2021

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 23, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs commented Mar 18, 2021 •

edited

Loading

AlekseyTs commented Mar 18, 2021 •

edited

Loading

RikkiGibson Mar 23, 2021 •

edited by jcouv

Loading