Skip to content

Revert "Raise IndexDefect when deleting element at out of bounds index (#17821)" #18461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@
- `math.round` now is rounded "away from zero" in JS backend which is consistent
with other backends. See #9125. Use `-d:nimLegacyJsRound` for previous behavior.

- Instead of deleting the element at the last index,
`system.delete()` now raises `IndexDefect` when given index is out of bounds.

- Changed the behavior of `uri.decodeQuery` when there are unencoded `=`
characters in the decoded values. Prior versions would raise an error. This is
no longer the case to comply with the HTML spec and other languages
Expand Down
58 changes: 24 additions & 34 deletions lib/system.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,30 @@ proc del*[T](x: var seq[T], i: Natural) {.noSideEffect.} =
movingCopy(x[i], x[xl])
setLen(x, xl)

proc delete*[T](x: var seq[T], i: Natural) {.noSideEffect.} =
## Deletes the item at index `i` by moving all `x[i+1..]` items by one position.
##
## This is an `O(n)` operation.
##
## See also:
## * `del <#del,seq[T],Natural>`_ for O(1) operation
##
## .. code-block:: Nim
## var i = @[1, 2, 3, 4, 5]
## i.delete(2) # => @[1, 2, 4, 5]
template defaultImpl =
let xl = x.len
for j in i.int..xl-2: movingCopy(x[j], x[j+1])
setLen(x, xl-1)

when nimvm:
defaultImpl()
else:
when defined(js):
{.emit: "`x`.splice(`i`, 1);".}
else:
defaultImpl()

proc insert*[T](x: var seq[T], item: sink T, i = 0.Natural) {.noSideEffect.} =
## Inserts `item` into `x` at position `i`.
##
Expand Down Expand Up @@ -2129,40 +2153,6 @@ const
import system/dollars
export dollars

proc delete*[T](x: var seq[T], i: Natural) {.noSideEffect.} =
## Deletes the item at index `i` by moving all `x[i+1..^1]` items by one position.
##
## This is an `O(n)` operation.
##
## See also:
## * `del <#del,seq[T],Natural>`_ for O(1) operation
##
runnableExamples:
var s = @[1, 2, 3, 4, 5]
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")

template defaultImpl =
let xl = x.len
for j in i.int..xl-2: movingCopy(x[j], x[j+1])
setLen(x, xl-1)

when nimvm:
defaultImpl()
else:
when defined(js):
{.emit: "`x`.splice(`i`, 1);".}
else:
defaultImpl()


const
NimVersion*: string = $NimMajor & "." & $NimMinor & "." & $NimPatch
## is the version of Nim as a string.
Expand Down
3 changes: 3 additions & 0 deletions tests/gc/gcleak3.nim
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ for i in 0..1024:

proc limit*[t](a: var seq[t]) =
while s.len > 0:
#echo i
#GC_fullCollect()
if getOccupiedMem() > 3000_000: quit("still a leak!")
s.delete(0)

s.limit()

echo "no leak: ", getOccupiedMem()

32 changes: 1 addition & 31 deletions tests/stdlib/tsystem.nim
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ discard """
targets: "c cpp js"
"""

import stdtest/testutils

# TODO: in future work move existing `system` tests here, where they belong

import stdtest/testutils

template main =
block: # closure
Expand Down Expand Up @@ -43,34 +42,5 @@ template main =
# doAssert rawEnv(inner3) != rawEnv(inner1) # because `a` vs `b` # this doesn't hold
outer()

block: # system.delete
block:
var s = @[1]
s.delete(0)
doAssert s == @[]

block:
var s = @["foo", "bar"]
s.delete(1)
doAssert s == @["foo"]

block:
var s: seq[string]
doAssertRaises(IndexDefect):
s.delete(0)

block:
doAssert not compiles(@["foo"].delete(-1))

block: # bug #6710
var s = @["foo"]
s.delete(0)
doAssert s == @[]

block: # bug #16544: deleting out of bounds index should raise
var s = @["foo"]
doAssertRaises(IndexDefect):
s.delete(1)

static: main()
main()
5 changes: 5 additions & 0 deletions tests/system/tsystem_misc.nim
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ doAssert high(float) > low(float)
doAssert high(float32) > low(float32)
doAssert high(float64) > low(float64)

# bug #6710
var s = @[1]
s.delete(0)


proc foo(a: openArray[int]) =
for x in a: echo x

Expand Down