Implement IEquatable<T> in record types #44994

cston · 2020-06-09T19:17:00Z

RikkiGibson · 2020-06-09T20:20:42Z

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

+            if (declaration.Kind == DeclarationKind.Record)
+            {
+                var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T);
+                baseInterfaces.Add(type.Construct(this));


Does this cause us to implement the interface "non-nullably?" Should we instead implement the interface with a nullable type argument, e.g. record Rec : IEquatable<Rec?>

I do not see mention of IEquatable in the records spec.

The parameter to IEquatable<T>.Equals() is [AllowNull] T other.

I do not see mention of IEquatable in the records spec.

Good catch, thanks. I've updated dotnet/csharplang#3552.

The parameter to IEquatable.Equals() is [AllowNull] T other.

So it is, thank you! It might be good to add tests that demonstrate the synthesized "strongly typed Equals" methods accept null arguments without warnings or runtime errors. Since this test gap is not related to adding IEquatable implementation, will not block the PR over it.

AlekseyTs · 2020-06-10T02:50:21Z

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

+            if (declaration.Kind == DeclarationKind.Record)
+            {
+                var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T);
+                baseInterfaces.Add(type.Construct(this));


baseInterfaces.Add [](start = 16, length = 18)

What if the interface already specified explicitly? #Closed

AlekseyTs · 2020-06-10T02:51:42Z

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

@@ -319,6 +319,12 @@ internal override ImmutableArray<NamedTypeSymbol> GetDeclaredInterfaces(ConsList
                }
            }

+            if (declaration.Kind == DeclarationKind.Record)
+            {
+                var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T);


DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T); [](start = 27, length = 73)

Please properly handle missing and otherwise bad types. #Closed

Use-site errors are already reported for interfaces. See RecordTests.IEquatableT_09. #Closed

Use-site errors are already reported for interfaces. See RecordTests.IEquatableT_08.

Do we understand where and why that error is reported?

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

SourceNamedTypeSymbol.InterfacesNoUseSiteDiagnostics() adds use-site diagnostics for interfaces to Compilation.DeclarationDiagnostics. #Closed

SourceNamedTypeSymbol.InterfacesNoUseSiteDiagnostics() adds use-site diagnostics for interfaces to Compilation.DeclarationDiagnostics.

I do not see anything there explicitly tasked to check use-site error on the implemented interfaces. Either we should find the place that explicitly fulfills the task, or we should do that here explicitly. Getting duplicated doesn't bother me at all.

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

AlekseyTs · 2020-06-10T02:55:16Z

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

+            if (declaration.Kind == DeclarationKind.Record)
+            {
+                var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T);
+                baseInterfaces.Add(type.Construct(this));


baseInterfaces.Add [](start = 16, length = 18)

We should make sure we test error scenarios around this interface. Places like that usually try to locate matching syntax in the base list. Obviously, in this case there is no syntax to find. We should make sure compiler doesn't crash. #Closed

I have not found any use of GetBaseListOpt, FindBaseRefSyntax, FirstDeclarationWithExplicitBases that doesn't check the return type in the case of interfaces. #Closed

I have not found any use of GetBaseListOpt, FindBaseRefSyntax, FirstDeclarationWithExplicitBases that doesn't check the return type in the case of interfaces.

It looks like SourceNamedTypeSymbol.GetCorrespondingBaseListLocation can return null in our scenario. Consequently SourceMemberContainerTypeSymbol.GetImplementsLocation can return null. I see a number of call sites where result of calling this API is dereferenced or passed as a location for diagnostics. I suggest that we either confirm that those call sites are not reachable in our scenario, or change them to use one of the two helpers that already handle null result.

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

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

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

+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();
+
+            AssertEx.Equal(new[] { "System.IEquatable<A<T>>" }, comp.GetMember<NamedTypeSymbol>("A").AllInterfacesNoUseSiteDiagnostics.ToTestDisplayStrings());


AllInterfacesNoUseSiteDiagnostics [](start = 101, length = 33)

What do we return from Interfaces API?
#Closed

AlekseyTs · 2020-06-10T03:01:38Z

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

+    public virtual bool Equals(B other) => false;
+}";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();


comp.VerifyDiagnostics(); [](start = 12, length = 25)

Please run the code and observe expected methods called. Otherwise, how do we know that right methods are going to be hooked up? #Closed

AlekseyTs · 2020-06-10T03:02:49Z

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

+record B : A<object>, IEquatable<A<object>>, IEquatable<B>
+{
+    bool IEquatable<A<object>>.Equals(A<object> other) => false;
+    bool IEquatable<B>.Equals(B other) => false;


bool IEquatable.Equals(B other) => false; [](start = 4, length = 44)

Is this going to be a valid code without explicit IEquatable<B> in the base list? #Closed

Logged #45026.

AlekseyTs · 2020-06-10T03:06:27Z

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

+record B : A<object>, IEquatable<A<object>>, IEquatable<B>
+{
+    public override bool Equals(A<object> other) => false;
+    public virtual bool Equals(B other) => false;


public virtual bool Equals(B other) => false; [](start = 4, length = 45)

Please add a test for a declaration like that, but without explicit IEquatable<B> in the base list. #Closed

AlekseyTs · 2020-06-10T03:07:24Z

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

+record B : A<object>, IEquatable<A<object>>, IEquatable<B>
+{
+    public override bool Equals(A<object> other) => false;
+    public virtual bool Equals(B other) => false;


public virtual bool Equals(B other) => false; [](start = 4, length = 45)

Please also add a test for a scenario when this method is

not public

not virtual #Closed

AlekseyTs · 2020-06-10T03:08:59Z

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

+        public void IEquatableT_01()
+        {
+            var source =
+@"record A<T>;


record A [](start = 2, length = 11)

Please add a test for scenario when IEquatable<T> has unexpected API in it. #Closed

IEquatableT_12

AlekseyTs · 2020-06-10T03:11:21Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2020-06-10T16:09:05Z

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

+            if (declaration.Kind == DeclarationKind.Record)
+            {
+                var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T).Construct(this);
+                if (baseInterfaces.IndexOf(type, SymbolEqualityComparer.IgnoringNullable) < 0)


SymbolEqualityComparer.IgnoringNullable [](start = 49, length = 39)

I think we should ignore all aspects here #Closed

… 44920

AlekseyTs · 2020-06-10T17:19:05Z

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

-            var compB = CreateCompilation(new[] { sourceB, IsExternalInitTypeDefinition }, references: new[] { refA }, parseOptions: TestOptions.RegularPreview);
-            compB.VerifyDiagnostics(
-                // (1,32): error CS8864: Records may only inherit from object or another record
-                // record B(object P, object Q) : A


Please add a record with explicit interface specified. Also, please assert Interfaces for all of them #Closed

AlekseyTs · 2020-06-10T20:27:13Z

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

+            if (declaration.Kind == DeclarationKind.Record)
+            {
+                var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T).Construct(this);
+                if (baseInterfaces.IndexOf(type, SymbolEqualityComparer.AllIgnoreOptionsPlusNullableWithUnknownMatchesAny) < 0)


AllIgnoreOptionsPlusNullableWithUnknownMatchesAny [](start = 72, length = 49)

This doesn't ignore all aspects #Closed

AlekseyTs · 2020-06-10T20:34:18Z

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

+    bool IEquatable<R>.Equals(R other) => false;
+}";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();


comp.VerifyDiagnostics(); [](start = 12, length = 25)

We should run and observe expected behavior. And also observe that we are not going to delegate to equality of the derived type when called through IEquatable<R> interface. Not blocking for this PR

AlekseyTs · 2020-06-10T20:39:25Z

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

+record B : A<int>, System.IEquatable<B>;
+";
+            var comp = CreateCompilation(source);
+            comp.MakeTypeMissing(WellKnownType.System_IEquatable_T);


comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); [](start = 12, length = 56)

It appears that we are not testing realistic scenario. I might be wrong, but I believe regular lookup isn't affected by this helper. We want the type be missing for real. #Closed

AlekseyTs · 2020-06-10T20:40:10Z

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

+record B : A<int>, IEquatable<B>;
+";
+            var comp = CreateCompilation(source);
+            comp.MakeTypeMissing(WellKnownType.System_IEquatable_T);


comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); [](start = 12, length = 56)

Same comment here. #Closed

AlekseyTs · 2020-06-10T20:40:37Z

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

+                Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "IEquatable<B>").WithArguments("System.IEquatable`1").WithLocation(3, 20));
+
+            var type = comp.GetMember<NamedTypeSymbol>("A");
+            AssertEx.Equal(new[] { "System.IEquatable<A<T>>", "System.IEquatable<A<T>>[missing]" }, type.InterfacesNoUseSiteDiagnostics().ToTestDisplayStrings());


System [](start = 36, length = 6)

Howe do we know it is from System? #Closed

AlekseyTs · 2020-06-10T20:41:17Z

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

+                Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "System.IEquatable<B>").WithArguments("System.IEquatable`1").WithLocation(2, 20));
+
+            var type = comp.GetMember<NamedTypeSymbol>("A");
+            AssertEx.Equal(new[] { "System.IEquatable<A<T>>", "System.IEquatable<A<T>>[missing]" }, type.InterfacesNoUseSiteDiagnostics().ToTestDisplayStrings());


AssertEx.Equal(new[] { "System.IEquatable<A>", "System.IEquatable<A>[missing]" } [](start = 12, length = 86)

It looks like we have a duplicate type in the list. #Closed

AlekseyTs · 2020-06-10T20:43:01Z

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

-                "System.Type C.EqualityContract { get; }",
-                "System.Object C.R { get; init; }",
-            };
-            AssertEx.Equal(expectedMembers, actualMembers);


public bool Equals(A other) => false; [](start = 4, length = 37)

This is a success case, should run and observe (not blocking for this PR)

AlekseyTs · 2020-06-10T21:09:00Z

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

+            AssertEx.Equal(new[] { "System.IEquatable<A<T>>[missing]" }, type.AllInterfacesNoUseSiteDiagnostics.ToTestDisplayStrings());
+
+            type = comp.GetMember<NamedTypeSymbol>("B");
+            AssertEx.Equal(new[] { "System.IEquatable<B>", "System.IEquatable<B>[missing]" }, type.InterfacesNoUseSiteDiagnostics().ToTestDisplayStrings());


System.IEquatable", "System.IEquatable[missing]" [](start = 36, length = 54)

It looks like we have a duplicate type in the list. This is not blocking for this PR

AlekseyTs · 2020-06-10T21:14:29Z

Done with review pass (iteration 8) #Closed

AlekseyTs · 2020-06-10T21:41:37Z

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

+        }
+
+        [Fact]
+        public void IEquatableT_16()


IEquatableT_16 [](start = 20, length = 14)

Did this scenario crash before the last commit? #Closed

No, because there is an explicit interface that is used as the fallback location.

AlekseyTs

LGTM (iteration 9), assuming CI is passing

Implement IEquatable<T> in record types

ec52f37

cston requested a review from a team as a code owner June 9, 2020 19:17

cston added Area-Compilers New Language Feature - Records Records labels Jun 9, 2020

cston added this to the 16.7 milestone Jun 9, 2020

RikkiGibson reviewed Jun 9, 2020

View reviewed changes

RikkiGibson approved these changes Jun 9, 2020

View reviewed changes

cston requested a review from a team June 9, 2020 23:44

AlekseyTs reviewed Jun 10, 2020

View reviewed changes

PR feedback

f4f8714

AlekseyTs reviewed Jun 10, 2020

View reviewed changes

cston added 3 commits June 10, 2020 09:49

Ignore all differences

7c26785

Merge remote-tracking branch 'upstream/release/dev16.7-preview3' into…

d337c88

… 44920

Report use-site diagnostics explicitly

d246908

AlekseyTs reviewed Jun 10, 2020

View reviewed changes

cston added 2 commits June 10, 2020 10:19

Remove comment

77b87ee

More missing type tests

13e4e80

AlekseyTs reviewed Jun 10, 2020

View reviewed changes

PR feedback

dbd01fc

AlekseyTs reviewed Jun 10, 2020

View reviewed changes

PR feedback

6ce1611

AlekseyTs reviewed Jun 10, 2020

View reviewed changes

AlekseyTs approved these changes Jun 10, 2020

View reviewed changes

cston merged commit 12fbed0 into dotnet:release/dev16.7-preview3 Jun 11, 2020

cston deleted the 44920 branch June 11, 2020 01:17

cston mentioned this pull request Jun 11, 2020

Record types should implement IEquatable<T> #44920

Closed

Implement IEquatable<T> in record types #44994

Implement IEquatable<T> in record types #44994

Conversation

cston commented Jun 9, 2020

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

RikkiGibson Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

cston Jun 10, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 10, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

cston Jun 10, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 10, 2020 • edited Loading

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 10, 2020 • edited Loading

AlekseyTs Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

cston Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

RikkiGibson Jun 9, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

cston Jun 10, 2020 •

edited by AlekseyTs

Loading

cston Jun 10, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

cston Jun 10, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs commented Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

AlekseyTs commented Jun 10, 2020 •

edited

Loading

AlekseyTs Jun 10, 2020 •

edited

Loading

cston Jun 10, 2020 •

edited

Loading