Skip to content

delete(seq, index) with invalid index deletes an element instead of raising #16544

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
timotheecour opened this issue Jan 1, 2021 · 6 comments · Fixed by #17821
Closed

delete(seq, index) with invalid index deletes an element instead of raising #16544

timotheecour opened this issue Jan 1, 2021 · 6 comments · Fixed by #17821
Labels
Easy Invalid Code Acceptance Everything related to compiler not complaining about invalid code

Comments

@timotheecour
Copy link
Member

timotheecour commented Jan 1, 2021

Example

when true:
  var a = @[1,2,3]
  a.delete 6
  echo a

Current Output

@[1, 2]

Expected Output

raise IndexError via raiseIndexError2

Additional Information

devel 1.5.1 505d043
a fix should include a comprehensive test

note that RangeDefect is raised if passing i == 0 with x.len == 0

Error: unhandled exception: value out of range: -1 notin 0 .. 9223372036854775807 [RangeDefect]

note that del correctly raises:
Error: unhandled exception: index 6 not in 0 .. 2 [IndexDefect]

@timotheecour timotheecour added the Invalid Code Acceptance Everything related to compiler not complaining about invalid code label Jan 1, 2021
@juancarlospaco
Copy link
Collaborator

I hit the bug too, more examples:

var a: seq[int] = @[0]
a.delete(999999999999)
var a: seq[int] = @[0]
discard a.pop()
a.delete(999999999999)
var a: seq[int] = @[0]
discard a.pop()
a.insert(42, 0)
a.delete(999999999999)

del works consistently on all cases:

Error: unhandled exception: index out of bounds, the container is empty [IndexDefect] or similar.

@ringabout
Copy link
Member

What do you expect subStr with invalid index?
subStr(s, -4, 10) or ?

See also
#16280 (comment)

@arnetheduck
Copy link
Contributor

well, I argue in #16492 that first and foremost, all these operations would ideally present a logically consistent picture, both native types, library types and at compile time - in fact, the most important thing is that they are consistent with each other - ie when one is defined in a particular way, the others follow.

Generally, I probably have a slight preference for being lenient with ranges / slices (towards python, but also C++ allows some leniency in these kinds of operations), ie simply return the overlapping part, but one could argue for strictness I guess - there's already lots of Defect/panics all over the place which don't really need to be panics, so from that point of view, strictness and lots of panics that users have to guard against is more nim-like.

@hnicke
Copy link
Contributor

hnicke commented Apr 20, 2021

Just stumbled across this too.
Coming from kotlin and python, I really didn't expect delete to remove the last element if index is out of bounds.
I consider this to be dangerous.
Instead, I expect it too either raise an Exception or return nil (where else it would return the removed value).

@hnicke
Copy link
Contributor

hnicke commented Apr 21, 2021

Is the general consensus that delete should behave exactly like del in this case, e.g. like below?

let xs = @[0,1]
xs.delete(2)
# Error: unhandled exception: index 2 not in 0 .. 1 [IndexDefect]

@Araq
Copy link
Member

Araq commented Apr 21, 2021

Can confirm, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Invalid Code Acceptance Everything related to compiler not complaining about invalid code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants