Align addition of a synthesized record.Equals(record? other) in records with the latest design. by AlekseyTs · Pull Request #45647 · dotnet/roslyn

AlekseyTs · 2020-07-04T00:33:39Z

Related to #45296.

From specification:
includes a synthesized strongly-typed overload of Equals(R? other) where R is the record type.
The method is public, and the method is virtual unless the record type is sealed.
The method can be declared explicitly.
It is an error if the explicit declaration does not match the expected signature or accessibility, or the explicit declaration is not virtual and the record type is not sealed.

public virtual bool Equals(R? other);

…ds with the latest design. Related to dotnet#45296. From specification: includes a synthesized strongly-typed overload of `Equals(R? other)` where `R` is the record type. The method is `public`, and the method is `virtual` unless the record type is `sealed`. The method can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or the explicit declaration is not `virtual` and the record type is not `sealed`. ```C# public virtual bool Equals(R? other); ```

AlekseyTs · 2020-07-06T17:09:29Z

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

gafter · 2020-07-06T17:40:38Z

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

                            !hidingMember.IsAccessor() &&
-                            (hiddenMember.IsAbstract || hiddenMember.IsVirtual || hiddenMember.IsOverride))
+                            (hiddenMember.IsAbstract || hiddenMember.IsVirtual || hiddenMember.IsOverride) &&
+                            !(hidingMember is SynthesizedRecordEquals))


hidingMember is SynthesizedRecordEquals [](start = 30, length = 39)

We should avoid type tests for particular implementation types as it makes the compiler fragile. Also, as used here, it appears to be crossing abstraction boundaries.

Please add some kind of API that can be used here instead of a type test for the intended semantic meaning. #Resolved

We should avoid type tests for particular implementation types as it makes the compiler fragile. Also, as used here, it appears to be crossing abstraction boundaries.

Please add some kind of API that can be used here instead of a type test for the intended semantic meaning.

In general we prefer doing things like that, and we do make exceptions in some cases where it makes sense. I think this is one of the cases like that, where addition of the new API (that we usually prefer to be abstract) for the sake of this method is not worth pollution of the API space and is not worth the work making sure every possible flavor of a Symbol implements it properly.

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

The design problem we have that causes us to want to implement such a virtual method on every symbol implementation is exactly the same problem we have with code like this, which should (for the same reason) have a case for every symbol implementation type even though only one is different. (How do you ensure every new symbol type is reviewed to see if it should be handled here?)

In cases like this encapsulating the type tests inside an extension method is a way of isolating the components. A method encapsulating a switch expression and a virtual method are complementary techniques (you can use one when design constraints prevent the other).
#Resolved

The design problem we have that causes us to want to implement such a virtual method on every symbol implementation is exactly the same problem we have with code like this, which should (for the same reason) have a case for every symbol implementation type even though only one is different. (How do you ensure every new symbol type is reviewed to see if it should be handled here?)

In cases like this encapsulating the type tests inside an extension method is a way of isolating the components. A method encapsulating a switch expression and a virtual method are complementary techniques (you can use one when design constraints prevent the other).

Sorry, I see no difference, if we have an API we have to make sure it behaves correctly for all possible usage scenarios, where as here we only have to worry about one specific scenario. And, having an extension method (as opposed to having an abstract method) doesn't really solve the problem: "How do you ensure every new symbol type is reviewed to see if it should be handled here?" Because one has to know to look into that method, one has to know it exists. Finally, for every new symbol, the developer is responsible making sure that compiler behaves properly when the symbol is used - testing goes a long way, that is how I figured this method had to be adjusted, there was no magic arrow pointing at it.
I opened an issue #45724 to see if we would want to do something about this later.

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

gafter · 2020-07-06T17:40:54Z

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

                }

-                if (!hidingMemberIsNew && !diagnosticAdded && !hidingMember.IsAccessor() && !hidingMember.IsOperator())
+                if (!hidingMemberIsNew && !(hidingMember is SynthesizedRecordEquals) && !diagnosticAdded && !hidingMember.IsAccessor() && !hidingMember.IsOperator())


hidingMember is SynthesizedRecordEquals [](start = 44, length = 39)

Same here. #Resolved

AlekseyTs · 2020-07-07T00:22:34Z

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

AlekseyTs · 2020-07-07T16:15:31Z

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

RikkiGibson · 2020-07-07T22:51:00Z

Taking a look now. #Resolved

RikkiGibson · 2020-07-07T22:48:41Z

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

+                                                F.Property(F.This(), _equalityContract),
+                                                F.Property(other, _equalityContract));
+
+                    retExpr = retExpr is null ? (BoundExpression)contractsEqual : F.LogicalAnd(retExpr, contractsEqual);


It seems like 'retExpr' will never be null here. (This is not blocking the PR as the code was not changed by this PR.) #Pending

It seems like 'retExpr' will never be null here. (This is not blocking the PR as the code was not changed by this PR.)

I am working on EqualityContract right now and planning making changes in this branch of the enclosing if, I will clean this up as well.

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

RikkiGibson · 2020-07-07T22:49:52Z

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

-                    // This method is an override of a strongly-typed Equals method from a base record type.
-                    // The definition of the method is as follows, and _otherEqualsMethod
-                    // is the method to delegate to (see B.Equals(A), C.Equals(A), C.Equals(B) above):
+                    // https://github.com/dotnet/roslyn/issues/45296 Deal with type mismatch for the EqualityContract


I am not certain what this means, and the linked issue does not clarify. #Resolved

I am not certain what this means, and the linked issue does not clarify.

This is a note to me to pay attention to this aspect in my next PR, which is again going to be related to this issue. It is not meant to stay here any significant amount of time

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

RikkiGibson · 2020-07-07T22:55:06Z

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

                {
-                    // This method is the strongly-typed Equals method where the parameter type is
-                    // the containing type.
+                    MethodSymbol? baseEquals = ContainingType.GetMembersUnordered().OfType<SynthesizedRecordBaseEquals>().Single().OverriddenMethod;


It doesn't seem necessary to enumerate the containing type members. The original approach which passed the symbol as a constructor parameter seems simpler. #Resolved

It doesn't seem necessary to enumerate the containing type members. The original approach which passed the symbol as a constructor parameter seems simpler.

I think that avoiding passing unnecessary information and avoiding wasting memory to store it is preferred.

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

RikkiGibson · 2020-07-07T23:19:04Z

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

+@"
+record A
+{
+    public virtual Boolean Equals(A other)


Is it intentional that this is testing the behavior when the return type of Equals is an error type? If so, consider using a more obvious error type name such as ERROR. #WontFix

RikkiGibson · 2020-07-07T23:24:21Z

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

 }";
            var comp = CreateCompilation(source);
            comp.VerifyDiagnostics(
+                // (3,17): error CS8872: 'A.Equals(A)' disallows overriding and containing 'record' is not sealed.


Consider using diagnostic language like: 'A.Equals(A)' must allow overriding because the containing record is not sealed. #Pending

AlekseyTs · 2020-07-08T00:05:38Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off.

AlekseyTs · 2020-07-08T13:40:01Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off.

jcouv · 2020-07-09T00:35:24Z

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

+                    Parameters: ImmutableArray.Create<ParameterSymbol>(
+                                    new SourceSimpleParameterSymbol(owner: this,
+                                                                    TypeWithAnnotations.Create(ContainingType, NullableAnnotation.Annotated),
+                                                                    ordinal: 0, RefKind.None, "", isDiscard: false, Locations)),


"" [](start = 94, length = 2)

nit: Consider naming the parameter. Maybe "other"? It might show up in decompilation and MetadataAsSource scenarios. #Closed

#45621 #Resolved

nit: Consider naming the parameter. Maybe "other"? It might show up in decompilation and MetadataAsSource scenarios.

Tracked separately by #45621.

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

jcouv · 2020-07-09T00:40:53Z

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

+                else
                {
-                    var member = equalityContract.ContainingType.GetMembers("Equals").FirstOrDefault(m =>
+                    thisEquals = (MethodSymbol)existingEqualsMethod;


Do we care which type the existing Equals(R?) method might be defined on? (update: I see this is covered in RecordEquals_02)

public record Base { public bool Equals(R? ...) { ...} } public record R : Base... { // will use `Base.Equals(R?)` }

Do we care about nullability differences? ie. what if I have a bool Equals(R! ...) defined in source? #Closed

Spec doesn't say anything about that, I am interpreting this as - we don't. Feel free to bring this question to LDM.

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

Added an LDM question to test plan

In reply to: 451935125 [](ancestors = 451935125,451900200)

jcouv · 2020-07-09T00:40:59Z

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

+                    {
+                        // There is a signature mismatch, an error was reported elsewhere
+                        F.CloseMethod(F.ThrowNull());
+                        return;


Would it be better if we avoided generating a method body in the error case? #Closed

Would it be better if we avoided generating a method body in the error case?

This is an error scenario. It is an established patter to CloseMethod like this when we are unable to produce something meaningful. I am following it.

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

jcouv · 2020-07-09T00:41:05Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>Constant value may overflow at runtime (use 'unchecked' syntax to override)</value>
  </data>
+  <data name="ERR_NotOverridableAPIInRecord" xml:space="preserve">
+    <value>'{0}' disallows overriding and containing 'record' is not sealed.</value>


'record' [](start = 53, length = 8)

nit: Feels like we don't need the quotes on record #Pending

nit: Feels like we don't need the quotes on record

Will Adjust in the next PR

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

jcouv · 2020-07-09T00:55:25Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

 }
 ");
            comp.VerifyDiagnostics(
+                // (4,17): error CS8872: 'C.Equals(C)' disallows overriding and containing 'record' is not sealed.


nit: the wording might be more helpful as "C.Equals(C) must allow overriding because the containing record is not sealed" #Pending

nit: the wording might be more helpful as "C.Equals(C) must allow overriding because the containing record is not sealed"

Rikki already suggested that and I already incorporated this change in the next PR

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

jcouv · 2020-07-09T00:57:50Z

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

-                "System.Object C.Q { get; }",
-            };
-            AssertEx.Equal(expectedMembers, actualMembers);
+        System.Console.WriteLine(a1.Equals(a2));


It may not be the core focus on this PR, but would it be worthwhile exercising A.Equals(A) too? #Closed

It may not be the core focus on this PR, but would it be worthwhile exercising A.Equals(A) too?

I assume scenario like that has been already covered and, as you mentioned, this is not related to the goal of this PR.

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

jcouv · 2020-07-09T01:13:07Z

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

-                        retExpr = retExpr is null ? (BoundExpression)contractsEqual : F.LogicalAnd(retExpr, contractsEqual);
+                        // There was a problem with overriding of base equals, an error was reported elsewhere
+                        F.CloseMethod(F.ThrowNull());
+                        return;


Same comment here. Can we avoid generating in such error scenarios? #Closed

Same comment here. Can we avoid generating in such error scenarios?

Same response. It might or might not work. I'd rather not spend time on this experiment, unless you see specific value in that.

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

jcouv

Done with review pass (iteration 1)

jcouv

LGTM Thanks (iteration 1)

AlekseyTs added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 4, 2020

AlekseyTs requested review from a team, RikkiGibson, cston and jcouv July 6, 2020 03:08

AlekseyTs added Area-Compilers Feature - Records Records and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jul 6, 2020

AlekseyTs marked this pull request as ready for review July 6, 2020 03:09

gafter reviewed Jul 6, 2020

View reviewed changes

AlekseyTs mentioned this pull request Jul 7, 2020

Re-evaluate if there would be a real benefit from encapsulation of type tests in SourceMemberContainerSymbol.CheckNonOverrideMember into a helper API #45724

Closed

RikkiGibson approved these changes Jul 7, 2020

View reviewed changes

jcouv reviewed Jul 9, 2020

View reviewed changes

jcouv approved these changes Jul 9, 2020

View reviewed changes

jcouv self-assigned this Jul 9, 2020

AlekseyTs merged commit 00f7f0c into dotnet:master Jul 9, 2020

ghost added this to the Next milestone Jul 9, 2020

JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020

Conversation

AlekseyTs commented Jul 4, 2020

Uh oh!

AlekseyTs commented Jul 6, 2020

Uh oh!

gafter Jul 6, 2020 • edited by AlekseyTs 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!

gafter Jul 6, 2020 • edited by AlekseyTs 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!

gafter Jul 6, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 7, 2020

Uh oh!

AlekseyTs commented Jul 7, 2020

Uh oh!

RikkiGibson commented Jul 7, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson Jul 7, 2020 • edited by AlekseyTs 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 Jul 7, 2020 • edited by AlekseyTs 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 Jul 7, 2020 • edited by AlekseyTs 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 Jul 7, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 7, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 8, 2020

Uh oh!

AlekseyTs commented Jul 8, 2020

Uh oh!

jcouv Jul 9, 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!

RikkiGibson Jul 9, 2020 • edited by AlekseyTs 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!

jcouv Jul 9, 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 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!

gafter Jul 6, 2020 •

edited by AlekseyTs

Loading

gafter Jul 6, 2020 •

edited by AlekseyTs

Loading

gafter Jul 6, 2020 •

edited by AlekseyTs

Loading

RikkiGibson commented Jul 7, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jul 7, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jul 7, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jul 7, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jul 7, 2020 •

edited by AlekseyTs

Loading

RikkiGibson Jul 7, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited

Loading

RikkiGibson Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited

Loading

AlekseyTs Jul 9, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited

Loading