Skip to content

Fix assertion in RangeIterator to handle types of T, not int#123901

Closed
vcsjones wants to merge 2 commits into
dotnet:mainfrom
vcsjones:fix-debug-assert-range
Closed

Fix assertion in RangeIterator to handle types of T, not int#123901
vcsjones wants to merge 2 commits into
dotnet:mainfrom
vcsjones:fix-debug-assert-range

Conversation

@vcsjones

@vcsjones vcsjones commented Feb 2, 2026

Copy link
Copy Markdown
Member

RangeIterator had a Debug.Assert in it assuming that the range cannot be larger than int.MaxValue. For Range, this is true as Range is constrained to int.

However, Sequence will try to use the RangeIterator when its step is 1 for any type of INumber.

static RangeIterator<T>? TryUseRange<TLarger>(T start, T endInclusive, T step, TLarger maxValue) where TLarger : INumber<TLarger>
{
if (step == T.One &&
TLarger.CreateTruncating(endInclusive) - TLarger.CreateTruncating(start) + TLarger.One <= maxValue)
{
return new RangeIterator<T>(start, endInclusive + T.One);
}
return null;
}

In debug builds of the runtime, a sequence with more the int.MaxValue items would fail in the int.CreateChecked.

In a release build of the runtime, this works because the entire Debug.Assert expression is erased.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI 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.

Pull request overview

This PR fixes a Debug.Assert in RangeIterator<T> that incorrectly assumed all ranges would fit within int.MaxValue. The assertion was failing in debug builds when Sequence used RangeIterator for numeric types larger than int with step size of 1.

Changes:

  • Fixed the assertion to use generic type comparison instead of converting to int
  • Added test to validate sequences with size larger than int.MaxValue work correctly

Reviewed changes

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

File Description
src/libraries/System.Linq/src/System/Linq/Range.cs Fixed Debug.Assert to compare using T.Zero instead of converting to int
src/libraries/System.Linq/tests/SequenceTests.cs Added regression test for sequences larger than int.MaxValue

@stephentoub

Copy link
Copy Markdown
Member

🤖 Copilot Code Review — PR #123901

Holistic Assessment

Motivation: The problem is real — the old assertion used int.CreateChecked which throws OverflowException in debug builds when RangeIterator<T> is used with a range larger than int.MaxValue (e.g., for long sequences via Enumerable.Sequence).

Approach: The fix changes the assertion to compare directly in type T, which is correct for generic numeric types. However, the old assertion was inadvertently acting as a guard against a deeper issue: RangeIterator<T> implements IList<T> with Count returning int, but TryUseRange in Sequence.cs allows ranges up to long.MaxValue/ulong.MaxValue for those types.

Summary: ⚠️ Needs Human Review. The assertion fix itself is correct, but it removes an implicit safeguard that was catching a pre-existing bug in TryUseRange. A human reviewer should decide whether this PR should also address the underlying TryUseRange issue or whether that should be a follow-up.


Detailed Findings

⚠️ Pre-existing Bug Exposed — TryUseRange allows ranges larger than int.MaxValue

Flagged by: Gemini, GPT (confirmed by independent analysis)

The TryUseRange method in Sequence.cs (lines 84-94) passes type-specific max values (e.g., long.MaxValue, ulong.MaxValue) as the limit, but RangeIterator<T>.Count (line 40 of Range.SpeedOpt.cs) uses int.CreateTruncating:

csharp public int Count => int.CreateTruncating(_endExclusive - _start);

For a sequence like Enumerable.Sequence(0L, 2L + int.MaxValue, 1L):

  • TryUseRange returns a RangeIterator<long> (because 2L + int.MaxValue < long.MaxValue)
  • Count would truncate to a small/negative value
  • ToArray(), ToList(), Count(), etc. would all behave incorrectly

The old int.CreateChecked assertion was implicitly catching this in debug builds by throwing. This PR removes that guard.

Recommendation: Either:

  1. Accept this PR and file a follow-up issue to fix TryUseRange to limit all paths to int.MaxValue
  2. Or include the TryUseRange fix in this PR by changing lines 91-94 to pass int.MaxValue instead of the type's max value

💡 Assertion Clarity — Consider endExclusive >= start instead

Flagged by: Opus, GPT

The new assertion endExclusive - start >= T.Zero has a subtle issue for unsigned types: if endExclusive < start (violating the precondition), unsigned subtraction would underflow to a large positive value, causing the assertion to incorrectly pass.

While callers validate this precondition, a clearer assertion would be:
csharp Debug.Assert(endExclusive >= start);

This directly expresses the invariant and works correctly for all numeric types.


💡 Test Coverage — Consider validating Count or LongCount

Flagged by: Opus, GPT, Gemini

The test only calls First(), which verifies the assertion doesn't throw but doesn't validate the iterator's correctness for the large range scenario. If the TryUseRange bug is not fixed, this test would pass while the sequence has incorrect behavior for other operations.

Consider either:

  • Extending the test to verify LongCount() returns the expected value, or
  • Adjusting the test to verify that sequences larger than int.MaxValue fall back to IncrementingIterator (not RangeIterator)

Cross-Cutting Analysis

  • Sibling types: The issue is specific to long, ulong, nint, nuint where type max > int.MaxValue
  • Callers: Enumerable.Range(int, int) is unaffected (constrained to int). Only Enumerable.Sequence<T> is affected.
  • Test quality: The new test is a reasonable regression test for the assertion fix but doesn't cover the exposed TryUseRange issue

Models contributing to this review: Claude Sonnet (primary), Claude Opus, Gemini 3 Pro, GPT-5.2

@jeffhandley jeffhandley added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 8, 2026
@jeffhandley

Copy link
Copy Markdown
Member

@vcsjones the Copilot review posted above looks to have some actionable feedback related to the test failures that Build Analysis shows as well.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

eiriktsarpalis
eiriktsarpalis previously approved these changes Mar 23, 2026
@eiriktsarpalis eiriktsarpalis dismissed their stale review March 23, 2026 11:34

Changes look good, however as @jeffhandley has pointed out the new test is failing in a number of CI legs.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@github-actions github-actions Bot locked and limited conversation to collaborators May 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Linq needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants