666399 - (harmony:generators) new Harmony syntax for generators

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Herman [:dherman]]

Harmony specifies that generators have a distinguished header syntax. (We have to figure out in what modes we want to do this, and in what modes we want to allow leaving out the *.)

Dave

Comment 1

[Brendan Eich [:brendan]]

I'm likely to patch all the new generator goodness (yield* and return e;) in Harmony here, or in sub-bugs.

/be

Comment 2

[Tim Chevalier]

Attachment: Patch that implements function* syntax for generators

The patch implements declaring generators with "function*" instead of "function". To do that, I moved the "is a generator" bit into the FunctionBox for a given function.

Comment 3

[Mark S. Miller]

Are the return path semantics also fixed, i.e., is the "returned value/outcome" turned into the last generated value/outcome?

Comment 4

[Tim Chevalier]

Mark -- no, I haven't implemented that change yet.

Comment 5

[Dave Herman [:dherman]]

(In reply to Mark S. Miller from comment #3)
> Are the return path semantics also fixed, i.e., is the "returned
> value/outcome" turned into the last generated value/outcome?

That's not the semantics of generator return values; the returned value is thrown as a special GeneratorReturn value rather than yielded as another yield result. (This is a generalization of StopIteration, which was the original Python design and the original ES4 design as well.)

In particular, in a for-of loop, returning a value does not cause another iteration of the loop.

Dave

Comment 6

[Mark S. Miller]

(In reply to Dave Herman [:dherman] from comment #5)
> (In reply to Mark S. Miller from comment #3)

> [...] the returned value is
> thrown as a special GeneratorReturn value rather than yielded as another
> yield result. (This is a generalization of StopIteration, [...]

Sorry, I knew that once: http://wiki.ecmascript.org/doku.php?id=strawman:async_functions#reference_implementation

That page and http://wiki.ecmascript.org/doku.php?id=harmony:generators both still refer to this as a StopIteration. Has in indeed changed its name?

Comment 7

[Dave Herman [:dherman]]

> That page and http://wiki.ecmascript.org/doku.php?id=harmony:generators both
> still refer to this as a StopIteration. Has in indeed changed its name?

I haven't really settled on a name. There're also some details I'm still discussing with Brendan about |return;| vs |return (void 0);| vs falling-off-the-end.

Dave

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 601093 [details] [diff] [review]
Patch that implements function* syntax for generators

Tim, is there any particular reason this is a "feedback" request rather than a review request?

This gets feedback+ from me. I'll be happy to do a full review now, though an updated patch would be welcome.

Comment 9

[Tim Chevalier]

Jason - it was feedback because this was my first patch to Ionmonkey and I wasn't really sure what I was doing :-) I will try to make an updated patch this week (not that it will add anything new, as I haven't worked on this since submitting the feedback request, but at least it will be based off a more current version) and post here for review.

Comment 10

[Tim Chevalier]

I remembered the other reason why this was feedback and not review: because a few test cases failed and I wasn't able to figure out why. Now that I've rebased, there are some new failing tests. I'll try to look at those and post the updated patch within a couple days.

Comment 11

[Tim Chevalier]

Attachment: Support for function* syntax in front end

Comment 12

[Tim Chevalier]

I've attached an updated patch for review. Although I had been originally planning to work on implementing generators in the JIT as well, I think I'm not going to be working on that; so anyone else should feel free to take it on.

A few test cases fail:

* ecma_5/Expressions/named-accessor-function and ecma_5/extensions/cross-global-getPrototypeOf under jstests. Not sure what's going on with these two -- they don't seem related to anything I changed.

* js1_7/geniter/regress-355834 under jstests and jaeger/testBug550743 under jit-tests. I do know why these two tests fail: both of them use yield in a function object created with new Function(). I'm not sure whether that should be just disallowed now that generators have to be declared with a special syntax, or whether there should be a different variant on Function() for making generators dynamically.

Sorry about how long this took!

Comment 13

[:Benjamin Peterson]

Comment on attachment 645655 [details] [diff] [review]
Support for function* syntax in front end

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

We may have to support the old implicit generator declaration for a while when harmony is not opted into. Perhaps Jason can say what the backwards compatibility constraints on generators today are.

We'll have to ask dherman about what's supposed to happen in bodies passed to Function() with yield.

I see the spec also names the yield keyword "yield*". I imagine you're planning to implement that in a latter patch?

::: js/src/frontend/Parser.cpp
@@ +1438,5 @@
> +
> +    TokenKind tk;
> +    bool funIsGenerator = false;
> +#ifdef JS_HAS_GENERATORS
> +    tk = tokenStream.getToken();

You can use tokenStream.matchToken(TOK_START) here.

@@ +1448,5 @@
> +    }
> +#endif
> +
> +    RootedPropertyName funName(context);
> +    tk = tokenStream.getToken(TSF_KEYWORD_IS_NAME);

Also here.

@@ +1587,4 @@
>  
>      /* Initialize early for possible flags mutation via destructuringExpr. */
>      SharedContext funsc(context, /* scopeChain = */ NULL, fun, funbox, sms);
> +    if (funIsGenerator) {

Drop braces.

@@ +2711,5 @@
>      if (tt == TOK_YIELD) {
> +           if ( !(tc->sc->funIsGenerator()) ) {
> +               JS_ReportErrorNumber(context, JSREPORT_ERROR, NULL,
> +                                      JSMSG_YIELD_OUTSIDE_GENERATOR_FUN);
> +                    return NULL;

Indentation is strange here.

::: js/src/tests/js1_8/genexps/regress-634472.js
@@ +121,4 @@
>  }
>  
>  function expectError1(err, ctx, msg) {
> +  // print(err);

Kill these lines.

@@ +135,4 @@
>             ? { simple: expect, call: expect }
>             : expect;
>    expectError1(error(wrapCtx(expr)), exps.simple, msg);
> +  if (call) {

Don't need braces.

@@ +154,4 @@
>    printBugNumber(BUGNUMBER);
>    printStatus (summary);
>  
> +  //  let mylen = 3;

What's this line?

Comment 14

[:Benjamin Peterson]

(In reply to Benjamin Peterson from comment #13)
> I see the spec also names the yield keyword "yield*". I imagine you're
> planning to implement that in a latter patch?

Ignore this. A gross misreading on my part.

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Benjamin Peterson from comment #13)
> We may have to support the old implicit generator declaration for a while
> when harmony is not opted into. Perhaps Jason can say what the backwards
> compatibility constraints on generators today are.

I see two distinct tasks here:

1. Add support for function* without removing any support for starless generators.
   We can do this part now.

2. Enable support for function*, but not starless generators, in the default version
   of the language (the one used for <script> tags with no explicit version tag).
   Starless generators would continue to be accepted in JS1.7, 1.8, 1.85, and
   whatever other weird versions have them.

   There's no rush to do this part; IMHO it should wait until we know what the
   standard semantics will be in detail, and we're comfortable with the way our
   implementation stands up against the standard.

We can't just drop support for starless generators. That would break way too much code. The tests in this patch are just the tip of the iceberg.

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to Tim Chevalier from comment #12)
> * ecma_5/Expressions/named-accessor-function and
> ecma_5/extensions/cross-global-getPrototypeOf under jstests. Not sure what's
> going on with these two -- they don't seem related to anything I changed.

That's bug 776760.

> * js1_7/geniter/regress-355834 under jstests and jaeger/testBug550743 under
> jit-tests. I do know why these two tests fail: both of them use yield in a
> function object created with new Function().

Ah. I'm not necessarily opposed to breaking compatibility there. We will probably have to in order to implement the standard.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 645655 [details] [diff] [review]
Support for function* syntax in front end

We have to keep starless generators working for now. Otherwise this approach is the right one; just a few style nits.

I suggest keeping all the test changes in a separate patch and adding tests for the new syntax. (But please do go ahead and change all the jit-test/tests/debug tests over to use the new syntax; I happen to know know that those particular tests are all about semantics, not syntax.)

This case is a new thing for us:
    function* f() { ... /* body with no 'yield' */ }
so it needs a few tests. Things I can think of to test:
  - `f()` should return a generator-iterator, but not execute the body of f
  - `f().next()` should run the body of f and then throw StopIteration
  - any exceptions that occur in f should be propagated.
  - `f.toString()` should return the source, and it should work even if the
    body of f is empty or all spaces.

Please also test that
    function* f1() 13;
    function* f2() ({a: 1, b: 2});
    function* f2(x) (yield x);
are all SyntaxErrors.

In addition to Benjamin's comments:

>+    }
>+    else {

Write "} else {" on one line.

But in this case, drop the braces altogether (because the "if", "then", and "else" parts all fit on single lines).

SpiderMonkey house style is described here:
  https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style

>+           if ( !(tc->sc->funIsGenerator()) ) {

Don't use extra parentheses or space here. Instead:

             if (!tc->sc->funIsGenerator()) {

(However, since we must keep starless generators, at least for the time being, this change has to be reverted.)

Thanks for the patch; I hope you have time to fix this up!

Comment 18

[Tim Chevalier]

Thanks, I'll fix this up. I'm away next week, but I'm setting aside time in the first week of August to clean up.

Comment 19

[Tim Chevalier]

There's a test, js/tests/js1_7/lexical/regress-351515.js, that tests that you can name a function yield(). Should that still be legal, or is yield going to be a keyword in Harmony? I'm keeping the current behavior for now, but I'm not sure whether this is going to be complicated (if we allow a function called yield in general, do we need to forbid it inside a generator?)

Comment 20

[Mark S. Miller]

(In reply to Tim Chevalier from comment #19)
> There's a test, js/tests/js1_7/lexical/regress-351515.js, that tests that
> you can name a function yield(). Should that still be legal, or is yield
> going to be a keyword in Harmony? I'm keeping the current behavior for now,
> but I'm not sure whether this is going to be complicated (if we allow a
> function called yield in general, do we need to forbid it inside a
> generator?)

In ES5, "yield" is reserved in strict code but not in strict code. Thus, it must not be accepted as a function name in strict code in order to be ES5 conformant.

Given the current consensus for ES6, IIUC generators can be strict or non-strict. This does raise the question of how "yield" should be treated within a non-strict generator in ES6, or more generally within non-strict ES6 code. I do not recall this being discussed.

For ES6 strict code, "yield" is simply a keyword and must still not be accepted as a function name.

Comment 21

[Tim Chevalier]

Jason - re:

> Please also test that
>    function* f1() 13;
>    function* f2() ({a: 1, b: 2});
>    function* f2(x) (yield x);
> are all SyntaxErrors."

Currently, these are all TypeErrors. Is that behavior okay? Or do these really need to be SyntaxErrors?

Comment 22

[Tim Chevalier]

Attachment: Support for function* syntax in front end (now backwards-compatible)

Patch 1 of 5. This one adds support for function*, but also allows starless generators when ALLOW_STARLESS_GENERATORS is defined.

Comment 23

[Tim Chevalier]

Attachment: Change function to function* in JIT debug tests

Patch 2 of 5

Comment 24

[Tim Chevalier]

Attachment: Change function to function* in other existing tests. (No hurry to apply this one.)

Patch 3 of 5

Comment 25

[Tim Chevalier]

Attachment: Print out function* objects properly

Patch 4 of 5. Before this gets committed I can squash this into patch 1, but for now I just wanted to upload stuff.

Comment 26

[Tim Chevalier]

Attachment: New function* tests

Patch 5 of 5. Tests for various cases involving function* without a yield in the body.

Comment 27

[Tim Chevalier]

I uploaded a series of five patches that replace the previous one. I think I've addressed all the issues that Benjamin and Jason raised, but let me know if I missed anything or if there are any other problems.

Comment 28

[Jason Orendorff [:jorendorff]]

(In reply to Tim Chevalier from comment #21)
> >    function* f1() 13;
> >    function* f2() ({a: 1, b: 2});
> >    function* f2(x) (yield x);
> 
> Currently, these are all TypeErrors. Is that behavior okay? Or do these
> really need to be SyntaxErrors?

SyntaxErrors for sure.

Comment 29

[Jason Orendorff [:jorendorff]]

(In reply to Tim Chevalier from comment #19)
> There's a test, js/tests/js1_7/lexical/regress-351515.js, that tests that
> you can name a function yield().

I doubt we will want to break that test. I'll ask dherman.

Comment 30

[:Benjamin Peterson]

Comment on attachment 651926 [details] [diff] [review]
Support for function* syntax in front end (now backwards-compatible)

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

::: js/src/frontend/Parser.cpp
@@ +1423,5 @@
> +    funIsGenerator = tokenStream.matchToken(TOK_STAR);
> +#endif
> +
> +    RootedPropertyName funName(context);
> +    /* tjc: no sure if allowing yield as a function name is ok, but there's 

Space between above statement and comment. Also, we've been moving to C++ "//" style comments.

@@ +2590,5 @@
>  
>  #if JS_HAS_GENERATORS
>      if (tt == TOK_YIELD) {
> +#ifdef ALLOW_STARLESS_GENERATORS
> +        if ((tc->parenDepth) == 0)

Don't need extra parenthesis.

@@ +2596,5 @@
> +#else
> +        if (!tc->sc->funIsGenerator()) {
> +            JS_ReportErrorNumber(context, JSREPORT_ERROR, NULL,
> +                                  JSMSG_YIELD_OUTSIDE_GENERATOR_FUN);
> +                return NULL;

Indentation looks wrong.

@@ +4852,5 @@
>  }
>  
>  /*
> + * Error out if we saw a yield outside a generator function (or note that it is one,
> + * if we're in backwards-compatible mode).

Don't think parenthesis are required.

@@ +6803,4 @@
>                      pn->pn_xflags |= PNX_NONCONST;
>  
>                      /* NB: Getter function in { get x(){} } is unnamed. */
> +                    pn2 = functionDef(op == JSOP_GETTER ? Getter : Setter, Expression);

Doesn't this make the getter named?

Comment 31

[:Benjamin Peterson]

Comment on attachment 651929 [details] [diff] [review]
Print out function* objects properly

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

::: js/src/jsfun.cpp
@@ +617,4 @@
>              if (!out.append("("))
>                  return NULL;
>          }
> +        if (fun->script()->isGenerator && !out.append("function* "))

I think this should be rewritten to be

if (fun->script()->isGenerator)
...
else
...

Comment 32

[:Benjamin Peterson]

Comment on attachment 651930 [details] [diff] [review]
New function* tests

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

::: js/src/tests/geniter_harmony/functionstar-no-yield-print.js
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +//-----------------------------------------------------------------------------
> +var BUGNUMBER     = "666399";
> +var summary = "function* f() { ... } with no yield in the body; f.toString() works"

I would be happier if this particular tests was jit-tests/tests/basic/function-tostring-generator.js.

@@ +22,5 @@
> +function* h() {                 }
> +
> +try
> +{
> +  print(f.toString());

What's the value of using print() here?

Comment 33

[Jason Orendorff [:jorendorff]]

OK, dherman has explained this to me. There are three cases:

1. In JS1.8+, 'yield' is always a keyword.

2. Otherwise, if the immediately enclosing function
   (counting getters and setters, but not generator-expressions, as functions)
   is a function*,
   then 'yield' is a keyword.

3. Otherwise, we are in global code, or eval code, or else
   the immediately enclosing function/getter/setter is NOT a function*,
   and so 'yield' is an identifier.

And there is one additional rule:

4. Yield-expressions are not allowed in generator-expressions.

That is, if the immediately enclosing function/getter/setter/generator-expression
is a generator-expression,
AND the immediately enclosing function/getter/setter is a function*,
then 'yield' is a keyword, as required by point 2 above;
but it's a SyntaxError in that context.

OK. Having said all that, we *think* this is pretty much exactly what we already do in every case except #2.

Comment 34

[Jason Orendorff [:jorendorff]]

Comment on attachment 651926 [details] [diff] [review]
Support for function* syntax in front end (now backwards-compatible)

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

The main issue here is that -- of course -- this will need a bunch of unit tests. They're easy to write; look in js/src/tests or js/src/jit-test (and you can search for existing tests that contain `yield`).

Benjamin's points are good too.

::: js/src/frontend/Parser.cpp
@@ +1425,5 @@
> +
> +    RootedPropertyName funName(context);
> +    /* tjc: no sure if allowing yield as a function name is ok, but there's 
> +       a test case that wants it
> +       Do we have to forbid naming a function yield in a generator context?

Oh, I just realized, the situation is that in SpiderMonkey, no token is ever treated as a keyword when it's a function name:

    function if() {}         // ok
    function var() {}        // ok
    function function() {}   // ok
    function yield() {}      // ok
    typeof this.yield;       // "function"

This is nonstandard. We added it for no terribly compelling reason in bug 343675. Chrome doesn't support it.  js1_5/LexicalConventions/regress-343675.js, which was added at the same time, tests for this exact thing.

For now, leave this "feature" intact and we'll consider dropping it separately.

<legalese>
Here's what the standard says. http://ecma-international.org/ecma-262/5.1/#sec-7.6 contains these constructions:

  IdentifierName ::
      IdentifierStart
      IdentifierName IdentifierPart

  Identifier ::
      IdentifierName but not ReservedWord

The standard expects IdentifierName after a `.` but Identifier after `function`.

  FunctionDeclaration :
      function Identifier ( FormalParameterList_opt ) { FunctionBody }

  MemberExpression :
      …
      MemberExpression . IdentifierName
      …
</legalese>

@@ +1428,5 @@
> +       a test case that wants it
> +       Do we have to forbid naming a function yield in a generator context?
> +
> +       This breaks js1_5/LexicalConventions/regress-343675.js and
> +        js1_7/lexical/regress-351515.js, but I'm not sure why it ever worked

Yeah, we have to fix those for now.

@@ +1439,5 @@
> +        JS_ReportErrorNumber(context, JSREPORT_ERROR, NULL, JSMSG_UNNAMED_FUNCTION_STMT);
> +        return NULL;
> +    }
> +
> + 

Nit: Remove the second blank line here.

@@ +2590,4 @@
>  
>  #if JS_HAS_GENERATORS
>      if (tt == TOK_YIELD) {
> +#ifdef ALLOW_STARLESS_GENERATORS

I don't think we need a new #ifdef. Always allow starless generators (in JS1.8+).

@@ +2595,5 @@
>              tc->sc->setFunIsGenerator();
> +#else
> +        if (!tc->sc->funIsGenerator()) {
> +            JS_ReportErrorNumber(context, JSREPORT_ERROR, NULL,
> +                                  JSMSG_YIELD_OUTSIDE_GENERATOR_FUN);

Is this case possible? How can this happen? I would imagine that in versions less than 1.8, `yield` just wouldn't be scanned as a keyword outside of generators. So tt would never be TOK_YIELD and we wouldn't get here.

@@ +4872,5 @@
>              parser->reportError(NULL, JSMSG_BAD_RETURN_OR_YIELD, js_yield_str);
>              return false;
>          }
>          if (tc->hasReturnExpr) {
> +            // Generators aren't allowed to return

"aren't allowed to return a value" (since they are allowed to `return;`)

Comment 35

[Jason Orendorff [:jorendorff]]

Disregard the comment about tests; I didn't mean to post that bit. :-P

Comment 36

[Jason Orendorff [:jorendorff]]

Comment on attachment 651929 [details] [diff] [review]
Print out function* objects properly

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

Another possibility is


    if (!out.append(fun->script()->isGenerator() ? "function*" : "function"))
        return NULL;

Comment 37

[Jason Orendorff [:jorendorff]]

Comment on attachment 651930 [details] [diff] [review]
New function* tests

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

Do these tests work with the test runner?

I think the right directory for these is js/src/tests/ecma_6/generators, but js/src/jit-test/tests/generators would be even better.

::: js/src/tests/geniter_harmony/functionstar-no-yield-exception.js
@@ +23,5 @@
> +}
> +
> +try
> +{
> +  f().next();

Call f() outside the try-block, so that the only call that happens within the try block is the .next() call.

Incidentally, if you'd rather have a 7-line test than a 39-line test, you can put this in some directory under jit-test:

  load(libdir + "asserts.js");

  function* f() {
      throw "wombat";
  }
  var iter = f();
  assertThrowsValue(function () { iter.next(); }, "wombat");

::: js/src/tests/geniter_harmony/functionstar-no-yield-print.js
@@ +35,5 @@
> +  failed = true;
> +}
> +
> +expect = false;
> +actual = failed;

Equivalent jit-test:

  function* f() { throw "wombat"; }
  function* g() {}
  function* h() {                 }
  assertEq(f.toString(), "function* f() { throw \"wombat\"; }");
  assertEq(g.toString(), "function* g() {}");
  assertEq(h.toString(), "function* h() {                 }");

::: js/src/tests/geniter_harmony/functionstar-no-yield.js
@@ +33,5 @@
> +
> +expect = false;
> +actual = failed;
> +
> +reportCompare(expect, actual, summary);

Equivalent jit-test is 2 lines:

function *f() { throw "FAIL"; }
var it = f();  // don't throw

::: js/src/tests/geniter_harmony/functionstar-syntax-errors.js
@@ +15,5 @@
> + * BEGIN TEST *
> + **************/
> +
> +/*
> + * tjc: I'm not sure if these should raise a TypeError or a SyntaxError

SyntaxError.

::: js/src/tests/geniter_harmony/shell.js
@@ +8,5 @@
> +// explicitly turn on js185
> +// XXX: The browser currently only supports up to version 1.8
> +if (typeof version != 'undefined')
> +{
> +  version(185);

I think these tests should pass in all versions. If so, don't set the version here; leave this file empty (and if you make these jit-tests, there's no need for a shell.js file at all).

Comment 38

[Jason Orendorff [:jorendorff]]

Comment on attachment 651927 [details] [diff] [review]
Change function to function* in JIT debug tests

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

Thanks!

Comment 39

[Jason Orendorff [:jorendorff]]

Comment on attachment 651928 [details] [diff] [review]
Change function to function* in other existing tests. (No hurry to apply this one.)

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

OK, all these patches are OK to land, once the review comments are addressed.

::: js/src/jit-test/tests/jaeger/testBug550743.js
@@ -1,4 @@
> -expected = '';
> -
> -function g(code) {
> -  f = Function(code);

Doesn't this have to land with the other changes?

In fact I think we should add an extra test that specifically tests this one thing: what happens when you call Function(code) and code contains yield syntax? I think technically we'll be making a potentially breaking change here (right?) but it probably won't break anyone in practice.

::: js/src/tests/js1_8/genexps/regress-634472.js
@@ +36,5 @@
>  
> +/*
> +const mycases = [  { expr: "(1, yield 2 for (x in []))",      top: JSMSG_TOP_YIELD, fun: { simple: JSMSG_GENEXP_YIELD, call: JSMSG_GENEXP_PAREN },
> +                     gen: JSMSG_GENEXP_YIELD, desc: "yield w/ arg at end of list in genexp" }];
> +*/

You added some commented-out code here. Probably didn't mean to leave it in.

@@ +151,5 @@
>    printStatus (summary);
>  
> +  let mylen = cases.length;
> +
> +  for (let i = 0, len = mylen; i < len; i++) {

I don't understand the change here.

Comment 40

[Jason Orendorff [:jorendorff]]

One more comment--

If the test changes leave no tests of the old starless syntax, we need to have some tests for that; and we also need to test that the starless syntax is *not* recognized except in JS1.8+.

Comment 41

[Terrence Cole [:terrence]]

(In reply to Jason Orendorff [:jorendorff] from comment #37)
> ::: js/src/tests/geniter_harmony/functionstar-no-yield.js
> @@ +33,5 @@
> > +
> > +expect = false;
> > +actual = failed;
> > +
> > +reportCompare(expect, actual, summary);
> 
> Equivalent jit-test is 2 lines:
> 
> function *f() { throw "FAIL"; }
> var it = f();  // don't throw

Equivalent jstest in 3 lines: 

function *f() { throw "FAIL"; }
var it = f();  // don't throw
reportCompare(0, 0, 'ok');

Comment 42

[Tim Chevalier]

I added the ALLOW_STARLESS_GENERATORS ifdef to make it easier to identify what code to remove once starless generators are no longer supported. Or is that "never"? :-)

Comment 43

[Jason Orendorff [:jorendorff]]

(In reply to Tim Chevalier from comment #42)
> I added the ALLOW_STARLESS_GENERATORS ifdef to make it easier to identify
> what code to remove once starless generators are no longer supported. Or is
> that "never"? :-)

Possibly. Certainly it takes an effort to get anything removed.

Comment 44

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

This is burning me on my quest to get code coverage for Moz addon code.  

Esprima's harmony branch fully supports the "function *" style, which of course, SM doesn't like.  

I am going to suggest the "ALLOW_STARLESS_GENERATORS" an option to `esprima.parse()`, but it seems a bit hackish.

Comment 45

[Jason Orendorff [:jorendorff]]

Gregg, congratulations finding this bug -- I couldn't find it :)

But this bug isn't what’s burning you.  We could fix this bug and ship it and it still would not solve your problem at all.

The problem you're running into is that we have a lot of existing JS code using starless generators. I doubt that will change in time for you, so you need to bypass.

Comment 46

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Noted :)

I could however force *my own* code to just use starred generators :)  I have a ton of small fixes for esprima that I will package up tomorrow or friday.  This is the only one that seems like it doesn't have a very clean solution.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ping? Any movement here? Would be lovely to start using harmony generators in new tests so that they don't have to be converted at some later point when we want to drop support for the current generator syntax.

Comment 48

[Tim Chevalier]

I haven't forgotten about this, but it's just that Rust has been needing my attention. I do have a day marked on my calendar this month to integrate the latest round of review comments and submit a new patch, though.

Comment 49

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Tim, I look forward to it once you get time!

Comment 50

[Brendan Eich [:brendan]]

Sorry, I'm a bad owner. Tim is obvious assignee. Tim, ok if I transfer assignment? If not then maybe Jason will take it.

/be

Comment 51

[Tim Chevalier]

Brendan: I went ahead and took it over (I think). Still planning on trying to finish my patch this month.

Comment 52

[Brendan Eich [:brendan]]

Thanks for taking -- sorry again for seeming to squat on it. Fortunately it didn't slow anyone down this time, but if I fail again it's cuz I'm behind on bugmail and being busy and-or lame. As usual, feel free to ping me on IRC to get any bug unstuck.

/be

Comment 53

[Dave Herman [:dherman]]

Can someone help me understand: the bug title was changed to say "syntax and semantics" but it appears this is still only a bug for new syntax, right? I'm about to file separate bugs for various aspects of the new semantics.

Dave

Comment 54

[Tim Chevalier]

Dave: yes, I've only been working on syntax. I changed the bug title.

Comment 55

[Tim Chevalier]

I'm working on this again -- have a new patch based off Friday's revision of Ionmonkey, and all jit-test and jstests tests but 3 pass (I'm looking at those 3).

I re-read all the comments and have a few questions for Jason (or for anyone else who might know):

1. Comment 34 -- "The main issue here is that -- of course -- this will need a bunch of unit tests." One of the patches that I submitted previously changes all the tests that use yield to use function* instead of function. Is this enough? Or are you saying more tests are needed? (I understand that some tests for the old "function" syntax for generators might be needed too, but that's what the next question is about.)

2. I feel like I may be confused about backwards compatibility. Am I correct in thinking that for now, all that needs to be done is for me to remove the ALLOW_STARLESS_GENERATORS ifdef that I added, and just always allow starless generators?

3. Relatedly, I'm confused about comment 40. "we also need to test that the starless syntax is *not* recognized except in JS1.8+." -- should this say that the starless syntax is *not* recognized except in JS 1.8 and older versions? If not, then I'm confused. I thought that older versions should accept the starless syntax, but newer versions should only accept the syntax with function* for generators and reject the starless syntax.

Comment 56

[Dave Herman [:dherman]]

> 3. Relatedly, I'm confused about comment 40. "we also need to test that the
> starless syntax is *not* recognized except in JS1.8+." -- should this say
> that the starless syntax is *not* recognized except in JS 1.8 and older
> versions? If not, then I'm confused. I thought that older versions should
> accept the starless syntax, but newer versions should only accept the syntax
> with function* for generators and reject the starless syntax.

The confusion is that JavaScript is not linearly versioned. Brace yourself, this isn't gonna be pleasant...

Version number for *JavaScript* (as opposed to ECMAScript) is a Mozilla-specific thing that no other JS engines honor. Basically, there are two families of JS versioning in our engine: one for when the user didn't specify an explicit version, and one when they did. The explicit versioning is Mozilla-specific, and is basically a failed experiment; the overwhelming majority of the web doesn't use it. The HTML5 zeitgeist and the "One JavaScript" (1JS) movement, meanwhile, have moved away from opt-in versioning as an anti-pattern.

So what does this mean for generators? Mozilla introduced them in our Mozilla-specific explicit-opt-in <script type="application/javascript;version=1.8"> support. When users opt in to version 1.8 or greater, they are actually getting an old fork of the ECMAScript language, and so they get our legacy Mozilla-specific, starless generators.

But in JavaScript that did *not* get an explicit opt-in, they always have and always will provide the latest, greatest version of Mozilla's web-compatible implementation of the latest ECMAScript standard. There, we can't and shouldn't support starless generators. We should, however, support starred generators in implicit opt-in.

tl;dr:
- explicit opt-in versioning => Mozilla's defunct fork of ECMAScript => starless generators
- no opt-in versioning: web-compatible ES6 => starred generators.

HTH,
Dave

Comment 57

[Tim Chevalier]

Attachment: Support for function* syntax in front end

Comment 58

[Tim Chevalier]

Attachment: Change function to function* in non-JS-1.8 tests

Comment 59

[Tim Chevalier]

I've just submitted two patches (comment 57 and comment 58). 57 contains all the front-end changes plus the JIT debug test changes (as per an old comment from jorendorff). 58 contains all the test of the test changes.

Though I read comment 56 from dherman several times, I'm afraid I still may have misunderstood the versioning situation. In particular, jorendorff's comment to the effect of "We have to keep starless generators working for now" seems to contradict dherman's implication that starless generators should *not* work unless JS 1.8 is explicitly requested. (Unless starless generators are never used without an explicit 1.8 version, I guess. I'm still a newb at this.)

In any case, I figured I would submit what I had and let any issues shake themselves out in the review process :-)

Comment 60

[Dave Herman [:dherman]]

(In reply to Tim Chevalier from comment #59)
> 
> Though I read comment 56 from dherman several times, I'm afraid I still may
> have misunderstood the versioning situation. In particular, jorendorff's
> comment to the effect of "We have to keep starless generators working for
> now" seems to contradict dherman's implication that starless generators
> should *not* work unless JS 1.8 is explicitly requested. (Unless starless
> generators are never used without an explicit 1.8 version, I guess. I'm
> still a newb at this.)

No contradiction, because as you guessed, starless generators were never enabled outside of JS 1.8. (Well, also JS 1.7, but that also requires explicit opt-in. For the purposes of this bug, both JS 1.7 and JS 1.8 should get the same treatment: starless generators work in those opt-in modes, but not in normal JS.)

Dave

Comment 61

[Andy Wingo [:wingo]]

More tests related to ES6 generators:

  http://code.google.com/p/v8/source/browse/trunk/test/mjsunit/harmony/generators-iteration.js
  http://code.google.com/p/v8/source/browse/trunk/test/mjsunit/harmony/generators-parsing.js
  http://code.google.com/p/v8/source/browse/trunk/test/mjsunit/harmony/generators-objects.js
  http://code.google.com/p/v8/source/browse/trunk/test/mjsunit/harmony/generators-runtime.js

Note that the various meta-objects are probably different in ES6 and JS1.8.

Comment 62

[Andy Wingo [:wingo]]

AFAIU then, the plan would be to add function* to JS 1.8 alongside starless generators.

Exposing it to the web initially is a bad idea, because it will take quite a few commits to get an implementation of generators that conforms to the latest ES6 draft specs, and it's not a good idea to expose an incompatible feature.

Once function* in JS1.8 is working along the lines of ES6 generators, it could then be exposed to the web, assuming that is the mid-term goal.

Comment 63

[Andy Wingo [:wingo]]

Give the semantic and syntactic differences between JS 1.8 generators (legacy generators -- legacy in the sense of awesome and before their time, but soon to be superceded) and ES6 generators, I think there needs to be a bit of groundwork before these patches; see bug 884749 for more.

Comment 64

[Jason Orendorff [:jorendorff]]

Comment on attachment 751550 [details] [diff] [review]
Support for function* syntax in front end

Agreed.

Comment 65

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators

Add an isStarGenerator flag to FunctionBox in the parser.  This allows
the parser to handle "yield" in ways appropriate to the context.

Add a GeneratorSyntaxKind enumeration, and use it in the parser to
indicate whether a function's body is parsed as a non-generator, as a
legacy generator, or as a star generator.

Always parse "yield" as TOK_YIELD, regardless of the version.  Add
TokenStream::currentName() to retrieve the current token's name, which
works for TOK_NAME or TOK_YIELD.  The parser should check the validity
of "yield" as a name, if needed, using checkYieldNameValidity().

Allow star generators to be lazily parsed.

Separate parsing of return statements from yield expressions.

Add "standard" meta-directive to tests/lib/jittests.py, which calls
version(0) before parsing a test file.

Comment 66

[Andy Wingo [:wingo]]

Comment on attachment 786260 [details] [diff] [review]
new Harmony syntax for generators

The patch I just attached implements the syntactic parts of ES6 generators support in SM.  Instantiating a generator will throw a not-implemented exception.

Parsing generators but not implementing them probably breaks Q.async's heuristics to choose between the legacy or ES6 protocol; oh well, I suppose we can fix Q and related code.

Still to do would be all the semantics: having ES6 generators return boxed result objects, terminate with done: true instead of stopiteration, the nest of meta-objects and prototypes specified in the ES6 draft, and yield*.  Then we can look at doing baseline JIT support.

Comment 67

[Jeff Walden [:Waldo]]

Comment on attachment 786260 [details] [diff] [review]
new Harmony syntax for generators

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

The old system for tokenizing yield was cleaner than this one, but I don't immediately see how this could be simplified back in that direction.  :-\  I think you're covering all the bases, so this'll work well enough.

This looks broadly pretty great, with a few more assertions added, some more tests, and clarification on one or two actual points about the code.  But I'm going to put my foot down on the jittests.py change.  Use a jstest instead (the only real change you'll need to make is to put a |if (typeof reportCompare === "function") \n reportCompare(true, true);| at the end of it), add the version(0) bit to the shell.js instead.

Definitely looking forward to the next version of this!

::: js/src/frontend/BytecodeCompiler.cpp
@@ +284,5 @@
>               */
>              JSFunction *fun = evalCaller->functionOrCallerFunction();
>              Directives directives(/* strict = */ fun->strict());
> +            ObjectBox *funbox = parser.newFunctionBox(/* fn = */ NULL, fun, pc.addr(),
> +                                                      directives, NotGenerator);

Is NotGenerator actually right here?  Assuming my memory/reading are correct, this would apply to eval calls inside generator functions, both star and legacy.  So can't this be specifying wrong information?  For star that obviously doesn't matter yet.  But what about legacy?  Or are you just depending on this wrong information never being queried?

If you're depending on never-queriedness here...I dunno.  It smells.  I'll defer to jorendorff as to whether it smells too much.

@@ +397,5 @@
>  
>      uint32_t staticLevel = lazy->staticLevel(cx);
>  
>      Rooted<JSFunction*> fun(cx, lazy->function());
> +    GeneratorSyntaxKind generatorSyntaxKind = lazy->isStarGenerator() ? StarGenerator : NotGenerator;

Add a MOZ_ASSERT(!lazy->isLegacyGenerator()) that'll trip if legacy generators ever get lazy-parsed, so we know to fix this.

::: js/src/frontend/Parser.cpp
@@ +878,5 @@
>      argsbody->makeEmpty();
>      fn->pn_body = argsbody;
>  
> +    FunctionBox *funbox = newFunctionBox(fn, fun, /* outerpc = */ NULL, inheritedDirectives,
> +                                         generatorSyntaxKind);

Add an assert that fun's generator-ness is consistent with generatorSyntaxKind.

@@ +2703,5 @@
>      if (tt == TOK_NAME) {
> +        (void) tokenStream.getToken();
> +        label.set(tokenStream.currentName());
> +    } else if (tt == TOK_YIELD) {
> +        (void) tokenStream.getToken();

tokenStream.consumeKnownToken(TOK_YIELD);

@@ +4222,5 @@
>      /* Don't parse 'for each' loops. */
> +    if (allowsForEachIn()) {
> +        TokenKind tt = tokenStream.peekToken();
> +        // Not all "yield" tokens are names, but the ones that aren't names are
> +        // invalid in this context anyway.

I assume you've added a few different tests that "for yield" triggers a syntax error?

@@ +4566,5 @@
> +template <typename ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::yieldExpression()
> +{
> +    uint32_t begin = pos().begin;

For consistency this should start with JS_ASSERT(tokenStream.isCurrentTokenType(TOK_YIELD));.

@@ +4579,5 @@
> +        JS_ASSERT(pc->lastYieldOffset != ParseContext<ParseHandler>::NoYieldOffset);
> +    } else {
> +        // We are in code that has not seen a yield, but we are in JS 1.7 or
> +        // later.  Try to transition to being a legacy generator.
> +        JS_ASSERT(tokenStream.isCurrentTokenType(TOK_YIELD));

The assertion at start of the function covers this, so this one can be removed.

@@ +4595,5 @@
> +        pc->sc->asFunctionBox()->setIsLegacyGenerator();
> +
> +        if (pc->funHasReturnExpr) {
> +            /* As in Python (see PEP-255), disallow return v; in generators. */
> +            // FIXME: make sure reportBadReturn can handle a null parsenode

I'm pretty sure this is the case now, except that you'll get an error location referring to the yield, not to the return.  The simple hackaround would seem to be to convert

MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous generator function returns a value")

to

MSG_DEF(JSMSG_YIELD_FORBIDDEN_AFTER_RETURN_VALUE, 209, 0, JSEXN_TYPEERR, "function that returns a value can't be a generator")

This message seems no worse than the previous one, and it avoids the location looking wrong.

@@ +4645,5 @@
> +    (void) isDelegatingYield;
> +
> +    Node pn = handler.newUnary(PNK_YIELD, JSOP_YIELD, begin, exprNode);
> +    if (!pn)
> +        return null();

You can just |return handler.newUnary(...);| here.

@@ +6782,5 @@
> +      case TOK_YIELD:
> +        if (!checkYieldNameValidity())
> +            return null();
> +        return identifierName();
> +

I'd prefer if you had

      case TOK_YIELD:
        if (!checkYieldNameValidity())
            return null();
      case TOK_NAME:
        return identifierName();

so it's clear that in this context, |yield| is handled exactly as any random name would be handled.

I don't particularly care one way or another if there's a /* fall through */ or not in that, feel free to add or not as you wish.

::: js/src/frontend/Parser.h
@@ -498,5 @@
>      Node forStatement();
>      Node switchStatement();
>      Node continueStatement();
>      Node breakStatement();
> -    Node returnStatementOrYieldExpression();

\o/ Good to see this die!

@@ +558,5 @@
>  
>      Node identifierName();
>  
> +    typedef MutableHandle<PropertyName*> MutableHandlePropertyName;
> +    bool matchLabel(MutableHandlePropertyName label);

Just use the <> form, your "abbreviation" isn't an abbreviation overall.  :-)

::: js/src/frontend/SharedContext.h
@@ +294,1 @@
>      bool isLegacyGenerator()        const { return funCxFlags.isLegacyGenerator; }

Star and legacy generators are mutually exclusive.  Could you expand these slightly to assert not being the other, when these two methods return true?  Just an extra detection measure for things going awry somewhere.

@@ +302,1 @@
>      void setIsLegacyGenerator()            { funCxFlags.isLegacyGenerator        = true; }

These two could also use isn't-the-other assertions.

::: js/src/jit-test/tests/generators/es6-syntax.js
@@ +61,5 @@
> +function* yield() { (yield 3) + (yield 4); }
> +assertSyntaxError("function* yield() { \"use strict\"; (yield 3) + (yield 4); }");
> +
> +// In classic mode, yield is a normal identifier, outside of generators.
> +function yield(yield) { yield: yield (yield + yield (0)); }

MY EYES

@@ +66,5 @@
> +
> +// Yield is always valid as a key in an object literal.
> +({ yield: 1 });
> +function* g() { yield ({ yield: 1 }) }
> +function* g() { yield ({ get yield() { return 1; }}) }

Might as well put in a |yield obj.yield;| as well to cover property names (although they're parsed differently enough it probably doesn't matter -- just for completeness).

@@ +95,5 @@
> +// The name of the NFE is let-bound in G, so is invalid.
> +assertSyntaxError("function* g() { yield (function yield() {}); }");
> +
> +// In generators, yield is invalid as a formal argument name.
> +assertSyntaxError("function* g(yield) { yield (10); }");

More tests:

// Interaction of yield-name with lookups, in contexts where generator-ness may or may not (for all I know) propagate:
var yield = { value: 17, done: false };
function* f() { yield eval("yield"); } // throw a SyntaxError?  yield 17?  not gonna investigate now to find out ;-)

// Interaction of scope-chain lookups from eval inside star generators
eval("function g() { function* f() { yield f.caller; } return f(); }");

// yield in other contexts inside generators
assertSyntaxError("function* f(yield) {}");
assertSyntaxError("function* f(x = yield) {}");
assertSyntaxError("function* f(x = yield 17) {}");
assertSyntaxError("function* f([yield]) {}");
assertSyntaxError("function* f({ yield }) {}");
assertSyntaxError("function* f(...yield) {}");

Also that |for yield| thing:

assertSyntaxError("for yield");
assertSyntaxError("for yield (;;) {}");
assertSyntaxError("for yield (x of y) {}");
assertSyntaxError("for yield (var i in o) {}");

::: js/src/tests/lib/jittests.py
@@ +133,5 @@
>                      elif name == 'mjitalways':
>                          test.jitflags.append('--always-mjit')
> +                    elif name == 'standard':
> +                        test.jitflags.append('-e')
> +                        test.jitflags.append('version(0)')

So...

In theory this is fine.  But our harnesses are messily complex enough already.  I don't want us adding more complexity, when there isn't really a good reason for it.  And there's really not, here.

Could you put your test in js/src/tests/ecma_6/Generators/foo.js, have a js/src/tests/ecma_6/Generators/shell.js that includes this version(0), and avoid having to touch the harnesses at all?

I kind of hate to put my foot down on something so stupid, but someone's gotta do it, if our test harnesses are ever going to be unifiable into one, or (even better) if we could upstream all our tests to test262.  This is a step in the wrong direction.

::: js/src/vm/Keywords.h
@@ +73,5 @@
>      macro(public, public_, TOK_STRICT_RESERVED, JSVERSION_DEFAULT) \
>      macro(static, static_, TOK_STRICT_RESERVED, JSVERSION_DEFAULT) \
> +    /* ES5 future reserved keyword in strict mode, keyword in JS1.7 even when \
> +       not strict, keyword inside function* in all versions.  Punt logic to \
> +       parser. */ \

Fairly sure our style here should be

    /* \
     * ES5 future reserved keyword in strict mode, keyword in JS1.7 even when \
     * not strict, keyword inside function* in all versions.  Punt logic to \
     * parser. \
     */ \
    macro(yield, yield, TOK_YIELD, JSVERSION_DEFAULT) \

possibly without the backslashes inside the comment if they're not necessary (I'm not sure offhand if they are).

Comment 68

[Andy Wingo [:wingo]]

Thank you for the review!  Will fix the issues and nits.

Some specific comments below.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #67)
> The old system for tokenizing yield was cleaner than this one, but I don't
> immediately see how this could be simplified back in that direction.

Yes I don't know.  The issue is that "yield" now parses contextually.  Before there was a version-dependent thing going on, but that wasn't contextual.

Another possible solution would be to add a tokenstream flag to indicate that we are in a star generator, and thus yield is contextually a keyword, regardless of version or strictness.  It still complicates almost all call sites though, and you'd probably have to add those tokenstream flags to the parse context; and making the tokenizer contextual isn't so nice, given the putback buffer.

Yet another possibility would be to abstract out name parsing more in the parser, with higher-level peekIdentifier / parseIdentifier / parseName helpers; but that doesn't work so well with the switch(tt) { ... } style, so I didn't go too far in that direction.

But yes, a bit nasty.

> I'm going to put my foot down on the jittests.py change.  Use a jstest
> instead

No prob.

> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +284,5 @@
> >               */
> >              JSFunction *fun = evalCaller->functionOrCallerFunction();
> >              Directives directives(/* strict = */ fun->strict());
> > +            ObjectBox *funbox = parser.newFunctionBox(/* fn = */ NULL, fun, pc.addr(),
> > +                                                      directives, NotGenerator);
> 
> Is NotGenerator actually right here?  Assuming my memory/reading are
> correct, this would apply to eval calls inside generator functions, both
> star and legacy.  So can't this be specifying wrong information?  For star
> that obviously doesn't matter yet.  But what about legacy?  Or are you just
> depending on this wrong information never being queried?

I think it's right, yes.  I can't find the reference ATM but "yield" is only valid in ES6 in function code that is defined with function*, and eval makes eval code.  For similar reasons, yield isn't valid in eval code in legacy generators either.  Tests needed, but try it and see in the legacy case.

> // Interaction of yield-name with lookups, in contexts where generator-ness
> may or may not (for all I know) propagate:

Good idea.

> var yield = { value: 17, done: false };

Error in strict mode or in a generator, fine otherwise.

> function* f() { yield eval("yield"); } // throw a SyntaxError?  yield 17? 
> not gonna investigate now to find out ;-)

Syntax error in strict mode, otherwise yield outer "yield" binding.  Can't yet test semantics though.

> // Interaction of scope-chain lookups from eval inside star generators
> eval("function g() { function* f() { yield f.caller; } return f(); }");

OK

> // yield in other contexts inside generators
> assertSyntaxError("function* f(yield) {}");

Already tested

> assertSyntaxError("function* f(x = yield) {}");
> assertSyntaxError("function* f(x = yield 17) {}");
> assertSyntaxError("function* f([yield]) {}");
> assertSyntaxError("function* f({ yield }) {}");
> assertSyntaxError("function* f(...yield) {}");

These ones require JS 1.7 so they need to be in a different test.  Gaaaaaaaah.

> Also that |for yield| thing:
> 
> assertSyntaxError("for yield");
> assertSyntaxError("for yield (;;) {}");
> assertSyntaxError("for yield (x of y) {}");
> assertSyntaxError("for yield (var i in o) {}");

OK.

Will submit an updated patch.  Thanks again for the review!

Comment 69

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v2

Add an isStarGenerator flag to FunctionBox in the parser.  This allows
the parser to handle "yield" in ways appropriate to the context.

Add a GeneratorSyntaxKind enumeration, and use it in the parser to
indicate whether a function's body is parsed as a non-generator, as a
legacy generator, or as a star generator.

Always parse "yield" as TOK_YIELD, regardless of the version.  Add
TokenStream::currentName() to retrieve the current token's name, which
works for TOK_NAME or TOK_YIELD.  The parser should check the validity
of "yield" as a name, if needed, using checkYieldNameValidity().

Allow star generators to be lazily parsed.

Separate parsing of return statements from yield expressions.

Comment 70

[Andy Wingo [:wingo]]

Sorry for the noise; clumsy hg bzexport.

Updated patch addresses issues and fixes nits.  Specific comments not already made:

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #67)

> ::: js/src/frontend/Parser.cpp
> @@ +878,5 @@
> >      argsbody->makeEmpty();
> >      fn->pn_body = argsbody;
> >  
> > +    FunctionBox *funbox = newFunctionBox(fn, fun, /* outerpc = */ NULL, inheritedDirectives,
> > +                                         generatorSyntaxKind);
> 
> Add an assert that fun's generator-ness is consistent with
> generatorSyntaxKind.

Can we actually do that?  That piece of information is on the script, which we have yet to parse; it is present on the lazy script I guess.  I would punt on the assertion here and trust that the incoming generatorSyntaxKind is correct, but it's your call.

> @@ +4595,5 @@
> > +        pc->sc->asFunctionBox()->setIsLegacyGenerator();
> > +
> > +        if (pc->funHasReturnExpr) {
> > +            /* As in Python (see PEP-255), disallow return v; in generators. */
> > +            // FIXME: make sure reportBadReturn can handle a null parsenode
> 
> I'm pretty sure this is the case now, except that you'll get an error
> location referring to the yield, not to the return.  The simple hackaround
> would seem to be to convert
> 
> MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous
> generator function returns a value")
> 
> to
> 
> MSG_DEF(JSMSG_YIELD_FORBIDDEN_AFTER_RETURN_VALUE, 209, 0, JSEXN_TYPEERR,
> "function that returns a value can't be a generator")
> 
> This message seems no worse than the previous one, and it avoids the
> location looking wrong.

I'd rather do that in a followup.  I was careful not to change expected errors in the JS 1.8 / 1.7 tests and I'd rather not mix semantic changes with error changes.

> ::: js/src/frontend/SharedContext.h
> @@ +294,1 @@
> >      bool isLegacyGenerator()        const { return funCxFlags.isLegacyGenerator; }
> 
> Star and legacy generators are mutually exclusive.  Could you expand these
> slightly to assert not being the other, when these two methods return true? 
> Just an extra detection measure for things going awry somewhere.

I instead added an assertion in the only setter -- seems good enough, no?

> // Interaction of yield-name with lookups, in contexts where generator-ness
> may or may not (for all I know) propagate:
> var yield = { value: 17, done: false };
> function* f() { yield eval("yield"); } // throw a SyntaxError?  yield 17? 
> not gonna investigate now to find out ;-)
> 
> // Interaction of scope-chain lookups from eval inside star generators
> eval("function g() { function* f() { yield f.caller; } return f(); }");

Will push in a followup once I can implement the semantics.

Comment 71

[Jeff Walden [:Waldo]]

Comment on attachment 788086 [details] [diff] [review]
new Harmony syntax for generators v2

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

Okay, this is basically good.  Needs a few more changes -- flag me on a much-smaller diff atop this one and I'll r+ that -- and then you can merge the two diffs together into one patch for landing.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +284,5 @@
>               */
>              JSFunction *fun = evalCaller->functionOrCallerFunction();
>              Directives directives(/* strict = */ fun->strict());
> +            ObjectBox *funbox = parser.newFunctionBox(/* fn = */ NULL, fun, pc.addr(),
> +                                                      directives, NotGenerator);

But NotGenerator, here, is supposed to be consistent with |fun|.  And given this:

js> (function outer() { eval("foo: 2"); })()

Breakpoint 1, js::frontend::CompileScript (cx=0x1b8d8d0, alloc=0x1b6fe98, scopeChain=(JSObject * const) 0x7ffff1860680 [object Call] delegate, evalCaller=0x7ffff1855400, 
    options=..., chars=0x1b9e7f0, length=6, source_="foo: 2", staticLevel=2, extraSct=0x0) at /home/jwalden/moz/mfbt/js/src/frontend/BytecodeCompiler.cpp:295
295	                                                      directives, NotGenerator);
(gdb) lis
290	             * wishes to decompile it while it's running.
291	             */
292	            JSFunction *fun = evalCaller->functionOrCallerFunction();
293	            Directives directives(/* strict = */ fun->strict());
294	            ObjectBox *funbox = parser.newFunctionBox(/* fn = */ NULL, fun, pc.addr(),
295	                                                      directives, NotGenerator);
296	            if (!funbox)
297	                return NULL;
298	            bce.objectList.add(funbox);
299	        }
(gdb) p fun
$2 = (JSFunction *) 0x7ffff1860640 [object Function "outer"]

I'm pretty sure that if you could actually execute a direct eval inside a generator, you'd hit this, and NotGenerator would be the wrong thing to pass.  This doesn't bite now, but it will eventually.  I guess this can be fixed in subsequent work, although it seems better to just figure out the generator kind from |fun| itself now.  Might be worth a little helper function on JSFunction for it, as I think you have it a few other places too.

::: js/src/frontend/Parser.cpp
@@ +1100,5 @@
> +            }
> +        } else {
> +            JS_ASSERT(pc->isStarGenerator());
> +            JS_ASSERT(kind != Arrow);
> +            JS_ASSERT(type == StatementListBody);

This just caught my eye as dodgy.  And, sure enough:

js> function* f() yield 7
Assertion failure: type == StatementListBody, at /home/jwalden/moz/mfbt/js/src/frontend/Parser.cpp:1104
Segmentation fault (core dumped)

Lurvely, eh?

Given the expression-closure thing is dead, definitely we shouldn't allow the expression-closure-y syntax for star generators.

Assuming I r+ the overall patch, please flag me on a followup patch that implements this restriction, then land both patches in a single change after review.

::: js/src/ion/AsmJS.cpp
@@ +4603,5 @@
>  
>      AsmJSParseContext *outerpc = m.parser().pc;
>  
>      Directives directives(outerpc);
> +    FunctionBox *funbox = m.parser().newFunctionBox(fn, fun, outerpc, directives, NotGenerator);

Your patch will blithely allow stuff like this:

[jwalden@find-waldo-now src]$ gcc-dbg/js
js> (function* f() { "use asm"; function g() { return 0; } return g; })()()
warning: successfully compiled asm.js code (total compilation time 7ms)
0

I think Parser<FullParseHandler>::asmJS(Node list) needs an early-bail, or something, to handle this.  asm.js parsing of star generator bodies, while it'll probably bail soon enough, is just tempting fate.

::: js/src/jsfun.cpp
@@ +1235,5 @@
>          JS_ASSERT(script->length != 0);
> +        result = script->isGenerator();
> +    } else if (fun->isInterpretedLazy()) {
> +        LazyScript *lazy = fun->lazyScriptOrNull();
> +        result = lazy && (lazy->isStarGenerator() || lazy->isLegacyGenerator());

Hm, yeah, might as well write it future-proofly.

::: js/src/jsiter.cpp
@@ +1478,5 @@
> +    JS_ASSERT(stackfp->script()->isGenerator());
> +
> +    if (stackfp->script()->isStarGenerator) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_ES6_UNIMPLEMENTED);
> +        return NULL;

This is cool for now, but if we don't have a full implementation of generators by the time the next uplift happens, we're going to want to insert code in the parser that treats new-style generators as a syntax error.  Just noting -- cool for now, but this establishes a minor deadline for full (if perhaps non-performant) implementation.

::: js/src/tests/ecma_6/Generators/syntax.js
@@ +4,5 @@
> +// http://code.google.com/p/v8/source/browse/branches/bleeding_edge/test/mjsunit/harmony/generators-parsing.js
> +
> +function assertSyntaxError(str) {
> +    var msg;
> +    var evil = eval;

Cute.

@@ +66,5 @@
> +
> +// Checks that yield is a valid label in classic mode, but not valid in a strict
> +// mode or in generators.
> +function f() { yield: 1 }
> +assertSyntaxError("function f() { \"use strict\"; yield: 1 }")

Use 'use strict', nicer to read.

@@ +71,5 @@
> +assertSyntaxError("function* g() { yield: 1 }")
> +
> +// Yield is only a keyword in the body of the generator, not in nested
> +// functions.
> +function* g() { function f() { yield (yield + yield (0)); } }

Add a |yield| argument to f, to cover all the bases.  Or is that a let-ful binding, in which case you can't add it while still having this parse?  If so, add an assertSyntaxError for it.

::: js/src/vm/Keywords.h
@@ +75,5 @@
> +    /* \
> +       ES5 future reserved keyword in strict mode, keyword in JS1.7 even when \
> +       not strict, keyword inside function* in all versions.  Punt logic to \
> +       parser. \
> +     */ \

This needs the margin-line column of '*' I had in my comment.

Comment 72

[Andy Wingo [:wingo]]

Hi!

Thanks again for the careful review.  One comment:

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #71)
> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +284,5 @@
> >               */
> >              JSFunction *fun = evalCaller->functionOrCallerFunction();
> >              Directives directives(/* strict = */ fun->strict());
> > +            ObjectBox *funbox = parser.newFunctionBox(/* fn = */ NULL, fun, pc.addr(),
> > +                                                      directives, NotGenerator);
> 
> But NotGenerator, here, is supposed to be consistent with |fun|.

Are you sure?  I don't think so.  A few examples:

  function foo() { eval("yield 1") }

Clearly the eval can't turn foo into a generator, so in this case we must conclude that the eval code is parsed as NotGenerator, regardless of other issues.

  function bar() { yield 1; eval("yield 2;") }

In this case the question is, can the eval code yield from the outer legacy generator?  Here since we are dealing with legacy generators, we can test with an unpatched SM:

  js> function bar() { yield 1; eval("yield 2;"); }
  js> var o = bar();        
  js> o.next()
  1
  js> o.next()
  typein:1:0 SyntaxError: yield not in function:
  typein:1:0 yield 2;
  typein:1:0 ^

Finally there are two cases for function*: the JS 1.7 case in which yield is always a keyword and the "standard" case in which yield is a only contextually a keyword.  In the standard case:

  js> function* baz() { yield 1; eval("yield 2;"); }

How do we parse the eval code?  Unfortunately I don't think the current spec draft says anything about it, except that YieldExpression is only valid within a function* -- and the eval code is not within a function*.  Perhaps the spec could be clearer.  However I think the semantics should be that within a yield, YieldExpression is not parsed.  So you would get:

  // untested
  js> version(0);
  js> var yield = 17;
  js> function* qux() { return eval("yield"); }
  js> qux().next()
  { value: 17, done: true }

Note that this does allow you to declare variables named "yield" in a function*:

  js> version(0)
  js> function* zargle() { eval("var yield = 33;"); yield eval("yield"); }
  js> zargle().next()
  { value: 33, done: false }

In summary: parsing eval code as NotGenerator is the only sane thing to do IMO.

Comment 73

[Andy Wingo [:wingo]]

Humm, I think I misinterpreted your comment, :waldo.  I see that that functionbox is not part of the parse context of the eval code, but rather a part of the eval script's captured environment.  You are right; apologies for the verbiage!

Comment 74

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v3

Add a GeneratorKind enumeration, and use it in the parser and runtime to
indicate whether a function is a non-generator, a legacy generator, or a
star generator.

Always parse "yield" as TOK_YIELD, regardless of the version.  Add
TokenStream::currentName() to retrieve the current token's name, which
works for TOK_NAME or TOK_YIELD.  The parser should check the validity
of "yield" as a name, if needed, using checkYieldNameValidity().

Parse "function*" as the StarGenerator GeneratorKind, and allow star
generators to be lazily parsed.

Separate parsing of return statements from yield expressions.

Comment 75

[Andy Wingo [:wingo]]

Hi,

I apologize for submitting an updated patch, but one of your remarks made me rethink the way I was doing things.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #71)
> ::: js/src/frontend/BytecodeCompiler.cpp
> Might be worth a little helper
> function on JSFunction for it, as I think you have it a few other places too.

Indeed, the GeneratorSyntaxKind thing I had was working OK but really it's a GeneratorKind -- a kind for parsing and also a kind at runtime (e.g. for generator functions).  So I renamed it and all uses, and changed the isStarGenerator / isLegacyGenerator flags to be enumerated values.

I put the enumerated values in as 2-bit member bitfields, declared as unsigned integers out of superstition, given some surrounding comments about VS2010's ability to correctly pack bitfields of unrelated types together.

Other than this change, and the reactions to your comments below, this patch is the same.  It neatly fixes this parseFunctionBody() issue by just passing the function's generatorKind() on to the parser.

> js> function* f() yield 7
> Assertion failure: type == StatementListBody, at

Fixed, and added a test.

> js> (function* f() { "use asm"; function g() { return 0; } return g; })()()
> warning: successfully compiled asm.js code (total compilation time 7ms)

Likewise.

> This is cool for now, but if we don't have a full implementation of
> generators by the time the next uplift happens, we're going to want to
> insert code in the parser that treats new-style generators as a syntax
> error.  Just noting -- cool for now, but this establishes a minor deadline
> for full (if perhaps non-performant) implementation.

I'm close.  I have a full implementation of the prototype mess, and I think I'll be able to have the boxed return values in by the end of the week.  Not sure what to do about yield* though.

> > +function* g() { function f() { yield (yield + yield (0)); } }
> 
> Add a |yield| argument to f, to cover all the bases.

Fixed.

Thanks again for the reviews, and apologies for the churn!

Comment 76

[Jeff Walden [:Waldo]]

Comment on attachment 788985 [details] [diff] [review]
new Harmony syntax for generators v3

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

> function* zargle() { eval("var yield = 33;"); yield eval("yield"); }

Oh, that's dastardly.  Definitely one for future testing when semantics are go.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +66,5 @@
>              return false;
>      }
>  
>      // It's an error to use |arguments| in a legacy generator expression.
> +    if (script->isGeneratorExp && script->generatorKind() == LegacyGenerator) {

Hmm.  Direct comparisons against enums make me slightly unhappy.  Could you make this an isLegacyGenerator() accessor instead?  Same for isStarGenerator() as well.  People should, ideally, only have to deal with actual kinds if they want/need to handle all the cases, or if they're passing kindness along, not just if they're checking for one kind or another (or for non-generator-ness or generator-ness).

@@ +284,5 @@
>               */
>              JSFunction *fun = evalCaller->functionOrCallerFunction();
>              Directives directives(/* strict = */ fun->strict());
> +            ObjectBox *funbox = parser.newFunctionBox(/* fn = */ NULL, fun, pc.addr(),
> +                                                      directives, fun->generatorKind());

...like here -- a generatorKind() accessor makes perfect sense here.

@@ +398,5 @@
>      uint32_t staticLevel = lazy->staticLevel(cx);
>  
>      Rooted<JSFunction*> fun(cx, lazy->function());
> +    ParseNode *pn = parser.standaloneLazyFunction(fun, staticLevel, lazy->strict(),
> +                                                  lazy->generatorKind());

I'd keep the !isLegacyGenerator() here as documentation, and to aid in subsequent efforts at implementing lazy parsing for legacy generators.  (Although, to be honest, I'd be somewhat surprised if we ever lazy-parse legacy generators.  :-)  Still a reasonable aspiration in any case.)

::: js/src/frontend/Parser.cpp
@@ +460,5 @@
>      bindings(),
>      bufStart(0),
>      bufEnd(0),
>      ndefaults(0),
> +    generatorKind_(static_cast<uint16_t>(generatorKind)),

Add a standalone method in some suitably-central place (presumably after GeneratorKind's definition), that performs this cast only after asserting that |generatorKind| is one of the actual enumerations, please.  Then use it here.

@@ +1850,5 @@
>      // so we can skip over them after accounting for their free variables.
>      if (LazyScript *lazyOuter = handler.lazyOuterFunction()) {
>          JSFunction *fun = handler.nextLazyInnerFunction();
> +        FunctionBox *funbox = newFunctionBox(pn, fun, pc, Directives(/* strict = */ false),
> +                                             fun->generatorKind());

Keep around the !isLegacyGenerator() assert here for bookkeeping if that ever changes.

@@ +4564,5 @@
> +    JS_ASSERT(tokenStream.isCurrentTokenType(TOK_YIELD));
> +    uint32_t begin = pos().begin;
> +
> +    switch (pc->generatorKind()) {
> +      case StarGenerator:

The switch-statement formulation here is indeed much nicer than what was in previous patches.

I vaguely wonder if this might not be better with the star-generator code moved to a separate method, but I'm inclined to think that's maybe too much, given the other two cases have fallthrough.

@@ +5358,5 @@
>  
> +    if (tt == TOK_YIELD && (versionNumber() >= JSVERSION_1_7 || pc->isGenerator()))
> +    {
> +        return yieldExpression();
> +    }

This shouldn't be braced.

::: js/src/frontend/SharedContext.h
@@ +289,5 @@
> +        // A generator kind can be set at initialization, or when "yield" is
> +        // first seen.  In both cases the transition can only happen from
> +        // NotGenerator.
> +        JS_ASSERT(this->generatorKind() == NotGenerator);
> +        generatorKind_ = static_cast<uint16_t>(generatorKind);

More cast use, and shadowing, and generatorKind == this->generatorKind() killing (see infra).

::: js/src/jsscript.cpp
@@ +2906,5 @@
>       */
>      if (script->needsArgsObj())
>          return true;
>  
> +    JS_ASSERT(script->generatorKind() == NotGenerator);

!isGenerator() is nicer, no need for finicky enums.

::: js/src/jsscript.h
@@ +601,5 @@
>      bool            needsArgsObj_:1;
>  
> +    // Generators are either legacy-style (JS 1.7+ starless generators with
> +    // StopIteration), or ES6-style (function* with boxed return values).
> +    unsigned        generatorKind_:2;

Hmm.  What MSVC dislikes is adjacent bitfields of different types.  bool/unsigned are probably different enough to trigger this, if I read tea leaves from <http://stackoverflow.com/questions/3919194/what-is-vc-doing-when-packing-bitfields>.

There's nothing wrong with using |unsigned| here -- but if you're going to do it, I think you need to change all the other adjacent bitfields to use |unsigned| as the underlying type as well.  :-\

Note that JSScript is finicky enough about size that we really do need everything to pack well.  Other cases like FunctionBox, possibly not so much.  (Although, we probably should do it for FunctionBox as well, even if it's less pressing because FunctionBox isn't so long-lived.)

@@ +640,5 @@
>      jsbytecode *argumentsBytecode() const { JS_ASSERT(code[0] == JSOP_ARGUMENTS); return code; }
>      void setArgumentsHasVarBinding();
>  
> +    js::GeneratorKind generatorKind() const {
> +        return static_cast<js::GeneratorKind>(generatorKind_);

This should use the kind-asserting creation function mentioned earlier.  (Technically it's verified when set -- but best to assert all the time, to catch memory corruption issues earlier.)

@@ +647,5 @@
> +    void setGeneratorKind(js::GeneratorKind generatorKind) {
> +        // A script only gets its generator kind set as part of initialization,
> +        // so it can only transition from NotGenerator.
> +        JS_ASSERT(this->generatorKind() == js::NotGenerator);
> +        generatorKind_ = static_cast<uint32_t>(generatorKind);

Ditto for this.

@@ +1228,5 @@
>          return (HeapPtrFunction *)&freeVariables()[numFreeVariables()];
>      }
>  
> +    GeneratorKind generatorKind() const {
> +        return static_cast<GeneratorKind>(generatorKind_);

And use it here.

@@ +1239,5 @@
> +        // so it can only transition from NotGenerator.
> +        JS_ASSERT(this->generatorKind() == NotGenerator);
> +        // Legacy generators cannot currently be lazy.
> +        JS_ASSERT(generatorKind != LegacyGenerator);
> +        generatorKind_ = static_cast<uint32_t>(generatorKind);

And here again.

I don't think you need the final |generatorKind == this->generatorKind()| assertions in this file.  Also, you should rename all the parameters (for these local methods) to |kind|, perhaps -- otherwise you'll get -Wshadow warnings, and I believe various people don't like those.

Comment 77

[Jeff Walden [:Waldo]]

Oh, and no need to apologize for asking for more reviewing when you're making changes.  :-)  If it were super-trivial, sure, that'd be one thing, but the interdiff clearly crossed that line here.

Comment 78

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v4

Add a GeneratorKind enumeration, and use it in the parser and runtime to
indicate whether a function is a non-generator, a legacy generator, or a
star generator.

Always parse "yield" as TOK_YIELD, regardless of the version.  Add
TokenStream::currentName() to retrieve the current token's name, which
works for TOK_NAME or TOK_YIELD.  The parser should check the validity
of "yield" as a name, if needed, using checkYieldNameValidity().

Parse "function*" as the StarGenerator GeneratorKind, and allow star
generators to be lazily parsed.

Separate parsing of return statements from yield expressions.

Comment 79

[Andy Wingo [:wingo]]

Comment on attachment 789498 [details] [diff] [review]
new Harmony syntax for generators v4

OK, new patch fixes all the nits AFAIK.  Setting r? again to verify the GeneratorKindAsBits and GeneratorKindFromBits definitions, and to check the bitpacking.

In particular, in JSScript it seemed the easiest thing to do was to take a byte, given that there are so many 1-bit fields that factoring them all to be the same type would be madness, and the wasted 6 bits probably don't push us into allocating another word, given that there are thirty-some bitfields already, and they are already offset by a byte for they array flags.

I have added both waldo and jorendorff as reviewers again; if one of you does an r+, please let me know if the other r+ is needed before checking in.  Thanks!

Comment 80

[André Bargull [:anba]]

Can you give the various `function*g(){}` generator function declarations in the test case different names, because per the current ES6 draft duplicate lexically declared names are an early syntax error. Whether this syntax restriction already needs to be implemented in this patch or as a follow-up is not up to me to decide.

Comment 81

[Jeff Walden [:Waldo]]

Comment on attachment 789498 [details] [diff] [review]
new Harmony syntax for generators v4

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

I think this patch is good to land now.  But patch quality doesn't change the fact that we should have two people aware of how things work here.  So I think you still want to flag jorendorff for a look here -- not necessarily a full review, if he doesn't have the time, but I think he should at least do a solid skim of it.

::: js/src/jsscript.h
@@ +409,5 @@
> +enum GeneratorKind { NotGenerator, LegacyGenerator, StarGenerator };
> +
> +static inline unsigned GeneratorKindAsBits(GeneratorKind generatorKind) {
> +    return static_cast<unsigned>(generatorKind);
> +}

static inline unsigned
GeneratorKindAsBits(GeneratorKind generatorKind)

With linkage/type/etc. stuff on one line, function name and arguments on next.

@@ +413,5 @@
> +}
> +
> +static inline GeneratorKind GeneratorKindFromBits(unsigned val) {
> +    JS_ASSERT(val <= StarGenerator);
> +    return static_cast<GeneratorKind>(val);

Same style nitpick as above.

Comment 82

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v5

Add a GeneratorKind enumeration, and use it in the parser and runtime to
indicate whether a function is a non-generator, a legacy generator, or a
star generator.

Always parse "yield" as TOK_YIELD, regardless of the version.  Add
TokenStream::currentName() to retrieve the current token's name, which
works for TOK_NAME or TOK_YIELD.  The parser should check the validity
of "yield" as a name, if needed, using checkYieldNameValidity().

Parse "function*" as the StarGenerator GeneratorKind, and allow star
generators to be lazily parsed.

Separate parsing of return statements from yield expressions.

Comment 83

[Andy Wingo [:wingo]]

Comment on attachment 790202 [details] [diff] [review]
new Harmony syntax for generators v5

Updated patch fixes style nit and is rebased onto inbound.  Thanks for the review, setting checkin-needed :)

r=Waldo

Comment 84

[Ryan VanderMeulen [:RyanVM]]

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

Comment 85

[Ryan VanderMeulen [:RyanVM]]

And backed out for bustage. Trykthxbai.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a869023cf9b6

https://tbpl.mozilla.org/php/getParsedLog.php?id=26553947&tree=Mozilla-Inbound

Comment 86

[Andy Wingo [:wingo]]

OK.  At the end of the 16-bit fields, the mod-CellSize=8 alignment is 6 bytes.  Previously there was one 8-bit field, resulting in mod-Cellsize alignment of 7, and then 35 (!!!) bitfields -- which resulted in clang packing into mod-CellSize of 0, which is as the good lord intended.  Adding a byte for GeneratorKind pushed the size to the next boundary -- probably rounded to 32 bits, on that target, so mod-CellSize is 4.

On 64-bit machines AFAIK we don't have to worry about this restriction because the struct contains 64-bit values, which have 64-bit alignment on those machines.  (I've been testing on 64-bit.)

So on 32-bit we can try to squeeze into the 5 bits that are there, but perhaps that would upset Visual Studio.  Or we can pad.  Or we can squeeze into the ArrayBitsT byte, which has three unused bits.  I think I'll do the latter, out of superstition and stinginess.

Comment 87

[Andy Wingo [:wingo]]

(In reply to André Bargull from comment #80)
> Can you give the various `function*g(){}` generator function declarations in
> the test case different names, because per the current ES6 draft duplicate
> lexically declared names are an early syntax error.

Thanks for the note, André.  Can you point out this part of the ES6 draft?  I'm not seeing it in http://people.mozilla.org/~jorendorff/es6-draft.html#sec-13.1.1.1.  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-10.5.3 seems to suggest that multiple function declarations with the same name are totally kosher.

Comment 88

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v6

Setting r? -- let me know if that's inappropriate for this kind of change.

Comment 89

[André Bargull [:anba]]

Okay, here's come some fun spec jumping through various sections:

[13.1.1.1 Static Semantics]:
>  It is a Syntax Error if the LexicallyDeclaredNames of StatementList contains any duplicate entries.
>  It is a Syntax Error if any element of the LexicallyDeclaredNames of StatementList also occurs in the VarDeclaredNames of StatementList.

Similar restrictions in [14.1.1 Static Semantics] and in [12.1.1.1 Static Semantics].

Let's stick with 13.1 Function Definitions in the following, but the rules are similar for 14.1 Script.

Then go to [13.1.1.1 Static Semantics] "Static Semantics: LexicallyDeclaredNames", entry for "FunctionStatementList : StatementList". 

And then back to [12.1.1.1 Static Semantics] for the definition of "TopLevelLexicallyDeclaredNames", for generator functions the entry "StatementListItem : Declaration" is of interest, "Declaration" is defined in [12 Statements and Declarations]. 

And finally go to [13.4.1.1 Static Semantics] for the definition of BoundNames for generator functions.

Comment 90

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v7

Attached is a rebased version of the same patch, which also kills an unused-var warning in release mode.  Since :Waldo already r+'d a previous version of this patch that was checked in and backed out due to a build failure, I'm going to treat these changes as build fixes and re-mark this patch as r=Waldo and set checkin-needed.  Please let me know if this is inappropriate.

Comment 91

[Ryan VanderMeulen [:RyanVM]]

Please run this through Try first.

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=058711658b32

Comment 93

[Andy Wingo [:wingo]]

Attachment: new Harmony syntax for generators v8

Add a GeneratorKind enumeration, and use it in the parser and runtime to
indicate whether a function is a non-generator, a legacy generator, or a
star generator.

Always parse "yield" as TOK_YIELD, regardless of the version.  Add
TokenStream::currentName() to retrieve the current token's name, which
works for TOK_NAME or TOK_YIELD.  The parser should check the validity
of "yield" as a name, if needed, using checkYieldNameValidity().

Parse "function*" as the StarGenerator GeneratorKind, and allow star
generators to be lazily parsed.

Separate parsing of return statements from yield expressions.

Comment 94

[Andy Wingo [:wingo]]

Comment on attachment 792706 [details] [diff] [review]
new Harmony syntax for generators v8

Updated patch fixes the test failure in browsers by depending on a patch in bug 907072.  r=Waldo from previous patch version.

Comment 95

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=b6662460cbb7, courtesy of jonco, to see if we're good, given that bug 907072 has gone in.

Comment 96

[Andy Wingo [:wingo]]

There were multiple patches in that try.  https://tbpl.mozilla.org/?tree=Try&rev=432512599f62 is a try with just my patch and it looks green with an orange in ggc, but a previous run (https://tbpl.mozilla.org/?tree=Try&rev=058711658b32) was green so it appears that THUNDERCATS ARE GO

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26d92ba69fe6

Comment 98

[Jeff Walden [:Waldo]]

Comment on attachment 790686 [details] [diff] [review]
new Harmony syntax for generators v6

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

A mini-followup-patch to fix these little nits is fine.  :-)

::: js/src/jsscript.h
@@ +531,5 @@
>  
>      uint16_t        nslots;     /* vars plus maximum stack depth */
>      uint16_t        staticLevel;/* static level for display maintenance */
>  
> +    // 4-bit fields.

I'd say

  // Bit fields.

as the four-bitness is very esoteric and isn't something there's much reason to call out explicitly.

@@ +547,4 @@
>    private:
>      // The bits in this field indicate the presence/non-presence of several
>      // optional arrays in |data|.  See the comments above Create() for details.
> +    uint8_t         hasArrayBits:4;

How about |uint8_t hasArrayBits : ARRAY_KIND_BITS;|?  Then you don't need a static_assert() (we can use C++11 static_assert() now, so don't use JS_STATIC_ASSERT in future patches).  This also makes very clear that enough bits are used.

@@ +549,5 @@
>      // optional arrays in |data|.  See the comments above Create() for details.
> +    uint8_t         hasArrayBits:4;
> +
> +    // The GeneratorKind of the script.
> +    uint8_t         generatorKindBits_:4;

I understand the desire for superstition (although really we should go through this junk and make it all the same uint8_t type, or whatever, at some point -- not here, at least).  But if you're doing that, at least make the superstition explicit:

    uint8_t generatorKindBits_ : 2;

    // Unused padding bits; feel free to steal them if you need them.
    uint8_t padToByte : 2;

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26d92ba69fe6

Comment 100

[Gregory Szorc [:gps]]

Are there any plans to update the MDN docs? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators makes no reference to the new syntax :/

Comment 101

[Gregory Szorc [:gps]]

Where is the love for the docs? Someone just told me in a review I should use function* and I have absolutely no idea what the state of this feature implementation is because there are no docs. Building software on a platform of uncertainty due to insufficient documentation is a disaster waiting to happen.

Comment 102

[David Bruant]

(In reply to Gregory Szorc [:gps] from comment #101)
> Where is the love for the docs? Someone just told me in a review I should
> use function* and I have absolutely no idea what the state of this feature
> implementation is because there are no docs. Building software on a platform
> of uncertainty due to insufficient documentation is a disaster waiting to
> happen.
Indeed. Sorry for the delay. I wrote
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/The_Iterator_protocol#With_a_generator
It doesn't explain everything about generators but has some working example with the new syntax.
I scaffolded https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function* to explain generators in depth, but there is no content yet.
Moving forward, I think the "Iterators and generators" page will be retired (among other things because Iterator is not a thing in ES6 and should be removed from SpiderMonkey. bug 881061 )

Comment 103

[Gregory Szorc [:gps]]

Thanks for the docs!

Comment 104

[Tooru Fujisawa [:arai]]

Following documents are already updated:
  https://developer.mozilla.org/en-US/Firefox/Releases/26
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*

Comment 105

[Florian Scholz (Open Web Docs)]

Thanks for the docs everyone. I opened bug 1109166 as we still need to make coherent docs to clearly separate old generators and ES6 ones in MDN content. But that is out of scope for this dev-doc-needed request.