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..308d67a73 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; } @@ -57,23 +60,30 @@ 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; - } + // They're the same range, so now consider the kind. Equal kinds + // (including both null) compare as equal. + 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; - } + // 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; } - // This has a kind but that doesn't. - 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) => 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..04fe0a1da --- /dev/null +++ b/test/PowerShellEditorServices.Test/Language/FoldingReferenceEqualityTests.cs @@ -0,0 +1,88 @@ +// 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")); + } + + [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)); + } + } +}