Unify Span/Memory slicing exceptions

[MihaZupan]

Fixes #115239
Fixes #53622
Fixes #90939

We no longer report the "start" parameter name in these exceptions.

We could keeep it in the .Slice(int) overloads (no explicit length), but we were already inconsistent there too since Span doesn't report the parameter, but ReadOnlySpan does.

[Copilot]

Pull Request Overview

This PR unifies the exception behavior for Span/Memory slicing operations by removing parameter name disclosures and consolidating bounds checking into MemoryExtensions.ValidateSliceArguments.

• Replaces legacy bounds checking (with TARGET_64BIT conditionals) with calls to MemoryExtensions.ValidateSliceArguments or MemoryExtensions.SliceArgumentsAreValid.
• Updates multiple methods and tests to throw exceptions without including parameter name details.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
System/String.cs	Replaces custom bounds checking with MemoryExtensions.SliceArgumentsAreValid.
System/String.Manipulation.cs	Updates Substring slicing bounds check to use ValidateSliceArguments.
System/Span.cs	Replaces legacy conditional bounds checks with a unified ValidateSliceArguments call.
System/Runtime/InteropServices/MemoryMarshal.cs	Updates bounds check to use ValidateSliceArguments.
System/ReadOnlySpan.cs	Replaces conditional checks with a unified ValidateSliceArguments call.
System/ReadOnlyMemory.cs	Updates slicing bounds checking and exception throwing to the new unified approach.
System/Range.cs	Removes parameter name from ArgumentOutOfRange exception.
System/MemoryExtensions.cs	Adds SliceArgumentsAreValid and ValidateSliceArguments methods to encapsulate bounds checking.
System/Memory.cs	Replaces legacy bounds checks with ValidateSliceArguments calls.
System/ArraySegment.cs	Updates argument validation to use SliceArgumentsAreValid.
Tests	Adjust tests to expect exceptions without a parameter name.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

[jkotas]

This introduces two more levels of inlining. We may want to check that it is not regressing code quality.

[MihaZupan]

Total bytes of delta: -5703 (-0.01 % of base)

I did spot check a couple diffs from PMI MihuBot/runtime-utils#1079, not seeing anything egregious (not seeing a lot less inlining).

As expected there's a bunch of small size improvements from cold blocks sharing the same throw helper (just Throw() instead of Throw() and Throw(int)).
E.g. MemoryExtensions:Trim[int], GetRedactedPathAndQuery, Json.Serialization.Converters.Int128Converter:WriteCore

Most diffs are from string.Substring(int, int) getting inlined more.
E.g. ParseTimeZoneIds, EncodingTable:GetEncodings(), TimeZoneInfo:.ctor

And a handful of cases where a helper is no longer being inlined, e.g.
Http3Frame:TryReadIntegerPair

[MihaZupan]

StringSegment appears to have more correct validation here w.r.t. reporting parameter names and it's relying on ReadOnlySpan for it in a few places, but the ambiguous validation in span/memory doesn't seem to be reachable from it.

I'll have to tweak that one, unless we'd be fine with just throwing a less informative argument exception.
(it could also probably be made a bit cheaper since we're currently validating stuff twice in some cases)

Edit: Kept StringSegment behavior the same, just also using the more efficient checks.

[MihaZupan]

Any thoughts on this one @dotnet/area-system-memory?

[tannergooding]

It might be worth a discussion on if we should have some general-purpose public API for this, just due to how frequently it's used across all the types, the problems with getting the checks correct and efficient, etc.

Doing so would fit into the general premise of centralizing potentially dangerous code as well.

More ideally we could just use the idiomatic code without casts as well, and the JIT would do the right thing. Many of the scenarios have been fixed in the JIT, but require length to be known positive for it to be safe.

[tannergooding]

We've actually gotten several complaints over us not reporting the different reasons for ArgumentOutOfRangeException accurately.

Centralizing things is good and makes it easier to ensure consistency. However, I think we may want to have the cheap early check and then have a ThrowHelper.ThrowArgumentOutOfRangeExceptionForInvalidSlice(sourceLength, start, length) which goes and determines which input was actually bad so we can provide accurate failure info as the way to be consistent.

[MihaZupan]

Past discussions have leaned towards removing such checks instead (FWIW this'd also be my preference in this case): #115239 (comment), #53622 (comment).

Do you think it'd be worth extra cost on all call sites given we're talking about Span?

[tannergooding]

I don't think either of the arguments linked for removing are good here.

The overall UX of the code will always be more important than the perf and providing decent error messages is a huge part of that UX. We've gotten actual customers asking about this and while there may be some cases where there is ambiguities or multiple failures, the same is true of nearly all methods that validate multiple inputs. There are equally many if not more places where the error can be unambiguous or provide concrete info to one of the failure points. -- Notably it recently got raised on the TensorSpan types which were simply copying what Span was already doing. CC. @jeffhandley, @ericstj who were part of that discussion

The "cost" to do this also isn't in a hot path, it's in an explicitly slow/cold path. The "overhead" of doing the call is also minimal. You have up to 3 moves to get the sourceLength, start, length into their positions and then up to 3 moves to pass down the ExceptionArgument enum ids for building the names. All the actual logic to figure out which was bad would be in the throw helper. So we're not impacting any hot path and while there is some increase to the code output size (24 bytes is the upper end) for each place we validate, it results in improved UX. -- The cost could also be reduced by not passing down ExceptionArgument ids for the common cases, instead having an overload that defaults them to the typical value (as most of our APIs are consistent).

Centralizing the logic also makes it easier to tune and to fix cases like the misleading exception issue you raised. We should likely do simple iterative checks to determine which error to raise to maximize the ability to report actionable info:

1. Check each of sourceLength, start, and length is positive
2. Check start is less than sourceLength
3. Check length is less than or equal to sourceLength - start

If we do that, there would be no ambiguities. The only negative is if multiple inputs are bad you aren't told about all of them at once, but this is normal for .NET

[xtqqczze]

However, I think we may want to have the cheap early check and then have a ThrowHelper.ThrowArgumentOutOfRangeExceptionForInvalidSlice(sourceLength, start, length) which goes and determines which input was actually bad so we can provide accurate failure info as the way to be consistent.

An argument against this is we would have to pass the parameter names in order to report correctly, e.g. start could be start, startIndex, offset, desiredStartIndex, etc.

[tannergooding]

That shouldn't be an issue, especially in the face of the other considerations. There are ways to handle that as well if we had any concerns over the "cost"

Ultimately the UX for the user is the most important factor here and I lean strongly towards us removing such annotations being a big negative to the UX.

There are many other places (the majority of throwing APIs, by far) throughout the core libraries we explicitly pass such things down, because having the diagnostic information is important (and we've made explicit efforts over past releases to make it easier and more automatic to pass such state into the throw helpers).

[MihaZupan]

It's a bit late in 10 to deal with potential perf regressions from changing validation (#115275 (comment)). We can try again later