884794 - Rename JS1.7 generators to "legacy generators"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andy Wingo [:wingo]]

In preparation for supporting ES6 generators, the patch in this bug renames fields and methods like "isGenerator" to "isLegacyGenerator".  JS 1.8 generators are legacy in the sense of "venerable and awesome but ultimately not what we want people to use going forward".  This will allow the parser to know whether it's in an ES6 generator versus a JS1.8 generator, and to let the various parts of the runtime know this as well.

Sorry for the lack of a description; this was my first time using hg newbug and I thought it would have uploaded the patch directly.

Comment 2

[Andy Wingo [:wingo]]

Attachment: Rename JS1.8 generators to "legacy generators"

Comment 3

[Andy Wingo [:wingo]]

Attachment: Rename JS1.8 generators to "legacy generators"

Comment 4

[Andy Wingo [:wingo]]

Comment on attachment 764724 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"

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

::: js/src/frontend/Parser.cpp
@@ +3203,1 @@
>      if (tt == TOK_YIELD) {

AFAICS this comment that I introduce here is incorrect -- we get TOK_YIELD in all modes, and the strict mode case is already handled in the tokenizer.  Will fix.

Comment 5

[Andy Wingo [:wingo]]

FWIW this comment (In reply to Andy Wingo from comment #4)
> Comment on attachment 764724 [details] [diff] [review]
> Rename JS1.8 generators to "legacy generators"
> 
> Review of attachment 764724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/Parser.cpp
> @@ +3203,1 @@
> >      if (tt == TOK_YIELD) {
> 
> AFAICS this comment that I introduce here is incorrect -- we get TOK_YIELD
> in all modes, and the strict mode case is already handled in the tokenizer. 
> Will fix.

This bugzilla comment was incorrect.  If yield is seen in non-strict web mode, it falls through as TOK_NAME.

Comment 6

[Andy Wingo [:wingo]]

Attachment: Rename JS1.8 generators to "legacy generators"

Comment 7

[Andy Wingo [:wingo]]

Comment on attachment 766574 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"

Refreshed patch.  Since the two of you are working on / reviewing parser things, adding you as reviewers; please let me know if there is a better strategy.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 766574 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"

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

::: js/src/frontend/Parser.cpp
@@ +3198,5 @@
>      if (!pn)
>          return null();
>  
> +    // Legacy generators are identified by the presence of "yield" in their
> +    // bodies.  We only see yield as TOK_YIELD in JS 1.8 mode, not in web mode.

I'd rather you said "in JS 1.8 scripts and not in unversioned scripts."  The web can use JS1.8, just needs the right type= on the script element.

::: js/src/jsiter.cpp
@@ +1016,5 @@
>               */
>              ni->props_cursor = ni->props_array;
>          }
>      }
> +    // FIXME: Only close legacy generators.

Please put this inside the isGenerator() block -- comments before elses screw with reading comprehension, because it's less immediately obvious where the if-statement ends.

@@ +1021,1 @@
>      else if (obj->isGenerator()) {

This was pre-existing, but this should be

    } else if (obj->isGenerator()) {

::: js/src/tests/js1_8/genexps/regress-667131.js
@@ +5,5 @@
>  
>  
>  //-----------------------------------------------------------------------------
>  var BUGNUMBER = 667131;
> +var summary = 'yield ignored if maybeNoteLegacyGenerator called too late';

We have all sorts of old, bogus, obsolete symbol references in tests -- no need to fix these up if you don't want to, in the future.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 766574 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"

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

::: js/src/frontend/Parser.cpp
@@ +5609,3 @@
>  {
>      ParseContext<ParseHandler> *pc = parser->pc;
> +    // yieldCount is only incremented when we see yield in legacy JS 1.8 code.

Drop the word "legacy" here and say "JS 1.8+ code" (since 1.8.5 has legacy generators too). Or say "in legacy generators" if we really are not going to bump yieldCount within ES6 generators; that wasn't clear to me.

@@ +5612,4 @@
>      if (pc->yieldCount > 0) {
>          if (!pc->sc->isFunctionBox()) {
> +            // FIXME: This error should be detected eagerly, when the yield is
> +            // seen.

Definitely.

::: js/src/jsanalyze.cpp
@@ +1825,5 @@
>      /*
>       * Always construct arguments objects when in debug mode and for generator
>       * scripts (generators can be suspended when speculation fails).
> +     *
> +     * FIXME: Don't build arguments for ES6 generator expressions.

A quick warning: When you get to this, note that not having an 'arguments' binding means (I think) that direct eval in a generator expression must deoptimize arguments in the nearest enclosing function that has arguments. Otherwise eval("arguments") could blow up. See bug 852016.

::: js/src/jsscript.cpp
@@ +588,5 @@
>              script->isGenerator = true;
>          if (scriptBits & (1 << IsGeneratorExp))
>              script->isGeneratorExp = true;
> +        if (scriptBits & (1 << IsLegacyGenerator))
> +            script->isLegacyGenerator = true;

After changing anything to do with "XDR" (or the bytecode) please bump XDR_BYTECODE_VERSION in js/src/vm/Xdr.h.

Or "increment the subtrahend", as the comment there says.

Comment 10

[Andy Wingo [:wingo]]

Sorry for the delay; another project got in the way, but I'm totally on this generators thing now.

Changed bug title to indicate that the generators were present in JS 1.7; it was generator comprehensions that landed in JS 1.8.  Thanks for the review, pushing a rebased patch and marking as checkin-needed.

Comment 11

[Andy Wingo [:wingo]]

Attachment: Rename JS1.7+ generators to "legacy generators"

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d67875c9897

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d67875c9897