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)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: jruderman, Assigned: brendan)

References

()

Details

(Keywords: testcase, verified1.8.1)

Attachments

(4 files, 3 obsolete files)

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
See also bug 349802.
*** Bug 349802 has been marked as a duplicate of this bug. ***
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
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"
Status: NEW → ASSIGNED
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"
Flags: blocking1.8.1? → blocking1.8.1+
*** Bug 350652 has been marked as a duplicate of this bug. ***
Contrast with bug 351070, where braces are added by the decompiler, changing the scope for "let". (Brendan pointed this out in the other bug.)
Attached patch fix (obsolete) (deleted) — Splinter Review
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)
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)
Attached patch fix, v2 (obsolete) (deleted) — Splinter Review
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)
Attached patch fix, v3 (obsolete) (deleted) — Splinter Review
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)
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)
Attached patch fix, v4 (deleted) — Splinter Review
Attachment #237274 - Flags: review?(mrbkap)
Attachment #237274 - Flags: review?(mrbkap) → review+
Attached patch fix, v4a (deleted) — Splinter Review
Checking this with your re-r+. /be
Attachment #237277 - Flags: review?(mrbkap)
Attachment #237277 - Flags: approval1.8.1?
Attachment #237277 - Flags: review?(mrbkap) → review+
Fixed on trunk. /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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?
Attached patch fix, v4b (deleted) — Splinter Review
Attachment #237283 - Flags: review+
Attachment #237283 - Flags: approval1.8.1?
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?
Blocks: 351794
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+
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?
verified fixed 1.9 20060909 windows/mac*/linux
Status: RESOLVED → VERIFIED
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+
Fixed on the 1.8 branch. /be
Keywords: fixed1.8.1
verified fixed 1.8 1.9 2006091022 windows/mac*/linux
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: