Jason Orendorff [:jorendorff] Assignee
	Description • 12 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.

	Cameron McCormack (:heycam)
	Comment 1 • 12 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 12 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 12 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 12 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 12 years ago

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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 12 years ago

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

	Jeff Walden [:Waldo]
	Comment 9 • 12 years ago

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?

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 12 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

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 12 years ago

Unbitrotted, carrying forward review.

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 12 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 13 • 12 years ago

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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 14 • 12 years ago

Unbitrotted, but this is the wrong approach and I'm going to reimplement it almost from scratch.

	Jason Orendorff [:jorendorff] Assignee
	Comment 15 • 12 years ago

Unbitrotted.

	Jason Orendorff [:jorendorff] Assignee
	Comment 16 • 12 years ago

Same as v1 actually, I think.

	Jason Orendorff [:jorendorff] Assignee
	Comment 17 • 12 years ago

Carrying r+ forward again.

	Jason Orendorff [:jorendorff] Assignee
	Comment 19 • 12 years ago

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?

	Jason Orendorff [:jorendorff] Assignee
	Comment 21 • 12 years ago

Jim's not around, but that's OK. This part can wait a week or three.

	Jason Orendorff [:jorendorff] Assignee
	Comment 23 • 12 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.

	Brian Hackett [Laid off!]
	Comment 24 • 12 years ago

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?

	Brian Hackett [Laid off!]
	Comment 25 • 12 years ago

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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 26 • 12 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.

	Brian Hackett [Laid off!]
	Comment 27 • 12 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 28 • 12 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.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 12 years ago

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

r=me.  Sorry for the lag....

	Jason Orendorff [:jorendorff] Assignee
	Comment 30 • 12 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

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 12 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 32 • 12 years ago

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

One more to go. Please keep open still.

	Jim Blandy :jimb
	Comment 33 • 12 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.

	Jim Blandy :jimb
	Comment 34 • 12 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!!!

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f44eca03418c

	(no longer active)
	Comment 36 • 12 years ago

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

	(no longer active)
	Comment 37 • 12 years ago

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.  :-)

	(no longer active)
	Comment 38 • 12 years ago

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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 39 • 12 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.

	Luke Wagner [:luke]
	Comment 40 • 12 years ago

Try again?

	Ed Morley [:emorley]
	Comment 41 • 12 years ago

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

	Jim Blandy :jimb
	Comment 42 • 12 years ago

Can this bug be closed, then?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 43 • 11 years ago

(In reply to Jim Blandy :jimb from comment #42)
> Can this bug be closed, then?

	Jason Orendorff [:jorendorff] Assignee
	Comment 44 • 11 years ago

Yes. This is done.

	Tooru Fujisawa [:arai]
	Comment 45 • 10 years ago

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