Merge pull request #344 from marius-bughiu/chore/code-review/topk-alias-guard

fix(PartialSort): reject a TopK destination that overlaps its source

Author: marius-bughiu

CHANGELOG.md

@@ -27,6 +27,7 @@ All notable changes to Celerity are documented here. This project follows [Keep
 
 ### Fixed
 
+- `PartialSort.TopK` now throws `ArgumentException` when its `destination` overlaps its `source`, instead of silently returning a wrong answer and writing to the source it documents as untouched. Disjoint slices of one array are still accepted, matching `RadixSort` and `CountingSort`.
 - Eight documentation links pointed at anchors that do not exist: seven `CeleritySet` / `SwissSet` references in `docs/api/collections.md` and one in `CHANGELOG.md`. GitHub deletes `<`, `>` and `,` from a heading without substituting a separator, so `CeleritySet&lt;T, THasher&gt;` anchors as `#celeritysett-thasher`, not the `#celerityset-t-thasher` everyone writes. Closes [#339](https://github.com/marius-bughiu/Celerity/issues/339).
 
 ## [2.5.0] - 2026-08-02

docs/api/sorting.md

@@ -182,6 +182,8 @@ struct hashers follow. Nulls sort first under the natural order, matching `Compa
 | `TopK<T, TComparer>(ReadOnlySpan<T> source, Span<T> destination, TComparer comparer)` | The same under a custom order — pass a reversing comparer to take the *smallest*. |
 
 **Exceptions.** `ArgumentOutOfRangeException` when `count` is negative or greater than `keys.Length`.
+`ArgumentException` when a `TopK` `destination` shares storage with its `source`. As everywhere else
+in the package, only genuine overlap is rejected — two disjoint slices of one array are fine.
 
 **Properties worth relying on.**
 
@@ -195,7 +197,8 @@ struct hashers follow. Nulls sort first under the natural order, matching `Compa
   an `IComparer<T>`-typed helper and so boxes a `struct` comparer on every call. If you want
   introsort's constant factor over a whole span, call the BCL directly.
 - **Not stable**, in any form.
-- **`TopK` never writes to `source`** and allocates nothing — the destination *is* the heap.
+- **`TopK` never writes to `source`** and allocates nothing — the destination *is* the heap, which
+  is why a `destination` overlapping `source` is rejected rather than silently answered wrongly.
 
 ```csharp
 using Celerity.Sorting;

src/Celerity.Sorting/PartialSort.cs

@@ -130,6 +130,7 @@ public static void Sort<T, TComparer>(Span<T> keys, int count, TComparer compare
     /// <param name="source">The elements to scan. Not modified.</param>
     /// <param name="destination">Receives the top <c>destination.Length</c> elements; its length is <c>k</c>.</param>
     /// <returns>The number of elements written — <c>destination.Length</c>, or <c>source.Length</c> when the source is shorter.</returns>
+    /// <exception cref="ArgumentException"><paramref name="destination"/> shares storage with <paramref name="source"/>.</exception>
     public static int TopK<T>(ReadOnlySpan<T> source, Span<T> destination)
         where T : IComparable<T> =>
         TopK<T, ComparableComparer<T>>(source, destination, default);
@@ -148,9 +149,15 @@ public static int TopK<T>(ReadOnlySpan<T> source, Span<T> destination)
     /// <param name="destination">Receives the top <c>destination.Length</c> elements; its length is <c>k</c>.</param>
     /// <param name="comparer">The comparer defining the order.</param>
     /// <returns>The number of elements written — <c>destination.Length</c>, or <c>source.Length</c> when the source is shorter.</returns>
+    /// <exception cref="ArgumentException"><paramref name="destination"/> shares storage with <paramref name="source"/>.</exception>
     public static int TopK<T, TComparer>(ReadOnlySpan<T> source, Span<T> destination, TComparer comparer)
         where TComparer : struct, IComparer<T>
     {
+        // The destination *is* the heap, so an aliasing destination would rewrite the source the
+        // method promises not to touch and read back its own partial output — a wrong answer rather
+        // than a failure, which is the same reason the two other sorters reject overlapping buffers.
+        SortingGuard.RequireDistinctStorage(source, destination, nameof(destination));
+
         int k = destination.Length;
         if (k == 0)
         {

src/Celerity.Tests/Sorting/SortingArgumentValidationTests.cs

@@ -145,6 +145,30 @@ public void Sort_ShouldNotThrow_WhenThePayloadIsADisjointSliceOfTheSameArray()
         Assert.Equal([1, 2, 3], buffer.Take(3));
     }
 
+    [Fact]
+    public void TopK_ShouldThrow_WhenTheDestinationSharesStorageWithTheSource()
+    {
+        int[] buffer = [5, 1, 4, 2, 3];
+
+        var whole = Assert.Throws<ArgumentException>(
+            () => PartialSort.TopK<int>(buffer, buffer.AsSpan()));
+        Assert.Equal("destination", whole.ParamName);
+
+        var partial = Assert.Throws<ArgumentException>(
+            () => PartialSort.TopK<int>(buffer.AsSpan(0, 4), buffer.AsSpan(3, 2)));
+        Assert.Equal("destination", partial.ParamName);
+    }
+
+    [Fact]
+    public void TopK_ShouldSucceed_WhenTheDestinationOnlyNeighboursTheSource()
+    {
+        // Co-residence in one buffer is fine; only genuine overlap is rejected.
+        int[] buffer = [5, 1, 4, 2, 0, 0];
+
+        Assert.Equal(2, PartialSort.TopK<int>(buffer.AsSpan(0, 4), buffer.AsSpan(4, 2)));
+        Assert.Equal([5, 4], buffer.AsSpan(4, 2).ToArray());
+    }
+
     [Fact]
     public void ArgSort_ShouldThrow_WhenTheIndexBufferSharesStorageWithTheKeys()
     {