Do not override 'Equals(object)' in records. by CyrusNajmabadi · Pull Request #48767 · dotnet/roslyn

CyrusNajmabadi · 2020-10-19T18:33:44Z

depends on #47960 going in first.

Dotnet-GitSync-Bot · 2020-10-19T18:33:50Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

CyrusNajmabadi · 2020-10-23T20:42:59Z

src/Features/Core/Portable/PullMemberUp/MembersPuller.cs

                var overrideMembersSet = new HashSet<ISymbol>();
-                for (var symbol = selectedMember; symbol != null; symbol = symbol.OverriddenMember())
-                {
+                for (var symbol = selectedMember; symbol != null; symbol = symbol.GetOverriddenMember())


we had two methods that did the same thing. i removed one and moved everyone on to the other.

CyrusNajmabadi · 2020-10-23T20:43:37Z

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs

+                // for.  This is true for all implicit overrides *except* for the one for `bool object.Equals(object)`.
+                // This override is not one the user is allowed to provide their own override for as it must have a very
+                // particular implementation to ensure proper record equality semantics.
+                if (!member.IsImplicitlyDeclared || IsEqualsObjectOverride(member))


this is the fix. we specially case things that aren't truly user overridable due to language rules.

The comment here specifies that this is only for records, but I don't see any check for records in the code.

I'm a little confused, is overriding Equals(object) invalid for classes too? (That's my overall impression from the PR, despite the title)

so, the 'if' checks two things:

First, is the member is implicitly declared. If not, then we consider it. For normal classes, the member is not implicitly declared, because you have to actually write out your override.

However, with records the overrides are implict and are automatically provided. The user can override all the implicit ones except the core Equals(object) one.

So that's how this check works :)

I don't think its a big deal, but the comment could make it a bit more obvious that this is for records, where overriding Equals(object) is a compile error. I'm guessing you know better than me how likely it is that there will be a new language construct added where there is an implicitly declared Equals method that the user is allowed to override :)

that's fair :)

allisonchou · 2020-10-23T21:13:58Z

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/OverrideCompletionProviderTests.cs

+        public async Task ObjectEqualsInClass()
+        {
+            await VerifyItemIsAbsentAsync(@"
+class Program 


If overriding bool Equals(object obj) is invalid in the class case, should we indicate this to the user like we do with records (i.e. via red squiggle)?

Equals(object) is fine in the normal case. It's only invalid in the record case. But we detect htat just by looking for an implicit overridde of that member as only records do that.

allisonchou · 2020-10-23T21:39:26Z

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs

+                // for.  This is true for all implicit overrides *except* for the one for `bool object.Equals(object)`.
+                // This override is not one the user is allowed to provide their own override for as it must have a very
+                // particular implementation to ensure proper record equality semantics.
+                if (!member.IsImplicitlyDeclared || IsEqualsObjectOverride(member))


I'm a little confused, is overriding Equals(object) invalid for classes too? (That's my overall impression from the PR, despite the title)

ryzngard · 2020-10-23T20:48:28Z

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/OverrideCompletionProviderTests.cs

            var options = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest);
            var libComp = origComp.RemoveAllSyntaxTrees().AddSyntaxTrees(CSharpSyntaxTree.ParseText(before, options: options));
-            var libRef = MetadataReference.CreateFromImage(CompilationExtensions.EmitToArray(libComp));
+            var libRef = MetadataReference.CreateFromImage(Test.Utilities.CompilationExtensions.EmitToArray(libComp));


This is an extension method right? Why not just libComp.EmitToArray()?

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/OverrideCompletionProviderTests.cs

ryzngard · 2020-10-23T20:53:05Z

src/VisualStudio/Core/Def/Implementation/Library/ObjectBrowser/AbstractListItemFactory.cs

-                            overriddenMembers = new HashSet<ISymbol>();
-                        }
-
+                        overriddenMembers ??= new();


huh... slick, but I can't tell if I like it yet :)

ryzngard · 2020-10-23T20:53:39Z

src/Workspaces/CSharp/Portable/Rename/CSharpRenameRewriterLanguageService.cs

                }

-                if (symbol.IsOverride && symbol.OverriddenMember() != null)
+                if (symbol.IsOverride && symbol.GetOverriddenMember() != null)


In what case is the GetOverriddenMember() null AND IsOverride true?

not sure. likely overly paranoid code. but i could imagine a world where .IsOverride might be true in strange metadata, but compiler couldn't actually return the member that was overridden.

ryzngard · 2020-10-23T20:59:31Z

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs

+                // for.  This is true for all implicit overrides *except* for the one for `bool object.Equals(object)`.
+                // This override is not one the user is allowed to provide their own override for as it must have a very
+                // particular implementation to ensure proper record equality semantics.
+                if (!member.IsImplicitlyDeclared || IsEqualsObjectOverride(member))


The comment here specifies that this is only for records, but I don't see any check for records in the code.

CyrusNajmabadi added 2 commits October 19, 2020 09:51

Merge remote-tracking branch 'upstream/master' into recordOverrides

edf4383

Add test

a6bf13f

CyrusNajmabadi requested review from a team as code owners October 19, 2020 18:33

CyrusNajmabadi requested a review from a team October 19, 2020 18:33

jcouv added Area-IDE Feature - Records Records labels Oct 19, 2020

CyrusNajmabadi added 3 commits October 19, 2020 17:03

Merge remote-tracking branch 'upstream/master' into recordOverrides

3dbe1ec

Fix test

17af36b

Merge remote-tracking branch 'upstream/master' into recordOverrides

3fa78fe

CyrusNajmabadi commented Oct 23, 2020

View reviewed changes

allisonchou reviewed Oct 23, 2020

View reviewed changes

ryzngard reviewed Oct 23, 2020

View reviewed changes

Fixup testS

b7cca14

CyrusNajmabadi requested review from allisonchou, davidwengier and ryzngard October 25, 2020 04:49

davidwengier approved these changes Oct 25, 2020

View reviewed changes

CyrusNajmabadi merged commit b544ef0 into dotnet:master Oct 25, 2020

ghost added this to the Next milestone Oct 25, 2020

CyrusNajmabadi deleted the recordOverrides branch October 25, 2020 21:44

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

Conversation

CyrusNajmabadi commented Oct 19, 2020

Uh oh!

Dotnet-GitSync-Bot commented Oct 19, 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!

allisonchou Oct 23, 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!

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!

allisonchou Oct 23, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

allisonchou Oct 23, 2020 •

edited

Loading

allisonchou Oct 23, 2020 •

edited

Loading