Fix Issue 12086 - std.algorithm.remove + range of indices produces wr…

…ong results

changelog/std-algorithm-mutation-remove.dd

@@ -0,0 +1,27 @@
+`std.algorithm.mutation.remove` now only accepts integral values or pair of integral values as offset
+
+Previously, without being stated in the documentation,
+$(REF remove, std,algorithm) used to accept any values as `offset`.
+This behavior was very error-prone:
+
+---
+import std.algorithm, std.stdio;
+[0, 1, 2, 3, 4].remove(1, 3).writeln; // 0, 2, 4  -- correct
+[0, 1, 2, 3, 4].remove([1, 3]).writeln; // 0, 3, 4  -- incorrect
+---
+
+With this release, using arrays as individual elements is no longer valid.
+$(REF tuple, std,typecons) can be used to explicitly indicate that a range
+from `start` to `stop` (non-enclosing) should be removed:
+
+---
+import std.algorithm, std.stdio, std.typecons;
+[0, 1, 2, 3, 4].remove(tuple(1, 3)).writeln; // 0, 3, 4
+---
+
+However, only 2-tuples are allowed to avoid this un-intuitive scenario:
+
+---
+import std.algorithm, std.stdio, std.typecons;
+[0, 1, 2, 3, 4].remove(tuple(1, 3, 4)).writeln; // 0, 4?
+---

std/algorithm/mutation.d

@@ -1696,17 +1696,17 @@ enum SwapStrategy
 Eliminates elements at given offsets from `range` and returns the shortened
 range.
 
-For example, here is how to _remove a single element from an array:
+For example, here is how to remove a single element from an array:
 
 ----
 string[] a = [ "a", "b", "c", "d" ];
 a = a.remove(1); // remove element at offset 1
 assert(a == [ "a", "c", "d"]);
 ----
 
-Note that `remove` does not change the length of the original _range directly;
-instead, it returns the shortened _range. If its return value is not assigned to
-the original _range, the original _range will retain its original length, though
+Note that `remove` does not change the length of the original range directly;
+instead, it returns the shortened range. If its return value is not assigned to
+the original range, the original range will retain its original length, though
 its contents will have changed:
 
 ----
@@ -1715,7 +1715,7 @@ assert(remove(a, 1) == [ 3, 7, 8 ]);
 assert(a == [ 3, 7, 8, 8 ]);
 ----
 
-The element at _offset `1` has been removed and the rest of the elements have
+The element at offset `1` has been removed and the rest of the elements have
 shifted up to fill its place, however, the original array remains of the same
 length. This is because all functions in `std.algorithm` only change $(I
 content), not $(I topology). The value `8` is repeated because $(LREF move) was
@@ -1734,19 +1734,29 @@ assert(remove(a, 1, 3, 5) ==
 ----
 
 (Note that all indices refer to slots in the $(I original) array, not
-in the array as it is being progressively shortened.) Finally, any
-combination of integral offsets and tuples composed of two integral
-offsets can be passed in.
+in the array as it is being progressively shortened.)
+
+Tuples of two integral offsets can be used to remove an indices range:
+
+----
+int[] a = [ 3, 4, 5, 6, 7];
+assert(remove(a, 1, tuple(1, 3), 9) == [ 3, 6, 7 ]);
+----
+
+The tuple passes in a range closed to the left and open to
+the right (consistent with built-in slices), e.g. `tuple(1, 3)`
+means indices `1` and `2` but not `3`.
+
+Finally, any combination of integral offsets and tuples composed of two integral
+offsets can be passed in:
 
 ----
 int[] a = [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ];
 assert(remove(a, 1, tuple(3, 5), 9) == [ 0, 2, 5, 6, 7, 8, 10 ]);
 ----
 
 In this case, the slots at positions 1, 3, 4, and 9 are removed from
-the array. The tuple passes in a range closed to the left and open to
-the right (consistent with built-in slices), e.g. `tuple(3, 5)`
-means indices `3` and `4` but not `5`.
+the array.
 
 If the need is to remove some elements in the range but the order of
 the remaining elements does not have to be preserved, you may want to
@@ -1765,35 +1775,94 @@ movement to be done which improves the execution time of the function.
 
 The function `remove` works on bidirectional ranges that have assignable
 lvalue elements. The moving strategy is (listed from fastest to slowest):
-$(UL $(LI If $(D s == SwapStrategy.unstable && isRandomAccessRange!Range &&
+
+$(UL
+        $(LI If $(D s == SwapStrategy.unstable && isRandomAccessRange!Range &&
 hasLength!Range && hasLvalueElements!Range), then elements are moved from the
 end of the range into the slots to be filled. In this case, the absolute
-minimum of moves is performed.)  $(LI Otherwise, if $(D s ==
+minimum of moves is performed.)
+        $(LI Otherwise, if $(D s ==
 SwapStrategy.unstable && isBidirectionalRange!Range && hasLength!Range
 && hasLvalueElements!Range), then elements are still moved from the
 end of the range, but time is spent on advancing between slots by repeated
-calls to `range.popFront`.)  $(LI Otherwise, elements are moved
+calls to `range.popFront`.)
+        $(LI Otherwise, elements are moved
 incrementally towards the front of `range`; a given element is never
 moved several times, but more elements are moved than in the previous
-cases.))
+cases.)
+)
 
 Params:
     s = a SwapStrategy to determine if the original order needs to be preserved
-    range = a $(REF_ALTTEXT bidirectional range, isBidirectionalRange, std,_range,primitives)
+    range = a $(REF_ALTTEXT bidirectional range, isBidirectionalRange, std,range,primitives)
     with a length member
     offset = which element(s) to remove
 
 Returns:
-    a range containing all of the elements of range with offset removed
- */
+    A range containing all of the elements of range with offset removed.
+*/
 Range remove
 (SwapStrategy s = SwapStrategy.stable, Range, Offset...)
 (Range range, Offset offset)
-if (s != SwapStrategy.stable
-    && isBidirectionalRange!Range
+if (isBidirectionalRange!Range
     && hasLvalueElements!Range
     && hasLength!Range
     && Offset.length >= 1)
+{
+    import std.traits : isIntegral;
+    import std.typecons : isTuple;
+    static foreach (T; Offset)
+    {
+        static if (isTuple!T)
+        {
+            static assert(T.length == 2 &&
+                    isIntegral!(typeof(T.init[0])) && isIntegral!(typeof(T.init[0])),
+                "Each offset must be an integral or a tuple of two integrals." ~
+                "Use `arr.remove(pos1, pos2)` or `arr.remove(tuple(start, begin))`");
+        }
+        else
+        {
+            static assert(isIntegral!T,
+                "Each offset must be an integral or a tuple of two integrals." ~
+                "Use `arr.remove(pos1, pos2)` or `arr.remove(tuple(start, begin))`");
+        }
+    }
+
+    static if (s == SwapStrategy.stable)
+        return removeImplStable(range, offset);
+    else static if (s == SwapStrategy.unstable)
+        return removeImplUnstable(range, offset);
+    else
+        static assert(0, "Unknown SwapStrategy " ~ s);
+}
+
+///
+@safe pure unittest
+{
+    import std.typecons : tuple;
+
+    // Delete an index
+    assert([4, 5, 6].remove(1) == [4, 6]);
+
+    // Delete multiple indices
+    assert([4, 5, 6, 7, 8].remove(1, 3) == [4, 6, 8]);
+
+    // Use an indices range
+    assert([4, 5, 6, 7, 8].remove(tuple(1, 3)) == [4, 7, 8]);
+
+    // Use an indices range and individual indices
+    assert([4, 5, 6, 7, 8].remove(0, tuple(1, 3), 4) == [7]);
+}
+
+/// `SwapStrategy.unstable` is faster, but doesn't guarantee the same order of the original array
+@safe pure unittest
+{
+    assert([5, 6, 7, 8].remove!(SwapStrategy.stable)(1) == [5, 7, 8]);
+    assert([5, 6, 7, 8].remove!(SwapStrategy.unstable)(1) == [5, 8, 7]);
+}
+
+// split into Impl to be able to trigger deprecations
+private auto removeImplUnstable(Range, Offset...)(Range range, Offset offset)
 {
     Tuple!(size_t, "pos", size_t, "len")[offset.length] blackouts;
     foreach (i, v; offset)
@@ -1872,14 +1941,8 @@ if (s != SwapStrategy.stable
     return range;
 }
 
-/// Ditto
-Range remove
-(SwapStrategy s = SwapStrategy.stable, Range, Offset...)
-(Range range, Offset offset)
-if (s == SwapStrategy.stable
-    && isBidirectionalRange!Range
-    && hasLvalueElements!Range
-    && Offset.length >= 1)
+// split into Impl to be able to trigger deprecations
+private auto removeImplStable(Range, Offset...)(Range range, Offset offset)
 {
     auto result = range;
     auto src = range, tgt = range;
@@ -1923,24 +1986,6 @@ if (s == SwapStrategy.stable
     return result;
 }
 
-///
-@safe pure unittest
-{
-    import std.typecons : tuple;
-
-    auto a = [ 0, 1, 2, 3, 4, 5 ];
-    assert(remove!(SwapStrategy.stable)(a, 1) == [ 0, 2, 3, 4, 5 ]);
-    a = [ 0, 1, 2, 3, 4, 5 ];
-    assert(remove!(SwapStrategy.stable)(a, 1, 3) == [ 0, 2, 4, 5] );
-    a = [ 0, 1, 2, 3, 4, 5 ];
-    assert(remove!(SwapStrategy.stable)(a, 1, tuple(3, 6)) == [ 0, 2 ]);
-
-    a = [ 0, 1, 2, 3, 4, 5 ];
-    assert(remove!(SwapStrategy.unstable)(a, 1) == [0, 5, 2, 3, 4]);
-    a = [ 0, 1, 2, 3, 4, 5 ];
-    assert(remove!(SwapStrategy.unstable)(a, tuple(1, 4)) == [0, 5, 4]);
-}
-
 @safe unittest
 {
     import std.exception : assertThrown;
@@ -2012,6 +2057,20 @@ if (s == SwapStrategy.stable
     }
 }
 
+// Use of dynamic arrays as offsets is too error-prone
+// https://issues.dlang.org/show_bug.cgi?id=12086
+@safe unittest
+{
+    static assert(!__traits(compiles, [0, 1, 2, 3, 4].remove([1, 3]) == [0, 3, 4]));
+    static assert(__traits(compiles, [0, 1, 2, 3, 4].remove(1, 3) == [0, 2, 4]));
+    static assert(!__traits(compiles, assert([0, 1, 2, 3, 4].remove([1, 3, 4]) == [0, 3, 4])));
+    static assert(!__traits(compiles, assert([0, 1, 2, 3, 4].remove(tuple(1, 3, 4)) == [0, 3, 4])));
+
+    import std.range : only;
+    static assert(!__traits(compiles, assert([0, 1, 2, 3, 4].remove(only(1, 3)) == [0, 3, 4])));
+    static assert(__traits(compiles, assert([0, 1, 2, 3, 4].remove(1, 3) == [0, 2, 4])));
+}
+
 /**
 Reduces the length of the
 $(REF_ALTTEXT bidirectional range, isBidirectionalRange, std,_range,primitives) `range` by removing
@@ -2118,27 +2177,28 @@ if (isBidirectionalRange!Range
     import std.meta : AliasSeq;
     import std.range : iota, only;
     import std.typecons : Tuple;
-    alias S = Tuple!(int[2]);
+    alias E = Tuple!(int, int);
+    alias S = Tuple!(E);
     S[] soffsets;
     foreach (start; 0 .. 5)
     foreach (end; min(start+1,5) .. 5)
-          soffsets ~= S([start,end]);
-    alias D = Tuple!(int[2],int[2]);
+          soffsets ~= S(E(start,end));
+    alias D = Tuple!(E, E);
     D[] doffsets;
     foreach (start1; 0 .. 10)
     foreach (end1; min(start1+1,10) .. 10)
     foreach (start2; end1 .. 10)
     foreach (end2; min(start2+1,10) .. 10)
-          doffsets ~= D([start1,end1],[start2,end2]);
-    alias T = Tuple!(int[2],int[2],int[2]);
+          doffsets ~= D(E(start1,end1),E(start2,end2));
+    alias T = Tuple!(E, E, E);
     T[] toffsets;
     foreach (start1; 0 .. 15)
     foreach (end1; min(start1+1,15) .. 15)
     foreach (start2; end1 .. 15)
     foreach (end2; min(start2+1,15) .. 15)
     foreach (start3; end2 .. 15)
     foreach (end3; min(start3+1,15) .. 15)
-            toffsets ~= T([start1,end1],[start2,end2],[start3,end3]);
+            toffsets ~= T(E(start1,end1),E(start2,end2),E(start3,end3));
 
     static void verify(O...)(int[] r, int len, int removed, bool stable, O offsets)
     {