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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: brendan)

Tracking

(Blocks: 1 bug, {testcase, verified1.8.1})

Trunk
mozilla1.8.1
testcase, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

11 years ago
Related to bug 348904?
(Assignee)

Comment 2

11 years ago

*** This bug has been marked as a duplicate of 348904 ***
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
(Assignee)

Comment 3

11 years ago
Oops, not a dup, just similar.

/be
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 4

11 years ago
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
Assignee: general → brendan
Status: REOPENED → NEW
OS: Mac OS X 10.4 → All
Priority: -- → P2
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 5

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

Updated

11 years ago
Blocks: 336378
Status: NEW → ASSIGNED
Flags: blocking1.8.1?
(Assignee)

Comment 6

11 years ago
*** Bug 349624 has been marked as a duplicate of this bug. ***
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 7

11 years ago
See bug 349624 comment 1.

/be
(Assignee)

Comment 8

11 years ago
Created attachment 235017 [details] [diff] [review]
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
Attachment #235017 - Flags: review?(mrbkap)

Updated

11 years ago
Attachment #235017 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 9

11 years ago
Created attachment 235024 [details] [diff] [review]
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
Attachment #235017 - Attachment is obsolete: true
Attachment #235024 - Flags: review?(mrbkap)
(Assignee)

Comment 10

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

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

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

Comment 11

11 years ago
Fixed on trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #235027 - Flags: approval1.8.1?
Created attachment 235034 [details] [diff] [review]
Additional patch to fix the bustage

The old patch was clobbering function flags that were added during the js_EmitTree.
Attachment #235034 - Flags: review+
(Reporter)

Updated

11 years ago
Blocks: 349611
(Assignee)

Updated

11 years ago
Depends on: 349818
(Assignee)

Comment 13

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

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

a=schrep for drivers
Attachment #235027 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 15

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

/be
Keywords: fixed1.8.1
(Assignee)

Comment 16

11 years ago
Created attachment 235137 [details] [diff] [review]
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

11 years ago
Checking in regress-349605.js;
/cvsroot/mozilla/js/tests/js1_7/expressions/regress-349605.js,v  <--  regress-349605.js
initial revision: 1.1
Flags: in-testsuite+

Comment 18

11 years ago
verified fixed 1.8, 1.9 20060824 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 19

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

Comment 20

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

11 years ago
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
You need to log in before you can comment on or make changes to this bug.