Syntax error with 'blank' yield expressions

VERIFIED FIXED in mozilla1.8.1beta2

Status

()

P1
normal
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Seno.Aiko, Assigned: brendan)

Tracking

({verified1.8.1})

Trunk
mozilla1.8.1beta2
verified1.8.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
(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?
(Assignee)

Comment 1

13 years ago
Created attachment 231203 [details] [diff] [review]
proposed fix

Fairly straightforward fix.  I chose to disallow yield yield ... to match Python.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #231203 - Flags: review?(mrbkap)
(Assignee)

Updated

13 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1beta2
Attachment #231203 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 2

13 years ago
Created attachment 231362 [details] [diff] [review]
better fix

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)
(Assignee)

Comment 3

13 years ago
Comment on attachment 231362 [details] [diff] [review]
better fix

Grr, jetlag.

/be
Attachment #231362 - Attachment is obsolete: true
Attachment #231362 - Flags: review?(mrbkap)
(Assignee)

Comment 4

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

Comment 5

13 years ago
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?
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 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+
(Assignee)

Comment 7

13 years ago
Fixed on the 1.8 branch too.

/be
Keywords: fixed1.8.1
(Assignee)

Updated

13 years ago
Flags: blocking1.8.1?

Comment 8

13 years ago
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+

Comment 9

13 years ago
verified fixed 1.8, 1.9 windows/mac(ppc|tel)/linux 20060803
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 10

11 years ago
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.

Comment 11

11 years ago
/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.