Runtime: [API Change] Extension AsSpan(this string text) -> AsReadOnlySpan(this string text)

[benaadams]

From dotnet/runtime#21121

Extension change to match other ReadOnlySpan return methods

public static ReadOnlySpan<char> AsSpan(this string text);

to

public static ReadOnlySpan<char> AsReadOnlySpan(this string text);

[ahsonkhan]

cc @shiftylogic, @KrzysztofCwalina, @terrajobst - api-ready-for-review

Context, from https://github.com/dotnet/corefx/issues/18420#issuecomment-303443892:

From @jswolf19:
I've started work and noticed the API public static ReadOnlySpan<char> AsSpan(this string text).
I wonder if this naming might become confusing with the addition of the AsReadOnlySpan APIs.

From @ahsonkhan:
Good point. One option could be to rename AsSpan(this string text) to AsReadOnlySpan.

However, since the proposed APIs are extension methods on specific types, if you have a string, you won't see any AsReadOnlySpan extension method for it in IntelliSense, so it may not necessarily cause confusion.

With the additions from this API proposal, we get:
C# public static ReadOnlySpan<char> AsSpan(this string text); public static Span<T> AsSpan<T>(this T[] array); public static Span<T> AsSpan<T>(this ArraySegment<T> arraySegment); public static ReadOnlySpan<T> AsReadOnlySpan<T>(this T[] array); public static ReadOnlySpan<T> AsReadOnlySpan<T>(this ArraySegment<T> segment);

byte[] byteArray;
byteArray. [Options are AsSpan and AsReadOnlySpan]

string str;
str. [Only AsSpan is visible, which will return ReadOnlySpan<char> since string is immutable]

From @jswolf19:
I agree from a discoverability standpoint that AsSpan is the more intuitive name, as making the mental leap from string → immutable → ReadOnlySpan instead of Span while likely not hard for someone who would likely be using Span is not immediate in my mind.

From a readability standpoint, though I imagine
var span = str.AsReadOnlySpan();
might be preferable to
var span = str.AsSpan();
for mentally resolving type, especially in a context like source control where IntelliSense may not be available.
However, I also imagine that, for many use cases, differentiating Span from ReadOnlySpan may not be necessary (except of course when it is ^^). Just thought I'd bring it up.

From @ektrah:
+1 for changing public static ReadOnlySpan<char> AsSpan(this string text); to public static ReadOnlySpan<char> AsReadOnlySpan(this string text); if public static ReadOnlySpan<T> AsReadOnlySpan<T>(this T[] array); is added. It's confusing if AsSpan on strings is inconsistent with every other use of AsSpan and if there is a AsReadOnlySpan method which can be used in every case except for strings.

[weshaggard]

Api Review: Approved

• After decision the reviewers agreed we should be consistent and use AsReadOnlySpan for any methods that return a ReadOnlySpan.

[ahsonkhan]

@karelz, adding up-for-grabs

This API change is fairly easy to implement. It is a method rename (along with it's uses in tests/etc) and shouldn't take more than 1-2 hours (including testing).

[adriangodong]

First time contributor, I can take this.

[karelz]

@adriangodong cool! Collaborator invite sent. Just remember to turn off repo notifications (300+ per day), otherwise you will be DoS'd. Let me know when you accept, so that we can assign this issue to you.
Temporarily assigning to myself ...

Also be warned that Changing public APIs is a bit involved and not yet documented ideally.
You can find docs in wiki (also linked from repo README.MD). Please help us improve them (it is RW for everyone): https://github.com/dotnet/corefx/wiki/New-contributor-Docs#contributing-guide

[adriangodong]

@karelz Invite accepted

[stephentoub]

@ahsonkhan, the method in corelib is still named AsSpan:
https://github.com/dotnet/coreclr/blob/8a5e061576e8ff7e6505059302460dcdd2f463b5/src/mscorlib/shared/System/Span.NonGeneric.cs#L121
Should this issue be re-opened to track updating that as well, or is that tracked elsewhere?

[ahsonkhan]

@stephentoub, Thanks for catching that! I have re-opened this issue.

@adriangodong, would you like to tackle the rename in corelib as well?
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Span.NonGeneric.cs#L121

[adriangodong]

@ahsonkhan Sure, I'll take a look at that class.

[jkotas]

Remains to be done:

• Change references to AsSpan to AsReadOnlySpan in CoreFX repo (once the next Update CoreClr to ... goes into CoreFX repo)
• Delete the obsolete method from CoreCLR CoreLib

[adriangodong]

AsSpan -> AsReadOnlySpan in CoreFX is done (https://github.com/dotnet/corefx/pull/23563/commits/f4b098d4170e74e84d1530f4a40d821fe0f97cda).

@ahsonkhan @jkotas I have a question about the following test class:
https://github.com/dotnet/corefx/blob/f4b098d4170e74e84d1530f4a40d821fe0f97cda/src/System.Memory/tests/ReadOnlySpan/AsSpan.cs

I can't seem to use System.Span.AsReadOnlySpan extension from coreclr. What stops us from doing this and drop System.SpanExtensions.AsReadOnlySpan from corefx?

[adriangodong]

nvm, rtfm https://github.com/dotnet/coreclr#relationship-with-the-corefx-repository