952873 - Expose API for ForOfIterator, including non-fatal behavior on non-callable iterators

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

To implement the semantics proposed in http://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/0370.html I would like something just like ForOfIterator, but that would have an option to bail out without throwing on the "!callee.isObject() || !callee.toObject().isCallable()" test in init() and allow the caller to check whether that test failed inside the init() call.

What's a sane way to do this?  The obvious way seems to be to move the class declaration (or at least its data members and constructor, with the init() and next() methods on a subclass) to jsfriendapi and have friend api methods that call the subclass methods.  Seems cumbersome, but I don't see an obvious better option.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Expose a friend API for doing for-of iteration.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8351103 [details] [diff] [review]
Expose a friend API for doing for-of iteration.

I realize the naming is terrible; I was just trying to make minimal changes to the JS engine code as a proof of concept.

How does this seem as a general approach?

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8351103 [details] [diff] [review]
Expose a friend API for doing for-of iteration.

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

Looks fine to me. I'm pretty sure you can make those JS_FRIEND_APIs methods of the class. We have a few JS_FRIEND_API methods here and there.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8351103 [details] [diff] [review]
Expose a friend API for doing for-of iteration.

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

Seems more or less reasonable, modulo PublicForOfIterator being a bad name (not thinking much about what would be better).  Probably should be in js/public/Iteration.h as public API, too.  Oh, and the class and any exposed methods need API documentation by them in comments.

::: js/src/jsiter.cpp
@@ +1292,4 @@
>      if (!callee.isObject() || !callee.toObject().isCallable()) {
> +        if (!throwOnNonCallable) {
> +            return true;
> +        }

No braces here.

@@ +1317,5 @@
> +                      bool throwOnNonCallable)
> +{
> +    // Make sure that ForOfIterator doesn't have any members of its own that
> +    // it'll try to use
> +    JS_STATIC_ASSERT(sizeof(ForOfIterator) == sizeof(PublicForOfIterator));

static_assert with a reasonable error message.  JS_STATIC_ASSERT is bad and shouldn't be used in any new code (and should be removed from existing code, but doing so isn't trivial, ergo it lingers).

@@ +1360,5 @@
> +                      bool *done)
> +{
> +    // Make sure that ForOfIterator doesn't have any members of its own that
> +    // it'll try to use
> +    JS_STATIC_ASSERT(sizeof(ForOfIterator) == sizeof(PublicForOfIterator));

Ditto.

::: js/src/jsiter.h
@@ +237,5 @@
>   * it.close() may be skipped only if something in the body of the loop fails
>   * and the failure is allowed to propagate on cx, as in this example if DoStuff
>   * fails. In that case, ForOfIterator's destructor does all necessary cleanup.
>   */
> +class ForOfIterator : public PublicForOfIterator

This ForOfIterator is what should change name, I think.  Having ForOfIterator as the public thing seems roughly right to me.

@@ +245,2 @@
>  
> +    bool init(HandleValue iterable, bool throwOnNonCallable = true);

Make this an enum argument that spells out what happens.  Random true/false arguments with no context in the name are unhelpful to read.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename the cx member of ForOfIterator to cx_ instead.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Expose a friend API for doing for-of iteration.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365364 [details] [diff] [review]
part 1.  Rename the cx member of ForOfIterator to cx_ instead.

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

All right.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365365 [details] [diff] [review]
part 2.  Expose a friend API for doing for-of iteration.

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

r=me with the comments considered

::: js/src/jsfriendapi.h
@@ +1892,5 @@
> + *       if (!DoStuff(cx, val))
> + *         return false;
> + *     }
> + */
> +class MOZ_STACK_CLASS JS_FRIEND_API(ForOfIterator) {

Make it public, if you don't mind. jsapi.h.

@@ +1908,5 @@
> +        ThrowOnNonIterable,
> +        AllowNonIterable
> +    };
> +
> +    /*  

Splinter-detected trailing whitespace on this comment line.

@@ +1915,5 @@
> +     * of throwing.  Callers should then check valueIsIterable() before
> +     * continuing with the iteration.
> +     */
> +    JS_FRIEND_API(bool) init(JS::HandleValue iterable,
> +                             NonIterableBehavior nonIterableBehavior = ThrowOnNonIterable);

So, I guess SM is still a DLL on Windows. Historically we've tried to avoid going through JS_FRIEND/PUBLIC_APIs when the caller is inside SM, even resorting to weirdness to avoid the slower calling convention. Perhaps it doesn't matter here. What do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> What do you think?

I don't know, honestly.

What sort of API contortions would we need to avoid the public calling convention?  initFast/nextFast?  Something else?

My use case is outside SM, of course, so it's all the same for my purposes...

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Make it public, if you don't mind. jsapi.h.

Done, namespace JS, JS_PUBLIC_API.

> Splinter-detected trailing whitespace on this comment line.

Fixed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae58fcd61a9
   https://hg.mozilla.org/integration/mozilla-inbound/rev/92dfed2592ae

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b9cf7a013b62 for causing failures like these: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=33713242&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33713084&tree=Mozilla-Inbound

Comment 13

[Boris Zbarsky [:bzbarsky]]

Gah.  Fixed up those bits, did a try push at https://tbpl.mozilla.org/?tree=Try&rev=8af9ac44b027 and repushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ba0255edf1
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f45d7708826f

Comment 14

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/2ebe01b13b32 because somehow the build fixes ended up in the try-syntax changeset on try.  :(

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5ba0255edf1
https://hg.mozilla.org/mozilla-central/rev/f45d7708826f
https://hg.mozilla.org/mozilla-central/rev/2ebe01b13b32