ReadOnlySpan<T>.DangerousGetPinnableReference should return a readonly reference

[ektrah]

Should

namespace System
{
    public struct ReadOnlySpan<T>
    {
        public ref T DangerousGetPinnableReference()

be

        public ref readonly T DangerousGetPinnableReference()
                      ↑

?

That would make it slightly less dangerous.

[KrzysztofCwalina]

@VSadov, @jaredpar, what do you think?

[jaredpar]

Agree

[jkotas]

I am not sure whether there is any value in this. We should look at how this method is used. DangerousGetPinnableReference can be really only used together with other unsafe constructs:

• System.Runtime.CompilerServices.Unsafe.*: None of the methods on this type take ref readonly today. The first thing one would need to do is to cast away the readonly-ness. https://github.com/dotnet/corefx/issues/23916 is related to this.
• Pinning: Is it even possible to pin ref readonly?

[jaredpar]

What is the value in ReadOnlySpan at all if we can mutate with a method so easily? May as well not have the type

[ektrah]

I think there is a lot of value in statically verifying that some data passed by reference to a method is not mutated. However, it seems the way ref readonly is currently designed, changing an API from ref T to ref readonly T is a breaking change, so that ref readonly can only be used in new APIs. That limits its usefulness considerably. Casting away the readonly-ness will probably be the first thing everyone will do who wants to use both a new ref readonly T API and an existing ref T API. That doesn't mean that ref readonly should be avoided in new APIs IMHO.

[KrzysztofCwalina]

@jkotas, can we add readonly ref methods to Unsafe?

Without the readonly, users can do the following in safe context:

readonlySpan.DangerousGetPinnableReference() = newValue;

With readonly, they will have to be in an unsafe context.

[jkotas]

@jkotas, can we add readonly ref methods to Unsafe?

We can. My gut feel is that it is a lot of API clutter with negative value. We need to see how it would work in practice. E.g. rewrite https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/SpanExtensions.cs using readonly refs and ReadOnlySpan.DangerousGetPinnableReference returning readonly ref.

With readonly, they will have to be in an unsafe context.

How do you define "unsafe context"?

None of this needs C# unsafe keyword with or without this suggestion because C# unsafe keyword is limited to what the C# language considers unsafe. It does not extend to unsafe APIs.

DangerousGetPinnableReference is logically unsafe context. If you are using it, you are writing unsafe code.

[jkotas]

What is the value in ReadOnlySpan at all if we can mutate with a method so easily?

Unsafe APIs are important to have. If you have a simple unsafe API, it will be always relatively easy to call. We can make it harder by:

• Giving the API scary looking name. We choose to prefix the API name with "Dangerous".
• Making the API less discoverable: Hide or move away from Span the dangerous APIs #21263.
• Allowing the API to be called from C# unsafe context only: [CallerMustBeUnsafe] type attribute roslyn-analyzers#972

[KrzysztofCwalina]

How do you define "unsafe context"?

For the purpose of this discussion: it's whenever you call APIs on Unsafe class. I totally get that it's not as bullet proof as C# unsafe block, but it might be better than what we have today.

[mellinoe]

Isn't the whole point of this method to give me a pointer which is, by definition, not readonly?

[VSadov]

Do we really need the method?

Can we have the indexer return ref readonly? It could be more efficient in some scenarios, than an indexer that returns values and must always copy. Then span[0] would be be the same thing as the pinnable ref?

Note that right now you would need to cast away readonly before you can pin. Historically we would not let you take pointers (as in &) to readonly fields/elements/expressions and such since pointers require writable variables. I think this is bogus and can be relaxed. Once you opt into pointers, readonly is ignorable, since you can defeat the type system entirely.

[VSadov]

Basically the following should work (it works with Span, but not with RO span, since indexer returns a value).

    unsafe class Program
    {
        static void Main(string[] args)
        {
            ReadOnlySpan<int> sp = new int[10];

            fixed(int * ptr = &sp[0])
            {
                 . . . do stuff with ptr . . .
            }
        }      
    }

[VSadov]

IMO, we should:

1. make RO span indexer to return 'ref readonly`, that would immediately allow

fixed(int * ptr = &Unsafe.AsRef(sp[0])) // need to cast away readonly via AsRef

2. relax writeable requirement in the language when doing &, then you can do

fixed(int * ptr = &sp[0])

[jkotas]

fixed(int * ptr = &Unsafe.AsRef(sp[0]))

This does not work for spans of Length==0, and has unnecessary range check otherwise.

[VSadov]

@jkotas - I am not sure it should work with Length=0.
Attempt to do so is likely a bug. When not sure, you would need to check for emptiness one way or another. How else would it work? Return (int*)0? Then you would need the check anyways, before dereferencing the ptr.
This is also how fixed works with regular arrays, and Spans actually.

I think one range check is acceptable when fetching a pointer. It is likely to be very predictable and cheap, possibly even free if can CSE'd with user's check for Length!=0.
After that you can read/write/index the ptr with no checks and quickly amortize the expense if there is any. - Seems like a good balance between perf and safety.

[jkotas]

It is intentional that this method works the way it works with Length=0. This was discussed back during the original Span design discussions.

How else would it work?

It can return any pointer. It does not have to be 0. It depends on how the Span was created.

When not sure, you would need to check for emptiness one way or another

Yes, you have to check for emptiness. But you control when and how you do it.

I think one range check is acceptable when fetching a pointer

The range check is pretty significant cost when you are fetching the pointer for a very small cheap operation. Consider method like:

public static int ToInt32(ReadOnlySpan<byte> value)
{
        if (value.Length < sizeof(int))
            ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value);
        return Unsafe.ReadUnaligned<int>(ref value.DangerousGetPinnableReference());
}

If you add range checks to DangerousGetPinnableReference, this method will be significantly slower.

This is also how fixed works with regular arrays

IMHO, it is pretty unfortunate that fixed works that way because of it more expensive than necessary and thus unusable for small cheap operation. (I know how we ended up with it ... it does not make it right.)

[jkotas]

BTW: The idea of changing ReadOnlySpan indexer to return readonly ref sounds good to me.

[VSadov]

Lets change the ReadOnlySpan indexer as it would be a good change on its own.
Then we can check if ToInt32 will be that much slower on top of it.

Honestly - An API that allows writing into RO span and has undefined behavior with empty spans is hard to like.
(I still do not get - reference to what is returned when there is nothing to reference...)

Perhaps you are right though and perf penalty is indeed too big to ignore.

[jaredpar]

Unsafe APIs are important to have. If you have a simple unsafe API, it will be always relatively easy to call. We can make it harder by:

If this method were unsafe I wouldnt care. As written it is completely safe.

[jkotas]

This method is unsafe. It just does not require unsafe C# keyword.

[jaredpar]

As I said, it is a safe method on a safe type.

[VSadov]

dotnet/roslyn#22400 has enabled taking unmanaged/pinned addresses of readonly variables.

There is no reason for DangerousGetPinnableReference to be writeable now.

[jkotas]

We should look at this together:

• with https://github.com/dotnet/corefx/issues/23879 .
• with potential potential C# feature to correctly abstract pinning in the language. Ideally, fixed (int * p = foo) should expand into fixed (int * p = foo.GetDangerousPinneableReference()) underneath or something similar to let types control the pinning behavior.

[ahsonkhan]

So, what are the next steps for this? #23491 is in progress here and here

Do we plan to change DangerousGetPinnableReference to return ref readonly (along with potential rename to GetReference and moving it to MemoryMarshal?

cc @stephentoub

[VSadov]

See dotnet/csharplang#1100

It is still subject to discussions, but it seems reasonable to start changing DangerousGetPinnableReference to be ref readonly.

Perhaps change it to return nullptr in empty case, but may wait on that since it is just implementation. It does not need to be in the same single change.

[jkotas]

@VSadov Breaking changes in signatures of methods that are used in many places are very expensive to push through the system - they need to be staged carefully. @ahsonkhan is still working on changing the indexer to read-only. We should wait for that one to get pushed through first.

I think the plan for this one should be:

1. Add MemoryExtensions.GetReference (https://github.com/dotnet/corefx/issues/23879)
2. Convert all uses of DangerousGetPinnableReference in coreclr, corefx, corert, corefxlab, aspnet, ... to MemoryExtensions.GetReference
3. Change DangerousGetPinnableReference to whatever we like to make it fit the pinning pattern

Doing it this way will avoid the need for complex staging or things being on the floor for extensive periods of time.

[VSadov]

I would hope this change is easier than the indexer.
At least ref -> ref readonly is not binary-breaking, and perhaps JIT intrinsics do not need to change, if there are any.

Also fixed is already updated to work with readonly references, so that should mitigate some changes as well. (need to make sure we have the right compiler in CoreFx)

[jkotas]

Yes, it should be easier in the sense that we do not need to do complex staging for it. But it will still require changing hundreds of lines: E.g. out of ~200 calls to DangerousGetPinnableReference in corefx, ~30 of them are used with fixed and should "just work", unless they are performance sensitive and should rather call MemoryMarshal.GetReference to avoid the extra null check. The remaining ~170 will need to be fixed up manually. There is more in other repos.

[terrajobst]

Video

We don't care super much; we see the value in promoting generally safe behavior so the API suggestion looks sensible to me. However, we lack expertise to judge whether this causes clutter (additional casting, like C++ const). So I'd leave it to runtime/compiler folks to decided what to do here.

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/BI3iXFT8H7E?t=3414 (6 min duration)

[jkotas]

Change DangerousGetPinnableReference to whatever we like to make it fit the pinning pattern

We may want to just use less ugly GetPinnableReference name for the pinning pattern. Once it returns null for empty spans and in for read-only spans, there is really nothing dangerous about it.

[benaadams]

GetReference? unless there are other references you can get which you can't pin?

[jkotas]

This is about the name that will be used for dotnet/csharplang#1100 . GetReference sounds too generic for it. It needs to have something that ties it to pinning or fixed in the name, I think.

[benaadams]

Ah, the contract for a fixed statement. GetPinnableReference then sounds good as it ties it to the framework (e.g. GCHandle Pinned) whereas fixed would tie it to language (C#).