Closed Bug 345855 Opened 15 years ago Closed 15 years ago

Syntax error with 'blank' yield expressions

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1beta2

People

(Reporter: Seno.Aiko, Assigned: brendan)

References

Details

(Keywords: verified1.8.1)

Attachments

(1 file, 1 obsolete file)

(Split from bug 326466 comment 143)

The following expressions give syntax errors but should be allowed (note that yield is not followed by an expression here)

  x = 12 + (yield)
  foo(yield)

I falsely claimed that the following expressions have the same problem but they're fine:

  x = 12 + (yield 42)
  foo(yield 42)

I don't know why Python doesn't allow "yield yield", maybe it's one of those "edge cases" mentioned in PEP 342?
Attached patch proposed fix (obsolete) — Splinter Review
Fairly straightforward fix.  I chose to disallow yield yield ... to match Python.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #231203 - Flags: review?(mrbkap)
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1beta2
Attachment #231203 - Flags: review?(mrbkap) → review+
Attached patch better fixSplinter Review
The previous patch degraded error messages for cases such as return) gratuitously.

This patch also outlaws |yield yield|, to match Python.

/be
Attachment #231203 - Attachment is obsolete: true
Attachment #231362 - Flags: review?(mrbkap)
Comment on attachment 231362 [details] [diff] [review]
better fix

Grr, jetlag.

/be
Attachment #231362 - Attachment is obsolete: true
Attachment #231362 - Flags: review?(mrbkap)
Comment on attachment 231362 [details] [diff] [review]
better fix

No, this is fine.  Not sure what I was seeing, but this works in my Minefield build, and it makes sense. I chose to test (tt != TOK_YIELD || ...) in the #if'd line in order to make clearer what is going on with the (tt2 != tt && ...) right-hand-side of || there.

/be
Attachment #231362 - Attachment is obsolete: false
Attachment #231362 - Flags: review?(mrbkap)
Attachment #231362 - Flags: review?(mrbkap) → review+
Comment on attachment 231362 [details] [diff] [review]
better fix

Fixed on trunk.  This is another required js1.7 fix.

/be
Attachment #231362 - Flags: approval1.8.1?
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: blocking1.8.1?
Resolution: --- → FIXED
Comment on attachment 231362 [details] [diff] [review]
better fix

a=drivers, please land this on the MOZILLA_1_8_BRANCH.
Attachment #231362 - Flags: approval1.8.1? → approval1.8.1+
Fixed on the 1.8 branch too.

/be
Keywords: fixed1.8.1
Flags: blocking1.8.1?
Checking in regress-345855.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-345855.js,v  <--  regress-345855.js
initial revision: 1.1
Flags: in-testsuite+
verified fixed 1.8, 1.9 windows/mac(ppc|tel)/linux 20060803
Status: RESOLVED → VERIFIED
note, bug 384991 changed the behavior of js1_7/geniter/regress-345855.js on the trunk to no longer require yield be parenthesized which currently causes 

  expect = /SyntaxError: yield expression must be parenthesized/;
  try
  {
    eval('(function (){foo(yield 42)})');
    actual = 'No Error';
  }
  catch(ex)
  {
    actual = ex + '';
  }
  reportMatch(expect, actual, summary + ': function (){foo(yield 42)}');

to fail on spidermonkey trunk. I can either remove this test or change it to require no error which will show up on 1.8.1 as a known failure. 

Boyd, fyi for rhino.
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-345855.js,v  <--  regress-345855.js
new revision: 1.6; previous revision: 1.5
You need to log in before you can comment on or make changes to this bug.