{} around blocks of statements are eliminated, even when needed for "let" or "function"

VERIFIED FIXED in mozilla1.8.1

Status

()

P1
normal
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: jruderman, 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

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
See also bug 349802.
(Assignee)

Comment 2

12 years ago
*** Bug 349802 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

12 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: 336373
Flags: blocking1.8.1?
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

12 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

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

12 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

12 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 4

12 years ago
*** Bug 350652 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 5

12 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

12 years ago
Created attachment 237198 [details] [diff] [review]
fix

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

12 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

12 years ago
Created attachment 237247 [details] [diff] [review]
fix, v2

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

12 years ago
Created attachment 237262 [details] [diff] [review]
fix, v3

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

12 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

12 years ago
Created attachment 237274 [details] [diff] [review]
fix, v4
Attachment #237274 - Flags: review?(mrbkap)
Attachment #237274 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 237277 [details] [diff] [review]
fix, v4a

Checking this with your re-r+.

/be
Attachment #237277 - Flags: review?(mrbkap)
Attachment #237277 - Flags: approval1.8.1?
Attachment #237277 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 13

12 years ago
Fixed on trunk.

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

Comment 14

12 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

12 years ago
Created attachment 237283 [details] [diff] [review]
fix, v4b
Attachment #237283 - Flags: review+
Attachment #237283 - Flags: approval1.8.1?
(Assignee)

Comment 16

12 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?
(Assignee)

Updated

12 years ago
Blocks: 351794

Comment 17

12 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

12 years ago
Created attachment 237525 [details] [diff] [review]
1.8 branch patch for this bug and bug 351794

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 19

12 years ago
verified fixed 1.9 20060909 windows/mac*/linux
Status: RESOLVED → VERIFIED

Comment 20

12 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+
(Assignee)

Comment 21

12 years ago
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1

Comment 22

12 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.