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.

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

Add standard implicit array-to-Span conversions #73577

Merged

Conversation

jjonescz
Copy link
Member

Test plan: #73445

This adds only the first two conversions:

  • From any single-dimensional array_type with element type Ei to System.Span<Ei>
  • From any single-dimensional array_type with element type Ei to System.ReadOnlySpan<Ui>, provided that Ei is covariance-convertible to Ui

The others will be similar, so it will be easier to add them once these are fleshed out. Plus some of the other conversions will need new well-known members and I didn't want to blow up this PR unnecessarily with that.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 20, 2024
@jjonescz jjonescz changed the base branch from main to features/FirstClassSpan May 20, 2024 12:13
@jjonescz jjonescz force-pushed the FirstClassSpan-02-Conversion branch from 4412b6b to 4be5ec4 Compare May 20, 2024 14:12
@jjonescz jjonescz force-pushed the FirstClassSpan-02-Conversion branch 6 times, most recently from 8b98a9f to 67afe70 Compare May 21, 2024 09:47
@jjonescz jjonescz force-pushed the FirstClassSpan-02-Conversion branch from 67afe70 to a9556fa Compare May 21, 2024 11:03
@jjonescz jjonescz requested review from 333fred and cston May 21, 2024 12:30
@jjonescz jjonescz marked this pull request as ready for review May 21, 2024 12:30
@jjonescz jjonescz requested review from a team as code owners May 21, 2024 12:30
@@ -244,6 +244,7 @@ private static void AssertTrivialConversion(ConversionKind kind)
case ConversionKind.InterpolatedString:
case ConversionKind.InterpolatedStringHandler:
case ConversionKind.InlineArray:
case ConversionKind.ImplicitSpan:
Copy link
Member

@333fred 333fred May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is actually true? Won't we need to store the method symbol of the conversion on the conversion object? #Resolved

Copy link
Member Author

@jjonescz jjonescz May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem necessary, we can find the method during lowering. It's the same thing InlineArray conversion does. I think the difference between trivial conversions like ImplicitSpan/InlineArray and complex conversions that need a method symbol like user-defined conversions is that the latter need to do a more complex lookup to find the method symbol whereas the former just obtain a well-known member.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a matter of when we can find the method, the question is the impact to our public API; namely, will we expose the method symbol we look up in Conversion.MethodSymbol or not. I'm currently a bit torn; given that I think we will likely spec which methods the compiler calls for each conversion, it seems like we should include that method in the conversion. On the other hand, it may be that we need to have multiple methods (Span<string> -> ReadOnlySpan<object>, for example, will probably need to go to ReadOnlySpan<string> in between), and that dual method becomes more complicated to represent.

Copy link
Member Author

@jjonescz jjonescz Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the question is the impact to our public API

I see, that sounds like a question for API review.

The docs of Conversion.MethodSymbol currently state that it's set only for user-defined and method group conversions. And indeed, for example, InlineArray conversion also does not expose the MethodSymbol.

There are other places that seem to rely on the fact that Conversion.MethodSymbol is set only for user-defined and method group conversions, e.g., Binder.ReportDiagnosticsIfObsolete - we report obsolete diagnostics only for user-defined methods and not built-in conversions.

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs Outdated Show resolved Hide resolved
@jjonescz
Copy link
Member Author

using Microsoft.CodeAnalysis.CSharp.Syntax;

The .Syntax one yes, in Conversion_Array_Span_Implicit_SemanticModel, the two above no, thanks.


In reply to: 2127872044


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

@jjonescz
Copy link
Member Author

            public static void M(this string[] x) => Console.Write(" s" + x[0]);

You're right, this test is not very interesting then, I will remove it; thanks.


In reply to: 2128039093


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

@jjonescz
Copy link
Member Author

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

I don't know what would be the advantage of disallowing user-defined conversions; but I guess the advantage of allowing them is more flexibility for users. Anyway it probably should not matter as users usually do not define their own Span types?


In reply to: 2128021447


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

@jjonescz
Copy link
Member Author

jjonescz commented May 24, 2024

            ReadOnlySpan<U> F2(T[] x) => (ReadOnlySpan<U>)x;

Is this explicit conversion from T[] to ReadOnlySpan determined through the pre-existing conversion code for finding user-defined conversions

Yes.

If so, that seems inconsistent with the handling of implicit conversions.

I wouldn't say it's inconsistent - if the new standard implicit span conversion isn't found, we also proceed to searching for other implicit conversions, even user-defined.

Should all conversions from arrays to spans (implicit and explicit) be handled - and spec'ed - directly, rather than relying on user-defined conversions?

They likely could, but I don't know if they should. Do you think that would be better? It also does not look like we have any precedent for this, at least the implementation of other conversions seems to always fall through to other conversions if the one in question does not match.

Consider checking the conversion returned from the semantic model in the following cases

Adding test _SemanticModel_02.


In reply to: 2127973330


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

@jjonescz
Copy link
Member Author

jjonescz commented May 24, 2024

        // (5,37): error CS0121: The call is ambiguous between the following methods or properties: 'C.E(Span<int>)' and 'C.E(ReadOnlySpan<int>)'

Note: I have moved these _Overloads tests among the other OverloadResolution_ tests

But the implicit span conversions are only defined from expressions, not from type.

Yes. I would like to define them from type, but I guess that's impossible if we want to have them gated by LangVersion. Is that correct? Also they need to access well-known types (Span/ROS) which also feels like something that needs Compilation, i.e., can be done only for conversions from expressions. (I guess we could use SpecialType instead but then Span/ROS would need to come from the corlib, and wouldn't that make netfx unsupported since there Span/ROS comes from System.Memory.dll?)

Does the proposal need to be updated to state that, if that's the case?

Yes, I will do that, thanks.

Or is this because the implicit span conversion from Span to ReadOnlySpan has not been implemented yet, and this case is expected to work unambiguously once the conversion is implemented?

Looks like it won't work because as you said, that would require the conversion to be from type, but it's currently from expression.

Unless we made the span conversion a type conversion but one that requires a compilation. For example, at least when determining better conversion target, it seems we could access the compilation fine. But I assume there are other scenarios that could break. I don't actually fully understand yet why type conversions in general don't have access to compilation and what would break (it's on my list to investigate).

Or we can improve the betterness rules to handle this.


In reply to: 2128004491


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

@jjonescz jjonescz requested a review from cston May 28, 2024 17:10
@cston
Copy link
Member

cston commented May 28, 2024

        // (5,37): error CS0121: The call is ambiguous between the following methods or properties: 'C.E(Span<int>)' and 'C.E(ReadOnlySpan<int>)'

Yes. I would like to define them from type, but I guess that's impossible if we want to have them gated by LangVersion. Is that correct? Also they need to access well-known types (Span/ROS) which also feels like something that needs Compilation, i.e., can be done only for conversions from expressions.

That sounds correct to me.


In reply to: 2128975997


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

@cston
Copy link
Member

cston commented May 28, 2024

            public static void M(this ReadOnlySpan<string> x) => Console.Write(" o" + x[0]);

It looks like this overload is never applicable. #Closed


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

@cston
Copy link
Member

cston commented May 28, 2024

            ReadOnlySpan<U> F2(T[] x) => (ReadOnlySpan<U>)x;

It seems inconsistent to specify and implement the implicit conversions from arrays to spans but rely on an unspecified set of user-defined conversions for any additional explicit conversions from arrays to spans.

It also does not look like we have any precedent for this, at least the implementation of other conversions seems to always fall through to other conversions if the one in question does not match.

Are the other cases where the compiler specifies and implements the implicit conversions strictly between pairs of special types (where the API is presumably well-defined) or are there cases that involve well-known types (where the API is more flexible)? And if the former, are there user-defined conversions for those pairs of types?

My sense is we should explicitly specify (and implement) the implicit and explicit conversions between arrays and spans, and not fallback to any additional user-defined conversions on those types. This seems like something that should be decided by language design.

Please consider adding a PROTOTYPE comment to determine whether explicit conversions should be specified and implemented directly.


In reply to: 2128891824


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

@cston
Copy link
Member

cston commented May 29, 2024

}

Consider also testing:

var a = new string[] { "a" };
a.M();

static class C
{
    public static void M(this object[] x) => ...
    public static void M(this ReadOnlySpan<object> x) => ...
}
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2453 in 05ffb16. [](commit_id = 05ffb167e6c02d81950cafc006bc2660477a44fd, deletion_comment = False)

@cston
Copy link
Member

cston commented May 29, 2024

}

Consider also testing:

var a = new string[] { "a" };
C.M(a);
...

static class C
{
    public static void M(object[] x) => ...
    public static void M(ReadOnlySpan<object> x) => ...
}
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:2330 in 05ffb16. [](commit_id = 05ffb167e6c02d81950cafc006bc2660477a44fd, deletion_comment = False)

@jjonescz
Copy link
Member Author

@333fred for the second review, thanks

@jjonescz jjonescz requested a review from 333fred June 11, 2024 08:26
@@ -244,6 +244,7 @@ private static void AssertTrivialConversion(ConversionKind kind)
case ConversionKind.InterpolatedString:
case ConversionKind.InterpolatedStringHandler:
case ConversionKind.InlineArray:
case ConversionKind.ImplicitSpan:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a matter of when we can find the method, the question is the impact to our public API; namely, will we expose the method symbol we look up in Conversion.MethodSymbol or not. I'm currently a bit torn; given that I think we will likely spec which methods the compiler calls for each conversion, it seems like we should include that method in the conversion. On the other hand, it may be that we need to have multiple methods (Span<string> -> ReadOnlySpan<object>, for example, will probably need to go to ReadOnlySpan<string> in between), and that dual method becomes more complicated to represent.

}

[Fact]
public void Conversion_Array_Span_Implicit_NullableAnalysis()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly confused why any of these work without warning. Shouldn't this all be an invariant scenario? Or does it fall out of array covariance?

Copy link
Member Author

@jjonescz jjonescz Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, nullability warnings are a bit weird here because user-defined conversions are considered as well. So even if a span conversion would not exist (and hence produce a warning), an UDC might exist and the warning isn't produced. In a follow-up PR, UDCs will be ignored, so this should be cleared up. There's already a PROTOTYPE comment for ignoring UDCs.

Can we ignore the nullability tests here and return to them in the follow up PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just leave a quick prototype comment above one of these tests to revisit.

Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "arg").WithArguments("string?[]", "System.ReadOnlySpan<string>").WithLocation(9, 47),
// (10,47): warning CS8619: Nullability of reference types in value of type 'string?[]' doesn't match target type 'ReadOnlySpan<object>'.
// ReadOnlySpan<object> M6(string?[] arg) => arg;
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "arg").WithArguments("string?[]", "System.ReadOnlySpan<object>").WithLocation(10, 47),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to the warning on M7 here?

Copy link
Member Author

@jjonescz jjonescz Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, investigating this again now it seems wrong. It happens because we check that the conversion exists with ConversionsBase.IncludeNullability=true - and we find one - an implicit user defined conversion - but we started with implicit span conversion. Anyway, as above, this will be resolved when UDCs are ignored.

}

[Fact]
public void Conversion_Array_Span_Implicit_NullableAnalysis_NestedArrays()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm similarly confused here. Shouldn't we have warnings on M3? And shouldn't both versions have warnings on M5?

}

[Fact]
public void Conversion_Array_ReadOnlySpan_Implicit_NullableAnalysis_NestedArrays()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused again. I'd have expected warnings on M3 and M7 in both scenarios.

Copy link
Member Author

@jjonescz jjonescz Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in both scenarios

C# 12 scenario isn't changed, so that might be a pre-existing bug. Compare also with _NestedNullability tests - looks like nested arrays have relaxed nullability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've investigated more and there are pre-existing inconsistencies which I think are out of scope for this feature:

  • HasExplicitArrayConversion does not consider nullability of element types
  • HasImplicitConversionFromArray does consider nullability of element types

}

[Theory, MemberData(nameof(LangVersions))]
public void Conversion_Array_Span_Implicit_NullableAnalysis_NestedNullability(LanguageVersion langVersion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this one works as I expected.

@jjonescz jjonescz requested a review from 333fred June 12, 2024 12:04
}

[Fact]
public void Conversion_Array_Span_Implicit_Out()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there variants of this with ref returns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ref assignments / ref returns cannot have conversions. But I can add a test to demonstrate that.

@jjonescz jjonescz merged commit c24c886 into dotnet:features/FirstClassSpan Jun 13, 2024
25 of 28 checks passed
@jjonescz jjonescz deleted the FirstClassSpan-02-Conversion branch June 13, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - First-class Span Types untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants