Compare semantic strings ordinally, matching their ordinal Equals - #277
Merged
Merged
Conversation
CompareTo and the <, <=, >, >= operators built on it delegated to string.CompareTo, a culture-sensitive linguistic comparison, while Equals and GetHashCode are ordinal. Sorting therefore varied with the running culture, and on Windows NLS two values differing only in punctuation compared equal while remaining unequal, so a SortedSet could discard one of them. SemanticPath's explicit IComparable<IPath>.CompareTo had the same defect and is fixed with it, so ordering agrees whichever interface a caller sorts through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XD56Kg65nsraeL7vTL9MgU
left.CompareTo(left) holds for any implementation, so it pinned nothing. Comparing against a second instance created from the same value pins the half of the contract this test is about: two Equals-equal values must also compare zero. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XD56Kg65nsraeL7vTL9MgU
|
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #276.
The bug
SemanticString<T>.CompareTo— both theobjectand theISemanticStringoverload — delegated tostring.CompareTo(string), a culture-sensitive linguistic comparison against the current thread culture.Equals/GetHashCodeare compiler-generated fromWeakStringand are ordinal. The<,<=,>,>=operators route throughCompareTo(object), so they inherited it too.Two consequences, both reachable through public API on every semantic string type:
"Zebra"vs"apple"compares+1linguistically and-1ordinally — a sign flip, so the same two values sort differently on different hosts.CompareTo == 0withoutEquals. On Windows NLS a hyphen carries minimal collation weight, so"Co-op"/"Coop"and"e-mail"/"email"compare equal while remaining unequal and differently hashed.SortedSet<T>andSortedDictionary<TKey,_>treat any two keys comparing0as duplicates, so one of two distinct, independently valid values — twoIbans, twoUuids — is silently discarded.The fix
string.CompareOrdinal(WeakString, …)in bothCompareTooverloads, per the issue's suggested fix, with a<remarks>recording why the ordering is ordinal.Semantics.Paths/SemanticPath.cscarried the same defect in its explicitIComparable<IPath>.CompareTo, which is the comparisonComparer<IPath>.Defaultactually uses when sorting a path collection. It is fixed in the same commit: leaving it would have meantpathA.CompareTo(pathB)ordering differently depending on which interface the caller went through, which is worse than either behaviour on its own.No public API change; only the ordering semantics of existing members.
Tests, and an honest note about what CI can prove
ktsu.SdksetsInvariantGlobalization=true(Sdk.props:639), so the test host runs with globalization off, wherestring.CompareTois already ordinal. The suite therefore cannot observe this bug at all — it is green before and after this change in the normal configuration. That does not make the bug theoretical:ktsu.Semanticsships as a library, and a consuming application sets its own globalization mode.So the regression was proved by re-running the same suite against the built test assembly with
System.Globalization.Invariantflipped tofalseandLANG=en_US.UTF-8:CompareTo_OrdersOrdinally,ComparisonOperators_OrderOrdinally,DefaultComparer_ForPathInterface_OrdersOrdinallyFour tests added:
CompareTo_OrdersOrdinally— both overloads, againststring.CompareOrdinalas the oracle.ComparisonOperators_OrderOrdinally— the four operators.CompareTo_IsZeroExactlyWhenEqual— the assertion the issue asks for, plus theSortedSetretention it protects. Regressive on Windows NLS only, since ICU does not collapse the punctuation pairs.DefaultComparer_ForPathInterface_OrdersOrdinally(PathSortingTests) — theIComparable<IPath>route.Release build of the full solution: 0 warnings, 0 errors.
One thing for you to decide
Since the suite passes with globalization enabled (row 4 above),
Semantics.Testcould set<InvariantGlobalization>false</InvariantGlobalization>and these tests would then genuinely guard the fix in CI rather than being green by construction. That is a change to a setting the org SDK sets deliberately, and it would affect all 1341 tests, so I have not made it here — but the measurement says it is safe today.🤖 Generated with Claude Code
https://claude.ai/code/session_01XD56Kg65nsraeL7vTL9MgU
Generated by Claude Code