deprecate sequtils.delete and add an overload with saner semantics consistent with system.delete (#18487)

* deprecate sequtils.delete and add an overload with saner semantics
* AssertionDefect => IndexDefect
* improve tsequtils
* add tests; use splice in js for optimization

changelog.md

@@ -346,6 +346,9 @@
 
 - Added `dom.setInterval`, `dom.clearInterval` overloads.
 
+- Deprecated `sequtils.delete` and added an overload taking a `Slice` that raises a defect
+  if the slice is out of bounds.
+
 ## Language changes
 
 - `nimscript` now handles `except Exception as e`.

lib/pure/collections/sequtils.nim

@@ -509,12 +509,51 @@ proc keepIf*[T](s: var seq[T], pred: proc(x: T): bool {.closure.})
       inc(pos)
   setLen(s, pos)
 
-func delete*[T](s: var seq[T]; first, last: Natural) =
+func delete*[T](s: var seq[T]; slice: Slice[int]) =
+  ## Deletes the items `s[slice]`, raising `IndexDefect` if the slice contains
+  ## elements out of range.
+  ##
+  ## This operation moves all elements after `s[slice]` in linear time.
+  runnableExamples:
+    var a = @[10, 11, 12, 13, 14]
+    doAssertRaises(IndexDefect): a.delete(4..5)
+    assert a == @[10, 11, 12, 13, 14]
+    a.delete(4..4)
+    assert a == @[10, 11, 12, 13]
+    a.delete(1..2)
+    assert a == @[10, 13]
+    a.delete(1..<1) # empty slice
+    assert a == @[10, 13]
+  when compileOption("boundChecks"):
+    if not (slice.a < s.len and slice.a >= 0 and slice.b < s.len):
+      raise newException(IndexDefect, $(slice: slice, len: s.len))
+  if slice.b >= slice.a:
+    template defaultImpl =
+      var i = slice.a
+      var j = slice.b + 1
+      var newLen = s.len - j + i
+      while i < newLen:
+        when defined(gcDestructors):
+          s[i] = move(s[j])
+        else:
+          s[i].shallowCopy(s[j])
+        inc(i)
+        inc(j)
+      setLen(s, newLen)
+    when nimvm: defaultImpl()
+    else:
+      when defined(js):
+        let n = slice.b - slice.a + 1
+        let first = slice.a
+        {.emit: "`s`.splice(`first`, `n`);".}
+      else:
+        defaultImpl()
+
+func delete*[T](s: var seq[T]; first, last: Natural) {.deprecated: "use `delete(s, first..last)`".} =
   ## Deletes the items of a sequence `s` at positions `first..last`
   ## (including both ends of the range).
   ## This modifies `s` itself, it does not return a copy.
-  ##
-  runnableExamples:
+  runnableExamples("--warning:deprecated:off"):
     let outcome = @[1, 1, 1, 1, 1, 1, 1, 1]
     var dest = @[1, 1, 1, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1]
     dest.delete(3, 8)

tests/stdlib/tsequtils.nim

@@ -1,3 +1,9 @@
+discard """
+  targets: "c js"
+"""
+
+# xxx move all tests under `main`
+
 import std/sequtils
 import strutils
 from algorithm import sorted
@@ -190,17 +196,6 @@ block: # keepIf test
   keepIf(floats, proc(x: float): bool = x > 10)
   doAssert floats == @[13.0, 12.5, 10.1]
 
-block: # delete tests
-  let outcome = @[1, 1, 1, 1, 1, 1, 1, 1]
-  var dest = @[1, 1, 1, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1]
-  dest.delete(3, 8)
-  doAssert outcome == dest, """\
-  Deleting range 3-9 from [1,1,1,2,2,2,2,2,2,1,1,1,1,1]
-  is [1,1,1,1,1,1,1,1]"""
-  var x = @[1, 2, 3]
-  x.delete(100, 100)
-  doAssert x == @[1, 2, 3]
-
 block: # insert tests
   var dest = @[1, 1, 1, 1, 1, 1, 1, 1]
   let
@@ -298,43 +293,45 @@ block: # toSeq test
     doAssert myIter.toSeq == @[1, 2]
     doAssert toSeq(myIter) == @[1, 2]
 
-  block:
-    iterator myIter(): int {.closure.} =
-      yield 1
-      yield 2
+  when not defined(js):
+    # pending #4695
+    block:
+        iterator myIter(): int {.closure.} =
+          yield 1
+          yield 2
 
-    doAssert myIter.toSeq == @[1, 2]
-    doAssert toSeq(myIter) == @[1, 2]
-
-  block:
-    proc myIter(): auto =
-      iterator ret(): int {.closure.} =
-        yield 1
-        yield 2
-      result = ret
-
-    doAssert myIter().toSeq == @[1, 2]
-    doAssert toSeq(myIter()) == @[1, 2]
-
-  block:
-    proc myIter(n: int): auto =
-      var counter = 0
-      iterator ret(): int {.closure.} =
-        while counter < n:
-          yield counter
-          counter.inc
-      result = ret
+        doAssert myIter.toSeq == @[1, 2]
+        doAssert toSeq(myIter) == @[1, 2]
 
     block:
-      let myIter3 = myIter(3)
-      doAssert myIter3.toSeq == @[0, 1, 2]
+      proc myIter(): auto =
+        iterator ret(): int {.closure.} =
+          yield 1
+          yield 2
+        result = ret
+
+      doAssert myIter().toSeq == @[1, 2]
+      doAssert toSeq(myIter()) == @[1, 2]
+
     block:
-      let myIter3 = myIter(3)
-      doAssert toSeq(myIter3) == @[0, 1, 2]
-    block:
-      # makes sure this does not hang forever
-      doAssert myIter(3).toSeq == @[0, 1, 2]
-      doAssert toSeq(myIter(3)) == @[0, 1, 2]
+      proc myIter(n: int): auto =
+        var counter = 0
+        iterator ret(): int {.closure.} =
+          while counter < n:
+            yield counter
+            counter.inc
+        result = ret
+
+      block:
+        let myIter3 = myIter(3)
+        doAssert myIter3.toSeq == @[0, 1, 2]
+      block:
+        let myIter3 = myIter(3)
+        doAssert toSeq(myIter3) == @[0, 1, 2]
+      block:
+        # makes sure this does not hang forever
+        doAssert myIter(3).toSeq == @[0, 1, 2]
+        doAssert toSeq(myIter(3)) == @[0, 1, 2]
 
 block:
   # tests https://github.com/nim-lang/Nim/issues/7187
@@ -452,4 +449,61 @@ block:
       for i in 0..<len:
         yield i
 
-  doAssert: iter(3).mapIt(2*it).foldl(a + b) == 6
+  when not defined(js):
+    # xxx: obscure CT error: basic_types.nim(16, 16) Error: internal error: symbol has no generated name: true
+    doAssert: iter(3).mapIt(2*it).foldl(a + b) == 6
+
+template main =
+  # xxx move all tests here
+  block: # delete tests
+    let outcome = @[1, 1, 1, 1, 1, 1, 1, 1]
+    var dest = @[1, 1, 1, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1]
+    dest.delete(3, 8)
+    doAssert outcome == dest, """\
+    Deleting range 3-9 from [1,1,1,2,2,2,2,2,2,1,1,1,1,1]
+    is [1,1,1,1,1,1,1,1]"""
+    var x = @[1, 2, 3]
+    x.delete(100, 100)
+    doAssert x == @[1, 2, 3]
+
+  block: # delete tests
+    var a = @[10, 11, 12, 13, 14]
+    doAssertRaises(IndexDefect): a.delete(4..5)
+    doAssertRaises(IndexDefect): a.delete(4..<6)
+    doAssertRaises(IndexDefect): a.delete(-1..1)
+    doAssertRaises(IndexDefect): a.delete(-1 .. -1)
+    doAssertRaises(IndexDefect): a.delete(5..5)
+    doAssertRaises(IndexDefect): a.delete(5..3)
+    doAssertRaises(IndexDefect): a.delete(5..<5) # edge case
+    doAssert a == @[10, 11, 12, 13, 14]
+    a.delete(4..4)
+    doAssert a == @[10, 11, 12, 13]
+    a.delete(1..2)
+    doAssert a == @[10, 13]
+    a.delete(1..<1) # empty slice
+    doAssert a == @[10, 13]
+    a.delete(0..<0)
+    doAssert a == @[10, 13]
+    a.delete(0..0)
+    doAssert a == @[13]
+    a.delete(0..0)
+    doAssert a == @[]
+    doAssertRaises(IndexDefect): a.delete(0..0)
+    doAssertRaises(IndexDefect): a.delete(0..<0) # edge case
+    block:
+      type A = object
+        a0: int
+      var a = @[A(a0: 10), A(a0: 11), A(a0: 12)]
+      a.delete(0..1)
+      doAssert a == @[A(a0: 12)]
+    block:
+      type A = ref object
+      let a0 = A()
+      let a1 = A()
+      let a2 = A()
+      var a = @[a0, a1, a2]
+      a.delete(0..1)
+      doAssert a == @[a2]
+
+static: main()
+main()