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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Yuh-Ruey Chen]

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?

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Brendan Eich [:brendan]]

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

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Brendan Eich [:brendan]]

(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

Comment 6

[Brendan Eich [:brendan]]

Attachment: 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

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Brendan Eich [:brendan]]

Attachment: 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

Comment 10

[Brendan Eich [:brendan]]

Attachment: 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

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Brendan Eich [:brendan]]

(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

Comment 13

[Brendan Eich [:brendan]]

Attachment: fix, v4

Bugzilla's interdiff is handy to verify things.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

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

I think I follow this.

Comment 15

[Brendan Eich [:brendan]]

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

/be

Comment 16

[Daniel Veditz [:dveditz]]

doesn't seem like something we'd block on, but you can ask for patch approval if you get the reviews.

Comment 17

[Brendan Eich [:brendan]]

(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

Comment 18

[Brendan Eich [:brendan]]

(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

Comment 19

[Bob Clary [:bc] (inactive)]

/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'

Comment 20

[Brendan Eich [:brendan]]

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

/be

Comment 21

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-321874.js,v  <--  regress-321874.js
new revision: 1.2; previous revision: 1.1

Comment 22

[Daniel Veditz [:dveditz]]

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.

Comment 23

[Brendan Eich [:brendan]]

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

Comment 24

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz

Comment 25

[Brendan Eich [:brendan]]

Fixed on the 1.8* branches.

/be

Comment 26

[Mike Shaver (:shaver emeritus)]

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

sr=shaver

Comment 27

[Dave Liebreich [:davel]]

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.

Comment 28

[Bob Clary [:bc] (inactive)]

v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac