Closed
Bug 349634
Opened 18 years ago
Closed 18 years ago
{} around blocks of statements are eliminated, even when needed for "let" or "function"
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8.1
People
(Reporter: jruderman, Assigned: brendan)
References
()
Details
(Keywords: testcase, verified1.8.1)
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
Here are two examples of {} elimination causing problems with "let".
> function () { let a = 3; { let a = 4; } }
function () { let a = 3; let a = 4; }
> function () { let a = 3; let a = 4; }
TypeError on line 1: redeclaration of variable a
> function () { try{}catch(v) { { let v = 3 } } }
function () { try { } catch (v) { let v = 3; } }
> function () { try { } catch (v) { let v = 3; } }
TypeError on line 1: redeclaration of variable v
Reporter | ||
Comment 1•18 years ago
|
||
See also bug 349802.
Assignee | ||
Comment 2•18 years ago
|
||
*** Bug 349802 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 3•18 years ago
|
||
This is kind of severe. It breaks more than let -- it also breaks functions in braces (which are function statements), decompiling them as function declarations, which if local to another function, can have any (useless) references to their names eliminated.
/be
Assignee: general → brendan
Blocks: js1.7
Flags: blocking1.8.1?
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1
Assignee | ||
Updated•18 years ago
|
Summary: {} around blocks of statements are eliminated, even when needed for "let" → {} around blocks of statements are eliminated, even when needed for "let" of "function"
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•18 years ago
|
Summary: {} around blocks of statements are eliminated, even when needed for "let" of "function" → {} around blocks of statements are eliminated, even when needed for "let" or "function"
Updated•18 years ago
|
Flags: blocking1.8.1? → blocking1.8.1+
Assignee | ||
Comment 4•18 years ago
|
||
*** Bug 350652 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 5•18 years ago
|
||
Contrast with bug 351070, where braces are added by the decompiler, changing the scope for "let". (Brendan pointed this out in the other bug.)
Assignee | ||
Comment 6•18 years ago
|
||
Relatively straightforward.
Look out for bugfixes: SRC_CATCH can't have a constant offsetBias, and anyway that magic number is a maintenance hazard! And the shell's source note printing wasn't right for SRC_CATCH applied to a JSOP_LEAVEBLOCK.
/be
Attachment #237198 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 237198 [details] [diff] [review]
fix
Work in progress, at this point. Consider:
js> function f(){ if (x){let y}else{let z}}
js> f
function f() {
if (x) {
{
let y;
}
} else {
{
let z;
}
}
}
/be
Attachment #237198 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•18 years ago
|
||
Another bug fix included: switch body must become a block scope if any let declarations occur at top level of case statements.
/be
Attachment #237198 -
Attachment is obsolete: true
Attachment #237247 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•18 years ago
|
||
This is tweaked so as not to brace the function body block scope wrongly, e.g. for
function (){if(x)let y; else{let z}print(y)}
This testcase still shows bug 351070, but that's a separate patch.
/be
Attachment #237247 -
Attachment is obsolete: true
Attachment #237262 -
Flags: review?(mrbkap)
Attachment #237247 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•18 years ago
|
||
Comment on attachment 237262 [details] [diff] [review]
fix, v3
Not quite right, here's a testcase showing the remaining bug:
js> function f() { { function f(){}; {} } }
js> f
function f() {
function f() {
}
}
TCF_HAS_CLOSURE needs to accumulate across calls to Statements from Statement.
/be
Attachment #237262 -
Attachment is obsolete: true
Attachment #237262 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #237274 -
Flags: review?(mrbkap)
Updated•18 years ago
|
Attachment #237274 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•18 years ago
|
||
Checking this with your re-r+.
/be
Attachment #237277 -
Flags: review?(mrbkap)
Attachment #237277 -
Flags: approval1.8.1?
Updated•18 years ago
|
Attachment #237277 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 13•18 years ago
|
||
Fixed on trunk.
/be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•18 years ago
|
||
Comment on attachment 237277 [details] [diff] [review]
fix, v4a
Another checkin (bug 350730) went in, so updating patch here for branch landing.
/be
Attachment #237277 -
Flags: approval1.8.1?
Assignee | ||
Comment 15•18 years ago
|
||
Attachment #237283 -
Flags: review+
Attachment #237283 -
Flags: approval1.8.1?
Assignee | ||
Comment 16•18 years ago
|
||
Comment on attachment 237283 [details] [diff] [review]
fix, v4b
There's a regression (bug 351794) to fix before proposing a 1.8 branch patch.
/be
Attachment #237283 -
Flags: approval1.8.1?
Comment 17•18 years ago
|
||
Checking in regress-349634.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-349634.js,v <-- regress-349634.js
initial revision: 1.1
Flags: in-testsuite+
Assignee | ||
Comment 18•18 years ago
|
||
I verified by cvs diff'ing against trunk revisions that this is the right combo of the patch for this bug, and the patch for bug 351794.
/be
Attachment #237525 -
Flags: review+
Attachment #237525 -
Flags: approval1.8.1?
Comment 20•18 years ago
|
||
Comment on attachment 237525 [details] [diff] [review]
1.8 branch patch for this bug and bug 351794
a=schrep
Attachment #237525 -
Flags: approval1.8.1? → approval1.8.1+
Comment 22•18 years ago
|
||
verified fixed 1.8 1.9 2006091022 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•