Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Change for-of loop to work in terms of .iterator() and .next()

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Other Branch
mozilla17
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(6 attachments, 10 obsolete attachments)

37.23 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
42.40 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
7.29 KB, patch
Details | Diff | Splinter Review
40.36 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
3.51 KB, patch
Details | Diff | Splinter Review
1.49 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Cc-ing some randomly chosen DOM and WebIDL people in hopes of getting free advice.

The for-of loop (added in bug 699565) is currently fairly magical. The iterator objects are not exposed to JS code. It is impossible for plain old Objects to be for-of-iterable; the only way for a user-defined object to be iterable is if it's a Proxy.

I've talked this over very briefly with dherman and samth, and I think we can make this simpler and at the same time a lot more user-friendly.

I propose that the for-of loop:

    for (V of EXPR)
        STMT

work like this:

    {
        let %iter = (EXPR).iterator();
        for (;;) {
            let %val;
            try {
                %val = %iter.next();
            } catch (exc) {
                if (exc instanceof StopIteration)
                    break;
                else
                    throw exc;
            }

            V = %val;
            STMT
        }
    }

where %val and %iter are different from all other identifiers.

With this change, it's easy to make an object iterable. Give it an iterator() method. We would define Array.prototype.iterator and so on--and also for the DOM.
Less magic sounds better to me.  We could easily have Web IDL require all objects with indexed properties (like NodeList) to have an iterator() function on their prototype.  Sounds like a no-brainer in terms of usefulness.  (On Window too?  Maybe...)
(Assignee)

Comment 2

5 years ago
Implementation notes.

JSOP_ITER will change to call .iterator(). The iternext/itermore opcodes already call .next as a fallback; it'll just become the normal behavior in the for-of case. This will be slow, for now. Type inference could help a lot.

Array.prototype.iterator will be added. It'll return an Iterator object with proto Iterator.prototype. The native function Iterator.prototype.next will be changed to be slightly generic, so that it'll work with Array iterators, generator iterators, and other types (like Map and Set iterators; see bug 725909), though not with arbitrary objects.

Proxy::iteratorNext and ProxyHandler::iteratorNext go away; next is an ordinary method call now.
(Assignee)

Comment 3

5 years ago
Created attachment 617074 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject

This patch doesn't affect user-visible behavior. Just a little C++ refactoring and moving stuff around.

I considered calling this "Enumerator" but PropertyIteratorObject is precisely what it is. Note that this kind of object is sometimes exposed to script (if you call Iterator({a:1, b:2}) you get a property iterator object; ES6 will probably change the API but still expose such iterators somehow) and sometimes not (a for-in loop makes an iterator of the same Class, but it lives in a stack slot, has getProto() == NULL, and doesn't escape).
Assignee: general → jorendorff
Attachment #617074 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

5 years ago
I just realized there's a pretty big bug in part 2 of this work. I will have to come back to it in 2 weeks since all next week is dedicated to debugger.
(Assignee)

Comment 5

5 years ago
Created attachment 617076 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), v1

Just for fun. The bug is that I didn't add an iterator method to the DOM classes that need one. bz already pointed out how to do it, I just forgot, and haven't submitted this stack to the try server yet.
(Assignee)

Comment 6

5 years ago
Created attachment 617080 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, v1

This is the main part, though it's not much bigger than part 1 or part 2...

The following changes are happening in this patch:

  - Up to now, for-of iteration was achieved by js_IteratorNext having
    magical knowledge of certain kinds of iterator object. With this patch,
    those special cases are removed and js_IteratorNext just calls iter.next().
    Simple sane semantics.

    Well, except for PropertyIteratorObjects, which are still special-cased.
    But that is strictly for speed (for-in is still the common case here
    and the only thing that shows up on benchmarks).

  - All iterators inherit from Iterator.prototype, which already has a .next()
    method. This method just calls js_IteratorNext. But now that would be
    circular, so I changed it to call a class hook:
      iterobj->getClass()->ext.iteratorNext
    If you try to do Iterator.prototype.next.call(obj) on an object that
    has a NULL iteratorNext class hook, it'll throw a TypeError at you.
    Not that you ever would.

  - PropertyIteratorObjects and ElementIteratorObjects each have an
    iteratorNext hook that gets the next property/element.

  - Wrapper::iteratorNext was really buggy. The replacement does the obvious
    thing and just delegates to the wrapped object's iteratorNext class hook.
    This also adds a "next" trap for scripted proxies.

Some of these design decisions are pretty subtle. Things may change later as ES6 coalesces. This is a snapshot of the current design. Certainly having for-of treat everything as a plain old object and call plain old .iterator() and .next() methods is a simplicity win for users; I don't expect TC39 to back away from that.

This will also help me make Maps and Sets iterable, which I'm eager to finish (bug 725909).
(Assignee)

Comment 7

5 years ago
Created attachment 617081 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v1
(Assignee)

Comment 8

5 years ago
Created attachment 617082 [details] [diff] [review]
Part 5 - make ctypes arrays iterable, v1
Comment on attachment 617074 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject

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

::: js/src/jsiter.cpp
@@ +486,3 @@
>      }
>  
> +    return (PropertyIteratorObject *) NewBuiltinClassInstance(cx, &PropertyIteratorObject::class_);

Use &asPropertyIterator(), please, like above.

::: js/src/jsiter.h
@@ +100,5 @@
>  
>      void mark(JSTracer *trc);
>  };
>  
> +class PropertyIteratorObject : public JSObject {

{ on new line per style guide.

::: js/src/jsobjinlines.h
@@ +49,5 @@
>  #include "jscntxt.h"
>  #include "jsdate.h"
>  #include "jsfun.h"
>  #include "jsgcmark.h"
> +#include "jsinterp.h"

Why this addition?
Attachment #617074 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 10

5 years ago
I need to add a few lines of code here to finish this bug:

https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dombindingsgen.py?rev=7082192622e6#717
Attachment #617076 - Attachment is patch: true
(Assignee)

Comment 11

5 years ago
Created attachment 632779 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v2

Unbitrotted, carrying forward review.
Attachment #617074 - Attachment is obsolete: true
Attachment #632779 - Flags: review+
(Assignee)

Comment 12

5 years ago
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #9)
> Use &asPropertyIterator(), please, like above.

Good idea. Done.

> > +class PropertyIteratorObject : public JSObject {
> { on new line per style guide.

OK.

> > +#include "jsinterp.h"
> Why this addition?

I dunno. It works fine without it, so I removed it again.
(Assignee)

Comment 13

5 years ago
Created attachment 632783 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), WIP 2

What comment 10 said still applies to this patch. It needs more work (and tests) in order not to break for-of on arraylike DOM objects.
Attachment #617076 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 632784 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, WIP 2

Unbitrotted, but this is the wrong approach and I'm going to reimplement it almost from scratch.
Attachment #617080 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 632785 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, WIP 2

Unbitrotted.
Attachment #617081 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 632792 [details] [diff] [review]
Part 5 - make ctypes arrays iterable, WIP 2

Same as v1 actually, I think.
Attachment #617082 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 633646 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v3

Carrying r+ forward again.
Attachment #632779 - Attachment is obsolete: true
Attachment #633646 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 633647 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), v3
Attachment #632783 - Attachment is obsolete: true
Attachment #633647 - Flags: review?(bhackett1024)
(Assignee)

Comment 19

5 years ago
Created attachment 633666 [details] [diff] [review]
Part 2a - Implement .iterator() for arraylike DOM objects

Things to look out for:

- Are there any arraylike DOM objects that don't get the new bindings, or don't have an IndexGetter?

- Are there any other DOM objects that should be iterable?

- Are there cases where one prototype object for an iterable interface inherits from another, so that we're pointlessly shadowing that .iterator method with another .iterator?
Attachment #633666 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

5 years ago
Created attachment 633667 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, v3
Attachment #632784 - Attachment is obsolete: true
Attachment #633667 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #633667 - Flags: review?(luke) → review?(bhackett1024)
(Assignee)

Updated

5 years ago
Attachment #633646 - Attachment description: v1 → Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v3
(Assignee)

Comment 21

5 years ago
Created attachment 633668 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v3

Jim's not around, but that's OK. This part can wait a week or three.
Attachment #632785 - Attachment is obsolete: true
Attachment #633668 - Flags: review?(jimb)
(Assignee)

Comment 22

5 years ago
Created attachment 633671 [details] [diff] [review]
Part 5 - make ctypes arrays iterable, v3
Attachment #632792 - Attachment is obsolete: true
Attachment #633671 - Flags: review?(bhackett1024)
(Assignee)

Comment 23

5 years ago
dev-doc-needed: Note that what it currently says here is already wrong:
https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of

It says that for-of loops get property values. That isn't right. 'for (x in obj)' iterates over the names of obj's properties. But 'for (x of obj)' will call obj.iterator() and loop over the values produced by that iterator's .next() method. This is almost exactly how Python's for-in loop works, only instead of a .iterator() method, Python has a .__iter__() method.

When this bug lands, users will be able to write their own iterators in order to make the for-of loop do whatever they want. For example:

  var obj = {iterator: function () { yield 1; yield 2; yield 3; }};
  for (var x of obj)
      print(x);

This prints 1, then 2, then 3.

Also, any object that inherits from Array.prototype is automatically iterable, just because Array.prototype has a .iterator method on it.

  var obj = Object.create(Array.prototype);
  obj.length = 3;
  obj[0] = 'A';
  obj[1] = 'B';
  obj[2] = 'C';

  for (var x of obj)
      print(x);

This prints A, then B, then C.
Keywords: dev-doc-needed
Comment on attachment 633647 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), v3

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

::: js/src/jsatom.tbl
@@ +53,5 @@
>  DEFINE_ATOM(index, "index")
>  DEFINE_ATOM(input, "input")
>  DEFINE_ATOM(toISOString, "toISOString")
> +DEFINE_ATOM(iterator, "iterator")
> +DEFINE_ATOM(iterator_, "__iterator__")

I think that iterator_ should have a more verbose/ugly name to distinguish it from the 'iterator' proper, maybe iteratorIntrinsic?
Attachment #633647 - Flags: review?(bhackett1024) → review+
Comment on attachment 633667 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, v3

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

::: js/src/jsiter.cpp
@@ +841,5 @@
> +    GlobalObject *global = GetCurrentGlobal(cx);
> +    RootedObject proto(cx, global->getOrCreateElementIteratorPrototype(cx));
> +    if (!proto)
> +        return NULL;
> +    RootedObject iterobj(cx, NewObjectWithGivenProto(cx, &class_, proto, global));

The root on iterobj itself isn't necessary, as setReservedSlot cannot trigger a GC.

@@ +872,5 @@
> +        obj = ValueToObject(cx, target);
> +        if (!obj)
> +            return false;
> +        if (!js_GetLengthProperty(cx, obj, &length))
> +            goto error;

The semantics here look weird and this at least needs a comment.  What are the conditions when it is OK to just propagate failures vs. necessary to close the iterator down?  Also, the error label should be renamed to closeIterator or something.

::: js/src/jsiter.h
@@ +97,5 @@
> + *      to any other object to create iterators over that object's elements.
> + *
> + *    - The next() method of an Array iterator is non-reentrant. Trying to reenter,
> + *      e.g. by using it on an object with a length getter that calls it.next() on
> + *      the same iterator, causes a TypeError.

How do we ensure that next() is non-reentrant?  ElementIteratorObject::next doesn't seem to do any guarding.  Either way, a comment in the implementation about this would be good.
Attachment #633667 - Flags: review?(bhackett1024) → review+
Attachment #633671 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

5 years ago
Blocks: 725909
(Assignee)

Comment 26

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #25)
> The root on iterobj itself isn't necessary, as setReservedSlot cannot
> trigger a GC.

Removed.

You found two bugs in ElementIteratorObject::next():

> The semantics here look weird and this at least needs a comment. [...]

Bug 1. Easily fixed. I changed it so that any time we return false, we do it via 'goto close;'.

> How do we ensure that next() is non-reentrant?

Bug 2. This one's a bit more work; I'll fix (and test) in a separate patch.

For the time being, re-entering will not cause anything horrible to happen, it'll just behave contrary to spec in some corner cases (and honestly there is no spec, the comment is sort of a guess).

BTW, both bugs have the same cause: I turned the old iteratorNext() code into a script-visible .next() method without re-reading it closely. And I had, perhaps foolishly, optimized iteartorNext() with the knowledge that iterator objects were not script-visible. Oops.

Thanks.
(In reply to Jason Orendorff [:jorendorff] from comment #26)
> For the time being, re-entering will not cause anything horrible to happen,
> it'll just behave contrary to spec in some corner cases (and honestly there
> is no spec, the comment is sort of a guess).

Hmm, if the spec doesn't insist on this, it might be better to just leave this as is and fix the later comment to not discuss reentrancy.  The behavior looks fine to me in the reentrant case, some implementation details exposed for sure but oh well.
(Assignee)

Comment 28

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #27)
> Hmm, if the spec doesn't insist on this, it might be better to just leave
> this as is and fix the later comment to not discuss reentrancy.  The
> behavior looks fine to me in the reentrant case, some implementation details
> exposed for sure but oh well.

Yup. I agree and I went through and re-spec'd it that way
http://wiki.ecmascript.org/doku.php?id=harmony:iterators#arrayiteratorprototype_.next
and changed the comment.

Comment 29

5 years ago
Comment on attachment 633666 [details] [diff] [review]
Part 2a - Implement .iterator() for arraylike DOM objects

r=me.  Sorry for the lag....
Attachment #633666 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 30

5 years ago
Landed the first four patches on m-i. Two to go.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a488b71b69a
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb49c3730a97
https://hg.mozilla.org/integration/mozilla-inbound/rev/4313740f1adc
https://hg.mozilla.org/integration/mozilla-inbound/rev/24feaa8bd894
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/3a488b71b69a
https://hg.mozilla.org/mozilla-central/rev/cb49c3730a97
https://hg.mozilla.org/mozilla-central/rev/4313740f1adc
https://hg.mozilla.org/mozilla-central/rev/24feaa8bd894
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44eca03418c

One more to go. Please keep open still.

Comment 33

5 years ago
Comment on attachment 633668 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v3

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

I think you'll also need to include strings in the array of roots maintained at the top of HeapReverser::traverseEdge. HeapReverser::nodeToValue needs to be fixed as well.

I'm also surprised ReferenceFinder::representable didn't need to be changed. Until it is, I think findReferences will fail to notice ropes referring to their left and right halves.
Attachment #633668 - Flags: review?(jimb)

Comment 34

5 years ago
Comment on attachment 633668 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v3

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

I think you'll also need to include strings in the array of roots maintained at the top of HeapReverser::traverseEdge. HeapReverser::nodeToValue needs to be fixed as well.

I'm also surprised ReferenceFinder::representable didn't need to be changed. Until it is, I think findReferences will fail to notice ropes referring to their left and right halves.

::: js/src/jit-test/tests/for-of/string-iterator-gc.js
@@ +1,4 @@
> +// String iterators keep the underlying string alive.
> +
> +load(libdir + "referencesVia.js");
> +var song = Array(17).join("na") + "batman!";

You've *got* use use the NaN trick here!!!
https://hg.mozilla.org/mozilla-central/rev/f44eca03418c
This bug, or something which was merged to mozilla-central in the same push, caused a permanent orange on Linux32 mochitest-chrome for builds without frame pointers (example log: <https://tbpl.mozilla.org/php/getParsedLog.php?id=13227357&tree=Profiling&full=1>).  This is important because framepointers are not enabled on Aurora, so this permaorange will appear there on the next uplift.  I backed out this patch as part of the rest of the js and xpconnect patches in the same merge push.  Please test this patch locally (and push to the try server if needed by taking out the --enable-profiling command from the Linux32 mozconfig) and reland when you make sure that it's not the cause behind the perma-orange.  I apologize in advance for the inconvenience in case this patch is not at fault.

Backout changeset:
https://hg.mozilla.org/mozilla-central/rev/b9c98f0d0fde
https://hg.mozilla.org/mozilla-central/rev/0d2b03dff288
https://hg.mozilla.org/mozilla-central/rev/aadf6091245b
https://hg.mozilla.org/mozilla-central/rev/86cf7f8a124a
https://hg.mozilla.org/mozilla-central/rev/015cbcaf8552
In order to test whether this patch is at fault, push it to try with --enable-profiling removed from browser/config/mozconfigs/linux32/nightly, and if you get a green Linux Moth run, you're good to go!

And in turn you can reproduce the crash on try with your patch, I'd suggest you keep the possibility of having hit a compiler bug in mind.  :-)
Also, note that these backouts indeed made the Linux32 Moth runs on the profiling branch green again, so we can be fairly certain that one of the backed out patches was at fault.

Updated

5 years ago
Depends on: 771027
(Assignee)

Updated

5 years ago
Depends on: 749010

Updated

5 years ago
Blocks: 768692
(Assignee)

Comment 39

5 years ago
I have been trying to land this for weeks now.

The backouts in comment 36 were due to the craziness in bug 749010, but my latest Try Server runs also show some orange.
  https://tbpl.mozilla.org/?tree=Try&rev=db99191b1353
On OSX only, the browser doesn't start. No clue why, no info in the log. On my machine (also a mac), a debug browser build runs fine.

Comment 40

5 years ago
Try again?

Updated

5 years ago
Blocks: 775781
https://hg.mozilla.org/mozilla-central/rev/18295d17cba2
https://hg.mozilla.org/mozilla-central/rev/7ecd5e911ba4
https://hg.mozilla.org/mozilla-central/rev/81b3c7b3a561
https://hg.mozilla.org/mozilla-central/rev/a59567902ad0

Comment 42

5 years ago
Can this bug be closed, then?

Updated

4 years ago
Depends on: 907077
(In reply to Jim Blandy :jimb from comment #42)
> Can this bug be closed, then?
Flags: needinfo?(jorendorff)
(Assignee)

Comment 44

4 years ago
Yes. This is done.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(jorendorff)
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [leave open]
Target Milestone: --- → mozilla25
Target Milestone: mozilla25 → mozilla17
Whiteboard: [DocArea=JS]
Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/17
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
    (updated compat table only, since the behavior is changed again)
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.