Throw if toSkip > Number.MAX_SAFE_INTEGER

[kmiller68]

Right now this happily accepts toSkip values bigger than Number.MAX_SAFE_INTEGER however that means implementing this in JS faithfully requires using a BigInt, which is sub-optimal for performance. I think anyone passing a value bigger than Number.MAX_SAFE_INTEGER is likely making a mistake anyway. It would take ~4 months to drive an iterator past Number.MAX_SAFE_INTEGER elements assuming each next took 1ns, which seems optimistic.

Presumably the correct error is a RangeError but I wouldn't necessarily block on a different one.

[michaelficarra]

I don't really like adding this restriction. Array.prototype.includes has no such limitation. Unfortunately, even if you implement it in native code using a uint64 as the counter (as boa did in their implementation), it will still overflow well before Number.MAX_VALUE. So I think I reluctantly support this change.

[bakkot]

Would you also throw for Infinity in that case (since it is > Number.MAX_SAFE_INTEGER)?

[kmiller68]

Would you also throw for Infinity in that case (since it is > Number.MAX_SAFE_INTEGER)?

That seems like a logical thing to throw for, as that's actually an infinite loop. If the user actually wants to infinite loop there are a dozen (IMO more readable) other ways to do that.

[michaelficarra]

It's not an infinite loop, it exhausts the iterator.

[kmiller68]

I don't really like adding this restriction. Array.prototype.includes has no such limitation.

That doesn't seem like a meaningful restriction on Arrays though since Arrays can't have a length bigger than UINT32_MAX, AFAIK. https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-array-exotic-objects-defineownproperty-p-desc.

It's not an infinite loop, it exhausts the iterator.

Fair enough, correction: That seems like a logical thing to throw for, as it is a no-op outside of draining the iterator (or hanging for an iterator with an effective length > Number.MAX_SAFE_INTEGER). If the user actually wants to drain the iterator there are a dozen (IMO more readable) other ways to do that.

[michaelficarra]

That doesn't seem like a meaningful restriction on Arrays though since Arrays can't have a length bigger than UINT32_MAX, AFAIK

Array methods should work with Array-likes, and Array-likes have a max length of Number.MAX_SAFE_INTEGER, but still this is a fair point. Okay, I retract my objection based on inconsistency with Array.prototype.includes.

If the user actually wants to drain the iterator there are a dozen (IMO more readable) other ways to do that.

Such as Iterator.prototype.drop. How do you implement that for large integral numbers? Shouldn't this parameter behave the same?

[nekevss]

Iterator.prototype.drop uses ToIntegerOrInfinity same as Array.prototype.includes.

This proposal does appear to handle POSTIVE_INFINITY and NEGATIVE_INFINITY differently than Array.prototype.includes though. Was there a reason for the difference there?

[michaelficarra]

@nekevss

1. The reason this proposal includes a second parameter at all is for parity with Array.prototype.includes. We wouldn't have included it otherwise.
2. The reason it accepts Infinity and rejects negatives is to align better with the model of iterators. Unlike arrays, iterators can be infinite and don't have a length or a way to walk backwards. This means the parameter acts more like the parameter of Iterator.prototype.drop.
3. The reason it's not exactly like Iterator.prototype.drop is that, since its introduction, we've adopted normative conventions that disallow coercion of non-Number parameters to Numbers.

That being said, the handling of large integral numbers should be the exact same as Iterator.prototype.drop and I'm wondering why it wasn't an issue for that method but it is for this one. Are implementations just noncompliant?

[kmiller68]

Such as Iterator.prototype.drop. How do you implement that for large integral numbers? Shouldn't this parameter behave the same?

We use a Number and it appears V8/SM do too https://github.com/v8/v8/blob/main/src/builtins/iterator-helpers.tq#L578, https://searchfox.org/firefox-main/source/js/src/builtin/Iterator.js#600. I would expect that any counter in an engine is using either a Number or (at most) uint64 and not an arbitrary precision integer.

Also, upon further investigation it seems drop does toIntegerOrInfinity(toNumber(arg)) https://tc39.es/ecma262/#sec-iterator.prototype.drop so using a Number would be fine. In that light I'm also open to changing the behavior here to toIntegerOrInfinity(toNumber(arg)) for consistency. I would not want the behavior of includes to differ from drop/take.

[michaelficarra]

@kmiller68 You're misreading the Iterator.prototype.drop spec. The argument is indeed converted to a Number (and then to a mathematical value), but the counter used to reach that target (which is potentially very large; larger than Number.MAX_SAFE_INTEGER) is a mathematical value that counts its way there by repeatedly adding 1. This is not something that can be done in the Number space.

[ljharb]

Not being able to pass Infinity is a huge DX hit imo, and if you have an infinite iterator (or even an absurdly big one) there’s tons of places you have to watch out for hangs, so i don’t care much if this is another one.

[bakkot]

It is technically true that using a Number for the counter is not a valid implementation of the spec, but no one is ever going to observe that divergence, so I think it's fine. It's no more wrong than the fact that execution environments have bounded RAM and therefore have a max representable BigInt.

(Kinda weird to use a float64 rather than a uint64 but I guess if you're writing the implementation in JS it's hard to use the more appropriate type.)

[michaelficarra]

I've opened a needs-consensus PR to change how iterator counters work: tc39/ecma262#3776. We should make the change proposed here iff the committee wants to go that route for the rest of the iterator built-ins. Personally, I don't like the change, but there doesn't seem to be much point in specifying something that nobody plans to implement correctly.