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.

Sign up for GitHub

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

Jump to bottom

Regression from first-class spans with overload picking #76443

Open
colejohnson66 opened this issue Dec 16, 2024 · 6 comments · May be fixed by #76572
Open

Regression from first-class spans with overload picking #76443

colejohnson66 opened this issue Dec 16, 2024 · 6 comments · May be fixed by #76572
Labels
Area-Compilers Feature - First-class Span Types untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@colejohnson66
Copy link
Contributor

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.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 16, 2024
@colejohnson66
Copy link
Contributor Author

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

@colejohnson66 colejohnson66 changed the title Regression from first-class spans Regression from first-class spans with overload picking Dec 16, 2024
@jcouv
Copy link
Member

jcouv commented Dec 20, 2024

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

@colejohnson66
Copy link
Contributor Author

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
Copy link
Member

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.

@jjonescz jjonescz linked a pull request Dec 27, 2024 that will close this issue
Open
@Sergio0694
Copy link
Contributor

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Compilers Feature - First-class Span Types untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Document breaks due to preferring ReadOnlySpan over Span jjonescz/roslyn
4 participants
@jjonescz @Sergio0694 @colejohnson66 @jcouv