StringSegment: more AsSpan overloads

[gfoidl]

Fixes #50428

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #50428

Author:	gfoidl
Assignees:	-
Labels:	area-Extensions-Primitives, new-api-needs-documentation
Milestone:	-

[xtqqczze]

Suggested change

	if (!HasValue || (start | length) < 0 || (uint)(start + length) > (uint)Length)
	if (!HasValue || start < 0 || (uint)(start + length) > (uint)Length)

MemoryExtensions.AsSpan(this string text, int start, int length) throws when length is negative, therefore this check is redundant.

[gfoidl]

Good catch!

The difference is that MemoryExtensions.AsSpan throws AOR with paramName start only, whilst the check here distinguishes between start and length to be on par with SubSegment, and SubString.

So what to do 🤷‍♂️

[xtqqczze]

OK, I missed that paramName was always start in MemoryExtensions.AsSpan. This seems like a bug?

Span(T[] array, int start, int length) just avoids specifying the paramName, which is maybe the best approach?

[gfoidl]

@eerhardt do you think this is a bug in MemoryExtensions.AsSpan?
Or who is the right person to ask this?

[eerhardt]

@GrabYourPitchforks is the person I'd ask...

It feels like a bug to me. It definitely doesn't seem correct that MemoryExtensions.AsSpan and [ReadOnly]Span.Slice act differently here. I assume the Span.Slice behavior is "more correct" because you can't definitively tell which parameter is really at fault here without doing more checks which just adds code to a very hot path.

Maybe the best approach would be to open an issue describing the difference. I don't think we should try to solve it here.

[eerhardt]

also FYI @tarekgh. Tracing back to dotnet/corefx@a68803c, it appears Span.Slice used start before that commit, but during that commit Span got split into Fast and Portable. The Portable path still used start, but the Fast path dropped the parameter name.

[gfoidl]

Filed #53622 for this.

[GrabYourPitchforks]

The MemoryExtensions behavior is intentional, but I agree it's weird. We want the API to be as fast as possible, which means we don't have the luxury of figuring out which of start or length was incorrect. We could pass a nullptr argument name to the ArgumentOutOfRangeException ctor, but honestly nobody expects ArgumentException.ParamName to be nullptr. So always passing "start" seems like the best solution given these constraints. If it's really a problem in practice, we could always create a detailed error message, just for this one scenario.

[eerhardt]

We could pass a nullptr argument name to the ArgumentOutOfRangeException ctor, but honestly nobody expects ArgumentException.ParamName to be nullptr.

But it is OK for Span?

runtime/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs

Lines 350 to 359 in dbb05eb

	public ReadOnlySpan<T> Slice(int start, int length)
	{
	#if TARGET_64BIT
	// See comment in Span<T>.Slice for how this works.
	if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#else
	if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#endif

[eerhardt]

(I guess we can have this discussion in #53622 😄)

[eerhardt]

This looks good to me. Thanks for the contribution here @gfoidl!

[GrabYourPitchforks]

I'll be the problem child. :)

Nit: I think these parameters should be called offset and length to match the parameter names on the existing StringSegment.Subsegment and StringSegment.Substring instance methods. Symmetry with existing methods on the type is generally preferred over symmetry with the MemoryExtensions.AsSpan extension methods.

@bartonjs @terrajobst @eerhardt thoughts?

[bartonjs]

I think these parameters should be called offset and length

My recollection is that this came up in the meeting, and we decided that "AsSpan"-consistency was the way to go.

string.Substring is startIndex/length; string.AsSpan is start/length. So AsSpan-consistency matches what we did for String.

[eerhardt]

@GrabYourPitchforks - I'm satisfied with this PR and think it is ready to merge. Let me know if you have any objections.

[GrabYourPitchforks]

@eerhardt Go for it!