349605 - Let expressions in "for" initial expression become let statements above the "for"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

> javascript:function() { alert(1); for((let(y=3) let(y=4) y); 0; x++) ; alert(6); }
function () { alert(1); let y = 3; let y = 4; for ((y); 0; x++) { } alert(6); }

This is incorrect because it expands the scope of each "y" by quite a bit: it used to be local to part of an expression, but now it covers the entire function.  The decompiled function doesn't even compile; it hits a "redeclaration of variable y" error.

Comment 1

[Jesse Ruderman]

Related to bug 348904?

Comment 2

[Brendan Eich [:brendan]]

*** This bug has been marked as a duplicate of 348904 ***

Comment 3

[Brendan Eich [:brendan]]

Oops, not a dup, just similar.

/be

Comment 4

[Brendan Eich [:brendan]]

It's the early enterblocks, d'oh.  Paging drbkap!

js> function f(){for(let x=(let (a=2)a*a);x<10;x++)print(x)}
js> f
function f() {
    let a = 2;
    for (let x = (a * a); x < 10; x) {
        print(print);
    }
}
js> dis(f)
main:
00000:  enterblock depth 0 {x: 0}
00003:  enterblock depth 1 {a: 0}
00006:  uint16 2
00009:  setlocal 1
00012:  pop
00013:  getlocal 1
00016:  getlocal 1
00019:  mul
00020:  leaveblockexpr 1
00023:  group
00024:  setlocal 0
00027:  pop
00028:  getlocal 0
00031:  uint16 10
00034:  lt
00035:  ifeq 56 (21)
00038:  name "print"
00041:  pushobj
00042:  getlocal 0
00045:  call 1
00048:  pop
00049:  localinc 0
00052:  pop
00053:  goto 28 (-25)
00056:  leaveblock 1
00059:  stop

Source notes:
  0:     9 [   9] xdelta  
  1:     9 [   0] decl    
  3:    24 [  15] xdelta  
  4:    24 [   0] decl    
  6:    27 [   3] for      cond 7 update 21 tail 25
 10:    45 [  18] xdelta  
 11:    45 [   0] pcbase   offset 7

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> It's the early enterblocks, d'oh.  Paging drbkap!

Or (I should have written) the js_printf'ing of output on first pop.

/be

Comment 6

[Brendan Eich [:brendan]]

*** Bug 349624 has been marked as a duplicate of this bug. ***

Comment 7

[Brendan Eich [:brendan]]

See bug 349624 comment 1.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix

Detailed comments:

- Remove #if JS_HAS_BLOCK_SCOPE bracketing from all internal block scope code
  now required for try/catch/finally, which is not deconfigurable.

- Fix EmitVariables to clear TCF_IN_FOR_INIT when recursively generating code
  for variable initializer expressions.

- Same fix for parenthesized expressions (the TOK_RP js_EmitTree case).

- Fix the decompiler to use BEGIN_LITOPX_CASE(JSOP_ENTERBLOCK)/END_LITOPX_CASE
  so that the JSOP_LITOPX-extended JSOP_ENTERBLOCK instruction works in scripts
  that have >= 64K literals before the block in question.  To match this, add
  the missing goto do_JSOP_ENTERBLOCK; statement in the switch for JSOP_LITOPX.

- Avoid unnecessary pn1 local in LetBlock.

- Use tc->blockChain where possible, in preference to longer (source&compiled)
  ATOM_TO_OBJECT(pnfoo->pn_atom) or ATOM_TO_OBJECT(tc->topScopeStmt->atom)

- Make the TOK_LET case in Statements, in jsparse.c, assert before breaking to
  the common auto-semicolon-insertion epilog.

- Clean up Variables a bit by ordering locals to match first-set order and to
  set and use tt early.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: fix, v2

Only change to last detailed comments affects this item:

- Same fix for parenthesized expressions (the TOK_RP js_EmitTree case, plus two
  E4X cases: @[...] and @ns::[...]).

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, v3

Without the fix to JSOP_TOATTRNAME's decompiler, which will await another bug.

/be

Comment 11

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Additional patch to fix the bustage

The old patch was clobbering function flags that were added during the js_EmitTree.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Created an attachment (id=235027) [edit]
> fix, v3
> 
> Without the fix to JSOP_TOATTRNAME's decompiler, which will await another bug.

Filed (by Jesse, his fuzzer beat me to it!) at bug 349822.

/be

Comment 14

[Mike Schroepfer]

Comment on attachment 235027 [details] [diff] [review]
fix, v3

a=schrep for drivers

Comment 15

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch, hand-merged patch coming next.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: hand-merged 1.8 branch version of the total fix

Plus jsparse.c (DeclareLetVar) fixing that should have gone into the hand-merged branch patch for bug 349507.

/be

Comment 17

[Bob Clary [:bc] (inactive)]

Checking in regress-349605.js;
/cvsroot/mozilla/js/tests/js1_7/expressions/regress-349605.js,v  <--  regress-349605.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.8, 1.9 20060824 windows/mac*/linux

Comment 19

[Bob Clary [:bc] (inactive)]

The behavior on the trunk changed 20060901

BUGNUMBER: 349605
STATUS: decompilation of let inside |for| statements
FAILED!: [reported from test()] decompilation of let inside |for| statements
FAILED!: [reported from test()] Expected value 'function () {
FAILED!: [reported from test()]     alert(1);
FAILED!: [reported from test()]     for ((let (y = 3) let (y = 4) y); 0; x++) {
FAILED!: [reported from test()]     }
FAILED!: [reported from test()]     alert(6);
FAILED!: [reported from test()] }', Actual value 'function () {
FAILED!: [reported from test()]     alert(1);
FAILED!: [reported from test()]     for (let (y = 3) let (y = 4) y; 0; x++) {
FAILED!: [reported from test()]     }
FAILED!: [reported from test()]     alert(6);
FAILED!: [reported from test()] }'
FAILED!: [reported from test()] 

probably due to Bug 348904. So the test case should be changed I suppose.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> The behavior on the trunk changed 20060901
> 
> BUGNUMBER: 349605
> STATUS: decompilation of let inside |for| statements
> FAILED!: [reported from test()] decompilation of let inside |for| statements
> FAILED!: [reported from test()] Expected value 'function () {
> FAILED!: [reported from test()]     alert(1);
> FAILED!: [reported from test()]     for ((let (y = 3) let (y = 4) y); 0; x++) {
> FAILED!: [reported from test()]     }
> FAILED!: [reported from test()]     alert(6);
> FAILED!: [reported from test()] }', Actual value 'function () {
> FAILED!: [reported from test()]     alert(1);
> FAILED!: [reported from test()]     for (let (y = 3) let (y = 4) y; 0; x++) {
> FAILED!: [reported from test()]     }
> FAILED!: [reported from test()]     alert(6);
> FAILED!: [reported from test()] }'
> FAILED!: [reported from test()] 
> 
> probably due to Bug 348904. So the test case should be changed I suppose.

See bug 350991, which needs to be settled by a more complete ECMA TG1 proposal for ES4/JS2 -- I'll update that bug as soon as I get an answer.  The issue is whether for(let(y=z)useful(y*y*y);cond;update)body is legal syntax.  If so, and I think it should be, then you should change to track the elimination of unnecessary parens, and bug 350991 will need to be fixed quickly.

/be

Comment 21

[Bob Clary [:bc] (inactive)]

Checking in regress-349605.js;
/cvsroot/mozilla/js/tests/js1_7/expressions/regress-349605.js,v  <--  regress-349605.js
new revision: 1.2; previous revision: 1.1
done