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

…ong results

changelog/std-algorithm-mutation-remove.dd

@@ -0,0 +1,26 @@
+`std.algorithm.mutation.remove` now only accepts integral values or pair of integral values as offset
+
+Previously, $(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 element is deprecated.
+$(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, 3, 4
+---

std/algorithm/mutation.d

@@ -78,8 +78,9 @@ T2=$(TR $(TDNW $(LREF $1)) $(TD $+))
 module std.algorithm.mutation;
 
 import std.range.primitives;
-import std.traits : isArray, isAssignable, isBlitAssignable, isNarrowString, Unqual, isSomeChar;
+import std.traits : isArray, isAssignable, isBlitAssignable, isNarrowString, Unqual, isSomeChar, isIntegral;
 // FIXME
+import std.meta : allSatisfy, templateOr;
 import std.typecons; // : tuple, Tuple;
 
 // bringToFront
@@ -1695,6 +1696,59 @@ enum SwapStrategy
     assert(arr == [10, 9, 8, 4, 5, 6, 7, 3, 2, 1]);
 }
 
+/**
+Checks whether a type is either an integral static array (e.g. int[2])
+or a tuple of two integral types
+*/
+private template isIntegralPair(T)
+{
+    import std.traits : isStaticArray;
+    static if (__traits(compiles, T.length > 0))
+    {
+        static if (T.length == 2)
+        {
+            static if (isStaticArray!T)
+                enum isIntegralPair = isIntegral!(typeof(T.init[0]));
+            else
+                enum isIntegralPair = T.length == 2 &&
+                            isIntegral!(typeof(T[0])) &&
+                            isIntegral!(typeof(T[1]));
+        }
+        else
+        {
+            enum isIntegralPair = false;
+        }
+    }
+    else
+    {
+        enum isIntegralPair = false;
+    }
+}
+
+// test tuples
+unittest
+{
+    static assert(!isIntegralPair!(typeof(tuple(0))));
+    static assert( isIntegralPair!(typeof(tuple(0, 0))));
+    static assert(!isIntegralPair!(typeof(tuple(0, 0, 0))));
+    static assert(!isIntegralPair!(typeof([0, 0])));
+    static assert(!isIntegralPair!(typeof(tuple(0., 0))));
+}
+
+// test static arrays
+unittest
+{
+    int[1] offset1;
+    int[2] offset2;
+    int[3] offset3;
+    static assert(!isIntegralPair!(typeof(offset1)));
+    static assert( isIntegralPair!(typeof(offset2)));
+    static assert(!isIntegralPair!(typeof(offset3)));
+
+    float[2] offset2f;
+    static assert(!isIntegralPair!(typeof(offset2f)));
+}
+
 /**
 Eliminates elements at given offsets from `range` and returns the shortened
 range.
@@ -1788,15 +1842,30 @@ Params:
 
 Returns:
     a range containing all of the elements of range with offset removed
+
+Note:
+    `remove` only accepts integral types or pairs of integral types as individual
+    offset. Use of other `Offset` types has been $(B deprecated).
  */
 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)
+    && Offset.length >= 1
+    && allSatisfy!(templateOr!(isIntegral, isIntegralPair),  Offset))
+{
+    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);
+}
+
+// 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)
@@ -1875,14 +1944,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;
@@ -1926,6 +1989,25 @@ if (s == SwapStrategy.stable
     return result;
 }
 
+// @@@DEPRECATED_2.084@@@
+deprecated("Use `arr.remove(pos1, pos2)` or `arr.remove(tuple(start, begin))`")
+Range remove
+(SwapStrategy s = SwapStrategy.stable, Range, Offset...)
+(Range range, Offset offset)
+if (isBidirectionalRange!Range
+    && hasLvalueElements!Range
+    && hasLength!Range
+    && Offset.length >= 1
+    && !allSatisfy!(templateOr!(isIntegral, isIntegralPair),  Offset))
+{
+    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
 {
@@ -2015,6 +2097,20 @@ if (s == SwapStrategy.stable
     }
 }
 
+// Use of dynamic arrays as offsets is too error-prone
+// https://issues.dlang.org/show_bug.cgi?id=12086
+deprecated
+@safe unittest
+{
+    assert([0, 1, 2, 3, 4].remove([1, 3]) == [0, 3, 4]);
+    assert([0, 1, 2, 3, 4].remove([1, 3, 4]) == [0, 3, 4]);
+    assert([0, 1, 2, 3, 4].remove(tuple(1, 3, 4)) == [0, 3, 4]);
+
+    // ranges are deprecated too
+    import std.range : only;
+    assert([0, 1, 2, 3, 4].remove(only(1, 3)) == [0, 3, 4]);
+}
+
 /**
 Reduces the length of the
 $(REF_ALTTEXT bidirectional range, isBidirectionalRange, std,_range,primitives) `range` by removing