Fix IdSpan equality violation: Create("") and default now correctly unequal#9902
Conversation
Co-authored-by: ReubenBond <203839+ReubenBond@users.noreply.github.com>
| public int CompareTo(IdSpan other) => _value.AsSpan().SequenceCompareTo(other._value.AsSpan()); | ||
| public int CompareTo(IdSpan other) | ||
| { | ||
| // Handle null cases explicitly |
There was a problem hiding this comment.
Such overly verbose comments add no value.
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in IdSpan by ensuring default(IdSpan) (null backing array) is no longer considered equal to IdSpan.Create("") (empty backing array), which previously violated the Equals/GetHashCode contract.
Changes:
- Update
IdSpan.Equals(IdSpan)to treatnulland empty arrays as unequal. - Update
IdSpan.CompareTo(IdSpan)to provide consistent ordering when either side has anullbacking array. - Add
IdSpanTestscovering equality, hash code consistency, and comparison edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Orleans.Core.Abstractions/IDs/IdSpan.cs |
Fixes equality and comparison behavior for null vs empty backing arrays. |
test/NonSilo.Tests/IdSpanTests.cs |
Adds regression tests for the equality/hash-code contract and comparison behavior. |
|
|
||
| Assert.True(emptyStringIdSpan.IsDefault); | ||
| Assert.Equal("", emptyStringIdSpan.ToString()); | ||
| // Hash code should be computed from empty byte array, not 0 | ||
| Assert.NotEqual(0, emptyStringIdSpan.GetHashCode()); |
There was a problem hiding this comment.
This assertion bakes in an implementation detail that the stable hash of an empty byte array is non-zero. To make the test resilient to future hash algorithm changes while still validating the contract, consider asserting that IdSpan.Create(string.Empty) has a different hash code than default(IdSpan) (and/or that a.Equals(b) implies equal hash codes), rather than asserting != 0 specifically.
| Assert.True(emptyStringIdSpan.IsDefault); | |
| Assert.Equal("", emptyStringIdSpan.ToString()); | |
| // Hash code should be computed from empty byte array, not 0 | |
| Assert.NotEqual(0, emptyStringIdSpan.GetHashCode()); | |
| IdSpan defaultIdSpan = default; | |
| Assert.True(emptyStringIdSpan.IsDefault); | |
| Assert.Equal("", emptyStringIdSpan.ToString()); | |
| // Hash code should differ from default(IdSpan) while still representing an "empty" value | |
| Assert.NotEqual(defaultIdSpan.GetHashCode(), emptyStringIdSpan.GetHashCode()); |
| public bool Equals(IdSpan obj) | ||
| { | ||
| if (_value == obj._value) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (_value is null || obj._value is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return _value.AsSpan().SequenceEqual(obj._value); | ||
| } |
There was a problem hiding this comment.
IdSpanCodec currently serializes based on value.AsSpan(). Since AsSpan() returns an empty span for both _value == null and _value.Length == 0, an IdSpan created from "" (empty byte[]) will be serialized the same as default and will deserialize back to default. With the new equality semantics (null != empty), that means serialization no longer round-trips the value. Consider updating IdSpanCodec to encode empty arrays distinctly from null/default (and add a regression test for that).
…nequal (dotnet#9902) * Initial plan * Fix IdSpan equality and comparison to distinguish null from empty arrays Co-authored-by: ReubenBond <203839+ReubenBond@users.noreply.github.com> * Address PR feedback: remove verbose comments and merge null checks --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ReubenBond <203839+ReubenBond@users.noreply.github.com> Co-authored-by: Reuben Bond <reuben.bond@gmail.com>
IdSpan.Create(string.Empty)anddefault(IdSpan)were equal despite having different hash codes, violating the fundamental contract that equal objects must have equal hash codes.Root cause:
EqualsusedSpan<byte>.SequenceEqualwhich treatsnull.AsSpan()andArray.Empty<byte>().AsSpan()as equal.Example of the bug:
Changes
IdSpan.Equals: Added explicit null checks before SequenceEqual comparison. Two IdSpans are equal only if both have null
_value, both have the same reference, or both have non-null arrays with identical byte sequences.IdSpan.CompareTo: Added null handling to maintain consistent ordering:
null < empty array < non-empty arrays.Tests: Added IdSpanTests.cs with coverage for equality, hash code consistency, and comparison edge cases.
Note:
IdSpan.IsDefaultintentionally returns true for both null and empty arrays (logical emptiness), but they remain unequal (different internal representation).Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Microsoft Reviewers: Open in CodeFlow