Last Comment Bug 574130 - (harmony:spreadarray) Prototype Harmony's spread operator for arrays in SpiderMonkey
: Prototype Harmony's spread operator for arrays in SpiderMonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla16
Assigned To: :Benjamin Peterson
: Jason Orendorff [:jorendorff]
Depends on: 761410 842884
Blocks: es6
  Show dependency treegraph
Reported: 2010-06-23 13:38 PDT by Brendan Eich [:brendan]
Modified: 2014-05-19 08:30 PDT (History)
19 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

draft implementation for array initializers (27.38 KB, patch)
2012-05-22 16:21 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
array implementation (24.35 KB, patch)
2012-06-04 18:38 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review
address review comments (25.46 KB, patch)
2012-06-05 18:28 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description User image Brendan Eich [:brendan] 2010-06-23 13:38:36 PDT
See the bug URL. We want tracing, jaegering, decompilation, as usual. Spread in array initialisers too.

Comment 1 User image Brendan Eich [:brendan] 2010-06-25 15:13:37 PDT
This time (ahem, unlike proxies -- which got a pass perhaps because they're used by wrappers and other clients, but maybe that's no excuse), use jsversion.h and a JS_HAS_SPREAD macro, like the old ones there (retire some, move minimum compile-time JSVERSION forward).

Same for harmony:restparams.

We can leave these config'ed off in Firefox 4 to reduce risk to the codebase, but we shouldn't hide them from users, including in final releases, out of fear of changing them during standardization later. IMHO!

Comment 2 User image :Benjamin Peterson 2012-05-22 16:21:34 PDT
Created attachment 626236 [details] [diff] [review]
draft implementation for array initializers
Comment 3 User image Jason Orendorff [:jorendorff] 2012-05-31 15:33:30 PDT
OK. I just chatted with dvander about this, and it would be easier for the jit if the only magic thing about [a, b, ...c, d] is the ...c part, so instead of:

      getlocal a

      getlocal b

      getlocal c

      getlocal d
    arrayspread 6

it looked like this:

      int 0
      getlocal a

      int 1
      getlocal b

      getlocal c

      getlocal d

Note that this is more like how array literals already look. More opcodes, but that helps all the analyses and makes for leaner, faster jitcode.

We already have JSOP_ARRAYPUSH; we emit it in array comprehensions. JSOP_ARRAYPUSHALL would be the new magic.

dvander says it'd be even better to expand the loop inline, but it's not necessary now. Easy enough to do that part later.
Comment 4 User image Jason Orendorff [:jorendorff] 2012-05-31 15:46:59 PDT
Another nice thing about JSOP_ARRAYPUSHALL is that its implementation can use js::ForOf to start with, and changing it later (once TC39 decides) won't affect bytecode.
Comment 5 User image Jason Orendorff [:jorendorff] 2012-05-31 19:40:50 PDT
Things to test:

- evaluation order in [a(), ...b(), c()]
- In the shell there's a wrap(obj) function;
  test [...wrap(obj)] does the same thing as [...obj]
- [...[x for (x of y)]]
  [...(x for (x of y))]
- Trailing commas and "Elision"s:
  [...x,]  // equivalent to [...x]
  [...x,,] // [...x, <hole>]
  [, ...x] // [<hole>, ...x]
- Syntax errors:
  (1 ... n)
  [1 ... n]
  [...x for (x of y)]
Comment 6 User image Jason Orendorff [:jorendorff] 2012-05-31 19:48:25 PDT
>- evaluation order in [a(), ...b(), c()]

In particular, b()'s iterator() method should be called before c(). Even if TC39 decides to use the .length protocol here, the order of evaluation is observable:

  js> [...{get length() { print("ONE"); return 0; }}, print("TWO")]

  (a bug; it should print ONE then TWO)
Comment 7 User image Jason Orendorff [:jorendorff] 2012-06-01 02:51:57 PDT fact, you can observe it using only arrays:

  js> var a = [1, 2, 3];
  js> [...a, a.pop()]
  [1, 2, 3]  // should be [1, 2, 3, 3]
Comment 8 User image :Benjamin Peterson 2012-06-04 18:38:24 PDT
Created attachment 630031 [details] [diff] [review]
array implementation
Comment 9 User image Jason Orendorff [:jorendorff] 2012-06-05 15:41:49 PDT
Comment on attachment 630031 [details] [diff] [review]
array implementation

Review of attachment 630031 [details] [diff] [review]:

Looks much better! Just a few comments. I would still like to look over the final draft.

::: js/src/frontend/Parser.cpp
@@ +6745,5 @@
>               * the JSOP_GETLOCAL and JSOP_SETLOCAL ops. These ops have an
>               * immediate operand, the local slot's stack index from fp->spbase.
>               *
>               * The array comprehension iteration step, array.push(i * j) in
>               * the example above, is done by <i * j>; JSOP_ARRAYCOMP <array>,

While you're in the neighborhood, please correct this typo.
It should say JSOP_ARRAYPUSH not COMP.

::: js/src/jsfriendapi.h
@@ +477,5 @@
>  #define JSITER_KEYVALUE   0x4   /* destructuring for-in wants [key, value] */
>  #define JSITER_OWNONLY    0x8   /* iterate over obj's own properties only */
>  #define JSITER_HIDDEN     0x10  /* also enumerate non-enumerable properties */
>  #define JSITER_FOR_OF     0x20  /* harmony for-of loop */
> +#define JSITER_NULL_EMPTY 0x40  /* null and undefined should be treated as empty iterator */

Blah! New flag! Don't you know how hard it is to get rid of these?! ;)

It seems unlikely that TC39 will treat null and undefined differently depending on whether you write
    [_v for (_v of x)]

So why not just change JSITER_FOR_OF's behavior? Preferably in another bug, since they're separable concerns. Maybe after running it by dherman.

::: js/src/jsinfer.cpp
@@ +3857,3 @@
>                  } else {
> +                    // Iterator could put arbitrary things into the array.
> +                    types->addType(cx, Type::UnknownType());

Nit: I think this reads a little better the other way around ("else if (op == JSOP_SPREAD)").

@@ +3862,5 @@
>          } else {
>              pushed[0].addType(cx, Type::UnknownType());
>          }
> +        switch (op) {
> +        case JSOP_SPREAD:

Style nit: indent case-labels and "default:" two spaces, per

::: js/src/jsinterp.cpp
@@ +3270,5 @@
>              goto error;
>      }
> +    if (op == JSOP_INITELEM_INC) {
> +        JS_ASSERT(obj->isArray());
> +        regs.sp[-2].setInt32(JSID_TO_INT(id) + 1);

Overflow check here.

@@ +3279,3 @@
>  }

Don't add another END_CASE. Elsewhere we only use one; the second one is just going to expand to a bit of unreachable code.

@@ +3289,5 @@
> +    while (true) {
> +        Value rval;
> +        bool onward;
> +        if (!IteratorMore(cx, iterator, &onward, &rval))
> +            goto error;

DRY suggests using js::ForOf (defined in jsiter.h) rather than duplicating its code.

It looks like JSOP_ITER/JSOP_ENDITER must be used with "try notes" in order to make sure the iterator is closed as we exit the loop due to an exception. (I don't think it makes any observable difference here, but it is hard to be sure since JSOP_ITER is an extension point for C++-based proxies, and there are all sorts of proxies out there.) Using js::ForOf would eliminate the need to worry about that.

@@ +3297,5 @@
> +        if (!IteratorNext(cx, iterator, &item))
> +            goto error;
> +        RootedId id(cx, INT_TO_JSID(count));
> +        if (!arr->defineGeneric(cx, id, item, NULL, NULL, JSPROP_ENUMERATE))
> +            goto error;

In JSOP_INITELEM, we have no guarantee that the index is really an integer (partly due to E4X), but here we do know and so we can use defineElement rather than defineGeneric. It'll be slightly faster, and we won't have to root a jsid.

@@ +3298,5 @@
> +            goto error;
> +        RootedId id(cx, INT_TO_JSID(count));
> +        if (!arr->defineGeneric(cx, id, item, NULL, NULL, JSPROP_ENUMERATE))
> +            goto error;
> +        count++;

Overflow check here too.

::: js/src/jsopcode.tbl
@@ +223,5 @@
>  OPDEF(JSOP_NEWOBJECT, 91, "newobject",  NULL,         5,  0,  1, 19,  JOF_OBJECT|JOF_TYPESET)
>  OPDEF(JSOP_ENDINIT,   92, "endinit",    NULL,         1,  0,  0, 19,  JOF_BYTE)
>  OPDEF(JSOP_INITPROP,  93, "initprop",   NULL,         5,  2,  1,  3,  JOF_ATOM|JOF_PROP|JOF_SET|JOF_DETECTING)
>  OPDEF(JSOP_INITELEM,  94, "initelem",   NULL,         1,  3,  1,  3,  JOF_BYTE|JOF_ELEM|JOF_SET|JOF_DETECTING)
> +OPDEF(JSOP_INITELEM_INC,95, "initelemincr", NULL,     1,  3,  2,  3,  JOF_BYTE|JOF_ELEM|JOF_SET)

"initelemincr" should be "initelem_inc".
Comment 10 User image :Benjamin Peterson 2012-06-05 18:28:53 PDT
Created attachment 630399 [details] [diff] [review]
address review comments

I can't say ForOf is much of an improvement. I had to engage in some horribleness to get the context needed in the operation. Perhaps my addled brain was just forgot the proper C++ way to do this.
Comment 11 User image :Benjamin Peterson 2012-06-06 16:09:49 PDT
It's ready to go. We'll get a better C++ for of API in bug 762285.
Comment 12 User image Jason Orendorff [:jorendorff] 2012-06-06 19:50:12 PDT
(In reply to Benjamin Peterson from comment #10)
> I can't say ForOf is much of an improvement. I had to engage in some
> horribleness to get the context needed in the operation.

Yeah. I didn't expect ForOf to be prettier, but it's more correct, with less duplication of tricky code.

I'll push this.
Comment 13 User image Jason Orendorff [:jorendorff] 2012-06-06 19:56:24 PDT
Comment 14 User image Jason Orendorff [:jorendorff] 2012-06-06 21:23:29 PDT
Benjamin, please file a new bug for the f(...x) syntax (and any other places where ... can be used).
Comment 15 User image :Benjamin Peterson 2012-06-06 21:32:28 PDT
See bug 762363.
Comment 16 User image Ed Morley [:emorley] 2012-06-07 05:45:56 PDT
(Please set the target milestone when landing on inbound)

Note You need to log in before you can comment on or make changes to this bug.