Align addition of a synthesized override of Base.Equals(Base? other) in records with the latest design. #45601
Conversation
…` in records with the latest design. Related to dotnet#45296. From specification: If the record type is derived from a base record type Base, the record type includes a synthesized override of the strongly-typed Equals(Base other). The synthesized override is sealed. It is an error if the override is declared explicitly. The synthesized override returns Equals((object?)other). This chnage also adjusts codegen to properly compare EqualityContracts.
AlekseyTs
changed the title
|
@cston, @jcouv , @RikkiGibson, @dotnet/roslyn-compiler Please review |
|
Taking a look #Resolved |
| return; | ||
| } | ||
|
|
||
| if (overridden is object && |
There was a problem hiding this comment.
This check seems redundant. #Pending
There was a problem hiding this comment.
This check seems redundant.
Yes, the previous if can be removed.
In reply to: 449207981 [](ancestors = 449207981)
| { | ||
| var retExpr = F.Call( | ||
| F.This(), | ||
| ContainingType.GetMembersUnordered().OfType<SynthesizedRecordObjEquals>().Single(), |
There was a problem hiding this comment.
Would it make sense to lookup members by name and then filter down to SynthesizedRecordObjEquals? #Resolved
There was a problem hiding this comment.
Would it make sense to lookup members by name and then filter down to SynthesizedRecordObjEquals?
The code is simpler this way and I don't think it would lead to any noticeable perf difference.
In reply to: 449208838 [](ancestors = 449208838)
|
|
||
| static void ensureEqualityComparerHelpers(SyntheticBoundNodeFactory F, ref MethodSymbol? equalityComparer_GetHashCode, ref MethodSymbol? equalityComparer_get_Default) | ||
| { | ||
| equalityComparer_GetHashCode ??= F.WellKnownMethod(WellKnownMember.System_Collections_Generic_EqualityComparer_T__GetHashCode, isOptional: false)!; |
There was a problem hiding this comment.
Consider using the MethodSymbol WellKnownMethod(WellKnownMember) overload, which passes isOptional: false and suppresses the nullable warning internally. #Pending
| @@ -83,6 +97,12 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState, | |||
| diagnostics.Add(ex.Diagnostic); | |||
| F.CloseMethod(F.ThrowNull()); | |||
| } | |||
|
|
|||
| static void ensureEqualityComparerHelpers(SyntheticBoundNodeFactory F, ref MethodSymbol? equalityComparer_GetHashCode, ref MethodSymbol? equalityComparer_get_Default) | |||
There was a problem hiding this comment.
Consider using [NotNull] on these parameters to avoid the need for ! after calls to this method. #Resolved
There was a problem hiding this comment.
Consider using [NotNull] on these parameters to avoid the need for ! after calls to this method.
This is a local function, I believe we still don't allow attributes on them in our code base.
In reply to: 449217511 [](ancestors = 449217511)
|
|
||
| IL_0000: ldnull | ||
| IL_0001: throw | ||
| } // end of method A::Equals |
There was a problem hiding this comment.
A [](start = 21, length = 1)
B #Pending
| "; | ||
| comp = CreateEmptyCompilation(source1, references: new[] { ref0 }, parseOptions: TestOptions.RegularPreview); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (3,26): error CS8869: 'A.GetHashCode()' does not override expected method from 'object'. |
There was a problem hiding this comment.
error CS8869: 'A.GetHashCode()' does not override expected method from 'object'. [](start = 27, length = 80)
Are we testing the corresponding scenario for Equals() where object.Equals() doesn't have the expected return type? #Pending
There was a problem hiding this comment.
Are we testing the corresponding scenario for Equals() where object.Equals() doesn't have the expected return type?
I don't think we do. They both go through the same helper. I can add one in the next PR.
In reply to: 449291035 [](ancestors = 449291035)
ghost
added this to the 
Related to #45296.
From specification:
If the record type is derived from a base record type Base, the record type includes a synthesized override of the strongly-typed Equals(Base other). The synthesized override is sealed. It is an error if the override is declared explicitly. The synthesized override returns Equals((object?)other).
This change also adjusts code-gen to properly compare EqualityContracts.