1066432 - Update ForOfIterator to the changes in Web IDL for sequences as iterables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The relevant change here is that Web IDL now commits to iterating once we Get a non-undefined value for @@iterator, as opposed to requiring a callable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Unfortunately, none of the existing APIs that are easy to call are overloading sequences with something else, so hard to add a test.  But I hacked in an API that tests the behavior locally, and my test passes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Update ForOfIterator to the changes in Web IDL's handling of sequences as iterables. We no commit to an iterable if we get a non-undefined value for the Symbol.iterator property, not just if we get a callable value

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8488430 [details] [diff] [review]
Update ForOfIterator to the changes in Web IDL's handling of sequences as iterables.  We no commit to an iterable if we get a non-undefined value for the Symbol.iterator property, not just if we get a callable value

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

This needs to be tested. Three ideas:

1. jsapi-tests. I think this is the way to go.

2. Maybe you can test this change based on error messages, though I guess probably not in the shell. With the changes, if obj[Symbol.iterator] is a number, for example, you'll get a TypeError like "3 is not a function", right? And without, it'll just fail to match any signature, so you'll get a different message?

3. I seem to recall there are a bunch of test-only DOM bindings (used to unit test Codegen.py and BindingUtils). It might be possible to test using those.

r=me with at least one test.

::: js/src/jsapi.h
@@ +5141,5 @@
>      };
>  
>      /*
> +     * Initialize the iterator.  If AllowNonIterable is passed then if getting
> +     * the @@iteartor property from iterable returns undefined init() will just

s/@@iteartor/@@iterator/

@@ +5143,5 @@
>      /*
> +     * Initialize the iterator.  If AllowNonIterable is passed then if getting
> +     * the @@iteartor property from iterable returns undefined init() will just
> +     * return true instead of throwing.  Callers should then check
> +     * valueIsIterable() before continuing with the iteration.

Pre-existing nit: Since it's required, please change "should" to "must".

Comment 4

[Boris Zbarsky [:bzbarsky]]

> And without, it'll just fail to match any signature, so you'll get a different message?

No, this only matters for cases when a sequence is overloaded with something, and I couldn't find any such examples so far in our IDL (though we plan to add some).

> 3. I seem to recall there are a bunch of test-only DOM bindings 

Most of these are not actually linked into the binary.  I _might_ be able to hook up something in the one that is, though.

Doing a jsapi-test here makes sense to me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with tests

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: JSAPI test, for review purposes

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8497771 [details] [diff] [review]
JSAPI test, for review purposes

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

Sweet tests.

::: js/src/jsapi-tests/testForOfIterator.cpp
@@ +11,5 @@
> +{
> +    JS::RootedValue v(cx);
> +    // Hack to make it simple to produce an object that has a property
> +    // named Symbol.iterator.
> +    EVAL("var obj = { '@@iterator': 5, [Symbol.iterator]: 5 }; obj;", &v);

If you make [Symbol.iterator] an actual method I'll be sure to clean this up in bug 918828. (That will cause the test to fail, which I'm sure to notice.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If you make [Symbol.iterator] an actual method

I'd been trying to make sure I could land in any order wrt bug 918828.

In any case, made it Array.prototype[Symbol.iterator]. Same thing for the mochitest in the patch.

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3a49bbc2c5

Comment 10

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/946948bfc6a3 to fix the mochitest bogosity.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce3a49bbc2c5
https://hg.mozilla.org/mozilla-central/rev/946948bfc6a3