The default bug view has changed. See this FAQ.

for-in doesn't allow call, grouped, or XMLName LHS expressions

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
major
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Yuh-Ruey Chen, Assigned: brendan)

Tracking

({crash, verified1.8.0.2, verified1.8.1})

Trunk
mozilla1.9alpha1
crash, verified1.8.0.2, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.0.1 -
blocking1.8.0.2 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rft-dl])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
The spec says that that the first operand in a for-in statement (the |a| in |for(a in b)|) is a LeftHandSideExpression. Besides primary and member expressions, LeftHandSideExpression also includes new, call, and grouping operators.

However, the following code results in a syntax error ("invalid for/in left-hand side"):

function foo() { for (a() in b); }

as does this:

function foo() { for (new a() in b); }

and this:

function foo() { for ((a) in b); }

That last example is the only one that really makes sense, but the others are still syntactically valid.

As a side note, Rhino accepts a call operator but not a new operator here.

Technically speaking, grouping operator could be used to put even more nonsensical expressions, e.g.

function foo() { for ((a * b) in b); }

Perhaps the spec needs some clarification on this?
(Assignee)

Comment 1

11 years ago
ECMA-262 Edition 3 12.6.4 says in part

6. Evaluate the LeftHandSideExpression ( it may be evaluated repeatedly).
7. Call PutValue(Result(6), Result(5)).

PutValue is specified with this first step:

8.7.2 PutValue (V, W)
1. If Type(V) is not Reference, throw a ReferenceError exception.

And Section 16 Errors says in part:

An implementation may treat any instance of the following kinds of runtime errors as a syntax error and therefore report it early:
[. . .]
• Attempts to call PutValue on a value that is not a reference (for example, executing the assignment statement 3=4).

This bug is INVALID.  Please attend to Section 16 when reading the spec.  Please also file a bug against Rhino if appropriate.  Thanks,

/be
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → INVALID
(Assignee)

Comment 2

11 years ago
Rhino may allow a call in an lvalue context because native methods can return what amounts to a Reference type -- someone who knows Rhino should vouch.

SpiderMonkey allows native functions to do the same, but it doesn't allow a call as you note in the LHS for for/in.  Reopening and morphing this bug to target just this problem.

/be
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: for-in doesn't allow all LHS expressions (ECMAScript compliance) → for-in doesn't allow call LHS expressions
(Assignee)

Updated

11 years ago
Assignee: general → brendan
Status: REOPENED → NEW
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 3

11 years ago
Don't care about fixing in JS1.6 RC1 -- Bob, twist my arm if you feel differently.

The native function in the JS shell that can return a Reference type is it.item():

js> it.item
function item() {
    [native code]
}
js> it.item(0)
js> it.item(0) = "zero"
zero
js> it.item(0)
zero

/be
Status: NEW → ASSIGNED
(Assignee)

Comment 4

11 years ago
Oh, and good point about for ((i) in o)...  SpiderMonkey until recently wrongly dereferenced parenthesized expressions (bug 320032).  Fixing that didn't allow this particular LHS, however.  Will fix here.

/be
Summary: for-in doesn't allow call LHS expressions → for-in doesn't allow call or grouped LHS expressions
(Assignee)

Comment 5

11 years ago
(In reply to comment #4)
> Oh, and good point about for ((i) in o)...

But as noted in comment 1, the concern in comment 0, that

function foo() { for ((a * b) in b); }

is allowed by the ECMA-262 Edition 3 spec, is invalid by definition of PutValue, and the ReferenceError may be reported early independent of run-time control flow due to the Errors section of the spec.

/be
(Assignee)

Comment 6

11 years ago
Created attachment 207192 [details] [diff] [review]
fix

I remember now getting lazy when hacking JS_HAS_LVALUE_RETURN, and blowing off the for-in LHS case.  It wasn't that hard, so shame on me.  Only took a few hunks of patching to jsparse.c and jsemit.c, and it reuses most of the JSOP_FORELEM and JSOP_ENUMELEM bytecoding, because JSOP_SETCALL returns a [obj, id] pair on the interpreter stack (AKA a Reference).

/be
Attachment #207192 - Flags: superreview?(shaver)
Attachment #207192 - Flags: review?(mrbkap)
(Assignee)

Comment 7

11 years ago
Comment on attachment 207192 [details] [diff] [review]
fix

More notes:

>-          default:              JS_ASSERT(mode == 0); break;
>+          default:              JS_ASSERT(mode <= JOF_NAME); break;

Bogus assertion, independent fix.  Here mode could be 0 or JOF_NAME.  This should get in soon, it was in my tree already, so I'm piggybacking here.

>             /* Check that the left side of the 'in' is valid. */
>+            while (pn1->pn_type == TOK_RP)
>+                pn1 = pn1->pn_kid;

This skips useless parenthesization of the LHS in for-in, and doesn't bother to recycle the TOK_RP nodes to the compiler's temporary node freelist (tc->nodeList), if any.  Not worth the code bloat, IMHO, to reduce slightly the cx->tempPool high-watermark for the hard and very rare case where someone parenthesizes a for-in LHS.

/be
(Assignee)

Comment 8

11 years ago
Demo session showing it.item testability and using Script to disassemble bytecode:

js> a = 1
1
js> b = 2
2
js> c = 3
3
js> s = Script('for (it.item(0) in this) print(it.item(0))')
for (it.item(0)[] in this) {
    print(it.item(0));
}

js> dis(s)
main:
00000:  push
00001:  this
00002:  toobject
00003:  forelem
00004:  ifeq 41 (37)
00007:  name "it"
00010:  getmethod "item"
00013:  pushobj
00014:  zero
00015:  setcall 1
00018:  enumelem
00019:  name "print"
00022:  pushobj
00023:  name "it"
00026:  getmethod "item"
00029:  pushobj
00030:  zero
00031:  call 1
00034:  call 1
00037:  popv
00038:  goto 3 (-35)
00041:  pop2
00042:  stop

Source notes:
  0:     4 [   4] while    offset 34
  2:    10 [   6] pcbase   offset 3
  4:    15 [   5] pcbase   offset 8
  6:    26 [  11] xdelta
  7:    26 [   0] pcbase   offset 3
  9:    31 [   5] pcbase   offset 8
 11:    34 [   3] pcbase   offset 15
js> s()
a
b
c
s

Oops, forgot the beloved decompiler (that stray [] in the LHS)!  I always do.  New patch soon.

/be
(Assignee)

Comment 9

11 years ago
Created attachment 207197 [details] [diff] [review]
fix, v2

Much better (I also fixed for each to work in the JSOP_FORELEM/ENUMELEM case):

js>  s = Script('for each (it.item(0) in this)print(it.item(0))')
for each (it.item(0) in this) {
    print(it.item(0));
}

js> a = 1
1
js> b = 2
2
js> c = 3
3
js> s()
for each (it.item(0) in this) {
    print(it.item(0));
}

1
2
3
js> t = Script('for (it.item(0) in this)print(it.item(0))')
for (it.item(0) in this) {
    print(it.item(0));
}

js> t()
s
a
b
c
t

/be
Attachment #207192 - Attachment is obsolete: true
Attachment #207197 - Flags: superreview?(shaver)
Attachment #207197 - Flags: review?(mrbkap)
Attachment #207192 - Flags: superreview?(shaver)
Attachment #207192 - Flags: review?(mrbkap)
(Assignee)

Comment 10

11 years ago
Created attachment 207215 [details] [diff] [review]
fix, v3

E4X, sigh.  The various XML names that could occur on the left-hand side of an assignment operator, or as the operand of ++ or --, could not be the LHS of in within a for-in loop.  This patch fixes the compiler to allow such madness as using an attribute as the for-in iteration variable, e.g.:

js> s = Script('with (<xml a="w"/>) for each (@a in <xml a="b"/>.@a)print(@a)')
with (<xml a="w"/>) {
    for each (@a in (<xml a="b"/>).@a) {
        print(@a);
    }
}

js> dis(s)
main:
00000:  startxml
00001:  string "<"
00004:  string "xml"
00007:  add
00008:  string "a"
00011:  addattrname
00012:  string "w"
00015:  addattrval
00016:  string "/>"
00019:  add
00020:  toxml
00021:  enterwith
00022:  push
00023:  startxml
00024:  string "<"
00027:  string "xml"
00030:  add
00031:  string "a"
00034:  addattrname
00035:  string "b"
00038:  addattrval
00039:  string "/>"
00042:  add
00043:  toxml
00044:  qnamepart "a"
00047:  toattrname
00048:  getelem
00049:  toobject
00050:  foreach
00051:  forelem
00052:  ifeq 77 (25)
00055:  qnamepart "a"
00058:  toattrname
00059:  bindxmlname
00060:  enumelem
00061:  name "print"
00064:  pushobj
00065:  qnamepart "a"
00068:  toattrname
00069:  xmlname
00070:  call 1
00073:  popv
00074:  goto 50 (-24)
00077:  pop2
00078:  leavewith
00079:  stop

Source notes:
  0:    48 [  48] xdelta
  1:    48 [   0] pcbase   offset 25
  3:    52 [   4] while    offset 22
  5:    69 [  17] xdelta
  6:    69 [   0] pcbase   offset 4
  8:    70 [   1] pcbase   offset 9
js> s()
b

/be
Attachment #207197 - Attachment is obsolete: true
Attachment #207215 - Flags: superreview?(shaver)
Attachment #207215 - Flags: review?(mrbkap)
Attachment #207197 - Flags: superreview?(shaver)
Attachment #207197 - Flags: review?(mrbkap)
(Assignee)

Comment 11

11 years ago
I'm thinking we do want this in JS1.6 RC1, given the E4X-related bugs, and the decompiler bugs (did anyone notice that doubled %s I fixed between the v2 and v3 patches?  Eeek).

/be
Blocks: 309169
Flags: blocking1.8.0.1?
(Assignee)

Updated

11 years ago
Severity: trivial → major
Keywords: crash
OS: Windows XP → All
Priority: P3 → P1
Hardware: PC → All
(Assignee)

Comment 12

11 years ago
(In reply to comment #7)
> (From update of attachment 207192 [details] [diff] [review] [edit])
> More notes:
> 
> >-          default:              JS_ASSERT(mode == 0); break;
> >+          default:              JS_ASSERT(mode <= JOF_NAME); break;
> 
> Bogus assertion, independent fix.  Here mode could be 0 or JOF_NAME.  This
> should get in soon, it was in my tree already, so I'm piggybacking here.

Ignore this, I was wrong -- it's a valid assertion.  See bug 321971.  I'll put a pure patch up that obsoletes the latest one, removing only this hunk.

/be
(Assignee)

Comment 13

11 years ago
Created attachment 207246 [details] [diff] [review]
fix, v4

Bugzilla's interdiff is handy to verify things.

/be
Attachment #207215 - Attachment is obsolete: true
Attachment #207246 - Flags: superreview?(shaver)
Attachment #207246 - Flags: review?(mrbkap)
Attachment #207215 - Flags: superreview?(shaver)
Attachment #207215 - Flags: review?(mrbkap)
(Assignee)

Updated

11 years ago
Summary: for-in doesn't allow call or grouped LHS expressions → for-in doesn't allow call, grouped, or XMLName LHS expressions
Comment on attachment 207246 [details] [diff] [review]
fix, v4

I think I follow this.
Attachment #207246 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 15

11 years ago
With mrbkap's ringing endorsement, I'm checking in now.  Leaving bug open for shaver's sr+ before nominating for branch.

/be
doesn't seem like something we'd block on, but you can ask for patch approval if you get the reviews.
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
(Assignee)

Comment 17

11 years ago
(In reply to comment #16)
> doesn't seem like something we'd block on, but you can ask for patch approval
> if you get the reviews.

You mean review, singular.  But SpiderMonkey is under single review, the sr? was me being scrupulous.  So don't disquality on that account.  Eitehr this is a blocker or it isn't.  If you minus it again saying it isn't, I won't cry.

/be
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
(Assignee)

Comment 18

11 years ago
(In reply to comment #17)
> Either this is
> a blocker or it isn't.  If you minus it again saying it isn't, I won't cry.

I was trying to make a point here about blockers, but probably that point was not going to come through clearly with this bug as its example.  So dveditz and I talked today, and I confirmed that what this bug represents is a corner case that was not implemented.  It should be fixed for the SpiderMonkey JS1.6 release, and for some 1.8x release, but it's not a blocker in my view.  It was a ridealong candidate in the good sense.

Since we don't have enough severity data to make strong blocker/non-blocker decisions that stick, and that don't come back to haunt us, it behooves us to be open to more prophylactic fixes.  And since we want to boost quality, we take some hygienic fixes.  But this bug isn't in either category.  It is a js1.6-wanted fix, so we'll have to merge it into a mini-branch off of the 1.8 branch.

Still waiting on shaver, who said he'd get to this on Thursday, maybe.  Another reason not to try for 1.8.0.1.

Marking fixed, since the fix is in the trunk with mrbkap's r+.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
Resolution: --- → FIXED

Comment 19

11 years ago
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-321874.js,v  <--  regress-321874.js

FAILED!: lhs must be a reference in (for lhs in rhs) ...: function foo(){for(a() in b);}
FAILED!: Expected value 'error', Actual value 'no error'
Status: RESOLVED → REOPENED
Flags: testcase+
Resolution: FIXED → ---
(Assignee)

Comment 20

11 years ago
The test is invalid.  The error is a run-time error, so you have to call function foo to get it.

/be
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 21

11 years ago
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-321874.js,v  <--  regress-321874.js
new revision: 1.2; previous revision: 1.1
Status: RESOLVED → VERIFIED
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2-
Flags: blocking1.8.0.2+
When you get unbusy enough to land this on the 1.8 and 1.8.0 branches ask for approval on the patch, we'll catch it there. It may be too late for 1.8.0.2 since this isn't really a blocker.
(Assignee)

Comment 23

11 years ago
Comment on attachment 207246 [details] [diff] [review]
fix, v4

This is well-baked too, a gap in standards compliance that can't be worked around.  This bug has the crash keyword, but that may refer only to assertion botching without the patch -- if not, then any real crashes need to be understood before we prioritize this one.

/be
Attachment #207246 - Flags: approval1.8.0.2?
Attachment #207246 - Flags: approval-branch-1.8.1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2-
Flags: blocking1.8.0.2+
Comment on attachment 207246 [details] [diff] [review]
fix, v4

approved for 1.8.0 branch, a=dveditz
Attachment #207246 - Flags: approval1.8.0.2? → approval1.8.0.2+
(Assignee)

Comment 25

11 years ago
Fixed on the 1.8* branches.

/be
Flags: blocking1.8.0.2+ → blocking1.8.0.2-
Keywords: fixed1.8.0.2, fixed1.8.1
Comment on attachment 207246 [details] [diff] [review]
fix, v4

sr=shaver
Attachment #207246 - Flags: superreview?(shaver) → superreview+
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates) since in-testsuite+ indicates a test case exists in the js test library.
Whiteboard: [rft-dl]

Comment 28

11 years ago
v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac
Keywords: fixed1.8.0.2, fixed1.8.1 → verified1.8.0.2, verified1.8.1
Flags: blocking1.8.1?
You need to log in before you can comment on or make changes to this bug.