should Iterator.from and AsyncIterator.from check callability of "next" method eagerly?

[michaelficarra]

As proposed by @rbuckton in #117 (comment).

[bakkot]

Specifically, they currently do this check eagerly when you pass an iterator, but not when you pass an iterable. Probably best to do the check in both cases.

[devsnek]

I think it makes sense to check. This is sort of an interesting timing case in the current use of GetIterator in the spec. It is usually immediately proceeded with a call to IteratorStep which uses Call which raises. I believe the only case where this is observable is with destructuring:

[] = { [Symbol.iterator]() { return {} } }; // does not throw

vs

[a] = { [Symbol.iterator]() { return {} } }; // throws

[michaelficarra]

I guess the equivalent in this proposal would be Iterator.prototype.take.call({}, 0). Should that throw? Maybe we should drop the callability check from GetIteratorDirect instead so this works like empty destructuring.

[bakkot]

I don't see any benefit to trying to achieve a perceived symmetry with empty array destructuring.

[michaelficarra]

The goal isn't to achieve symmetry with empty array destructuring. I think it just has the same user expectations. We probably intentionally designed destructuring to not check that the RHS produced a valid iterator since it won't be consumed. I can see the same rationale applying to iterator helpers that end up consuming nothing. Symmetry with empty destructuring is just a nice outcome.

[bakkot]

We probably intentionally designed destructuring to not check that the RHS produced a valid iterator since it won't be consumed.

I really don't think so. Why would we have? Rather, there's no callability check in GetIterator because normally it's redundant - you'll just be immediately calling it, so there's no need to check explicitly - and there's no reason to ever write [] = foo, so there's no reason to bother putting in an extra check just for that case.

If the spec was intentionally omitting checks on the RHS when the LHS was empty, it would avoid reading Symbol.iterator at all in that case. It doesn't.

But with Iterator.from it will be common to get an iterator without immediately invoking next, so a check is warranted in this case.

[michaelficarra]

I agree about Iterator.from since it'll move the failure closer to the actual error. But I don't think the same applies to take(0). It's not just deferring consumption — it will actually never be used.

[devsnek]

Sorry for the confusion, I mentioned the empty destructuring as sort of a way of showing how strange the "not checking" behavior is in our current spec. I definitely think we should check, and not try to match what destructuring does 😄

[ljharb]

Why is take(0) even an option?

[bakkot]

But I don't think the same applies to take(0). It's not just deferring consumption — it will actually never be used.

We'd have to special-case that to not do the check, because that check is normally done eagerly. I really do not think we should add a special case that to avoid giving an error when calling .take(0) on something with a non-callable next.

[bakkot]

And in fact right now validation of the receiver is done before coercing the arguments, and "validation of the receiver" includes checking that its .next is callable, so we actually can't add that special case without changing the order that we do coercion/validation.

[michaelficarra]

@bakkot It wouldn't be a special check, we would just remove the check from GetIteratorDirect and all the same failures will happen, just delayed until when the underlying iterator is first consumed (which will be never for .take(0) or if the wrapped iterator is never consumed).

[bakkot]

Ah, ok. I think that would be much worse; we should do validation up front.

[michaelficarra]

I'm actually fairly neutral at the moment. I like early failures but I also like not failing when there's no need to. If I had to pick, I guess I lean toward the latter since delayed failure feels more consistent with the delayed computation in the rest of the iterator helpers.

[bakkot]

I think "consistency with delayed computation" is much less important than (indeed, is subsumed by) "is this going to be a better or worse experience to program with", and I think delaying this validation is a much worse experience to program with.

[michaelficarra]

Is it not the case that not causing a failure is a better experience than causing one unforced?

[devsnek]

I think within a program using iterators, the size of it is probably the variable part (as opposed to what apis you're using on it), so I would think of that more as hiding errors that are likely to pop up later.

[bakkot]

Is it not the case that not causing a failure is a better experience than causing one unforced?

Not when the underlying program is in fact buggy, no.

It's only a better experience if the user actually expected that it would not fail. It will be much more common to run into this by passing the wrong data but also coincidentally not consuming the iterator for other reasons, in which case the underlying bug is masked. And that is definitely a worse experience.

[bergus]

If the spec was intentionally omitting checks on the RHS when the LHS was empty, …

I doubt this was intentional. Would it be out of scope for this proposal to introduce the callability check right inside GetIterator? Or should I open an issue in tc39/ecma262 and this needs a separate discussion in the TC39 plenary? Is empty array destructuring the only case where this is observable in the current language?

[michaelficarra]

@bergus open an issue on 262 and we can do a needs consensus PR. Unfortunately it's probably too short notice to do for the upcoming meeting, so it will have to wait until December.

[hax]

I know it may be too late, but I just found that I missed this issue.

Iterator.from(x) could return a bad iterator seems a bad idea, and break the expectation of programmers, especially people can check x to say whether it's a malformed iterator, but we can't check the result of Iterator.from(x), it's always a "good" iterator...

I understand the current behavior (lazy throw) is to keep let [] = Iterator.from({}) still work. But I think the better way is making Iterator.from(badIterator) also returns a bad iterator (eg. an object without next method), so people (or other parts of the spec/future proposals) could still keep the ability to check whether it's a malformed iterator.