351070 - Scope of let declaration changes during decompilation

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[nanto_vi (TOYAMA Nao)]

js> function f() { var a = 2; if (!!true) let a = 3; return a; }
js> f()
3
js> f
function f() {
    var a = 2;
    if (!!true) {
        let a = 3;
    }
    return a;
}
js> eval(uneval(f))
js> f()
2

I don't know where the scope of a let declaration in a statement without curly brackets should be.

Comment 1

[Brendan Eich [:brendan]]

Decompiler always braces, for simplicity -- obviously that changes the scope of let (let declarations, e.g. let a = 3, bind in the nearest enclosing block, or the function body if no such block).

Contrast with bug 349634, where unnecessary-before-let-was-introduced braces are elided by the decompiler.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix, plus make !JS_HAS_BLOCK_SCOPE compile cleanly

This turned out decently, after some noodling.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: js shell generated source testcase

This could be beefed up to test, for all the table entries, that braces do make the let a = 3 block-local and therefore change the expected return value.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix, v2

I fussed a bit, and eliminated a vacuous if (DONT_BRACE => braceOffset >= 0).  I'm pretty happy with this now.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, v3

Couldn't resist fussing once more: why unroll the memmove with an explicit store of the '\n' over the space before the brace, when it can be included in the move for free?

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fixed js shell test

D'oh, the test was broken.  This is better, and gives visual results (should be testable against reference decompilations).

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix, v4

Fixed to cope with JS_DONT_PRETTY_PRINT option, which suppresses newlines.  Thus the memmove must handle moving the newline down two chars in the pretty case.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix, v5

Final fix, I missed early output from the test:

function f1() {
    var n = 2, a = 2;
    if (!!true)
        let a = 3;
else {
        foopy();
    }
    return a;
}

Easily handled by js_printf'ing the "else" with the "\t} ", leaving the " {\n" to be js_printf'd separately with SET_MAYBE_BRACE(jp).

Dangling else works, as ever:

js> version(170)
0
js> function f(x){if (x) if (y) z; else let w }
js> f
function f(x) {
    if (x) {
        if (y) {
            z;
        } else
            let w;
    }
}
js> function f(x){if (x){ if (y) z;} else let w  }
js> f
function f(x) {
    if (x) {
        if (y) {
            z;
        }
    } else
        let w;
}
js> function f(x){if (x){ if (y) let z;} else let w  }
js> f
function f(x) {
    if (x) {
        if (y)
            let z;
    } else
        let w;
}

/be

Comment 9

[Brendan Eich [:brendan]]

This is important given let declarations for block scope in JS1.7.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, v6

Oops, I knew I forgot something that I thought of (like pink, best thoughts come in the shower, but no dive whiteboard or rubber memo-duck yet), then forgot:

js> version(170)
0
js> function f(){var a = 2; if (x) {let a = 3; print(a)} return a}
js> f
function f() {
    var a = 2;
    if (x)
        let a = 3;
        print(a);
    return a;
}

With this patch, it's all good:

js> function f(){var a = 2; if (x) {let a = 3; print(a)} return a}
js> f
function f() {
    var a = 2;
    if (x) {
        let a = 3;
        print(a);
    }
    return a;
}
js> function f(){var a = 2; if (x) {print(a);let a = 3} return a}
js> f
function f() {
    var a = 2;
    if (x) {
        print(a);
        let a = 3;
    }
    return a;
}
js> function f(){var a = 2; if (x) {let a = 3} return a}
js> f
function f() {
    var a = 2;
    if (x) {
        let a = 3;
    }
    return a;
}
js> function f(){var a = 2; if (x) let a = 3; return a}
js> f
function f() {
    var a = 2;
    if (x)
        let a = 3;
    return a;
}

Interdiff to the see the one crucial line.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 237570 [details] [diff] [review]
fix, v6

>+#if JS_HAS_BLOCK_SCOPE
>+                        if (jp->braceState == MAYBE_BRACE &&
>+                            pc + 1 == endpc &&
>+                            !strncmp(rval, var_prefix[SRC_DECL_LET], 4) &&
>+                            rval[4] != '(') {
>+                            SetDontBrace(jp);
>+                        }
>+#endif

Comment on what this is doing and r=mrbkap.

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix, v6 with comment and updated to track trunk

This merged up to tip, to get the patch for bug 352026 that was just committed.

/be

Comment 13

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 14

[Mike Schroepfer]

Comment on attachment 237788 [details] [diff] [review]
fix, v6 with comment and updated to track trunk

a=schrep for drivers.

Comment 15

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

Checking in regress-351070-01.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-351070-01.js,v  <--  regress-351070-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/block/regress-351070-02.js,v
done
Checking in regress-351070-02.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-351070-02.js,v  <--  regress-351070-02.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/block/regress-351070-03.js,v
done
Checking in regress-351070-03.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-351070-03.js,v  <--  regress-351070-03.js
initial revision: 1.1
done

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux