Comment on attachment 765884 [details] [diff] [review] Simplify detection of legacy generators Hi, This patch simplifies detection of legacy generators. It introduces a "generator parse mode" and a "last yield offset" that get updated whenever we see a yield. Detecting yield is just comparing the current last yield offset with the previous one. In the future, there will be a StarGenerator parse mode as well. There is a change in one JS 1.8 regression test, in that it expects to see an error about yield in a generator comprehension before an error about the generator comprehension not being parenthesized. Both are valid errors. I tried to eagerly error if a generator wasn't parenthesized, but that "regressed" on tests that expected the error about "yield" in a genexp first. I don't think this is a significant change; WDYT? Also in this patch, maybeNoteLegacyGenerator is unneeded, as the errors about returning a value in a generator are eagerly signalled, both when "return v" is seen in a generator and when a function with "return v" transitions to being a generator. Anyway it's hairy, but I hope isolating this patch is a useful step forward. Depends on bug 884794.

Andy, I bitrotted this a bit by landing bug 883333, but I think returnStatementOrYieldExpression() now has a 'begin' local variable that you can use instead of calling handler.getPosition(pn).begin (in two places). Talking to the handler should be a last result, especially asking it for information, because there are two handlers, and one always tells the truth and one always lies. (The front end was originally written by Raymond Smullyan.) Could you please use -p when generating the next patch here? Thanks!

Comment on attachment 765884 [details] [diff] [review] Simplify detection of legacy generators Review of attachment 765884 [details] [diff] [review]: ----------------------------------------------------------------- Yes! Please -p! Makes Splinter-reviewing so much easier when I can tell what method a change-hunk is changing. I can't wait til new syntax, where this awful backtracking doesn't have to happen, supplants the old and busted syntax. ::: js/src/frontend/Parser.cpp @@ +1062,5 @@ > + } > + > + if (pc->lastYieldOffset != startYieldOffset) { > + JS_ASSERT(pc->isLegacyGenerator()); > + if (kind == Arrow) { Hmm. This really should be detected earlier. Could you do a followup to track the kind of function-y thing being parsed and error out immediately when yield is encountered in an arrow function (or expression-body function)? If you think this'll fall out naturally from the rest of the parsing work, that's cool too. @@ +1067,5 @@ > + reportWithOffset(ParseError, false, pc->lastYieldOffset, > + JSMSG_YIELD_IN_ARROW, js_yield_str); > + return null(); > + } else if (type == ExpressionBody) { > + JS_ASSERT(pc->isLegacyGenerator()); You just asserted this! :-) @@ +3208,5 @@ > // 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. > if (tt == TOK_YIELD) { > if (!abortIfSyntaxParser()) > return null(); So, you should be aware that source coordinates in syntax-parsing mode are mostly bogus. The coordinates for function extents are sane, but I don't think most others are. That means lastYieldOffset may well be a bit broken. But for now, at least, you're saved because of this abort. In the long run we want to syntax-parse this stuff, tho; not sure what'll have to be done for this, really. @@ +3220,4 @@ > } else { > + // We are in a code that has not seen a yield, and in JS 1.8 so > + // "yield" parsed as TOK_YIELD. Try to transition to being a legacy > + // generator. Add an assert that tokenStream.versionNumber() >= JSVERSION_1_8. @@ +5825,5 @@ > if (allowsForEachIn() && tokenStream.matchContextualKeyword(context->names().each)) > pn2->pn_iflags |= JSITER_FOREACH; > MUST_MATCH_TOKEN(TOK_LP, JSMSG_PAREN_AFTER_FOR); > > + uint32_t startYieldOffset = pc->lastYieldOffset; Hmm. :-\ Yeah, I think this is a bit too bitrotted by jorendorff's patch-stack for me to reasonably review it. Stopping off here, I'll look at the rest of this when an updated patch is posted. ::: js/src/frontend/Parser.h @@ +99,5 @@ > > + /* Functions start off being parsed as NotGenerator. In JS 1.8 mode, a > + NotGenerator will transition to LegacyGenerator if it sees a "yield", and > + the offset of the last yield that was parsed will be stored in > + lastYieldOffset. Otherwise lastYieldOffset is NoYieldOffset. */ /* * Functions start off... * NotGenerator will transition... * the offset... * lastYieldOffset... */ @@ +101,5 @@ > + NotGenerator will transition to LegacyGenerator if it sees a "yield", and > + the offset of the last yield that was parsed will be stored in > + lastYieldOffset. Otherwise lastYieldOffset is NoYieldOffset. */ > + enum GeneratorParseMode { NotGenerator, LegacyGenerator }; > + static const uint32_t NoYieldOffset = -1; UINT32_MAX

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > @@ +3208,5 @@ > > // 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. > > if (tt == TOK_YIELD) { > > if (!abortIfSyntaxParser()) > > return null(); > > So, you should be aware that source coordinates in syntax-parsing mode are > mostly bogus. The coordinates for function extents are sane, but I don't > think most others are. That means lastYieldOffset may well be a bit broken. > > But for now, at least, you're saved because of this abort. In the long run > we want to syntax-parse this stuff, tho; not sure what'll have to be done > for this, really. I didn't look at this too closely, but I don't think it'll be a problem. A field in ParseContext suffices to tell us if we're looking at a legacy generator or not, so I *think* the location is only really needed for error messages. And as I mentioned in comment 3, we can get that information without asking the handler for it.

Comment on attachment 765884 [details] [diff] [review] Simplify detection of legacy generators Review of attachment 765884 [details] [diff] [review]: ----------------------------------------------------------------- Also clearing the review request for now. But looking forward to reviewing an updated patch!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > I can't wait til new syntax, where this awful backtracking doesn't have to > happen, supplants the old and busted syntax. For reals... > ::: js/src/frontend/Parser.cpp > @@ +1062,5 @@ > > + } > > + > > + if (pc->lastYieldOffset != startYieldOffset) { > > + JS_ASSERT(pc->isLegacyGenerator()); > > + if (kind == Arrow) { > > Hmm. This really should be detected earlier. Could you do a followup to > track the kind of function-y thing being parsed and error out immediately > when yield is encountered in an arrow function (or expression-body > function)? If you think this'll fall out naturally from the rest of the > parsing work, that's cool too. I'll do a followup. Currently this error is caught after parsing the function; this patch moves the check into functionBody(), but still after parsing the function. I'm pretty sure detecting the error eagerly will alter some of the JS1.8 reftests, but that's probably OK. > @@ +1067,5 @@ > > + reportWithOffset(ParseError, false, pc->lastYieldOffset, > > + JSMSG_YIELD_IN_ARROW, js_yield_str); > > + return null(); > > + } else if (type == ExpressionBody) { > > + JS_ASSERT(pc->isLegacyGenerator()); > > You just asserted this! :-) Heh, indeed ;) Fixed. > @@ +3208,5 @@ > > // 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. > > if (tt == TOK_YIELD) { > > if (!abortIfSyntaxParser()) > > return null(); > > So, you should be aware that source coordinates in syntax-parsing mode are > mostly bogus. The coordinates for function extents are sane, but I don't > think most others are. That means lastYieldOffset may well be a bit broken. > > But for now, at least, you're saved because of this abort. In the long run > we want to syntax-parse this stuff, tho; not sure what'll have to be done > for this, really. As Jason notes, it seems I can get this information reliably from the tokenStream. Given that, perhaps we can syntax-parse this stuff already... > > @@ +3220,4 @@ > > } else { > > + // We are in a code that has not seen a yield, and in JS 1.8 so > > + // "yield" parsed as TOK_YIELD. Try to transition to being a legacy > > + // generator. > > Add an assert that tokenStream.versionNumber() >= JSVERSION_1_8. Good idea; it's actually JSVERSION_1_7 it seems. (The novelty in 1.8 is generator expressions, which ES6 calls generator comprehensions.) > ::: js/src/frontend/Parser.h > @@ +99,5 @@ > > > > + /* Functions start off being parsed as NotGenerator. In JS 1.8 mode, a > > + NotGenerator will transition to LegacyGenerator if it sees a "yield", and > > + the offset of the last yield that was parsed will be stored in > > + lastYieldOffset. Otherwise lastYieldOffset is NoYieldOffset. */ > > /* > * Functions start off... > * NotGenerator will transition... > * the offset... > * lastYieldOffset... > */ As you like. > > @@ +101,5 @@ > > + NotGenerator will transition to LegacyGenerator if it sees a "yield", and > > + the offset of the last yield that was parsed will be stored in > > + lastYieldOffset. Otherwise lastYieldOffset is NoYieldOffset. */ > > + enum GeneratorParseMode { NotGenerator, LegacyGenerator }; > > + static const uint32_t NoYieldOffset = -1; > > UINT32_MAX Done.

Comment on attachment 766605 [details] [diff] [review] Simplify detection of legacy generators Review of attachment 766605 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +1066,5 @@ > + if (kind == Arrow) { > + reportWithOffset(ParseError, false, pc->lastYieldOffset, > + JSMSG_YIELD_IN_ARROW, js_yield_str); > + return null(); > + } else if (type == ExpressionBody) { Nit: don't put an else after a return (twice)

Comment on attachment 766605 [details] [diff] [review] Simplify detection of legacy generators Review of attachment 766605 [details] [diff] [review]: ----------------------------------------------------------------- This is kinda gnarly, but it's better than the current system, I agree. ::: js/src/frontend/Parser.cpp @@ +3221,5 @@ > } else { > + // We are in a code that has not seen a yield, and in JS 1.8 so > + // "yield" parsed as TOK_YIELD. Try to transition to being a legacy > + // generator. > + JS_ASSERT(tokenStream.currentToken().type == TOK_YIELD); This assert seems kind of vacuous given we tested |tt| above, and that came from exactly this expression. Remove it. @@ +3232,5 @@ > + > + pc->generatorParseMode = ParseContext<ParseHandler>::LegacyGenerator; > + pc->lastYieldOffset = begin; > + > + if (pc->funHasReturnExpr) { For reader sanity, let's handle the error cases *before* we continue on to actually set the generator bits here. So move this up just after the |if (!is function box) {}| block.

Okeydoke, patch updated, with no regression on the jit tests or the jstests. Since some time has passed and this is a gnarly patch, setting r? again. Let me know if some other course of action is better.

Comment on attachment 784343 [details] [diff] [review] Simplify detection of legacy generators Updated patch folds in a typo fix from bug 886322 comment 11.

Comment on attachment 784343 [details] [diff] [review] Simplify detection of legacy generators Review of attachment 784343 [details] [diff] [review]: ----------------------------------------------------------------- Not that Bugzilla interdiff always works (and didn't here, but I could always hope it migth), but it's nice for patches on a bug to have different names, so that it's easier to find the right thing to interdiff against. ::: js/src/frontend/Parser.cpp @@ +4438,5 @@ > + return null(); > + } > + > + pc->generatorParseMode = ParseContext<ParseHandler>::LegacyGenerator; > + pc->lastYieldOffset = begin; This pc->lastYieldOffset = begin could move outside the if-else totally. (I feel like I noted this once before and maybe you had some reason for not doing it, but I don't have time to review the bug and see.)

Comment on attachment 786220 [details] [diff] [review] Simplify detection of legacy generators Uploading version with fixed nits, r=Waldo.