1261348 - Deep destructuring errors are unclear

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Michał Gołębiowski-Owczarek [:mgol]]

In case of a simple destructuring failure, Firefox (unlike Chrome, see https://crbug.com/599850) displays a nice error message:

    var [a] = 2
    > TypeError: 2 is not iterable

If the failure is deeper, though, the message becomes convoluted:

    var [b, [a]] = [2, 3]
    > TypeError: [2, 3][Symbol.iterator](...).next(...).value is not iterable

This is weird because Firefox should try to iterate `3` in this case so I'm not sure why it shows a failure in the main array iterator.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for the report! We're actually trying to improve error documentation these days, so this report comes right on time.

> If the failure is deeper, though, the message becomes convoluted:
> 
>     var [b, [a]] = [2, 3]
>     > TypeError: [2, 3][Symbol.iterator](...).next(...).value is not iterable
> 
> This is weird because Firefox should try to iterate `3` in this case so I'm
> not sure why it shows a failure in the main array iterator.

It is actually correct, although not readable:

- [2, 3][Symbol.iterator](...) is an iterator on [2, 3]
- iter.next(...).value is just a value of this iterator, either: `2` or `3` (the latter, in this case).
- 3 is not iterable, which is the correct issue.

But it probably could be made more readable indeed.

Comment 2

[Michał Gołębiowski-Owczarek [:mgol]]

I see, thanks for the explanation. My ideal error message would be something like (view in a monospace font):

    [b, [a]] = [2, 3]
                   ^------- value not iterable

but such ASCII art might be too much. ;) "3 is not iterable" would be quite good here, it would be a little more similar to the simpler example where the error is already readable. Optionally:

   `3` in `[2, 3]` is not iterable

Comment 3

[Nicolas B. Pierron [:nbp]]

I am not sure this is a safe thing to do in general if the user override the iterator.
I think the original message makes a lot of sense if we re-define the iterator.

The problem here is that we are seeing artifact which are not part of the original sources, but produced by the BytecodeEmitter.

The question I wonder, is what should we do if the value is opaque:

  > var [a, [b]] = c;
  TypeError: c[Symbol.iterator](...).next(...).value is not iterable

Would it make sense to look inside `c` for giving the value on which we iterated over?
This might in fact be worse:

  > var [a, [b]] = c;
  TypeError: `3` in `c` is not iterable

Where does this `3` comes from?

Maybe giving an array-like view might be better (but incorrect):

  > var [a, [b]] = c;
  TypeError: The second element of `c` is not iterable

  > var [a, [b]] = [2, 3];
  TypeError: The second element of `[2, 3]` is not iterable

where `<x>[Symbol.iterator](...).value`            is converted into  `The first element of <x>`
      `<x>[Symbol.iterator](...).next(...).value`  is converted into  `The second element of <x>`
      `<x>[Symbol.iterator](...).<…y>.value`       is converted into  `The element <y> of <x>`

Also, I am not sure this would be a correct error message if `c` is a generator.  Can we speak about elements?

Comment 4

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I am not sure this is a safe thing to do in general if the user override the
> iterator.

It'd be nice to have the best of both worlds: if one overrides the iterator they've sort of agreed to have less ideally tailored error messages. Meanwhile, for original built-in iterators the messages might be more telling. Would that be achievable?

>   > var [a, [b]] = c;
>   TypeError: The second element of `c` is not iterable
> 
>   > var [a, [b]] = [2, 3];
>   TypeError: The second element of `[2, 3]` is not iterable
> 
> (...)
> 
> Also, I am not sure this would be a correct error message if `c` is a
> generator.  Can we speak about elements?

How about "the second value"? After all, iterators return values if my terminology is right.

Comment 5

[Jason Orendorff [:jorendorff]]

I'm trying to figure out what's reasonably achievable here.

The problem here is that we expand the destructuring assignment into method calls in the front end.

Maybe we could mark the auto-generated method accesses to say "this opcode is the result of desugaring; no method call actually appears in the source code". Then, when an error happens, the disassembler could notice the flag and bail out, falling back on ValueToSourceForError.

If we do it right, we would still get a nice error code for the simple case:

    var [a] = blog.length;
    > TypeError: blog.length is not iterable

and we would get a less confusing error code in the nested case:

    [b, [a]] = [2, 3]
    > TypeError: 3 is not iterable

(Incidentally, it's pretty silly that ValueToSourceForError attempts to call .toSource() on a value that just got done causing an error. We should fix that.)