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

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla35
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created 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
Attachment #8488430 - Flags: review?(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".
Attachment #8488430 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 4

4 years ago
> 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.
(Assignee)

Updated

4 years ago
Attachment #8488430 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 8497771 [details] [diff] [review]
JSAPI test, for review purposes
Attachment #8497771 - Flags: review?(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.)
Attachment #8497771 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 8

4 years ago
> 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.
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3a49bbc2c5
Flags: in-testsuite+
Target Milestone: --- → mozilla35
https://hg.mozilla.org/mozilla-central/rev/ce3a49bbc2c5
https://hg.mozilla.org/mozilla-central/rev/946948bfc6a3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 1073853
Depends on: 1079640
You need to log in before you can comment on or make changes to this bug.