Regression from first-class spans with overload picking

[colejohnson66]

Version Used: 9.0.200.preview

Steps to Reproduce:

1. Use MemoryMarshal.Cast with an array
2. Expect Span<T> overload, but get ReadOnlySpan<T> overload

https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKHQAYACdbAOgCUBXGAFyjAFMKBJB5gJwHsAHAMrcAblADGzAM4BuAgXQBmMphIBhADYRJkggG8CJQ2SXoALCQCyzegAseCABQBKA0f34jnkgh41g65gBtAF0Segh/ZhIAXhIYGnV1AEJZDy9DAT4IGAAeBJ4YAHMAPm8pehjLZjAeLgBPCwguSRsIdQpVLXocnz8AuBJ8ouKHcMinVM8AXwIpoA===

using System;
using System.Runtime.InteropServices;

public class Class
{
    public void Method()
    {
        double[] table = null!;
        Span<ulong> dest = MemoryMarshal.Cast<double, ulong>(table);
    }
}

Diagnostic Id:

error CS0029: Cannot implicitly convert type 'System.ReadOnlySpan' to 'System.Span'

Expected Behavior:

The Span<T> overload is used, as it was on .NET 8 and .NET 9 RC2.

Actual Behavior:

The ReadOnlySpan<T> overload is used instead.

[colejohnson66]

Forcing the call to use the Span<T> overload (with either a cast or MemoryExtensions.AsSpan) causes the issue to go away.

[jcouv]

Tagging @jjonescz to confirm, but I think this will be fixed by #76300

[colejohnson66]

If this is intended as a breaking change, that’s fine with me (I’ve already fixed the code). It just isn’t documented as one.

[jjonescz]

This is an expected breaking change. ReadOnlySpan is now preferred over Span to avoid ArrayTypeMismatchExceptions that would occur at runtime if you used covariant arrays.

See also https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-12-04.md#preferring-readonlyspant-over-spant-conversions.

I think this will be fixed by #76300

That PR doesn't fix it, it makes it broader in fact, per LDM discussion linked above.

It just isn’t documented as one.

I will open a PR to document the break, thanks.

[Sergio0694]

@jjonescz why is the rule applied to all arrays, and not just those that could possibly be covariant? Converting from T[] to ReadOnlySpan<T> effectively loses information, would it make sense to only do that where it's actually needed? For instance, if you have a string[], or some MyValueTypeArray[], etc. these can't possibly be covariant arrays. Can't those convert to Span<T>?

[jjonescz]

That wasn't discussed but I suspect the LDT wouldn't be in favor of complicating the rules like that.

[h3xds1nz]

This change unfortunately also seems to break the following scenario, at least it does on preview lang currently with .NET 9.

byte[] bytes = new byte[24];
ref SomeStruct bytesAsStruct = ref MemoryMarshal.AsRef<SomeStruct>(bytes);

Now if you need a writable ref, you're forced either

1. to explicitly cast into Span<byte> (whether in declaration or in the parameter)
2. use the more verbose: ref SomeStruct unwindInfo = ref Unsafe.As<byte, SomeStruct>(ref MemoryMarshal.GetReference(bytes)).

Neither of which are as nice as it used to be.

[Sergio0694]

@jjonescz would you be supportive of bringing this additional covariance support to LDM, with motivating examples such as this one? It just feels wrong to me to have ROS<T> be the natural type for all array types, including those that cannot possibly be covariant, if the whole reason why that natural type was chosen was exact because of covariance 😅

[jjonescz]

This change unfortunately also seems to break the following scenario,

That is a very similar scenario to the one in the issue description, and the mitigation is also the same - add AsSpan():

ref S bytesAsStruct = ref MemoryMarshal.AsRef<S>(bytes.AsSpan());

[jjonescz]

would you be supportive of bringing this additional covariance support to LDM, with motivating examples such as this one? It just feels wrong to me to have ROS<T> be the natural type for all array types, including those that cannot possibly be covariant, if the whole reason why that natural type was chosen was exact because of covariance

Array covariance was the original reason, but now after LDM discussion the rule is even broader - it doesn't apply to just arrays anymore but to anything that's convertible to ReadOnlySpan or Span - dotnet/csharplang#8779. The motivation was to simplify the rules:

We also considered the question of whether we should take a bigger step. Our proposed rules only impact when arrays are converted to span types. This is yet another element to a decoder ring, another special case that users need to know to understand how overload resolution will be applied to their APIs. Could we instead make a broader, simpler rule, where we just always prefer ReadOnlySpan over Span? We don't think we have enough data to make such a decision today. We don't know of any APIs that this would negatively affect, but more investigation needs to be done before making a call.

But it also says "We don't think we have enough data to make such a decision today. We don't know of any APIs that this would negatively affect, but more investigation needs to be done before making a call." so I guess these MemoryMarhsal APIs are the data. I will ask around.

[h3xds1nz]

That is a very similar scenario to the one in the issue description, and the mitigation is also the same - add AsSpan():
ref S bytesAsStruct = ref MemoryMarshal.AsRef<S>(bytes.AsSpan());

Yeah, as I've noted myself. I'm simply pointing out its less intuitive than it used to be, making first-class support to be a second-class in some cases haha. Not trying to discard any work in this matter, I'll be happy if those particular cases mentioned are considered once more.

[jaredpar]

For instance, if you have a string[], or some MyValueTypeArray[], etc. these can't possibly be covariant arrays.

This argument would imply that say sealed types should naturally convert to Span<T> vs. ROSpan<T>: after all there is no issue of variance. That logic though would mean that whether a type is sealed or not impacts the semantics of the language. It is effectively a breaking change to remove sealed from a type. That's not an avenue I'm interested in going down.

Limiting it to only types which are known to be struct doesn't feel worth the extra complexity. Instead it seems better to uniformly say that if you have an array and want to force it to be Span<T> then use .AsSpan().