Use more consistent argument validation in Immutable collection types#127157
Draft
Use more consistent argument validation in Immutable collection types#127157
Conversation
…ncy and overflow handling Update range validation in Sort, GetRange, Reverse, CopyTo, BinarySearch, and FindIndex methods across ImmutableArray, ImmutableList, and their Builders to use the consistent pattern: - Requires.Range(index >= 0 && index <= this.Count, nameof(index)); - Requires.Range(count >= 0 && (uint)(index + count) <= (uint)this.Count, nameof(count)); This ensures: 1. The correct parameter is reported as out of range (index vs count) 2. Overflow is handled via unsigned cast comparison 3. All methods use a consistent validation pattern Add test coverage for range validation in all updated methods. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f96d676a-8dac-4803-a986-ffa3fa632c37 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Both public callers (ImmutableList<T> and ImmutableList<T>.Builder) already validate arguments before delegating to Node.Sort and Node.Reverse. The Node's own Requires.Range/NotNull calls are therefore unreachable and can be converted to Debug.Assert. For ImmutableList.Reverse(int, int), validation was previously only in the Node — added Requires.Range to the public method to maintain the public contract, then converted the Node's checks to Debug.Assert. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/38cf8f63-313a-4f03-b51a-7f322887754c Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…ter validation Introduce Requires.ValidateRange(index, count, listCount, indexParameterName) to consolidate the repeated index/count range validation pattern. Applied across ImmutableList, Builder, and Node, replacing 10 duplicate two-line Requires.Range pairs with single ValidateRange calls. Also normalize whitespace so every validation block is followed by a blank line before the method body. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c34e5da8-17ce-4dc8-b489-4316b2fce76b Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…lder Replace duplicate index+count range validation pairs with the shared ValidateRange helper in four locations: - ImmutableArray_1.cs IndexOf (startIndex, count) - ImmutableArray_1.cs Sort (index, count) - ImmutableArray_1.Builder.cs IndexOf (startIndex, count) - ImmutableArray_1.Builder.cs Sort (index, count) Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d4cf6216-8bd8-470d-becc-84c58ee08b67 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…to Debug.Assert Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3b4d1d4f-650f-475e-b3bf-7815be22bc4b Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…bug.Assert + uint cast Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3b4d1d4f-650f-475e-b3bf-7815be22bc4b Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…eReverseRange helper Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/781e992a-9b7f-4694-b96b-ad274867bc99 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…use single Range check for FindLastIndex(startIndex, match); remove Builder.LastIndexOf fast path Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c527b2ab-9167-426d-9181-4b2065835708 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…-list validation tests Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c527b2ab-9167-426d-9181-4b2065835708 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…and simplify IndexOf validation Replace separate Requires.Range(arrayIndex >= 0) + Requires.Range(array.Length >= arrayIndex + count) with combined Requires.Range(arrayIndex >= 0 && (uint)(arrayIndex + count) <= (uint)array.Length) in 13 CopyTo locations across immutable collection types. Also replace wasteful Requires.ValidateRange(index, this.Count - index, this.Count) in ImmutableList<T>.Builder.IndexOf(T, int) with direct Requires.Range((uint)index <= (uint)this.Count). Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…tions Add tests verifying that CopyTo with int.MaxValue arrayIndex correctly throws ArgumentOutOfRangeException for: - ImmutableDictionary/SortedDictionary (base test class) - ImmutableDictionary/SortedDictionary Builder (base test class) - ImmutableHashSet/SortedSet (base test class) - ImmutableHashSet Builder - ImmutableSortedSet Builder - ImmutableSortedDictionary Keys/Values (KeysOrValuesCollectionAccessor) - ImmutableList Builder IndexOf(T, int) with overflow index Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182 Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
MihaZupan
April 20, 2026 13:26
View session
MihaZupan
changed the title
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-collections |
Member
|
Follow-up to #124967 to fix more places to handle argument validation more consistently. cc: @prozolic |
… directly In ImmutableArray<T> and ImmutableArray<T>.Builder, intermediate overloads that compute count as Length/Count - startIndex were delegating to the full overload which called ValidateRange on both index and count. Since count derived from Length - index is guaranteed valid when index is valid, the count validation was wasteful. Extract private IndexOfCore/LastIndexOfCore helpers and have intermediate overloads validate just the index before calling them directly, matching the approach already used in ImmutableList<T>.Builder.IndexOf(T, int). Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/33415a3f-837b-4dd6-b39c-a9859f8736fe Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Copilot
AI
requested review from
MihaZupan and
Copilot
and removed request for
Copilot
April 20, 2026 14:06
…te index directly" This reverts commit 1ddc14e. Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…cks-in-helpers # Conflicts: # src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Member
|
Should we get rid of the |
Member
Good idea, sounds like something Copilot could complete easily. |
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.
ImmutableArray_1.csRequires.ValidateRangehelper vs main's two-line overflow-safe check (both do the same validation)Requires.ValidateRangecall