Skip to content

Unify Span/Memory slicing exceptions #115275

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented May 3, 2025

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.

@MihaZupan MihaZupan added this to the 10.0.0 milestone May 3, 2025
@MihaZupan MihaZupan self-assigned this May 3, 2025
@Copilot Copilot AI review requested due to automatic review settings May 3, 2025 18:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

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

if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
ThrowHelper.ThrowArgumentOutOfRangeException();
#endif
MemoryExtensions.ValidateSliceArguments(_length, start, length);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

MihaZupan commented May 3, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants