Last Comment Bug 884794 - Rename JS1.7 generators to "legacy generators"
: Rename JS1.7 generators to "legacy generators"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Andy Wingo [:wingo]
:
Mentors:
Depends on: 648949
Blocks: harmony:generators 885695
  Show dependency treegraph
 
Reported: 2013-06-19 06:18 PDT by Andy Wingo [:wingo]
Modified: 2013-08-01 14:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rename JS1.8 generators to "legacy generators" (22.90 KB, patch)
2013-06-19 06:23 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Rename JS1.8 generators to "legacy generators" (22.88 KB, patch)
2013-06-19 06:29 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Rename JS1.8 generators to "legacy generators" (23.89 KB, patch)
2013-06-24 01:32 PDT, Andy Wingo [:wingo]
jwalden+bmo: review+
jorendorff: review+
Details | Diff | Splinter Review
Rename JS1.7+ generators to "legacy generators" (24.68 KB, patch)
2013-07-31 07:54 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review

Description Andy Wingo [:wingo] 2013-06-19 06:18:52 PDT

    
Comment 1 Andy Wingo [:wingo] 2013-06-19 06:22:49 PDT
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] 2013-06-19 06:23:43 PDT
Created attachment 764722 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"
Comment 3 Andy Wingo [:wingo] 2013-06-19 06:29:46 PDT
Created attachment 764724 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"
Comment 4 Andy Wingo [:wingo] 2013-06-19 08:29:49 PDT
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] 2013-06-21 01:48:50 PDT
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] 2013-06-24 01:32:44 PDT
Created attachment 766574 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"
Comment 7 Andy Wingo [:wingo] 2013-06-24 01:35:14 PDT
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] (remove +bmo to email) 2013-06-25 10:01:38 PDT
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] 2013-06-25 11:42:51 PDT
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] 2013-07-31 07:39:40 PDT
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] 2013-07-31 07:54:26 PDT
Created attachment 783769 [details] [diff] [review]
Rename JS1.7+ generators to "legacy generators"
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-07-31 14:23:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d67875c9897
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-08-01 14:00:22 PDT
https://hg.mozilla.org/mozilla-central/rev/4d67875c9897

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