Fix missing upper-bound guard in LastIndexOf and FindLastIndex

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.

[prozolic]

fixes#70950

Add the missing index < Count validation to LastIndexOf and FindLastIndex on ImmutableList<T>.Node.cs.
Previously, passing index == Count did not throw ArgumentOutOfRangeException and instead returned an incorrect (off-by-one) result silently. This aligns the behavior with ImmutableArray.

Changes:

• LastIndexOf: Add upper-bound check to the Requires.Range validation (index >= 0 && index < this.Count)
• FindLastIndex(int, int, Predicate<T>): Add upper-bound check to the Requires.Range validation (startIndex >= 0 && startIndex < this.Count)
• Both: Return -1 when called with an empty range (starting index is 0 and count is 0)

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes an off-by-one/validation hole in ImmutableList<T> search APIs by adding missing upper-bound checks for LastIndexOf and FindLastIndex, aligning behavior with ImmutableArray<T> (notably throwing when index/startIndex == Count) and handling the empty-range case consistently.

Changes:

• Add index/startIndex < Count validation to ImmutableList<T>.Node.LastIndexOf and ImmutableList<T>.Node.FindLastIndex(startIndex, count, match).
• Return -1 for the empty-range case (startIndex/index == 0 && count == 0) instead of throwing.
• Expand test coverage for single-element lists, multiple matches, and the index/startIndex == Count failure mode.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Collections.Immutable/tests/IndexOfTests.cs	Extends shared LastIndexOf test helper to cover single-element cases and index == Count validation.
src/libraries/System.Collections.Immutable/tests/ImmutableListTestBase.cs	Adds FindLastIndex coverage for single/multi-element lists and multiple matches, including startIndex == Count throwing.
src/libraries/System.Collections.Immutable/tests/ImmutableListTest.cs	Adds targeted regression tests for multiple matches and consistency with ImmutableArray<T> throwing behavior.
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs	Implements the missing upper-bound guards and the empty-range -1 behavior in the underlying node search methods.

[stephentoub]

🤖 Copilot Code Review — PR #124161

Holistic Assessment

Motivation: This PR addresses a real bug where ImmutableList<T>.LastIndexOf and FindLastIndex returned incorrect (off-by-one) results when index == Count, silently accepting invalid input instead of throwing. The linked issue #70950 has clear reproduction and was acknowledged as a bug by maintainers.

Approach: The fix correctly adds the missing upper-bound validation (index < this.Count) and handles the empty-range edge case (index == 0 && count == 0) by returning -1 before validation. This matches the existing pattern in ImmutableArray<T> and aligns with List<T> behavior for non-empty collections.

Net positive: ✅ This is a net positive — it fixes a documented bug, aligns behavior with ImmutableArray, and adds comprehensive test coverage.

---

Detailed Findings

✅ Implementation Matches ImmutableArray Pattern

The implementation follows the exact pattern used in ImmutableArray_1.cs (lines 256-262) and ImmutableArray_1.Builder.cs (lines 891-896):

• Early return for startIndex == 0 && count == 0 returning -1
• Then Requires.Range(index >= 0 && index < this.Count, nameof(index))

This ensures consistency across the immutable collection types.

✅ FindLastIndex Also Correctly Fixed

The FindLastIndex(int startIndex, int count, Predicate<T> match) overload had the same bug and is fixed with the same pattern. The Requires.NotNull(match, ...) check is correctly kept before the early return, ensuring null arguments are still validated.

✅ Test Coverage is Comprehensive

The tests cover:

• Empty collections with index=0, count=0 → returns -1
• Single-element lists with valid and invalid indices
• Multiple-element lists with multiple matches
• index == Count → throws ArgumentOutOfRangeException
• Cross-validation with ImmutableArray behavior (LastIndexOfConsistentWithImmutableArray)
• Exhaustive iteration through index/count combinations matching List<T> behavior

💡 Minor Suggestion: Consider count == 0 with index > 0 behavior

The current implementation requires index < Count even when count == 0 (except for the special case index == 0 && count == 0). For example, LastIndexOf(item, index: 2, count: 0) on a 2-element list will throw.

List<T>.LastIndexOf with count == 0 returns -1 without throwing (for any valid index <= Count - 1). ImmutableArray has the same behavior as this PR (throws when index >= Length even if count == 0).

Since this matches ImmutableArray, the current behavior is correct for consistency. Just flagging that List<T> differs slightly here.

✅ Breaking Change is Documented and Justified

This is a bucket 2 breaking change (decreasing the range of accepted parameter values). However:

1. The previous behavior was buggy (returning incorrect indices)
2. The new behavior aligns with ImmutableArray and List<T> for the index >= Count case
3. Code relying on the previous behavior was likely already buggy

---

Summary

LGTM. The fix correctly addresses the off-by-one bug in LastIndexOf and FindLastIndex by adding the missing index < this.Count validation while handling the empty-range edge case. The implementation matches ImmutableArray's pattern exactly, and comprehensive tests verify both the throwing and correct-result behaviors.

[ViveliDuCh]

@prozolic Thanks for picking this up and for the thorough fix! The changes to both LastIndexOf and FindLastIndex look good to me — the added upper-bound guard on index/startIndex brings ImmutableList in line with ImmutableArray, and the tests cover the key scenarios well.