fixed system.delete (#18507)

3 files changed, 26 insertions, 12 deletions

diff --git a/changelog.md b/changelog.md
index 6dddd8a01..097beac2c 100644
--- a/changelog.md
+++ b/changelog.md
@@ -4,7 +4,18 @@
 
 ## Changes affecting backward compatibility
 
-- Deprecated `std/mersenne`
+- Deprecated `std/mersenne`.
+
+- `system.delete` had a most surprising behavior when the index passed to it was out of
+  bounds (it would delete the last entry then). Compile with `-d:nimStrictDelete` so
+  that an index error is produced instead. But be aware that your code might depend on
+  this quirky behavior so a review process is required on your part before you can
+  use `-d:nimStrictDelete`. To make this review easier, use the `-d:nimAuditDelete`
+  switch, it pretends that `system.delete` is deprecated so that it is easier to see
+  where it was used in your code.
+
+  `-d:nimStrictDelete` will become the default in upcoming versions.
+
 
 - `cuchar` is now deprecated as it aliased `char` where arguably it should have aliased `uint8`.
   Please use `char` or `uint8` instead.
diff --git a/lib/system.nim b/lib/system.nim
index 57d620e57..2790187f4 100644
--- a/lib/system.nim
+++ b/lib/system.nim
@@ -2134,7 +2134,12 @@ const
 import system/dollars
 export dollars
 
-proc delete*[T](x: var seq[T], i: Natural) {.noSideEffect.} =
+when defined(nimAuditDelete):
+  {.pragma: auditDelete, deprecated: "review this call for out of bounds behavior".}
+else:
+  {.pragma: auditDelete.}
+
+proc delete*[T](x: var seq[T], i: Natural) {.noSideEffect, auditDelete.} =
   ## Deletes the item at index `i` by moving all `x[i+1..^1]` items by one position.
   ##
   ## This is an `O(n)` operation.
@@ -2147,12 +2152,10 @@ proc delete*[T](x: var seq[T], i: Natural) {.noSideEffect.} =
     s.delete(2)
     doAssert s == @[1, 2, 4, 5]
 
-    doAssertRaises(IndexDefect):
-      s.delete(4)
-
-  if i > high(x):
-    # xxx this should call `raiseIndexError2(i, high(x))` after some refactoring
-    raise (ref IndexDefect)(msg: "index out of bounds: '" & $i & "' < '" & $x.len & "' failed")
+  when defined(nimStrictDelete):
+    if i > high(x):
+      # xxx this should call `raiseIndexError2(i, high(x))` after some refactoring
+      raise (ref IndexDefect)(msg: "index out of bounds: '" & $i & "' < '" & $x.len & "' failed")
 
   template defaultImpl =
     let xl = x.len
diff --git a/tests/stdlib/tsystem.nim b/tests/stdlib/tsystem.nim
index b48034043..00be16275 100644
--- a/tests/stdlib/tsystem.nim
+++ b/tests/stdlib/tsystem.nim
@@ -53,8 +53,8 @@ template main =
       var s = @["foo", "bar"]
       s.delete(1)
       doAssert s == @["foo"]
-  
-    block:
+
+    when false:
       var s: seq[string]
       doAssertRaises(IndexDefect):
         s.delete(0)
@@ -66,8 +66,8 @@ template main =
       var s = @["foo"]
       s.delete(0)
       doAssert s == @[]
-  
-    block: # bug #16544: deleting out of bounds index should raise
+
+    when false: # bug #16544: deleting out of bounds index should raise
       var s = @["foo"]
       doAssertRaises(IndexDefect):
         s.delete(1)