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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

See also bug 349802.

Comment 2

[Brendan Eich [:brendan]]

*** Bug 349802 has been marked as a duplicate of this bug. ***

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

*** Bug 350652 has been marked as a duplicate of this bug. ***

Comment 5

[Jesse Ruderman]

Contrast with bug 351070, where braces are added by the decompiler, changing the scope for "let".  (Brendan pointed this out in the other bug.)

Comment 6

[Brendan Eich [:brendan]]

Attachment: 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

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Brendan Eich [:brendan]]

Attachment: 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

Comment 9

[Brendan Eich [:brendan]]

Attachment: 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

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brendan Eich [:brendan]]

Attachment: fix, v4

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix, v4a

Checking this with your re-r+.

/be

Comment 13

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 237277 [details] [diff] [review]
fix, v4a

Another checkin (bug 350730) went in, so updating patch here for branch landing.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: fix, v4b

Comment 16

[Brendan Eich [:brendan]]

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

Comment 17

[Bob Clary [:bc] (inactive)]

Checking in regress-349634.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-349634.js,v  <--  regress-349634.js
initial revision: 1.1

Comment 18

[Brendan Eich [:brendan]]

Attachment: 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

Comment 19

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060909 windows/mac*/linux

Comment 20

[Mike Schroepfer]

Comment on attachment 237525 [details] [diff] [review]
1.8 branch patch for this bug and bug 351794

a=schrep

Comment 21

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 2006091022 windows/mac*/linux