Fix CompareInfo.IsPrefix/IsSuffix with CompareOptions.IgnoreSymbols not ignoring leading/trailing symbols - #132397
Conversation
…ding/trailing symbols CanIgnoreAllCollationElements walked raw ICU collation elements and required each to equal zero. Collation-element iteration (ucol_next) does not apply the collator's alternate handling (UCOL_SHIFTED), so a character made ignorable by CompareOptions.IgnoreSymbols still produces a non-zero raw element, causing ComplexStartsWith/EndsWith to reject an otherwise-matching affix whenever symbols were skipped at the boundary. Compare the skipped substring against an empty string via ucol_strcoll instead, which honors the collator's configured strength and alternate handling like every other comparison in this file. Fixes dotnet#118521
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-globalization |
|
@dotnet-policy-service agree |
1 similar comment
|
@dotnet-policy-service agree |
|
@EgorBot -windows_x64 -linux_x64 --filter "StringSearch.*" |
|
@EgorBot -windows_x64 -linux_x64 --filter "System.Globalization.Tests.StringSearch.*" |
|
This change is a nice improvement because it also removes an existing ICU-vs-NLS divergence, not just the Since Comparing the current runtime (ICU, pre-fix) against NLS on the same inputs with
So the fix moves ICU toward the NLS behavior in all of these cases. The I also checked that the change stays precise and doesn't over-ignore: a combining mark is still not ignorable under |
|
/ba-g the failures are not related |
|
The benchmarks not showing any regression either. We should be good to go. |
tarekgh
left a comment
There was a problem hiding this comment.
Thanks @HarnageaGabriel for providing the fix.
Fixes #118521
Root cause
ComplexStartsWith/ComplexEndsWithinpal_collation.cuse ICU string search to locate the requested affix, then callCanIgnoreAllCollationElementsto check whether any skipped text before/after the match is entirely ignorable under the activeCompareOptions.CanIgnoreAllCollationElementswalked the raw collation elements returned byucol_next(viaucol_openElements) and required each one to equalUCOL_IGNORABLE(0). Raw collation-element iteration does not apply the collator'sUCOL_ALTERNATE_HANDLINGsetting — so whenCompareOptions.IgnoreSymbolssetsUCOL_ALTERNATE_HANDLING = UCOL_SHIFTEDon the collator, a symbol character that should now be ignorable (e.g.') still produces a non-zero raw collation element. This madeCanIgnoreAllCollationElementsincorrectly report the skipped text as "not ignorable," so:returned
falseinstead oftrueon ICU (non-Windows NLS, non-Apple-hybrid) platforms — those other implementations already used a different code path that isn't affected by this quirk, which is why the existing test file already had a platform-conditional branch documenting this exact case as a known ICU-backend bug pointing at this issue.I verified this against a live ICU instance: raw
ucol_nextelements for symbol characters made ignorable viaIgnoreSymbolsremain non-zero, whileucol_strcollagainst those same characters correctly honors the collator's strength/alternate-handling and reports equality with an empty string.Fix
CanIgnoreAllCollationElementsnow compares the skipped substring against an empty string usingucol_strcoll(the same high-level comparison entry point already used elsewhere in this file, e.g.GlobalizationNative_CompareString), instead of manually iterating and masking raw collation elements. This honors the collator's configured strength, alternate handling (IgnoreSymbols), and locale tailoring uniformly, rather than re-implementing that logic against the low-level iterator API. A non-null emptyUCharbuffer is passed (rather thanNULL) to avoid a documented old-ICU null-input issue that this file already works around elsewhere (ICU-9396).This helper is shared by both
ComplexStartsWithandComplexEndsWith, so the fix resolves the symmetricIsSuffixcase as well (e.g."Tests''".EndsWith("Tests", CompareOptions.IgnoreSymbols)), for which I added a regression test.Changes
src/native/libs/System.Globalization.Native/pal_collation.c: replaced the raw collation-element loop inCanIgnoreAllCollationElementswith anucol_strcoll-against-empty-string check.src/libraries/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.IsPrefix.cs: removed the platform-conditional "known ICU bug" branch for''Tests/Testsand set the expected result totrue(matched length 7) unconditionally.src/libraries/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.IsSuffix.cs: added the symmetricTests''/Testsregression case.Validation
I don't have a full native ICU build/toolchain available in this environment, so I wasn't able to run the managed test suite against a compiled
libSystem.Globalization.Native. I did directly probe ICU's C collation API (usearch_first/usearch_last, rawucol_nextvsucol_strcoll) to confirm the root cause and the fix's behavior character-by-character for the affected cases. The change is narrowly scoped to the single shared helper function and mirrors an existing, already-used API call pattern in the same file.