Skip to content
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

Easy reference equality comparer API #27683

Closed
jnm2 opened this issue Oct 20, 2018 · 5 comments · Fixed by #31753
Closed

Easy reference equality comparer API #27683

jnm2 opened this issue Oct 20, 2018 · 5 comments · Fixed by #31753
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Oct 20, 2018

Edit by @GrabYourPitchforks on 20 Jan 2020: API proposal is at https://github.com/dotnet/corefx/issues/32931#issuecomment-576453361.

There is an existing internal ReferenceEqualityComparer.

I would like to be able to do something like this:

if (list.SequenceEqual(originalItems, EqualityComparer.ByReference))
{
    // ...
}

Even if I'm not already overriding Equals to consider properties that mutate, using the default equality comparer is dangerous. Future distant changes would silently render this check ineffective. It should be explicitly a reference comparison.

The existing EqualityComparer<> class probably shouldn't be used because there's no way to constrain T : class for the ByReference member. (If C#'s Extension Everything work makes it through, a static extension method would allow putting the generic constraint on the extension method which would only show up on EqualityComparer<T> for referenced-typed T.)

Not having to specify a type parameter is even nicer. A ByReference member wouldn't have to be generic. So long as it returns IEqualityComparer<object>, contravariance allows it to just work for all reference types and blocks it for value types:

// Works
new[] { new MyClass() }.SequenceEqual(new[] { new MyClass() }, (IEqualityComparer<object>)comparer)

// CS1929: 'int[]' does not contain a definition for 'SequenceEqual' and the best
// extension method overload 'Queryable.SequenceEqual<int>(IQueryable<int>, IEnumerable<int>,
// IEqualityComparer<int>)' requires a receiver of type 'IQueryable<int>'
new[] { 1 }.SequenceEqual(new[] { 1 }, (IEqualityComparer<object>)comparer)

// On argument: Argument type 'System.Collections.Generic.IEqualityComparer<object>' is not
// assignable to parameter type 'System.Collections.Generic.IEqualityComparer<int>'
new[] { 1 }.SequenceEqual<int>(new[] { 1 }, (IEqualityComparer<object>)comparer)

There is no non-generic EqualityComparer class and it could be confusing to introduce one in the System.Collections.Generic namespace given the existing Comparer/Comparer<T> classes. Even so, it does provide the nicest syntax: EqualityComparer.ByReference.
Similar to what the non-generic immutable collection classes do, folks might expect EqualityComparer.Default<T>() to exist.

Alternatively, ReferenceEqualityComparer.Instance would work. It doesn't seem as cool to me as EqualityComparer.ByReference, but demand for it has obviously been low.

If nongeneric IEqualityComparer support is something you'd definitely want from this new API, it would have to return a class type which implements both. Variance is out, so the class would have to be generic and the syntax would have to specify a type parameter.

@GrabYourPitchforks
Copy link
Member

Is this the API proposal?

namespace System.Collections
{
    /* NEW proposed type */
    public sealed class ReferenceEqualityComparer : IEqualityComparer, IEqualityComparer<object>
    {
        private ReferenceEqualityComparer(); // no access to ctor

        // public access to singleton instance
        public static ReferenceEqualityComparer Instance { get; }

        public bool Equals(object? x, object? y);
        public int GetHashCode(object obj); // null argument disallowed
    }
}

@jnm2
Copy link
Contributor Author

jnm2 commented Jan 21, 2020

@GrabYourPitchforks Yes.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review labels Feb 4, 2020
@terrajobst
Copy link
Member

terrajobst commented Feb 4, 2020

Video

  • Let's put it in System.Collections.Generic (because System.Collections doesn't have a EqualityComparer anyway).
  • We don't want to put it on EqualityComparer<T> because it wouldn't vary by T
  • Based on Internet search, it seems most folks calls their implementation ReferenceEqualityComparer. We considered introducing a non-generic EqualityComparer with a property ReferenceEquality that felt odd. Also, having a dedicated sealed type allows implementations to check whether it's the special reference equality comparer. The field would have to guarantee that it's the same instance, which we might not want.
namespace System.Collections.Generic
{
    public sealed class ReferenceEqualityComparer : IEqualityComparer, IEqualityComparer<object>
    {
        private ReferenceEqualityComparer();
        public static ReferenceEqualityComparer Instance { get; }

        // Plus inherited members from IEqualityComparer, IEqualityComparer<object>
    }
}

@ahsonkhan
Copy link
Member

cc @Jozkee - once this is implemented, we should consider removing the custom comprarer used within JSON:

internal sealed class ReferenceEqualsEqualityComparer<T> : IEqualityComparer<T>

@Jozkee
Copy link
Member

Jozkee commented Feb 4, 2020

@ahsonkhan I will take a look at #31753. Thanks!

fredrikhr added a commit to thnetii/dotnet-common that referenced this issue Feb 5, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants