Fix dotnet/runtime#124968: IndexOf throws when startIndex == Length and count == 0

[github-actions]

AI Fix: ImmutableArray(T).IndexOf throws when startIndex == Length and count == 0

Issue: dotnet#124968
Root cause: The guard for startIndex in ImmutableArray(T).IndexOf was stricter than Array.IndexOf, causing an ArgumentOutOfRangeException when startIndex == Length and count == 0.
Fix: Loosened the guard to allow startIndex == Length when count == 0, matching Array.IndexOf. Added a test to verify correct behavior.

Self-review: Correctness ✅ | Tests ✅ | Breaking ✅

Note

This PR was generated by GitHub Copilot CLI (AI). Please review carefully.

Generated by Fix Issue Agent · ◷

[github-actions]

🔍 API Surface Review — PR #47

Public API changes detected: No (behavior change to existing API only)

Findings

✅ No New Public API Members

This PR modifies the behavior of ImmutableArray<T>.IndexOf(T, int, int, IEqualityComparer<T>?) to return -1 instead of throwing ArgumentOutOfRangeException when startIndex == Length and count == 0. No new types, methods, or properties are added, so no ref assembly update is required.

⚠️ ImmutableArray<T>.Builder.IndexOf Has the Same Bug

ImmutableArray_1.Builder.cs line 794 still uses the old pattern:

if (count == 0 && startIndex == 0)
{
    return -1;
}
Requires.Range(startIndex >= 0 && startIndex < this.Count, nameof(startIndex));

Calling builder.IndexOf(item, builder.Count, 0) will still throw. This should be fixed consistently.

⚠️ ImmutableArray<T>.LastIndexOf Has the Same Bug

ImmutableArray_1.cs line 254 still uses the old pattern:

if (startIndex == 0 && count == 0)
{
    return -1;
}
Requires.Range(startIndex >= 0 && startIndex < self.Length, nameof(startIndex));

For consistency with Array.LastIndexOf, consider applying a similar fix here.

⚠️ Early-Return Inconsistency in IndexOf

When count == 0 and startIndex is in the range [1, Length-1], the code falls through to validation and the loop rather than hitting the early return. This is functionally correct (the loop executes 0 iterations and returns -1), but the early-return path only special-cases startIndex == 0 and startIndex == Length. A simpler fix would be:

if (count == 0 && (uint)startIndex <= (uint)self.Length)
{
    return -1;
}

This would handle all valid count == 0 cases uniformly and keep the startIndex validation unchanged at startIndex < self.Length.

⚠️ Minor Behavioral Change in Exception Parameter Name

The startIndex validation was relaxed from startIndex < self.Length to startIndex <= self.Length. For count > 0 and startIndex == Length, this changes which parameter name appears in the ArgumentOutOfRangeException (from startIndex to count). Low impact but worth noting.

Verdict

WARNINGS ⚠️ — No new public API surface, so ref assembly and api-approved checks are not applicable. However, the same bug likely exists in Builder.IndexOf and LastIndexOf, and should be fixed together for consistency.

Generated by API Surface Review for issue #47 · ◷

[github-actions]

ImmutableArray(T).IndexOf Fix — Code Review

❌ Error: Builder.IndexOf has the same unfixed bug

ImmutableArray_1.Builder.cs:796-801 still uses the old pattern:

if (count == 0 && startIndex == 0) { return -1; }
Requires.Range(startIndex >= 0 && startIndex < this.Count, ...);

builder.IndexOf(item, builder.Count, 0) will throw ArgumentOutOfRangeException instead of returning -1. This is the exact same bug being fixed in the non-Builder path. Array.IndexOf accepts startIndex == array.Length when count == 0, so the Builder should too.

---

⚠️ Warning: Early return is now redundant (not wrong, but misleading)

After changing validation to startIndex <= self.Length, all count == 0 cases with valid startIndex ∈ [0, Length] pass validation and reach Array.IndexOf, which correctly returns -1. The explicit early return for startIndex == 0 || startIndex == self.Length is no longer needed for correctness — it's purely an optimization for two endpoints while silently relying on Array.IndexOf for intermediate values (e.g., IndexOf(item, 2, 0) on a 4-element array). Consider simplifying to just:

if (count == 0 && startIndex >= 0 && startIndex <= self.Length)
    return -1;

…or removing the early return entirely. Either way, add a comment explaining the intent.

---

⚠️ Warning: Test coverage is thin

The single [Fact] test only covers one scenario. Missing cases:

• Empty array: ImmutableArray<int>.Empty.IndexOf(1, 0, 0) — already worked but not regression-tested alongside this change.
• Intermediate startIndex: array.IndexOf(2, 2, 0) — exercises the non-early-return path with count == 0.
• Custom equality comparer: the manual loop path (lines 189-195) with count == 0 and startIndex == Length.
• Builder equivalent: builder.IndexOf(item, builder.Count, 0) — currently throws (see ❌ above).

---

💡 Suggestion: Use [Theory] instead of [Fact]

Per repo conventions, prefer [Theory] with [InlineData] over multiple duplicative [Fact] methods:

[Theory]
[InlineData(0, 0)]   // startIndex 0, count 0
[InlineData(4, 0)]   // startIndex == Length, count 0
[InlineData(2, 0)]   // intermediate startIndex, count 0
public void IndexOf_CountZero_ReturnsMinusOne(int startIndex, int count)
{
    var array = ImmutableArray.Create(1, 2, 3, 4);
    Assert.Equal(-1, array.IndexOf(2, startIndex, count));
}

---

💡 Suggestion: Missing blank line before [Fact] (style)

Line 2857-2858: no blank line between the closing brace of StructWithReferenceTypeField and the new [Fact] attribute. Other test methods in this file consistently have a blank separator line.

---

Summary

#	Severity	Finding
1	❌ Error	Builder.IndexOf has the identical bug — not fixed
2	⚠️ Warning	Early return handles only 2 of N valid count==0 startIndex values; rest depend on fall-through
3	⚠️ Warning	Test covers only one edge case; no empty-array, intermediate-index, comparer, or Builder tests
4	💡 Suggestion	Use [Theory]/[InlineData] per repo conventions
5	💡 Suggestion	Add blank line before test method

Generated by Code Review for issue #47 · ◷

[github-actions]

PR #47 — Review Synthesis

Summary of Specialist Reviews

Critical Error:

• ImmutableArray<T>.Builder.IndexOf still has the same bug as the main IndexOf method. The fix should be applied there as well for consistency and correctness.

Warnings:

• ImmutableArray<T>.LastIndexOf likely has the same bug; consider fixing it for parity with Array.LastIndexOf.
• The early return in IndexOf is now redundant; all valid count == 0 cases are handled by validation and the underlying call. Consider simplifying or removing the early return, and add a clarifying comment.
• Test coverage is thin: only one scenario is tested. Add cases for empty arrays, intermediate startIndex, custom comparers, and the Builder path.
• Prefer [Theory] with [InlineData] over multiple [Fact]s for test coverage, per repo conventions.
• Minor: Add a blank line before the new test method for style consistency.

API Surface:

• No new public API members; only behavior change. No ref assembly update needed.
• Exception parameter name may change in some cases (low impact).

Next Steps:

• Apply the fix to Builder.IndexOf (and consider LastIndexOf).
• Expand test coverage as above.
• Optionally, simplify the early return logic.

``

Note

This summary was generated by an AI review aggregator. Please review the full specialist comments for details and context.

Generated by Review Aggregator · ◷

[danmoseley]

Closing: superseded by pipeline improvements. 1 issue = 1 PR rule.