Implement remaining span conversions #74420

jjonescz · 2024-07-17T17:06:01Z

Test plan: #73445

333fred

Done review pass

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

333fred · 2024-07-18T20:30:48Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+        {
+            // (4,44): error CS0030: Cannot convert type 'System.ReadOnlySpan<int>' to 'System.Span<int>'
+            //      Span<int> M(ReadOnlySpan<int> arg) => (Span<int>)arg;
+            Diagnostic(ErrorCode.ERR_NoExplicitConv, "(Span<int>)arg").WithArguments("System.ReadOnlySpan<int>", "System.Span<int>").WithLocation(4, 44)


I would expect this to be legal. #Resolved

Why? There is no implicit nor explicit conversion from ReadOnlySpan to Span (the opposite conversion exists). There is an explicit span conversion only from array to (ReadOnly)Span. We could propose other explicit conversions but I'm not sure how we would emit them anyway.

Because the type defines a UDC that permits it.

Ah I missed that. Then you're right that looks a bit unexpected, let me check what's happening.

Okay, this UDC is ignored - per the speclet:

User-defined conversions are not considered when converting between

any single-dimensional array_type and System.Span<T>/System.ReadOnlySpan<T>,

any combination of System.Span<T>/System.ReadOnlySpan<T>,

string and System.ReadOnlySpan<char>.

This case is the second bullet point.

It does not care about converting from ReadOnlySpan to Span.

The speclet cares about that conversion actually - because it's the opposite conversion of the implicit Span->ReadOnlySpan, and the standard says that for any implicit conversion the opposite explicit conversion exists. So the speclet needs to say that the ROS->Span standard explicit conversion doesn't exist.

The only things the speclet should talk about are the conversions that it actually cares about.

So you are suggesting we only disallow UDCs for the conversions the compiler can emit?

My intent was that the compiler handles all conversions between Spans (otherwise it's inconsistent). I think @cston suggested that as well for example in #73577 (comment).

The spec should certainly say that no ROS->Span standard explicit conversion exists, but that isn't what we have here. We have a UDC. We shouldn't be disallowing this, as the feature doesn't care about it. It of course can't fall in the bucket of a standard implicit conversion; it can't contribute to the things that only "standard" conversions can. But it can absolutely still exist and be used, and be called by user code as any UDC can.

Okay, works for me. Updating the speclet in dotnet/csharplang#8312 (the second commit is relevant to this discussion).

I'm not sure why the speclet says that. The only things the speclet should talk about are the conversions that it actually cares about. It does not care about converting from ReadOnlySpan to Span.

My expectation is that if some conversions between spans and arrays, spans, and strings are handled directly in the compiler, then all such conversions should be handled in the compiler, and the compiler should not fallback to user-defined conversions between these types. Was this covered at LDM?

Was this covered at LDM?

It wasn't, adding an open question in dotnet/csharplang#8312.

jjonescz · 2024-07-30T18:27:06Z

@cston for a second review, thanks

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

cston · 2024-07-30T23:03:51Z

public void Conversion_Span_ReadOnlySpan_Implicit(

It looks like these tests use implicit and explicit casts. #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:327 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-07-30T23:10:59Z

Are we testing cases where there is not an implicit reference conversion between element types?

ReadOnlySpan<string> x = new ReadOnlySpan<object>();
ReadOnlySpan<object> y = new ReadOnlySpan<int>();
ReadOnlySpan<int?> z = new ReadOnlySpan<int>();
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:487 in d9e4b71. [](commit_id = d9e4b719f83b29c813db1e873475a3870f654400, deletion_comment = False)

cston · 2024-07-30T23:15:55Z

[InlineData("ReadOnlySpan<char> AsSpan(object o)", false)]

Consider testing:

public static class MemoryExtensions
{
    public static Span<char> AsSpan(string s) => default;
}
public static class MemoryExtensions
{
    internal static ReadOnlySpan<char> AsSpan(string s) => default;
}
internal static class MemoryExtensions
{
    public static ReadOnlySpan<char> AsSpan(string s) => default;
}
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:820 in d9e4b71. [](commit_id = d9e4b719f83b29c813db1e873475a3870f654400, deletion_comment = False)

cston · 2024-07-30T23:33:16Z

Consider testing:

ReadOnlySpan<string> x = new Span<string?>();
ReadOnlySpan<string?> y = new Span<string>();
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2305 in d9e4b71. [](commit_id = d9e4b719f83b29c813db1e873475a3870f654400, deletion_comment = False)

cston · 2024-07-30T23:35:38Z

                public static implicit operator ReadOnlySpan<string>(ReadOnlySpan<T> span) => default;

I'm surprised we allow user-defined conversions between spans and arrays, spans, or strings. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2320 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-07-30T23:38:49Z

                public static implicit operator Span<T>(ReadOnlySpan<T> span) => default;

I'm surprised we allow user-defined conversions from ReadOnlySpan<T> to Span<U>. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2434 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-07-30T23:43:04Z

                public static implicit operator Span<T>(string s) => default;

I'm surprised we allow user-defined conversions between string and spans. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2579 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-07-30T23:45:43Z

            ReadOnlySpan<string> M7(Span<object?> arg) => (ReadOnlySpan<string>)arg;

Should this be ... ?

ReadOnlySpan<object> M7(Span<string?> arg) => (ReadOnlySpan<object>)arg;
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2754 in d9e4b71. [](commit_id = d9e4b719f83b29c813db1e873475a3870f654400, deletion_comment = False)

cston · 2024-07-30T23:53:55Z

public void Conversion_Span_ReadOnlySpan_Implicit_NullableAnalysis_NestedNullability_Covariant()

Is this case different than Conversion_Span_ReadOnlySpan_Implicit_NullableAnalysis_NestedNullability? #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:3143 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-07-30T23:54:25Z

public void Conversion_ReadOnlySpan_ReadOnlySpan_Implicit_NullableAnalysis_NestedNullability_Covariant()

Is this case different than Conversion_ReadOnlySpan_ReadOnlySpan_Implicit_NullableAnalysis_NestedNullability? #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:3172 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-07-30T23:57:11Z

                public static explicit operator Span<T>(ReadOnlySpan<T> span) => throw null;

I'm surprised we allow explicit user-defined conversions between spans and arrays, spans, or strings. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:3684 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T08:23:29Z

                public static implicit operator ReadOnlySpan<string>(ReadOnlySpan<T> span) => default;

We didn't, but Fred said we should. See #74420 (comment). Specifically we now only disallow UDCs where span conversions exist - e.g., in this example, there is no span conversion between ROS<object> and ROS<string> so the UDC is allowed. The change is in d9e4b71 (could be simply reverted if we conclude we want to go with the previous approach instead).

In reply to: 2259370393

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2320 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T08:52:58Z

public void Conversion_Span_ReadOnlySpan_Implicit_NullableAnalysis_NestedNullability_Covariant()

Looks the same, I will remove this one, thanks.

In reply to: 2259384495

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:3143 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T08:54:35Z

public void Conversion_ReadOnlySpan_ReadOnlySpan_Implicit_NullableAnalysis_NestedNullability_Covariant()

Yes, this one tests I<out T>, the other one tests S<T>. The diagnostics are also different.

In reply to: 2259384850

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:3172 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T08:58:55Z

            ReadOnlySpan<string> M7(Span<object?> arg) => (ReadOnlySpan<string>)arg;

It was modeled on the previous test (only with a Span instead of an array). But I will add the test you are suggesting.

In reply to: 2259378657

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2754 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T09:27:17Z

Yes, the tests have suffix _UnrelatedElementType.

In reply to: 2259350562

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:487 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T09:43:17Z

These conversions are already tested in Conversion_Span_ReadOnlySpan_Implicit_NullableAnalysis.

In reply to: 2259368438

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2305 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-01T09:47:23Z

public void Conversion_Span_ReadOnlySpan_Implicit(

The _Implicit refers to the fact that this is the "implicit span conversion", as opposed to the "explicit span conversion" which exists only from array to (ReadOnly)Span if there is explicit conversion between the element types. Even with the cast, this test is only testing the implicit conversion (at least per the spec this is not an "explicit conversion").

I agree this is confusing, and I thought about removing this _Implicit from most tests but at this point it would be too many changes which seems unnecessary.

In reply to: 2259344500

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:327 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

cston · 2024-08-01T19:37:11Z

                public static implicit operator ReadOnlySpan<string>(ReadOnlySpan<T> span) => default;

Added a comment here. We should discuss at LDM whether to allow user-defined conversions, if not already discussed.

In reply to: 2262345108

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2320 in d9e4b71. [](commit_id = d9e4b71, deletion_comment = False)

jjonescz · 2024-08-02T18:37:51Z

@333fred for another look, thanks

jjonescz added the Feature - First-class Span Types label Jul 17, 2024

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 17, 2024

Implement remaining span conversions

d751f5f

jjonescz force-pushed the FirstClassSpan-10-MoreConversions branch from f7577ec to d751f5f Compare July 18, 2024 13:11

jjonescz marked this pull request as ready for review July 18, 2024 14:39

jjonescz requested a review from a team as a code owner July 18, 2024 14:39

jjonescz requested review from 333fred and cston July 18, 2024 14:39

333fred reviewed Jul 18, 2024

View reviewed changes

jjonescz added 2 commits July 19, 2024 09:58

Simplify assert

658fb39

Test more Span UDCs

e6c5386

jjonescz requested a review from 333fred July 22, 2024 08:46

Only ignore UDCs exactly matching compiler-defined conversions

d9e4b71

333fred approved these changes Jul 24, 2024

View reviewed changes