632028 - Reflect.parse('yield 0') should throw a SyntaxError

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

I mean, technically. Right?

Comment 1

[Dave Herman [:dherman]]

Attachment: moves the yield context check from jsemit to jsparse

Comment 2

[Dave Herman [:dherman]]

Surprisingly, we currently do the check in jsemit rather than jsparse. I've attached a patch that moves it into jsparse.

Igor, cdleary says you might know why we're doing it this way. Any reason why my patch might break things?

Thanks,
Dave

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 511767 [details] [diff] [review]
moves the yield context check from jsemit to jsparse

I have no idea why the check is where it is. Goes back to cvs.mozilla.org daze.

/be

Comment 4

[Dave Herman [:dherman]]

http://hg.mozilla.org/tracemonkey/rev/097da81cf423

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Backed out due to failing jsreftests: http://hg.mozilla.org/tracemonkey/rev/b4dbb007e924

Comment 6

[Brendan Eich [:brendan]]

Looks like some tests match exact exception message. Easy to fix by checking just exception kind (SyntaxError).

/be

Comment 7

[Dave Herman [:dherman]]

First, sorry about this -- I ran it on tryserver but thought the oranges I saw weren't due to me. I should've run the tests on my machine first.

> Looks like some tests match exact exception message. Easy to fix by checking
> just exception kind (SyntaxError).

Yeah. I can see how you might prefer a global "yield = 42" to give a syntax error for an invalid yield expression rather than a yield-out-of-context error, but then, we give return-out-of-context for "return = 42" anyway, so I'll just change the tests.

Dave

Comment 8

[Dave Herman [:dherman]]

Oh foo. Generator expression desugaring. In test js1_7/lexical/regress-351515.js we do

    var g = ((yield i) for (i in [1,2,3]));

and similar expressions, and apparently we wanted to allow this. I can imagine arguments both for and against allowing explicit |yield| in generator expressions. (I don't know if anyone actually relies on it, but probably not, right?)

Anyway, this appears to be the main reason why it's performed in the emitter: during parsing, we don't know whether we're inside a generator expression when we hit a |yield| token. After desugaring, the probably goes away. Hence doing it in the emitter.

Dave

Comment 9

[Brendan Eich [:brendan]]

Sorry, I didn't dig into cvs annotate thoroughly:

revision 3.250
date: 2007/05/18 01:41:17;  author: brendan%mozilla.org;  state: Exp;  lines: +12 -4
Support generator expressions for JS1.8 (380237, r=mrbkap).

So let's WONTFIX this bug.

/be

Comment 10

[Dave Herman [:dherman]]

Judging from bug 380237 comment 32, what to do about |yield| in the body of a generator expression was decided to be "what Python does." For Harmony, that's not good enough, so this is a detail we'll need to decide for TC39. But not in this bug.

> So let's WONTFIX this bug.

Well, this bug is about a Reflect.parse bug, and it's genuine. But my fix is no good. I'll have to add validation to jsreflect that parallels the validations in jsemit that couldn't be moved into jsparse.

BTW, it occurs to me that whichever decision we make for generator expressions in Harmony, it'll require some sort of later check that can't be done immediately on discovery of |yield|. Regardless of whether we allow or disallow |yield| in generator expressions, we don't know whether we're in one until we hit the |for| token. This could possibly still be done in jsparse, but we'd have to do something like set a bit indicating whether the current parenthesized expression contains a nested |yield|, so that when we encounter |for| (if we disallow |yield| in generator expressions) or close-parenthesis (if we allow |yield| in generator expressions) we can do the validation.

For now, doing it in jsemit is fine, since I don't want to add any risk. And I'll start over on this bug and do a jsreflect patch that duplicates the validation for Reflect.parse.

Dave

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/097da81cf423
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.

Comment 12

[Dave Herman [:dherman]]

> Note: not marking as fixed because fixed-in-tracemonkey is not present on the
> whiteboard.

Thanks, robotic cdleary! Note that the backout was also merged:

http://hg.mozilla.org/mozilla-central/rev/b4dbb007e924

Dave

Comment 13

[Dave Herman [:dherman]]

This bug will automatically be fixed when 634472 lands.

Dave

Comment 14

[Dave Herman [:dherman]]

This was fixed automatically a while ago when bug 634472 was fixed, and it already has a test case in js1_8_5/extensions/reflect-parse.js.

Dave