Runtime: Add GetPinnableReference back to Span and ReadOnlySpan

[benaadams]

Proposed Api

public readonly ref partial struct ReadOnlySpan<T>
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    public readonly ref T GetPinnableReference();
}

public readonly ref partial struct Span<T>
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    public ref T GetPinnableReference();
}

Question Unlike MemoryMarshal it should return null pointer should it return null pointer for zero length Span? (see further down https://github.com/dotnet/corefx/issues/28969#issuecomment-379979579) - though null refs are odd?

Rational

So C#7.3 is outish now https://docs.microsoft.com/en-us/visualstudio/releasenotes/vs2017-preview-relnotes#csharp and it supports:

Custom `fixed` statement:
 Types that implement a suitable `GetPinnableReference` can be used in a `fixed` statement.

However, this method on span has been moved to MemoryMarshal.GetReference so span cannot participate directly in a fixed statement.

To allow direct using a fixed statement the methods should be returned; with the caveat of adding the attribute [EditorBrowsable(EditorBrowsableState.Never)] so the route for a user getting a reference remaining via MemoryMarshal

cc @davidfowl, @GrabYourPitchforks, @stephentoub, @KrzysztofCwalina, @pakrym, @ahsonkhan @jkotas

[jkotas]

• It should be public ref T GetPinnableReference(), not public ref T GetPinnableReference<T>().
• Note that it is not same as MemoryMarshal.GetReference. GetPinnableReference() is expected to always return null pointer for zero length Span.

[benaadams]

Updated

[GrabYourPitchforks]

I'm honestly not too worried about the nullptr thing, and even the spec says they're still not 100% decided on it. Span<T> can't really differentiate between null or empty data, and even then you can't safely treat any char* that you get back from a ReadOnlySpan<char> as a LPCWSTR anyway (it's not guaranteed null-terminated). All of this is to say that I don't think it's a bad thing to defy the existing convention.

(To nitpick... yes, Span<T> _can_ internally differentiate between null or empty, but there was an explicit decision made a while back not to expose this distinction in the API surface, so the distinction is just an academic implementation detail.)

[jkotas]

I'm honestly not too worried about the nullptr thing

I am with you on this. If it was my call, I would make it same as Marshal.GetReference. Roslyn folks (@VSadov ) had a strong opinion about it returning null for zero length Spans, so that it has same behavior as arrays.

[GrabYourPitchforks]

Would it make sense to have GetPinnableReference() as an extension method on MemoryMarshal - kind of a sister API to GetReference()? The only difference would be the empty-to-null conversion, assuming we want to honor that convention.

[benaadams]

Would it make sense to have GetPinnableReference() as an extension method on MemoryMarshal

Wouldn't that mean you could only do

fixed (byte* p = span)

If you also did

using System.Runtime.InteropServices;

[jkotas]

extension method on MemoryMarshal

MemoryMarshal was reserved for non-extension methods so far.

[benaadams]

Would this be a 2.1 or 2.2 api?

[benaadams]

Would this be a 2.1 or 2.2 api?

i.e. would it be another 6 months at least before fixed works directly on Span?

[joshfree]

We’ll see what we can potentially do this week to possibly get this in for 2.1 RC.

[terrajobst]

Video

• The API looks fine. It's a bit weird to have these APIs on Span<T> and ReadOnlySpan<T>, it would be more consistent to have these APIs on MemoryMarshal. However, one could also argue that these API are are safe, as they return null for empty spans, so callers will get a null reference rather than being able to get into corrupted states.

• It's true that we mostly don't have extension methods there, but I don't see a reason why we wouldn't declare them as extension methods. However, the real issue is that moving them to MemoryMarshal is that without the using, the feature wouldn't work. Yes, its not different from Linq, but it seems desirable to have these APIs just work.

Hence, we believe these should be instance methods on Span<T> and ReadOnlySpan<T>. We'd still mark the APIs as [EditorBrowsable(Never)] as this API is plumbing only.

[VSadov]

I would mildly prefer MemoryMarshal because nullptr still does bother me. It is typesafe from CLR/IL perspective, but completely unexpected on the language level and the more contained its use the better. - None does null-checks for byref parameters and I wouldn't want them start now.

However I also see how MemoryMarshal approach might not be able to scale consistently when pinnability is added to more types.
For starters - we definitely want fixed support for the ImmutableArray. So where do we put its GetPinnable? - into ImmutableMarshal?

[KrzysztofCwalina]

MemoryMarshal is not in System namespace (where Span is) and I think it would be pretty bad experience is fixed worked on spans depending on which namespaces are imported.

[ahsonkhan]

Fixed in https://github.com/dotnet/corefx/pull/29003 / https://github.com/dotnet/coreclr/pull/17504