Use more consistent argument validation in Immutable collection types - #127157

Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom
copilot/fix-range-checks-in-helpers
Draft

Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom
copilot/fix-range-checks-in-helpers

Conversation

CopilotAI commented Apr 20, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor
  • Identify merge conflict in ImmutableArray_1.cs
  • Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  • Resolve conflict by keeping our Requires.ValidateRange call
  • Commit the merge

CopilotAIand others added 14 commits April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI self-assigned this Apr 20, 2026
CopilotAI review requested due to automatic review settings April 20, 2026 13:26
CopilotAI removed the request for review from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan changed the title Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service

dotnet-policy-serviceBot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

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

@MihaZupan

MihaZupan commented Apr 20, 2026

Copy link
Copy Markdown
Member

Follow-up to #124967 to fix more places to handle argument validation more consistently.
E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index
or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.

cc: @prozolic

@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI requested review from MihaZupan and Copilot and removed request for CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan added this to the 11.0.0 milestone Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI requested review from Copilot and removed request for CopilotApril 20, 2026 16:33
@jkotas

jkotas commented Apr 20, 2026

Copy link
Copy Markdown
Member

Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?

@eiriktsarpalis

eiriktsarpalis commented Apr 20, 2026

Copy link
Copy Markdown
Member

Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?

Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026
Closed
Closed
Open
prozolic
prozolic reviewed Apr 21, 2026
View reviewed changes

@prozolicprozolic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.

Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs
Comment on lines 729 to +730
public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);

@prozolicprozolicApr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}

It's the same as what I commented on IndexOf.

Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs
Comment on lines 629 to +630
public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);

@prozolicprozolicApr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}

How about adjusting it to use _root.IndexOf instead of this.IndexOf?

Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
internal Node Sort(Comparison<T> comparison)
{
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);

@prozolicprozolicApr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?
If you are able to make the change, could you also adjust the other places as well?

@prozolic

prozolic commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?

At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review
1 more reviewer
@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@MihaZupanMihaZupan

Copilot code reviewCopilot

Labels

area-System.Collections

Projects

None yet

Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Use more consistent argument validation in Immutable collection types - #127157
Draft
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Draft
Use more consistent argument validation in Immutable collection types#127157
MihaZupan with Copilot wants to merge 17 commits into
mainfrom 
copilot/fix-range-checks-in-helpers
Conversation
CopilotAI
commented
Apr 20, 2026•

edited

Loading

Copy link


Copy Markdown

Contributor
  •  Identify merge conflict in ImmutableArray_1.cs
  •  Analyze both sides: our Requires.ValidateRange helper vs main's two-line overflow-safe check (both do the same validation)
  •  Resolve conflict by keeping our Requires.ValidateRange call
  •  Commit the merge
CopilotAIand others
added 14 commits
April 19, 2026 19:07
@MihaZupan
Fix range checks in System.Collections.Immutable helpers for consiste…
a32b943
…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>
@MihaZupan
Convert redundant Requires.Range to Debug.Assert in Node.BinarySearch
371d011
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ce7b2aae-5f0d-42b0-a9d0-7055fae4370b
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Convert redundant checks in Node.Sort and Node.Reverse to Debug.Assert
625bbda
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>
@MihaZupan
Add Requires.ValidateRange shared helper and normalize blank lines af…
5c1e0b0
…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>
@MihaZupan
Apply Requires.ValidateRange to ImmutableArray and ImmutableArray.Bui…
77cc8cc
…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>
@MihaZupan
Replace duplicated Requires.* with Debug.Assert in ImmutableList<T>.Node
06ce702
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1fe4a74d-5349-47fc-b3f6-9db3815d884d
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan
Move validation from Node to public API surface; convert Node checks …
ff4da43
…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>
@MihaZupan
Add blank lines after Requires blocks; update ValidateRange to use De…
e91d47a
…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>
@MihaZupan
Drop 'startIndex == 0 && count == 0' fast paths; add Requires.Validat…
0df4554
…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>
@MihaZupan
Replace separate arrayIndex checks with overflow-safe combined form; …
426c717
…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>
@MihaZupan
Remove extension method LastIndexOf fast path; add overflow and empty…
5092452
…-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>
@MihaZupan
Replace two-line CopyTo range checks with single overflow-safe check …
33730cc
…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>
@MihaZupan
Add CopyTo and IndexOf overflow validation tests for immutable collec…
4d21fe0
…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>
@MihaZupan
Remove unnecessary success-path CopyTo call from SortedSet builder test
383214f
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6843c2b4-a169-4ebc-a8f8-eea958164182
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
self-assigned this
Apr 20, 2026
CopilotAI
review requested due to automatic review settings
April 20, 2026 13:26
CopilotAI
removed the request for review
from CopilotApril 20, 2026 13:26
@MihaZupanMihaZupan
changed the title
Replace two-line CopyTo range checks with single overflow-safe check and fix wasteful IndexOf validationUse more consistent argument validation in Immutable collection typesApr 20, 2026
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 20, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-collections

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Follow-up to #124967 to fix more places to handle argument validation more consistently.

E.g. type.IndexOf(value, index: int.MaxValue, count: 5) should blame the index

or to handle overflow cases that would bypass the validation if both the index and count were individually in range, but negative when combined.
cc: @prozolic
@MihaZupan
Optimize intermediate IndexOf/LastIndexOf overloads to validate index…
1ddc14e
… 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>
CopilotAI
requested review from
MihaZupan and Copilot
and removed request for
CopilotApril 20, 2026 14:06
@MihaZupanMihaZupan
added this to the 11.0.0 milestone
Apr 20, 2026
@MihaZupan
Revert "Optimize intermediate IndexOf/LastIndexOf overloads to valida…
2717ba7
…te index directly"
This reverts commit 1ddc14e.
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 14:32
@MihaZupan
Merge remote-tracking branch 'origin/main' into copilot/fix-range-che…
3316976
…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>
CopilotAI
requested review from
Copilot
and removed request for
CopilotApril 20, 2026 16:33
@jkotas
jkotas
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
@eiriktsarpalis
eiriktsarpalis
commented
Apr 20, 2026

Copy link


Copy Markdown

Member
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
Good idea, sounds like something Copilot could complete easily.

This was referenced Apr 20, 2026

Closed


Closed


Open

prozolic
prozolic
reviewed
Apr 21, 2026
View reviewed changes
@prozolicprozolic
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I left a few comments, but overall I think the range checks are aligned, and it looks great.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

729
 to 
+730

 public int LastIndexOf(T item, int startIndex, int count) =>
_root.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
this.LastIndexOf(item, startIndex, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintLastIndexOf(Titem,intstartIndex,intcount){Requires.ValidateReverseRange(startIndex,count,this.Count,nameof(startIndex));return_root.LastIndexOf(item,startIndex,count,EqualityComparer<T>.Default);}
It's the same as what I commented on IndexOf.
Comment thread...ies/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Builder.cs

Comment on lines

629
 to 
+630

 public int IndexOf(T item, int index, int count) =>
_root.IndexOf(item, index, count, EqualityComparer<T>.Default);
this.IndexOf(item, index, count, EqualityComparer<T>.Default);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

publicintIndexOf(Titem,intindex,intcount){Requires.ValidateRange(index,count,this.Count);return_root.IndexOf(item,index,count,EqualityComparer<T>.Default);}
How about adjusting it to use _root.IndexOf instead of this.IndexOf?
Comment thread...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
 internal Node Sort(Comparison<T> comparison)
 {
Requires.NotNull(comparison, nameof(comparison));
Debug.Assert(comparison != null);
@prozolicprozolicApr 21, 2026

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you please align the null checks to is not instead of != ?

If you are able to make the change, could you also adjust the other places as well?
@prozolic
prozolic
commented
Apr 21, 2026

Copy link


Copy Markdown

Contributor
Should we get rid of the Requires helper and switch to what we use everywhere else (ArgumentNullException.ThrowIfNull, etc.)?
At least I think the Requires.NotNull usages can be replaced with ArgumentNullException.ThrowIfNull.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@MihaZupanMihaZupanAwaiting requested review from MihaZupan
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@prozolicprozolicprozolic left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.


Assignees

@MihaZupanMihaZupan
Copilot code reviewCopilot

Labels

area-System.Collections

Projects


None yet


Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@jkotas@eiriktsarpalis@prozolic