Closed
Bug 349605
Opened 18 years ago
Closed 18 years ago
Let expressions in "for" initial expression become let statements above the "for"
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8.1
People
(Reporter: jruderman, Assigned: brendan)
References
Details
(Keywords: testcase, verified1.8.1)
Attachments
(3 files, 2 obsolete files)
25.27 KB,
patch
|
brendan
:
review+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
3.05 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
23.90 KB,
patch
|
Details | Diff | Splinter Review |
> 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•18 years ago
|
||
Related to bug 348904?
Assignee | ||
Comment 2•18 years ago
|
||
*** This bug has been marked as a duplicate of 348904 ***
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 3•18 years ago
|
||
Oops, not a dup, just similar. /be
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 4•18 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•18 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•18 years ago
|
Assignee | ||
Comment 6•18 years ago
|
||
*** Bug 349624 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Flags: blocking1.8.1? → blocking1.8.1+
Assignee | ||
Comment 7•18 years ago
|
||
See bug 349624 comment 1. /be
Assignee | ||
Comment 8•18 years ago
|
||
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•18 years ago
|
Attachment #235017 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 9•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
Fixed on trunk. /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #235027 -
Flags: approval1.8.1?
Comment 12•18 years ago
|
||
The old patch was clobbering function flags that were added during the js_EmitTree.
Attachment #235034 -
Flags: review+
Assignee | ||
Comment 13•18 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•18 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•18 years ago
|
||
Fixed on the 1.8 branch, hand-merged patch coming next. /be
Keywords: fixed1.8.1
Assignee | ||
Comment 16•18 years ago
|
||
Plus jsparse.c (DeclareLetVar) fixing that should have gone into the hand-merged branch patch for bug 349507. /be
Comment 17•18 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•18 years ago
|
||
verified fixed 1.8, 1.9 20060824 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Comment 19•18 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•18 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•18 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.
Description
•