Synthesize equality operators for records. #46497
Conversation
|
@cston, @jcouv, @333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review. #Closed |
1 similar comment
|
@cston, @jcouv, @333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review. #Closed |
| @@ -2067,6 +2067,13 @@ private void CheckForEqualityAndGetHashCode(DiagnosticBag diagnostics) | |||
| return; | |||
| } | |||
|
|
|||
| if (IsRecord) | |||
| { | |||
| // For records the warnings reported below are simply going to eco record specific errors, | |||
There was a problem hiding this comment.
eco [](start = 79, length = 3)
Typo? #Resolved
There was a problem hiding this comment.
| /// The record type includes synthesized '==' and '!=' operators equivalent to operators declared as follows: | ||
| /// | ||
| /// public static bool operator==(R? r1, R? r2) | ||
| /// => (object) r1 == r2 || (r1?.Equals(r2) ?? false); |
There was a problem hiding this comment.
(r1?.Equals(r2) ?? false) [](start = 37, length = 25)
Please consider changing this to match the implementation: (object)r1 != null && r1.Equals(r2)
Same comment for other classes. #Resolved
|
|
||
| try | ||
| { | ||
| // => (object)r1 == r2 || (r1?.Equals(r2) ?? false); |
There was a problem hiding this comment.
(r1?.Equals(r2) ?? false) [](start = 42, length = 25)
(object)r1 != null && r1.Equals(r2)
Same comment for InequalityOperator. #Resolved
| => throw null; | ||
|
|
||
| System.Boolean System.IEquatable<A>.Equals(A x) => throw null; | ||
| }").WithArguments("System.Boolean").WithLocation(2, 1), |
There was a problem hiding this comment.
Can the scope of the error be reduced to the type name rather than the entire declaration? #Resolved
There was a problem hiding this comment.
Can the scope of the error be reduced to the type name rather than the entire declaration?
That would be rather difficult to do because I believe that error is coming from bound node factory that is using syntax node as location for the error to report. The name is not a node, but a token, I believe. Given that this is an edge case, if core library is missing, there would be more errors all over the place, I think the current behavior is acceptable.
In reply to: 464575538 [](ancestors = 464575538)
| static void Test(A a1, A a2) | ||
| { | ||
| System.Console.WriteLine(""{0} {1} {2} {3}"", a1 == a2, a2 == a1, a1 != a2, a2 != a1); | ||
| } |
There was a problem hiding this comment.
} [](start = 4, length = 1)
Please test a1 == a1, a1 != a1 as well. #Resolved
| Test(null, new A(0)); | ||
| Test(new A(1), new A(1)); | ||
| Test(new A(2), new A(3)); | ||
| } |
There was a problem hiding this comment.
} [](start = 4, length = 1)
Consider testing Test(new A(3), new B()); #Resolved
|
@jcouv, @333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review, need a second sign-off. #Closed |
| BoundExpression recordEquals = F.LogicalAnd(F.ObjectNotEqual(r1, F.Null(F.SpecialType(SpecialType.System_Object))), | ||
| F.Call(r1, equals, r2)); | ||
|
|
||
| F.CloseMethod(F.Block(F.Return(F.LogicalOr(objectEqual, recordEquals)))); |
There was a problem hiding this comment.
nit: we probably don't need F.Block. Also applies in inequality operator #Resolved
There was a problem hiding this comment.
nit: we probably don't need F.Block. Also applies in inequality operator
Why is that? I do not have any appetite to experiment, when there is a wide spread pattern that works.
In reply to: 464596845 [](ancestors = 464596845)
ghost
added this to the 
Closes #46381.