-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Records: Support EqualityContract in Equals #44882
Conversation
Will take a look after lunch. |
/// A strongly-typed `public bool Equals(T other)` method. | ||
/// There are two types of strongly-typed Equals methods: | ||
/// the strongly-typed virtual method where T is the containing type; and | ||
/// overrides of the strongly-typed virtual methods from base record types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overrides of the strongly-typed virtual methods from base record types [](start = 8, length = 70)
According to the specification at https://github.com/dotnet/csharplang/blob/master/proposals/records.md no such overrides are supposed to be produced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec needs to be updated to handle calls to base.Equals(Base)
. For instance, the code below should report False False
rather than False True
.
using System;
data class A(int X);
data class B(int X, int Y) : A(X);
class Program
{
static void Main()
{
var b1 = new B(1, 2);
var b2 = new B(1, 3);
Console.WriteLine(b1.Equals(b2));
Console.WriteLine(((A)b1).Equals(b2));
}
}
@@ -85,9 +106,9 @@ public override ImmutableArray<TypeWithAnnotations> TypeArgumentsWithAnnotations | |||
|
|||
public override bool IsStatic => false; | |||
|
|||
public override bool IsVirtual => false; | |||
public override bool IsVirtual => true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsVirtual => true [](start = 29, length = 17)
This disagrees with the specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo remaining PROTOTYPE comments.
|
||
static void getOtherEquals(ArrayBuilder<MethodSymbol> otherEqualsMethods, PropertySymbol equalityContract) | ||
{ | ||
while ((equalityContract = equalityContract.OverriddenProperty) is object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there something specific we're trying to do by traversing up the equalityContract.OverriddenProperty chain instead of just up the BaseType chain?
|
||
public override RefKind RefKind => RefKind.None; | ||
|
||
public override TypeWithAnnotations TypeWithAnnotations { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider ordering auto properties before manual properties.
"System.Int32 B.GetHashCode()", | ||
"System.Boolean B.Equals(System.Object? )", | ||
"System.Boolean B.Equals(B? )", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the IDE experience is negatively affected by these parameters being unnamed? (no need to address in this PR)
IL_0012: ldc.i4.0 | ||
IL_0013: ret | ||
}"); | ||
verifier.VerifyIL("B1.Equals(B1)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be worthwhile to VerifyIL on B2.Equals(B2)
or perhaps on B1.Equals(A)
? (It looks like other tests have verified this but only in records with no properties.)
internal override IEnumerable<SecurityAttribute> GetSecurityInformation() | ||
=> Array.Empty<SecurityAttribute>(); | ||
|
||
internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be true
for the first method, and then false for every override
|
||
public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => ImmutableArray<SyntaxReference>.Empty; | ||
|
||
public override Accessibility DeclaredAccessibility => Accessibility.Public; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected
?
{ | ||
internal sealed class SynthesizedRecordEqualityContractProperty : PropertySymbol | ||
{ | ||
internal const string PropertyName = "EqualityContract"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to put this in WellKnownMemberNames
I'm not sure there's a test for overriding EqualityContract across emit reference. Can you validate there is one or add one? |
@@ -3060,7 +3066,7 @@ void addProperties(ImmutableArray<ParameterSymbol> recordParameters) | |||
#endif | |||
} | |||
|
|||
static bool hidesInheritedMember(Symbol symbol, NamedTypeSymbol type) | |||
static Symbol? getInheritedMember(Symbol symbol, NamedTypeSymbol type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getInheritedMember [](start = 27, length = 18)
What's the benefit of returning a Symbol
rather than a bool
? It looks like we're only using the value to test for null
. #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was using the Symbol
at one point, but not now. Reverted. #Pending
@@ -2989,10 +2989,16 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde | |||
addCopyCtor(); | |||
addCloneMethod(); | |||
|
|||
var thisEquals = addThisEquals(); | |||
var equalityContract = addEqualityContract(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var [](start = 12, length = 3)
nit: having type (PropertySymbol
) would be helpful for readability here
var otherEqualsMethods = ArrayBuilder<MethodSymbol>.GetInstance(); | ||
getOtherEquals(otherEqualsMethods, equalityContract); | ||
|
||
var thisEquals = addThisEquals(equalityContract, otherEqualsMethods.Count == 0 ? null : otherEqualsMethods[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, [](start = 59, length = 1)
nit: consider naming the second argument
{ | ||
var property = new SynthesizedRecordEqualityContractProperty(this, isOverride: getInheritedEqualityContract(this) is object); | ||
// https://github.com/dotnet/roslyn/issues/44903: Check explicit member has expected signature. | ||
if (!memberSignatures.ContainsKey(property)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not blocking this PR: should we check both whether the property and the get accessor are already in the list of members? Could we have the get accessor defined without property?
Oops, I didn't realize @agocke was already mostly done with review. I'll let him finish. |
{ | ||
var compilation = containingType.DeclaringCompilation; | ||
bool isStruct = parameterType.IsStructType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that records are emitted as class
when would this be true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cannot be true
currently. I left the code here since the code existed before this PR but I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're green now with two sign offs. I'd say go for the merge now and remove this later. Easy to squeeze in as a bug fix later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in #44912.
{ | ||
var method = new SynthesizedRecordEquals( | ||
this, | ||
parameterType: otherEqualsMethod.Parameters[0].Type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following case
record A<T> { }
record B : A<int> { }
Will otherEqualsMethod.Parameters[0].Type
be the closed or open generic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test case, thanks. It looks like it's handled correctly but I'll add a test.
Added in #44912.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It looks like EqualityContract was made |
See spec.