From a3f21b41c31c5bcea261f81383e35f70fa1014eb Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Fri, 12 Jun 2026 12:38:36 -0700 Subject: [PATCH 1/2] Drop unused `IEquatable` from range wrappers and fix `FoldingReference` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had silenced CA1067 (types implementing `IEquatable` 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` (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` 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> --- .editorconfig | 2 +- .../Extensions/EditorFileRanges.cs | 24 ++----- .../Services/TextDocument/FoldingReference.cs | 14 +++- .../Language/FoldingReferenceEqualityTests.cs | 71 +++++++++++++++++++ 4 files changed, 89 insertions(+), 22 deletions(-) create mode 100644 test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs diff --git a/.editorconfig b/.editorconfig index 0786fbe27..3512b5932 100644 --- a/.editorconfig +++ b/.editorconfig @@ -48,7 +48,7 @@ dotnet_diagnostic.CS1998.severity = suggestion dotnet_diagnostic.CS4014.severity = suggestion # CA1067: Should override Equals because it implements IEquatable -dotnet_diagnostic.CA1067.severity = silent +dotnet_diagnostic.CA1067.severity = error # CA1068: CancellationToken parameters must come last dotnet_diagnostic.CA1068.severity = error # CA1501: Avoid excessive inheritance diff --git a/src/PowerShellEditorServices/Extensions/EditorFileRanges.cs b/src/PowerShellEditorServices/Extensions/EditorFileRanges.cs index 901307a73..0c3d72e59 100644 --- a/src/PowerShellEditorServices/Extensions/EditorFileRanges.cs +++ b/src/PowerShellEditorServices/Extensions/EditorFileRanges.cs @@ -274,7 +274,7 @@ public interface ILspCurrentFileContext : IFileContext ILspFileRange SelectionRange { get; } } - internal readonly struct OmnisharpLspPosition : ILspFilePosition, IEquatable + internal readonly struct OmnisharpLspPosition : ILspFilePosition { private readonly Position _position; @@ -283,11 +283,9 @@ public interface ILspCurrentFileContext : IFileContext public int Line => _position.Line; public int Character => _position.Character; - - public bool Equals(OmnisharpLspPosition other) => _position == other._position; } - internal readonly struct OmnisharpLspRange : ILspFileRange, IEquatable + internal readonly struct OmnisharpLspRange : ILspFileRange { private readonly Range _range; @@ -296,11 +294,9 @@ public interface ILspCurrentFileContext : IFileContext public ILspFilePosition Start => new OmnisharpLspPosition(_range.Start); public ILspFilePosition End => new OmnisharpLspPosition(_range.End); - - public bool Equals(OmnisharpLspRange other) => _range == other._range; } - internal readonly struct BufferFilePosition : IFilePosition, IEquatable + internal readonly struct BufferFilePosition : IFilePosition { private readonly BufferPosition _position; @@ -309,15 +305,9 @@ public interface ILspCurrentFileContext : IFileContext public int Line => _position.Line; public int Column => _position.Column; - - public bool Equals(BufferFilePosition other) - { - return _position == other._position - || _position.Equals(other._position); - } } - internal readonly struct BufferFileRange : IFileRange, IEquatable + internal readonly struct BufferFileRange : IFileRange { private readonly BufferRange _range; @@ -326,12 +316,6 @@ public bool Equals(BufferFilePosition other) public IFilePosition Start => new BufferFilePosition(_range.Start); public IFilePosition End => new BufferFilePosition(_range.End); - - public bool Equals(BufferFileRange other) - { - return _range == other._range - || _range.Equals(other._range); - } } /// diff --git a/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs b/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs index cd45ec1e8..16c74cd8e 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs @@ -42,6 +42,9 @@ internal class FoldingReference : IComparable, IEquatable public int CompareTo(FoldingReference that) { + // A null instance sorts before (is less than) any actual reference. + if (that is null) { return 1; } + // Initially look at the start line if (StartLine < that.StartLine) { return -1; } if (StartLine > that.StartLine) { return 1; } @@ -73,7 +76,16 @@ public int CompareTo(FoldingReference that) return -1; } - public bool Equals(FoldingReference other) => CompareTo(other) == 0; + public bool Equals(FoldingReference other) => other is not null && CompareTo(other) == 0; + + public override bool Equals(object obj) => Equals(obj as FoldingReference); + + public override int GetHashCode() => + StartLine.GetHashCode() + ^ StartCharacter.GetHashCode() + ^ EndLine.GetHashCode() + ^ EndCharacter.GetHashCode() + ^ (Kind?.GetHashCode() ?? 0); } /// diff --git a/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs b/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs new file mode 100644 index 000000000..03c12edff --- /dev/null +++ b/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.PowerShell.EditorServices.Services.TextDocument; +using OmniSharp.Extensions.LanguageServer.Protocol.Models; +using Xunit; + +namespace PowerShellEditorServices.Test.Language +{ + public class FoldingReferenceEqualityTests + { + private static FoldingReference CreateFoldingReference() => new() + { + StartLine = 1, + StartCharacter = 2, + EndLine = 3, + EndCharacter = 4, + Kind = FoldingRangeKind.Region + }; + + [Fact] + public void EqualsNullReferenceReturnsFalse() + { + FoldingReference reference = CreateFoldingReference(); + FoldingReference other = null; + // Previously this threw a NullReferenceException via CompareTo. + Assert.False(reference.Equals(other)); + } + + [Fact] + public void EqualsNullObjectReturnsFalse() + { + FoldingReference reference = CreateFoldingReference(); + Assert.False(reference.Equals((object)null)); + } + + [Fact] + public void CompareToNullReturnsOne() + { + FoldingReference reference = CreateFoldingReference(); + // A null instance sorts before any actual reference. + Assert.Equal(1, reference.CompareTo(null)); + } + + [Fact] + public void EqualReferencesAreEqualWithSameHashCode() + { + FoldingReference first = CreateFoldingReference(); + FoldingReference second = CreateFoldingReference(); + Assert.True(first.Equals(second)); + Assert.True(first.Equals((object)second)); + Assert.Equal(first.GetHashCode(), second.GetHashCode()); + } + + [Fact] + public void DifferentReferencesAreNotEqual() + { + FoldingReference first = CreateFoldingReference(); + FoldingReference second = CreateFoldingReference(); + second.EndLine = 42; + Assert.False(first.Equals(second)); + } + + [Fact] + public void EqualsDifferentTypeReturnsFalse() + { + FoldingReference reference = CreateFoldingReference(); + Assert.False(reference.Equals("not a folding reference")); + } + } +} From 5186901135f273ee2b992fd4675c9c7c5146c6c8 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Fri, 12 Jun 2026 12:58:54 -0700 Subject: [PATCH 2/2] Fix `FoldingReference.CompareTo` antisymmetry for differing kinds 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> --- .../Services/TextDocument/FoldingReference.cs | 26 +++++++++---------- .../Language/FoldingReferenceEqualityTests.cs | 17 ++++++++++++ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs b/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs index 16c74cd8e..308d67a73 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs @@ -60,20 +60,18 @@ public int CompareTo(FoldingReference that) if (EndCharacter < that.EndCharacter) { return -1; } if (EndCharacter > that.EndCharacter) { return 1; } - // They're the same range, but what about kind - if (Kind == that.Kind) - { - return 0; - } - - // That has a kind but this doesn't. - if (Kind is null && that.Kind is not null) - { - return 1; - } - - // This has a kind but that doesn't. - return -1; + // They're the same range, so now consider the kind. Equal kinds + // (including both null) compare as equal. + if (Kind == that.Kind) { return 0; } + + // A reference without a kind sorts after one that has a kind. + if (Kind is null) { return 1; } + if (that.Kind is null) { return -1; } + + // Both have a kind but they differ: order deterministically by their + // string representation so the comparison stays antisymmetric (i.e. + // a.CompareTo(b) and b.CompareTo(a) have opposite signs). + return string.CompareOrdinal(Kind.Value.ToString(), that.Kind.Value.ToString()); } public bool Equals(FoldingReference other) => other is not null && CompareTo(other) == 0; diff --git a/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs b/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs index 03c12edff..04fe0a1da 100644 --- a/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs +++ b/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs @@ -67,5 +67,22 @@ public void EqualsDifferentTypeReturnsFalse() FoldingReference reference = CreateFoldingReference(); Assert.False(reference.Equals("not a folding reference")); } + + [Fact] + public void CompareToWithDifferingKindsIsAntisymmetric() + { + FoldingReference comment = CreateFoldingReference(); + comment.Kind = FoldingRangeKind.Comment; + FoldingReference region = CreateFoldingReference(); + region.Kind = FoldingRangeKind.Region; + + // Same range but different (non-null) kinds must order + // deterministically: the comparisons must have opposite signs so + // Array.Sort stays stable. Previously both returned -1. + int forward = comment.CompareTo(region); + int backward = region.CompareTo(comment); + Assert.NotEqual(0, forward); + Assert.Equal(-System.Math.Sign(forward), System.Math.Sign(backward)); + } } }