Rename JS1.7 generators to "legacy generators"

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wingo, Assigned: wingo)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
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)

Comment 2

4 years ago
Created attachment 764722 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"
(Assignee)

Updated

4 years ago
Assignee: general → wingo
(Assignee)

Comment 3

4 years ago
Created attachment 764724 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"
(Assignee)

Updated

4 years ago
Attachment #764722 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #764724 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Blocks: 666399
Depends on: 648949
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 885695
(Assignee)

Comment 6

4 years ago
Created attachment 766574 [details] [diff] [review]
Rename JS1.8 generators to "legacy generators"
(Assignee)

Updated

4 years ago
Attachment #764724 - Attachment is obsolete: true
Attachment #764724 - Flags: review?(jorendorff)
(Assignee)

Comment 7

4 years ago
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+
(Assignee)

Comment 10

4 years ago
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"
(Assignee)

Comment 11

4 years ago
Created attachment 783769 [details] [diff] [review]
Rename JS1.7+ generators to "legacy generators"
Attachment #766574 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d67875c9897
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d67875c9897
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.