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

API proposal: MemoryExtensions.ReferenceEqual #72968

Open
GrabYourPitchforks opened this issue Jul 27, 2022 · 7 comments
Open

API proposal: MemoryExtensions.ReferenceEqual #72968

GrabYourPitchforks opened this issue Jul 27, 2022 · 7 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Memory
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

API proposal: MemoryExtensions.ReferenceEqual

See #54794 for context. In a nutshell, we want developers to stop using the equality and inequality operators on Span<T> and ReadOnlySpan<T>.

ReadOnlySpan<char> span1, span2;

// Don't do this
// It checks for *referential* equality, which you probably didn't intend!
bool areEqual = (span1 == span2);

// Do this instead
// It checks for *content* equality, which you probably intended.
bool areEqual = span1.SequenceEqual(span2);

But there are some cases where callers really do need to check for referential equality, such as if they're trying to special-case "an arbitrary empty span" vs. "a span that specifically references nullptr." Another use case is string interning dictionaries, where callers may want to know as an optimization whether a span references a specific instance of a string value rather than simply containing the same contents.

Proposed API

namespace System
{
    // new APIs added to existing class (asm: System.Memory)
    public partial static class MemoryExtensions
    {
        public static bool ReferenceEqual<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> other);
        public static bool ReferenceEqual<T>(this Span<T> span, ReadOnlySpan<T> other);
    }
}

Discussion

If we eventually obsolete the span equality comparison operators, callers will need to answer the question "was this comparison intended to be a referential equality comparison or a value equality comparison?" We're betting on that most callers wanted value equality and will opt to change their call sites to target the existing SequenceEqual method. The new ReferenceEqual method proposed here provides an alternative for developers to preserve reference-equality comparison without needing to #pragma warning disable ... every call to the existing equality operators.

.NET already exposes two related contepts: static bool object.ReferenceEquals(object, object), which is pure referential equality; and static bool MemoryExtensions.SequenceEqual<T>(Span<T>, Span<T>) where T : IEquatable<T>, which performs content equality comparison.

For consistency with the existing MemoryExtensions.SequenceEqual API, we'll keep its parameter names in the proposed MemoryExtensions.ReferenceEqual API. We'll also drop the s from ReferenceEquals to match the existing extension method naming conventions.

Risks

The name ReferenceEqual might inadvertently convey "only referential equality" instead of the intended "both referential and length equality" behavior. That is, somebody might expect the following to return true.

ReadOnlySpan<char> span1 = "Hello!";
ReadOnlySpan<char> span2 = span1.Slice(0, 2);
bool equal = span1.ReferenceEqual(span2); // they both start at (reference) the same place!

We may be able to mitigate this risk through careful messaging and encouraging callers to think about "references to blocks of memory" rather than "references to addresses in memory". If you consider that a block of memory contains both a start and an end position - as distinct from just a single address - then the analogy makes sense.

@GrabYourPitchforks GrabYourPitchforks added area-System.Memory blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 27, 2022
@GrabYourPitchforks GrabYourPitchforks added this to the 7.0.0 milestone Jul 27, 2022
@GrabYourPitchforks GrabYourPitchforks self-assigned this Jul 27, 2022
@alexrp
Copy link
Contributor

alexrp commented Jul 27, 2022

The name ReferenceEqual might inadvertently convey "only referential equality"

FWIW, that was what I expected at first glance.

Considering that and the potential confusion with object.ReferenceEquals(), I feel like there has to be a better name here, though I don't have any good suggestions off the top of my head.

@DaZombieKiller
Copy link
Contributor

I feel like there has to be a better name here, though I don't have any good suggestions off the top of my head.

Maybe MemoryMarshal.AreSame/MemoryExtensions.IsSame, mirroring Unsafe.AreSame?

@Joe4evr
Copy link
Contributor

Joe4evr commented Jul 28, 2022

The name ReferenceEqual might inadvertently convey "only referential equality" instead of the intended "both referential and length equality" behavior. That is, somebody might expect the following to return true.

ReferenceAndLengthEquals? 🙃

@GrabYourPitchforks
Copy link
Member Author

I had considered "are same" but didn't like it because "same" and "equal" are too similar. The ambiguity would still exist.

Another alternative is RangeEqual, which clearly indicates it's talking about a range of memory. It also works with the Range slicing operator, which is a nice coincidence.

@terrajobst
Copy link
Member

  • We don't like ReferenceEquals because it's not clear that it covers start and length (as opposed to just start)
namespace System;

public partial static class MemoryExtensions
{
    public static bool ReferenceEqual<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> other);
    public static bool ReferenceEqual<T>(this Span<T> span, ReadOnlySpan<T> other);
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 2, 2022
@carlreinke
Copy link
Contributor

IdentityEqual?

@jeffhandley
Copy link
Member

Moving to 8.0.0 since we don't have agreement on the name yet.

@jeffhandley jeffhandley removed the blocking Marks issues that we want to fast track in order to unblock other important work label Aug 10, 2022
@jeffhandley jeffhandley modified the milestones: 7.0.0, 8.0.0 Aug 10, 2022
@tannergooding tannergooding modified the milestones: 8.0.0, Future Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Memory
Projects
None yet
Development

No branches or pull requests

8 participants