Remove ThrowHelper from Microsoft.Extensions.Primitives#130416
Open
svick wants to merge 4 commits into
Open
Conversation
Replace the local ThrowArgumentOutOfRangeException helper with the BCL ArgumentOutOfRangeException.ThrowIf* APIs. The hot-path single unsigned comparison is kept as a guard, delegating the throw to cold out-of-line helpers that re-check the signed value so the exception reports the actual argument (e.g. -1) with an enriched message instead of the unsigned wrap-around. Adds ThrowIfGreaterThan/ThrowIfGreaterThanOrEqual polyfills for downlevel target frameworks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace manual 'if (x < 0) throw' checks with ArgumentOutOfRangeException.ThrowIf* and remove the now-unnecessary ThrowValidationError fast-path helper. Perf is not a concern for this obsolete type, so validation runs unconditionally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @dotnet/area-extensions-primitives |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the internal ThrowHelper pattern from Microsoft.Extensions.Primitives, replacing it with BCL argument-validation helpers (ArgumentNullException.ThrowIfNull, ArgumentOutOfRangeException.ThrowIf*) and direct throw sites, while adding downlevel support via ExceptionPolyfills.
Changes:
- Delete
ThrowHelper.cs(and its enums) and update call sites to use BCLThrowIf*helpers / direct throws. - Update
StringSegmentto use new cold out-of-line throw helpers (keeping unsigned-comparison fast paths). - Extend
ExceptionPolyfillswithThrowIfGreaterThan/ThrowIfGreaterThanOrEqual, and adjust tests for updated exceptionParamName.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs | Updates expected ArgumentOutOfRangeException.ParamName to match new throw helper behavior. |
| src/libraries/Microsoft.Extensions.Primitives/src/ThrowHelper.cs | Deleted legacy throw-helper and supporting enums/resources indirection. |
| src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs | Replaces null-check ThrowHelper usage with ArgumentNullException.ThrowIfNull. |
| src/libraries/Microsoft.Extensions.Primitives/src/StringTokenizer.cs | Replaces ThrowHelper usage with ThrowIfNull / direct ArgumentNullException. |
| src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs | Introduces cold throw helpers to preserve inlining/fast-path checks while modernizing validation. |
| src/libraries/Microsoft.Extensions.Primitives/src/InplaceStringBuilder.cs | Simplifies validation/throws to modern helpers and direct throws. |
| src/libraries/Microsoft.Extensions.Primitives/src/CompositeChangeToken.cs | Replaces ThrowHelper null-check with ArgumentNullException.ThrowIfNull. |
| src/libraries/Microsoft.Extensions.Primitives/src/ChangeToken.cs | Replaces ThrowHelper validation and InvalidOperation throws with direct throws/resources. |
| src/libraries/Common/src/System/ExceptionPolyfills.cs | Adds downlevel polyfills for ArgumentOutOfRangeException.ThrowIfGreaterThan*. |
Comment on lines
+85
to
91
| ArgumentNullException.ThrowIfNull(value); | ||
| ArgumentOutOfRangeException.ThrowIfLessThan(offset, 0); | ||
| ArgumentOutOfRangeException.ThrowIfGreaterThan(count, value.Length - offset, nameof(offset)); | ||
| if (Capacity - _offset < count) | ||
| { | ||
| ThrowValidationError(value, offset, count); | ||
| throw new InvalidOperationException(SR.Format(SR.Capacity_NotEnough, value.Length, Capacity - _offset)); | ||
| } |
Member
Author
|
One more thing: I think |
Comment on lines
+88
to
92
| ArgumentOutOfRangeException.ThrowIfGreaterThan(count, value.Length - offset, nameof(offset)); | ||
| if (Capacity - _offset < count) | ||
| { | ||
| ThrowValidationError(value, offset, count); | ||
| throw new InvalidOperationException(SR.Format(SR.Capacity_NotEnough, value.Length, Capacity - _offset)); | ||
| } |
mrek-msft
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes
ThrowHelperfromMicrosoft.Extensions.Primitives, replacing it with the BCL argument-validation throw helpers (ArgumentNullException.ThrowIfNull,ArgumentOutOfRangeException.ThrowIf*)and direct
throwstatements. On downlevel target frameworks these are provided by theshared
ExceptionPolyfills.The new patterns are more succinct and more modern and should be just as efficient.
The perf consideration is that a method containing the
throwopcode is less likely to be inlined. This means the check (e.g.if (parameter is null)) should stay on the hot path, but the actual throwing should be done in another method (like the oldThrowHelper.Throw*Exception, an internal method in the new pattern orStringSegment.Throw*). Hiding the check in yet another method (likeArgumentNullException.ThrowIfNull) shouldn't hurt, because that method should be easily inlined.Changes
ThrowHelper.csalong with itsExceptionArgument/ExceptionResourceenums, and removes some per-type local throw helpers (InplaceStringBuilder'sThrowValidationError).ArgumentNullException.ThrowIfNull.ArgumentOutOfRangeException.ThrowIf*(e.g.ThrowIfLessThan). The caller's argument name is usually captured viaCallerArgumentExpression.InvalidOperationExceptionsites become directthrow new ...using the existingSRresources. These shouldn't be on the hot path where thethrowopcode would made a difference.ExceptionPolyfillsgainsThrowIfGreaterThan/ThrowIfGreaterThanOrEqual(matching the realArgumentOutOfRangeExceptionAPI surface) so the above works on the netstandard2.0 / net462 targets, where the real BCL methods aren't available.StringSegmentpreserves its single-comparison fast path; the obsoleteInplaceStringBuilder, where performance is mostly irrelevant, is simplified to straightforward validation.Note
This pull request description was drafted with the assistance of GitHub Copilot.