Runtime: Should Memory<T> implement IEquatable<T>?

[terrajobst]

In the .NET Standard review of Span<T> @marek-safar wrote:

Why it does not implement IEquatable<T> ?
...
Well, if it's hard why do you have public bool Equals(System.Memory<T> other) ? I think it'd make sense to have the interface here if the implementation is there too.

It seems odd to providing a strongly typed Equals method but not implementing IEquatable<T>.

@KrzysztofCwalina @stephentoub @GrabYourPitchforks @jkotas

[KrzysztofCwalina]

I think we should implement it. Thanking said that it will have unexpected semantics, i.e. quasi referential equality. It will be especially surprising if ReadOnlyMemory is used as a string slice. Of course, this are the semantics of the existing Equals, but the strange semantics might be more "in the face" when we implement the interface.

[jkotas]

The Memory<T> in corefxlab implemented it originally, but we have later removed it in: https://github.com/dotnet/corefxlab/pull/1607/files. Why did we remove it?

[danmosemsft]

@ahsonkhan for the question above.

[ahsonkhan]

Why did we remove it?

As @KrzysztofCwalina mentioned, it was because Equals does referential equality rather than deep value equality which might be unexpected for users. Regardless, we essentially implement the interface already (with the equals API), so I don't see any reason we shouldn't explicitly add the IEquatable back to the type.

Looking back at previous design discussions at the time when we removed it (i.e. https://github.com/dotnet/corefxlab/pull/1607), the meeting notes state: "We can remove the interface but keep the Equals methods", so that doesn't shed much light :p

[terrajobst]

@ahsonkhan @KrzysztofCwalina I still think we should add the interface. While I generally agree with the deep-vs-shallow equality problem, I think the current state is incosistent. Providing a strongly typed Equals but not implementing IEquatable feels inconsistent.

Unless someone pushes back, we should keep it approved an assign a dev to it :-)

[ostorc]

Is it up to grab? If so, could i try make PR for this issue?

[ahsonkhan]

Is it up to grab? If so, could i try make PR for this issue?

That sounds great. Feel free to ping me on the PR for review. Thanks for the contribution.

[ostorc]

@ahsonkhan should I add IEquatable to ReadOnlyMemory or is thing for another issue?

[ahsonkhan]

should I add IEquatable to ReadOnlyMemory or is thing for another issue?

Yes, you should. We don't have a separate issue for that since the interface applies naturally to the ReadOnly version as well.

[terrajobst]

@ahsonkhan

Is the plan to wait for a community member to do the work? It would be nice if we could complete this for 3.0 so that we can also expose it from .NET Standard 2.1.

[Gnbrkm41]

Has this not been done yet? I can try to make the changes real quick.

• Make changes to the Memory / ReadOnlyMemory in coreclr's System.Private.CoreLib (and open PR)
• Make the same changes in ref/System.Runtime's ones
• Add tests that compare these types via interface (should go into corefx/src/System.Memory/tests/(ReadOnly)Memory/Equality.cs?)

[ahsonkhan]

Yes, please, that would be awesome!

The previous PR was abandoned: https://github.com/dotnet/corefx/pull/35501

Ping me if you need any help (see https://github.com/dotnet/corefx/pull/35501#discussion_r259200377)

[Gnbrkm41]

Is that the only change required in coreclr? Shall I just open a PR (in coreclr) mentioning this issue with the changes?

[ahsonkhan]

Is that the only change required in coreclr? Shall I just open a PR (in coreclr) mentioning this issue with the changes?

Yep. The three changes you mentioned above is what is required. So the ref and test changes will be in corefx.

[Gnbrkm41]

Can this be closed now? I forgot to put Fixes: dotnet/corefx#32905 in the PR ☺️ .

[stephentoub]

Thanks :)

[stephentoub]

Thanks :)

[stephentoub]

@terrajobst, is there a standard follow-up to be done here?

[terrajobst]

Yep. I'll submit the PR.