-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix IndexOfAnyInRange inconsistent behavior with invalid ranges across vectorized/scalar paths #123365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: EgorBo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a bug in IndexOfAnyInRange and related methods where they behave inconsistently when highInclusive < lowInclusive depending on whether hardware intrinsics are enabled. The fix adds early validation checks in the vectorized code paths to handle invalid ranges consistently.
Changes:
- Added early validation checks in
NonPackedIndexOfAnyInRangeUnsignedNumberandLastIndexOfAnyInRangeUnsignedNumberto detect invalid ranges (wherehighInclusive < lowInclusive) - Added comprehensive tests covering invalid range scenarios for all affected APIs and data types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs | Added invalid range checks at the beginning of NonPackedIndexOfAnyInRangeUnsignedNumber and LastIndexOfAnyInRangeUnsignedNumber methods to return -1 for In-Range variants and first/last index for Except-Range variants |
| src/libraries/System.Memory/tests/Span/IndexOfAnyInRange.cs | Added two comprehensive test methods to verify invalid range behavior across all data types and vector sizes |
|
@copilot linux-arm32 failed with: linux-arm32 has no intrinsics so it executed the non-vectorized path. Please change default value of |
Co-authored-by: EgorBo <[email protected]>
... Fixed in commit b3053c3. Added invalid range checks to the IComparable-based methods ( |
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: stephentoub <[email protected]>
| } | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, please make this a [Theory] where the lowInclusive and highInclusive values are supplied as arguments. In addition to invoking it with values like 100 and 50 as you already have, please also test values that would flag off-by-one errors, like 100 and 99, as well as negative values if possible.
Also, please ensure there are existing tests that handle the case where lowInclusive == highInclusive. I believe they exist, but if they don't, please add some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 7c38104. Converted to [Theory] with 7 test cases including off-by-one errors (100, 99) and (50, 49), negative values (0, -1) and (-10, -20), and the original issue case (200, 100). The existing tests at line 133 and 138 already cover the lowInclusive == highInclusive case.
| // When highInclusive < lowInclusive, the range is invalid and all values are outside it. | ||
| if (lowInclusive.CompareTo(highInclusive) > 0) | ||
| { | ||
| return span.Length > 0 ? 0 : -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, nit:
| return span.Length > 0 ? 0 : -1; | |
| return span.IsEmpty ? -1 : 0; |
and similarly using IsEmpty in the other change where this pattern shows up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 8390cb3. Changed to use span.IsEmpty instead of span.Length > 0 in both IndexOfAnyExceptInRange and LastIndexOfAnyExceptInRange.
Co-authored-by: stephentoub <[email protected]>
Head branch was pushed to by a user without write access
Co-authored-by: stephentoub <[email protected]>
Description
IndexOfAnyInRangeand related APIs return inconsistent results for invalid ranges (wherehighInclusive < lowInclusive) depending on whether hardware intrinsics are enabled. The vectorized path computeshighInclusive - lowInclusive, which wraps to a large positive value for unsigned types, causing incorrect matches.Changes
MemoryExtensions.cs: Added early validation in the 4 public entry points to detect
highInclusive < lowInclusivebefore any path selection:IndexOfAnyInRange<T>- Returns -1 (no value matches invalid range)IndexOfAnyExceptInRange<T>- Returns -1 for empty spans, 0 for non-empty spans (all values outside invalid range)LastIndexOfAnyInRange<T>- Returns -1 (no value matches invalid range)LastIndexOfAnyExceptInRange<T>- Returns -1 for empty spans, span.Length - 1 for non-empty spans (all values outside invalid range)All checks use
lowInclusive.CompareTo(highInclusive) > 0to work correctly with all IComparable types, and usespan.IsEmptyfor better readability. This approach centralizes validation at the public API boundary, eliminating the need for duplicate checks across 6 internal helper methods.IndexOfAnyInRange.cs: Added comprehensive tests covering invalid ranges for all supported types (byte, sbyte, short, ushort, char, int, uint, long, ulong, nint, nuint, TimeSpan) across both vectorized and scalar code paths:
InvalidRange_HighLessThanLow_ReturnsMinus1- [Theory] with 7 test cases including off-by-one errors (100, 99) and (50, 49), negative values (0, -1) and (-10, -20), and various invalid rangesInvalidRange_SpecificValue_DoesNotMatch- Tests the exact issue scenario from the bug reportlowInclusive == highInclusivecaseTesting
Tests validate consistent behavior across input lengths 1-64, covering both intrinsic-enabled (vectorized) and intrinsic-disabled (scalar) scenarios. The Theory-based test approach provides comprehensive edge case coverage including off-by-one errors and negative value handling. All tests now pass on platforms with and without hardware intrinsics support.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.