-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix CharSearchValuesPolyfill._ascii array size from 8 to 4 elements #122668
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: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
|
@copilot please add a simple test |
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…amework Remove test from System.Memory.Tests (doesn't use polyfill) and add SearchValuesPolyfillTests.cs to System.Collections.Immutable.Tests which targets both .NET Core and .NET Framework, ensuring the polyfill is actually tested on downlevel targets. Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
src/libraries/System.Collections.Immutable/tests/SearchValuesPolyfillTests.cs
Outdated
Show resolved
Hide resolved
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 PR fixes a bug in CharSearchValuesPolyfill._ascii where the array was incorrectly sized as 8 elements instead of 4. ASCII characters (0-127) use bitmap lookup with offset c >> 5, which yields indices 0-3, requiring only 4 array elements. The oversized array caused non-ASCII characters (with offsets 4-7) to incorrectly pass bounds checks instead of falling through to NonAsciiContains(), resulting in incorrect Contains() results.
Key changes:
- Corrected
_asciiarray size fromuint[8]touint[4]in SearchValuesPolyfills.cs - Added regression tests in System.Collections.Immutable.Tests covering boundary conditions (chars 0, 127, 128, 65535)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/Common/src/System/Buffers/SearchValuesPolyfills.cs | Fixes array size from 8 to 4 elements to match ASCII bitmap algorithm requirements |
| src/libraries/System.Collections.Immutable/tests/SearchValuesPolyfillTests.cs | Adds new test class with theory-based tests for char boundary conditions to verify the fix |
| src/libraries/System.Collections.Immutable/tests/System.Collections.Immutable.Tests.csproj | Registers new test file in the project |
Description
CharSearchValuesPolyfill._asciiwas sized asuint[8]instead ofuint[4]. ASCII characters (0-127) use offsetc >> 5for bitmap lookup, yielding offsets 0-3. With 8 elements, non-ASCII characters (offset 4-7) incorrectly passed the bounds check instead of falling through toNonAsciiContains.Customer Impact
Polyfill users on .NET Standard 2.0 or .NET Framework targets get incorrect
Contains()results for non-ASCII characters. Example: creatingSearchValues.Create([char(200)])and checkingContains(char(200))returns false instead of true.Regression
No. This is a pre-existing bug in the polyfill code.
Testing
Built System.Text.Json for netstandard2.0 (uses polyfill). Added regression tests in
System.Collections.Immutable.Testswhich targets both .NET Core and .NET Framework (net481) withIncludeSpanPolyfills=trueenabled. Tests verify non-ASCII character lookup (char 200), mixed ASCII/non-ASCII characters, and boundary conditions (chars 127/128). All 22,551 tests pass with 0 failures.Risk
Low. One-line change corrects array size to match algorithm requirements. Polyfill only active for downlevel targets (.NET Standard 2.0, .NET Framework). Main .NET runtime uses separate SearchValues implementation unaffected by this change.
Original prompt
CharSearchValuesPolyfill._asciishould be 128 bits length #122667✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.