Drop unused IEquatable from range wrappers and fix FoldingReference#2302
Open
andyleejordan wants to merge 2 commits into
Open
Drop unused IEquatable from range wrappers and fix FoldingReference#2302andyleejordan wants to merge 2 commits into
IEquatable from range wrappers and fix FoldingReference#2302andyleejordan wants to merge 2 commits into
Conversation
We had silenced CA1067 (types implementing `IEquatable<T>` without overriding `object.Equals`/`GetHashCode`) instead of fixing it. Promote it to `error` and resolve the offenders at the root. The four wrapper structs in `EditorFileRanges.cs` — `OmnisharpLspPosition`, `OmnisharpLspRange`, `BufferFilePosition`, and `BufferFileRange` — are `internal`, only ever surfaced through their interfaces (`ILspFilePosition`/`IFileRange`/etc.), and never compared anywhere. Their `IEquatable<T>` (added reflexively in #1183) was dead code, and the parallel public classes (`FilePosition`/`LspFilePosition`/...) never implemented it. So rather than completing the contract with `Equals(object)`/`GetHashCode`/ operators, I dropped `IEquatable<T>` and the typed `Equals` entirely — less code, and CA1067 no longer applies to them. OmniSharp can't reach them either: they're `internal` (unnameable for any generic equality), `InternalsVisibleTo` lists only first-party assemblies, and the `ToOmnisharp*` conversions hand back OmniSharp's own types, so an instance never flows back into the library. `FoldingReference` is the one type that genuinely participates in comparison (`Array.Sort` via `SafeAdd`, and xUnit `Assert.Equal` in the folding tests), so it keeps `IEquatable`/`IComparable` and gets the real fix: - `Equals(FoldingReference other)` was `=> CompareTo(other) == 0`, and `CompareTo` dereferences its argument, so `Equals(null)` and `CompareTo(null)` threw a `NullReferenceException` instead of returning `false`/`1`. Both are now null-safe. - Added `Equals(object)` and a netstandard2.0-compatible `GetHashCode` (manual XOR, since `System.HashCode` is unavailable on that target). Added focused tests covering the `FoldingReference` null-safety, value equality, and hash-code consistency. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens equality semantics and analyzer enforcement by removing unused IEquatable<T> implementations from internal “range wrapper” structs, fixing FoldingReference’s null-unsafe equality/comparison behavior, and promoting CA1067 to an error to prevent regressions.
Changes:
- Removed
IEquatable<T>and typedEquals(T)from internal range-wrapper structs inEditorFileRanges.csto eliminate CA1067 obligations where equality isn’t used. - Made
FoldingReferenceequality/comparison null-safe and addedEquals(object)/GetHashCode()to satisfy CA1067. - Elevated
dotnet_diagnostic.CA1067.severityfromsilenttoerrorand added dedicated equality tests forFoldingReference.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs | Adds unit coverage for FoldingReference null-safety, equality, and hash-code behavior. |
| src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs | Fixes null handling in CompareTo/Equals, and implements Equals(object) + GetHashCode. |
| src/PowerShellEditorServices/Extensions/EditorFileRanges.cs | Removes unused IEquatable<T> implementations from internal range wrapper structs. |
| .editorconfig | Promotes CA1067 to error to prevent future equality-contract regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Copilot PR reviewer noticed that when two references share the same range but have different non-null `Kind`s, `CompareTo` fell through to `return -1` for both `a.CompareTo(b)` and `b.CompareTo(a)`. That violates the `IComparable` antisymmetry contract and can yield unstable ordering in `Array.Sort`. We now order differing non-null kinds deterministically with `string.CompareOrdinal` over their string representations (stable LSP values like `comment` and `region`), so the two directions always have opposite signs. The null-vs-kind cases are unchanged: a reference without a kind still sorts after one that has a kind. Added a test asserting the comparison is antisymmetric for differing kinds. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
What
Resolves the long-silenced CA1067 equality-contract debt, fixes a latent
NullReferenceExceptioninFoldingReference, and promotes CA1067 toerrorso it can't regress.Details
OmnisharpLspPosition,OmnisharpLspRange,BufferFilePosition,BufferFileRangeinEditorFileRanges.cs): these areinternal, only ever surfaced through their interfaces (ILspFilePosition/IFileRange/etc.), and never compared anywhere — so theirIEquatable<T>(added in Close over public APIs not designed for exposure #1183) was dead code. The parallel public classes (FilePosition/LspFilePosition/…) never implemented it. Rather than completing the contract withEquals(object)/GetHashCode/operators, this dropsIEquatable<T>and the typedEqualsat the root (net code deletion), which removes the CA1067 obligation entirely. OmniSharp can't reach these types either — they'reinternal(unnameable for any generic equality),InternalsVisibleTolists only first-party assemblies, and theToOmnisharp*conversions hand back OmniSharp's own types — so nothing relied on the equality.FoldingReferenceis the one type that genuinely participates in comparison (Array.SortviaSafeAdd, and xUnitAssert.Equalin the folding tests), so it keepsIEquatable/IComparableand gets the real fix:Equals(FoldingReference other)was=> CompareTo(other) == 0, andCompareTodereferences its argument, soEquals(null)/CompareTo(null)threw aNullReferenceExceptioninstead of returningfalse/1. Both are now null-safe.Equals(object)and a netstandard2.0-compatibleGetHashCode(manual XOR, sinceSystem.HashCodeis unavailable on that target)..editorconfig:CA1067promoted fromsilenttoerror.Tests
New
FoldingReferenceEqualityTests(6 tests): null-safety, value equality, hash-code consistency, and different-type handling. The main library builds clean (0 warnings / 0 errors) with CA1067 enforced, and 13/13 relevant net8.0 tests pass (6 new + 7TokenOperationsTests).🤖 Drafted by Copilot (Claude Opus 4.8) on behalf of @andyleejordan.