Closed Bug 884794 Opened 6 years ago Closed 6 years ago

Rename JS1.7 generators to "legacy generators"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: wingo, Assigned: wingo)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
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.
Summary: tip → Rename JS1.8 generators to "legacy generators"
Assignee: general → wingo
Attachment #764722 - Attachment is obsolete: true
Attachment #764724 - Flags: review?(jorendorff)
Depends on: 648949
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.
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.
Blocks: 885695
Attachment #764724 - Attachment is obsolete: true
Attachment #764724 - Flags: review?(jorendorff)
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.
Attachment #766574 - Flags: review?(jwalden+bmo)
Attachment #766574 - Flags: review?(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
@@ +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.
Attachment #766574 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #766574 - Flags: review?(jorendorff) → review+
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.
Summary: Rename JS1.8 generators to "legacy generators" → Rename JS1.7 generators to "legacy generators"
Attachment #766574 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d67875c9897
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.