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

[narimiran]

This is a too big breaking change to be part of Nim 1.x

[konsumlamm]

It seems to me that the previous behaviour was an undocumented bug, I don't see how this is a "big breaking change".

[narimiran]

Is it intended to change devel branch?

Yes, devel branch is still Nim 1.x

[Araq]

It seems to me that the previous behaviour was an undocumented bug

Code might rely on this behavior and we have to be more careful with breaking changes. The worst changes are the changes that secretly change behavior.

What should have been done here: "Deprecated system.delete, use sequtils.delete instead", for example. (Would also fight system.nim's bloat.)

[timotheecour]

Please do not revert this bugfix, it's an undocumented bug with no justification and I haven't heard any complaints about it. The behavior before the bugfix made no sense:

• raise defect if index negative
• don't raise defect if index out of bounds but input.len > 0
• but do raise if index >=0 and input len == 0 (why ??)

And led to aribtrary inconsistencies, eg: this printed @[1] in VM or C backend, but @[1, 2] in js backend

proc main =
  var a: seq[int] = @[1, 2]
  a.delete(3)
  echo a
static: main()
main()

(whereas it now raises on all backends after the bugfix)

The worst changes are the changes that secretly change behavior.

the worst bugs are silently accepting invalid inputs, which affects the rest of the program, and generating arbitrarily different results according the the backend

Every bugfix or feature addition is a potential breaking change for some use case; that's not a valid reason to not fix bugs (or single this one out vs all the other bugfixes that went in 1.5.1), the damage for keep the bug in is much larger.

what would be acceptable instead

• keep the bugfix as-is
• or add a -d:nimLegacyDeleteLenient opt-in flag to help some users transition to the new behavior, possibly customizable with logging to stderr to help with transition. This is what those flags are for.

[Araq]

The behavior before the bugfix made no sense: ...

It made a whole lot of sense as it was the natural outcome of the used algorithm, whereas the "fixed" version has more special casing for unknown benefits. Yes, "it's inconsistent with del" is not a bug, the proc never claimed to be "consistent with del". These sort of unwritten and unfullfilled requirements do not imply "bug".

And led to aribtrary inconsistencies, eg: this printed @[1] in VM or C backend, but @[1, 2] in js backend

That doesn't change anything, it changes the behavior for everybody who happens to not to use the JS backend. (Most of our users?)

This is what those flags are for.

And how many of these flags do you have to use from transitioning from 1.4 to 1.6? It's quickly becoming impractical for bigger projects.

[timotheecour]

It made a whole lot of sense as it was the natural outcome of the used algorithm, whereas the "fixed" version has more special casing for unknown benefits.

since when is bounds checking un-natural?

It's inconsistent not just with del or the rest of nim but also with itself, in so many ways: it raised a defect if container len was 0 but not if container len > 0 (and unless index was < 0).

And worse than that, attempting to delete index 6 ended up not just failing to raise, but also deleting a different index than the one requested:

when true:
  var a = @[10,11,12]
  a.delete 6
  echo a # @[10, 11] # i asked to delete index 6 and it deleted index 2 instead

I don't know any language other than nim that does that.

This is pure nonesense and that was indeed the consensus in #16544, no-one was arguing in favor of keeping that behavior:

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.

including you: #16544 (comment)

Can confirm, yes.

And how many of these flags do you have to use from transitioning from 1.4 to 1.6? It's quickly becoming impractical for bigger projects.

it's not impractical, it's a small finite set that's now following a convention (-d:nimLegacyX) and they're all listed under 1 section in the changelog (Changes affecting backward compatibility). Other languages also have transition flags. The alternative is worse (1 giant breaking release or "next vesion" with no transition path in between and very long waiting time to get those bug fixes (nim 2.0 ? 3.0?).

[Araq]

I don't like the special casing at all, it indicates an "inconsistency" ;-)

But let's assume I'm the only fool around that thinks this way, my outlined solution (deprecate system.delete) is the better one: Code can be updated easily and nothing breaks.

[Varriount]

I don't like the special casing at all, it indicates an "inconsistency" ;-)

Indicates a possible inconsistency. Unfortunately, indications don't always equal concrete evidence, especially in a programming context (if they did, life would be much more predictable). And at least in this case, the inconsistency is implementation-dependent:

proc remove*[T](sequence: var seq[T]; index: int) =
  # An alternate `delete` implementation, without a special-case "if" statement.
  var currentIndex = index
  while true:
    let newIndex = currentIndex + 1
    shallowCopy(sequence[currentIndex], sequence[newIndex])

    currentIndex = newIndex
    if currentIndex == len(sequence):
      break

  setLen(sequence, len(sequence) - 1)
    
    
var s  = @[1,2,3,4,5]
remove(s, 30)

Anyway, as @timotheecour points out, the real inconsistency is conceptual: the old implementation of delete was poorly aligned with common conceptual expectations. Honestly, if I stumbled across this behavior as a new user, I would assume it was a bug.

But let's assume I'm the only fool around that thinks this way, my outlined solution (deprecate system.delete) is the better one: Code can be updated easily and nothing breaks.

Requiring a module to be imported in order to perform a basic operation on a core datatype when all other basic operations are automatically available is also inconsistent, not to mention surprising and inconvenient.

[Araq]

Requiring a module to be imported in order to perform a basic operation on a core datatype when all other basic operations are automatically available is also inconsistent, not to mention surprising and inconvenient.

system.delete worked in its quirky ways for years and even our test suite depended on it. Half of the seq operations are already in sequtils, few complained about "surprising and inconvenient". You argue as if system.delete had no history, but it has. Reality beats idealism.

[Varriount]

And how much of that history is composed of new users stumbling on behaviors like this and tossing the language aside (either out of frustration or judgment of Nim's quality)? It's implementation behavior like this, along with the jumbled organization and design of the standard library, that hinders adoption.

Honestly, Nim should have done (and should still do) what Python 3 did: find all the big mistakes, fix them properly, and break everything, all at one time. The only reason it backfired for Python was because Python was actually used in a large number of large codebases. Nim, alas, doesn't have that problem.

[Araq]

Honestly, Nim should have done (and should still do) what Python 3 did: find all the big mistakes, fix them properly, and break everything, all at one time. The only reason it backfired for Python was because Python was actually used in a large number of large codebases. Nim, alas, doesn't have that problem.

We seek to release 1.6, not 2.0. And 2.0 would have a trimed down system.nim lacking delete, it would not have a bigger one.

[konsumlamm]

Ok, how about a solution that hopefully everyone is happy with: We revert the fix and deprecate system.delete and instead recommend using either sequtils.delete or add the following to sequtils:

func delete[T](s: var seq[T]; i: Natural) = delete(s, i, i)

The deprecation message (or the doc comments) could also mention the old (unexpected) behaviour.

Maybe we could also add an overload that has a slice argument, while we're at it (but that's completely optional)...

[Araq]

Ok, how about a solution that hopefully everyone is happy with: We revert the fix and deprecate system.delete and instead recommend using either sequtils.delete or add the following to sequtils: ...

That is exactly what I proposed too. I'm sorry for being terrible at communicating.

[narimiran]

Ok, I implemented sequtils.delete and deprecated the old system.delete. Now, how do we deal with:

Error: ambiguous call; both system.delete(x: var seq[T], i: Natural) and sequtils.delete(x: var seq[T], i: Natural) match for: (seq[int], int literal(1))

So for the user, it is not just import sequtils to fix it; and changing everywhere in ones code from a.delete(1) to sequtils.delete(a, 1) becomes tiresome.

[Araq]

So for the user, it is not just import sequtils to fix it; and changing everywhere in ones code from a.delete(1) to sequtils.delete(a, 1) becomes tiresome.

Well but that is the point. You cannot do this mechanically! You need to review every callsite, consciously!

[konsumlamm]

So for the user, it is not just import sequtils to fix it; and changing everywhere in ones code from a.delete(1) to sequtils.delete(a, 1) becomes tiresome.

Couldn't the user just add import system except delete?

[Varriount]

Well but that is the point. You cannot do this mechanically! You need to review every callsite, consciously!

Well but that is the point. You cannot do this mechanically! You need to review every callsite, consciously!

Which is what would need to be done anyway, without the revert.

I'd also like to add that, in keeping the "fixed" behavior, the following apply:

• No new kinds of exceptions are being raised. delete already throws a defect if the index is less than 0.
• The previous behavior was not stated in the documentation.
• The previous behavior was so surprising that multiple people did or would have assumed it was a bug.
• The new behavior actually uncovered an off-by-one error in the test suite logic.

If this still isn't convincing, what about raising/printing a deprecation warning when delete is passed an index greater than the length of the sequence, stating that a future version will change this behavior?

Alternately, what about deprecating both delete in system and sequtils, and adding "remove" to system?

[timotheecour]

sequtils.delete has a history of bugs (see #12506, #11535) and is still buggy and inconsistent with both system.delete before #17821 and after #17821.

See #18487 which adds an overload with saner semantics for sequtils.delete, that takes a slice instead and makes the behavior consistent with the recently fixed system.delete.

For system.delete, we shouldn't revert #17821 though as the behavior is just an aweful (and undocumented) bug that goes against the doc comment, where you ask to delete an index but it silently deletes another one instead; so an approach similar to #18486 isn't warranted in this case, but I'd be ok with an opt-in legacy flag to allow previous behavior and ease migration, as said earlier.

[Araq]

For system.delete, we shouldn't revert #17821 though as the behavior is just an aweful (and undocumented) bug that goes against the doc comment, where you ask to delete an index but it silently deletes another one instead; so an approach similar to #18486 isn't warranted in this case, but I'd be ok with an opt-in legacy flag to allow previous behavior and ease migration, as said earlier.

But why not deprecate system.delete, the new better sequtils.delete can be used instead of it too.

[timotheecour]

But why not deprecate system.delete, the new better sequtils.delete can be used instead of it too.

because the single index form is both more common and more intuitive than delete(a, i..i), so it makes sense to keep it. sequtils.delete can also add a single index form in the future (to unbloat system and group related procs together) but that would cause ambiguity errors with system.delete, so the correct way forward will be to make system.delete be a deprecated re-export from a private module, which would also be re-exported by sequtils, avoiding the ambiguity error. This is only possible if system.delete is kept correct (ie if #17821 is not reverted).

Again, sequtils.delete was just a bug, not some documented quirk, and its behavior surprised everyone (see #16544); I cannot find a single programming language that had its behavior); fixing it catches bugs (loudly, with an exception), and we should keep the bugfix for the benefit of everyone; no important_pacakges was affected, which is the best proxy we have today.

it is not a bug if somebody's code relies on it

Another fallacy that's anti-pragmatism; this would block any bug fix if someone creates a package that relies on all the documented bugs, undocumented behaviors, bad design decisions, and calls it a "mission critical release blocker" because it "impacts security". Their use case should not be at the detriment of making the language better for everybody else. Otherwise let's revert dragonbox (silent behavior change! my mission critical app relied on old behavior!) and most PRs since last release.

Copying bad design is not good design

This makes me cringe every single time I encounter a behavior only found in nim that goes against both common sense and what every other language does. Like the old sequtils.delete or many bugs that were fixed but threatened by the no breaking change policy. This catchphrase would have more weight if nim had less quirks.

[Araq]

This makes me cringe every single time I encounter a behavior only found in nim that goes against both common sense and what every other language does

Dunno why you bring this up, I don't remember anybody arguing for quirks just because of this slogan.

Otherwise let's revert dragonbox (silent behavior change! my mission critical app relied on old behavior!) and most PRs since last release.

Actually, for 1.6 I do intend to make Dragonbox opt-in... It's not anti-pragmatism to spread out changes more evenly across releases to improve the ease of compiler updates.

[konsumlamm]

sequtils.delete can also add a single index form in the future (to unbloat system and group related procs together) but that would cause ambiguity errors with system.delete, so the correct way forward will be to make system.delete be a deprecated re-export from a private module, which would also be re-exported by sequtils, avoiding the ambiguity error. This is only possible if system.delete is kept correct (ie if #17821 is not reverted).

As I mentioned, the ambiguity errors can be avoided with import system except delete.

[timotheecour]

As I mentioned, the ambiguity errors can be avoided with import system except delete.

that's a kludge that we shouldn't impose on new code. This also doesn't work inside include files (which, sadly, abound in compiler code), nor does it work for eg in when blocks, eg this doesn't work, not to mention is not something that should be the new recommended way:

when (NimMajor, NimMinor, NimPatch) >= (1, 5, 1):
  import system except delete
  import std/sequtils

var a = @[0,1,2]
a.delete(1)

Actually, for 1.6 I do intend to make Dragonbox opt-in... It's not anti-pragmatism to spread out changes more evenly across releases to improve the ease of compiler updates.

I don't think making Dragonbox/schubfach opt-in in 1.6 is a good idea but it should be discussed in a dedicated PR/RFC/issue. At the very least it should definitely not be opt-in in devel.

[Araq]

Has been handled differently.