1216379 - Improve the error message raised when using an iterable in a for...of loop

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

Currently, given:

for (x of val);

If val is not an iterable, we get the following error message:

"TypeError: val[Symbol.iterator] is not a function"

This is completely unreadable.  It would be nice if we got the following instead:

"TypeError: val is not an iterable and cannot be used in for...of loops"

Comment 1

[Boris Zbarsky [:bzbarsky]]

The basic problem, I suspect, is that we actually desugar for-of loops into their constituent bits (get Symbol.iterator, call it, etc) in the parser.  That gives us nice benefits like the JIT being able to optimize them well, but means that by the time the bytecode or JIT is executing we don't really know we're a for-of loop; we just know that we're doing the equivalent of "var iter = val[Symbol.iterator]();" and it's throwing.

For comparison, ForOfIterator will report JSMSG_NOT_ITERABLE in similar circumstances, because it knows we're trying to iterate, as will the GetIterator() self-hosting builtin....

We could maybe try to pattern-match and when doing JSMSG_NOT_FUNCTION see whether {0} looks like "something[Symbol.iterator]" and if so rewrite the message a bit or something?

Comment 2

[Jan de Mooij [:jandem]]

Attachment: WIP

This patch emits a new JSOP_CALLITER op instead of JSOP_CALL so that we can pattern match this. We could also use a source note or a nop bytecode after the CALL. Suggestions welcome.

It fixes for-of and also array destructuring and spread.

Here are some examples:

val = {}; for (var x of val) {}
before: TypeError: val[Symbol.iterator] is not a function
after:  TypeError: val is not iterable

var a = [...{}];
before: TypeError: (intermediate value)[Symbol.iterator] is not a function
after:  TypeError: ({}) is not iterable

var [a] = Math;
before: TypeError: Math[Symbol.iterator] is not a function
after:  TypeError: Math is not iterable

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8676136 [details] [diff] [review]
WIP

Review of attachment 8676136 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter.cpp
@@ +343,5 @@
> +static MOZ_NEVER_INLINE bool
> +ReportIsNotFunctionOrIterable(JSContext* cx, const CallArgs& args, MaybeConstruct construct)
> +{
> +    ScriptFrameIter iter(cx);
> +    if (!iter.done() && *iter.pc() == JSOP_CALLITER) {

Augh, bytecode inspection. We can't be sure that that CALLITER operation is what actually caused this function call. The CALLITER may have successfully called a native function that is now trying to do something else entirely. If so, the error message will now be wrong instead of just hard to understand. We've had this happen.

A more correct alternative would be to pass another bit to js::Invoke() when it is called directly from JSOP_CALLITER. (This approach of passing more info to Invoke could probably also be used to avoid using the expression decompiler when the non-function being called was not in fact produced by bytecode. Invoke is only called from slow paths, so this shouldn't hurt performance.)

Or, we could detect this error separately in all implementations of JSOP_CALLITER:
- the interpreter
- DoCallFallback
- a separate variant of js::jit::InvokeFunction
Again, we're already in a slow path.

Comment 4

[Jason Orendorff [:jorendorff]]

Alternatively, if there's a way to check, in ReportIsNotFunctionOrIterable, that `args` is really the same memory as the top of the interpreter stack, that would be sufficient. We would never give the more specific error message incorrectly.

But I don't know that such a test is possible when we're called from Ion.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Good point about natives calling back into Invoke, I should have seen that.

This patch throws the exception in the interpreter loop or Baseline's DoCallFallback. Ion bails to Baseline if the callee is not an object, so it doesn't need any changes.

I considered adding a `jsbytecode* maybepc = nullptr` to Invoke, but it seemed a bit too heavyweight for this.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> I considered adding a `jsbytecode* maybepc = nullptr` to Invoke,

(But if you'd prefer this (or some other argument type) I'd be happy to change it; I don't have a strong opinion on this.)

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8679066 [details] [diff] [review]
Patch

Review of attachment 8679066 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, I like it. Sorry for the slow review here. I don't know how I managed to not see it for so long. :-P

::: js/src/jit/BaselineIC.cpp
@@ +9025,5 @@
>                     op == JSOP_FUNAPPLY ||
>                     op == JSOP_EVAL ||
>                     op == JSOP_STRICTEVAL);
> +        if (op == JSOP_CALLITER && callee.isPrimitive()) {
> +            MOZ_ASSERT(argc == 0, "thisv must be on top of the stack");

Nice assertion!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5dfdfe0150

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d5dfdfe0150