Skip to content

Commit a3f21b4

Browse files
Drop unused IEquatable from range wrappers and fix FoldingReference
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>
1 parent 40cf5e1 commit a3f21b4

4 files changed

Lines changed: 89 additions & 22 deletions

File tree

.editorconfig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ dotnet_diagnostic.CS1998.severity = suggestion
4848
dotnet_diagnostic.CS4014.severity = suggestion
4949

5050
# CA1067: Should override Equals because it implements IEquatable<T>
51-
dotnet_diagnostic.CA1067.severity = silent
51+
dotnet_diagnostic.CA1067.severity = error
5252
# CA1068: CancellationToken parameters must come last
5353
dotnet_diagnostic.CA1068.severity = error
5454
# CA1501: Avoid excessive inheritance

src/PowerShellEditorServices/Extensions/EditorFileRanges.cs

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ public interface ILspCurrentFileContext : IFileContext
274274
ILspFileRange SelectionRange { get; }
275275
}
276276

277-
internal readonly struct OmnisharpLspPosition : ILspFilePosition, IEquatable<OmnisharpLspPosition>
277+
internal readonly struct OmnisharpLspPosition : ILspFilePosition
278278
{
279279
private readonly Position _position;
280280

@@ -283,11 +283,9 @@ public interface ILspCurrentFileContext : IFileContext
283283
public int Line => _position.Line;
284284

285285
public int Character => _position.Character;
286-
287-
public bool Equals(OmnisharpLspPosition other) => _position == other._position;
288286
}
289287

290-
internal readonly struct OmnisharpLspRange : ILspFileRange, IEquatable<OmnisharpLspRange>
288+
internal readonly struct OmnisharpLspRange : ILspFileRange
291289
{
292290
private readonly Range _range;
293291

@@ -296,11 +294,9 @@ public interface ILspCurrentFileContext : IFileContext
296294
public ILspFilePosition Start => new OmnisharpLspPosition(_range.Start);
297295

298296
public ILspFilePosition End => new OmnisharpLspPosition(_range.End);
299-
300-
public bool Equals(OmnisharpLspRange other) => _range == other._range;
301297
}
302298

303-
internal readonly struct BufferFilePosition : IFilePosition, IEquatable<BufferFilePosition>
299+
internal readonly struct BufferFilePosition : IFilePosition
304300
{
305301
private readonly BufferPosition _position;
306302

@@ -309,15 +305,9 @@ public interface ILspCurrentFileContext : IFileContext
309305
public int Line => _position.Line;
310306

311307
public int Column => _position.Column;
312-
313-
public bool Equals(BufferFilePosition other)
314-
{
315-
return _position == other._position
316-
|| _position.Equals(other._position);
317-
}
318308
}
319309

320-
internal readonly struct BufferFileRange : IFileRange, IEquatable<BufferFileRange>
310+
internal readonly struct BufferFileRange : IFileRange
321311
{
322312
private readonly BufferRange _range;
323313

@@ -326,12 +316,6 @@ public bool Equals(BufferFilePosition other)
326316
public IFilePosition Start => new BufferFilePosition(_range.Start);
327317

328318
public IFilePosition End => new BufferFilePosition(_range.End);
329-
330-
public bool Equals(BufferFileRange other)
331-
{
332-
return _range == other._range
333-
|| _range.Equals(other._range);
334-
}
335319
}
336320

337321
/// <summary>

src/PowerShellEditorServices/Services/TextDocument/FoldingReference.cs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ internal class FoldingReference : IComparable<FoldingReference>, IEquatable<Fold
4242
/// </summary>
4343
public int CompareTo(FoldingReference that)
4444
{
45+
// A null instance sorts before (is less than) any actual reference.
46+
if (that is null) { return 1; }
47+
4548
// Initially look at the start line
4649
if (StartLine < that.StartLine) { return -1; }
4750
if (StartLine > that.StartLine) { return 1; }
@@ -73,7 +76,16 @@ public int CompareTo(FoldingReference that)
7376
return -1;
7477
}
7578

76-
public bool Equals(FoldingReference other) => CompareTo(other) == 0;
79+
public bool Equals(FoldingReference other) => other is not null && CompareTo(other) == 0;
80+
81+
public override bool Equals(object obj) => Equals(obj as FoldingReference);
82+
83+
public override int GetHashCode() =>
84+
StartLine.GetHashCode()
85+
^ StartCharacter.GetHashCode()
86+
^ EndLine.GetHashCode()
87+
^ EndCharacter.GetHashCode()
88+
^ (Kind?.GetHashCode() ?? 0);
7789
}
7890

7991
/// <summary>
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
using Microsoft.PowerShell.EditorServices.Services.TextDocument;
5+
using OmniSharp.Extensions.LanguageServer.Protocol.Models;
6+
using Xunit;
7+
8+
namespace PowerShellEditorServices.Test.Language
9+
{
10+
public class FoldingReferenceEqualityTests
11+
{
12+
private static FoldingReference CreateFoldingReference() => new()
13+
{
14+
StartLine = 1,
15+
StartCharacter = 2,
16+
EndLine = 3,
17+
EndCharacter = 4,
18+
Kind = FoldingRangeKind.Region
19+
};
20+
21+
[Fact]
22+
public void EqualsNullReferenceReturnsFalse()
23+
{
24+
FoldingReference reference = CreateFoldingReference();
25+
FoldingReference other = null;
26+
// Previously this threw a NullReferenceException via CompareTo.
27+
Assert.False(reference.Equals(other));
28+
}
29+
30+
[Fact]
31+
public void EqualsNullObjectReturnsFalse()
32+
{
33+
FoldingReference reference = CreateFoldingReference();
34+
Assert.False(reference.Equals((object)null));
35+
}
36+
37+
[Fact]
38+
public void CompareToNullReturnsOne()
39+
{
40+
FoldingReference reference = CreateFoldingReference();
41+
// A null instance sorts before any actual reference.
42+
Assert.Equal(1, reference.CompareTo(null));
43+
}
44+
45+
[Fact]
46+
public void EqualReferencesAreEqualWithSameHashCode()
47+
{
48+
FoldingReference first = CreateFoldingReference();
49+
FoldingReference second = CreateFoldingReference();
50+
Assert.True(first.Equals(second));
51+
Assert.True(first.Equals((object)second));
52+
Assert.Equal(first.GetHashCode(), second.GetHashCode());
53+
}
54+
55+
[Fact]
56+
public void DifferentReferencesAreNotEqual()
57+
{
58+
FoldingReference first = CreateFoldingReference();
59+
FoldingReference second = CreateFoldingReference();
60+
second.EndLine = 42;
61+
Assert.False(first.Equals(second));
62+
}
63+
64+
[Fact]
65+
public void EqualsDifferentTypeReturnsFalse()
66+
{
67+
FoldingReference reference = CreateFoldingReference();
68+
Assert.False(reference.Equals("not a folding reference"));
69+
}
70+
}
71+
}

0 commit comments

Comments
 (0)