344601 - Function.prototype.toString turns let statements/expressions into let declarations

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[nanto_vi (TOYAMA Nao)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060713 Minefield/3.0a1

ToString/toSource of a function containing let statements/expressions gives let declarations instead of the let statemetns/expressions.

Reproducible: Always

Steps to Reproduce:
js> function f() { var i = 1; let (i = 2) {} return i; }
js> f();
1

Actual Results:  
js> f.toString();
function f() {
    var i = 1;
    let i = 2;
    return i;
}
js> eval(f.toString());
js> f();
2

Expected Results:  
js> f.toString();
function f() {
    var i = 1;
    let (i = 2) {
    }
    return i;
}
js> eval(f.toString());
js> f();
1

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, the decompiler needs some love.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Hack hack hack

This patch works, but is not ready for review. I'll explain the problems later.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Less hacky

This even avoids the additional bytecode (thanks to brendan for pointing out how to avoid it). Sadly, it leaves |for (let i = i in foo);| in a sad shape, but that's a less severe bug.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 233694 [details] [diff] [review]
Better

>+    /*
>+     * We pop the scope around initializers for for-in loop variables and let
>+     * statements/expressions. Let statements/expressions have a different
>+     * sourcenote sequence, so test here to see which case we are really in.
>+     */
>+    inLetHead = (popScope && !(tc->flags & TCF_IN_FOR_INIT));

Nit: don't need parens around rhs of = operator.

>+    /* If this is a let head, emit and return a srcnote on the pop. */
>+    if (!inLetHead) {
>+        *headNoteIndex = -1;
>+    } else {
>+        *headNoteIndex = js_NewSrcNote(cx, cg, SRC_DECL);
>+        if (*headNoteIndex < 0)
>+            return JS_FALSE;
>+    }
>+
>     return !(pn->pn_extra & PNX_POPVAR) || js_Emit1(cx, cg, JSOP_POP) >= 0;

Correct me if I'm wrong, but if (inLetHead), we shouldn't even consider emitting the pop here.  If true, this is better written:

>+    /*
>+     * We pop the scope around initializers for for-in loop variables and let
>+     * statements/expressions. Let statements/expressions have a different
>+     * sourcenote sequence, so test here to see which case we are really in.
>+     */
>+    inLetHead = (popScope && !(tc->flags & TCF_IN_FOR_INIT));

Nit: don't need parens around rhs of = operator.

    /* If this is a let head, emit and return a srcnote on the pop. */
    if (inLetHead) {
        *headNoteIndex = js_NewSrcNote(cx, cg, SRC_DECL);
        return *headNoteIndex >= 0;
    }

    *headNoteIndex = -1;
    return !(pn->pn_extra & PNX_POPVAR) || js_Emit1(cx, cg, JSOP_POP) >= 0;

>         /* Let and for loop heads evaluate initializers in the outer scope. */
>-        inHead = (pn2 != NULL || (cg->treeContext.flags & TCF_IN_FOR_INIT));
>+        popScope = (pn2 != NULL || (cg->treeContext.flags & TCF_IN_FOR_INIT));
> 
>         JS_ASSERT(pn->pn_arity == PN_LIST);
>-        if (!EmitVariables(cx, cg, pn, inHead))
>+        if (!EmitVariables(cx, cg, pn, popScope, &noteIndex))
>             return JS_FALSE;
> 
>+        if (noteIndex >= 0)
>+            tmp = CG_OFFSET(cg);

I say set tmp unconditionally (did GCC finally get this case right, and not warn?).

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
>     /* If this is a let head, emit and return a srcnote on the pop. */
>     if (inLetHead) {
>         *headNoteIndex = js_NewSrcNote(cx, cg, SRC_DECL);
>         return *headNoteIndex >= 0;
>     }

D'oh -- I was confused into thinking that in this case, the JSOP_POP was emitted by the caller.  Blake's added an assertion that the PNX_POPVAR flag is set, though, since the SRC_DECL counts on that.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Best

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 233702 [details] [diff] [review]
Best

r=me, thanks.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 10

[Bob Clary [:bc] (inactive)]

nanto you rock!

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

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233702 [details] [diff] [review]
Best

a=beltzner on behalf of drivers for the mozilla_1_8_branch

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060815 windows/mac(ppc|tel)/linux

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 348904 on the for-in issue.

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060818 windows/mac*/linuc