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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: nanto_vi (TOYAMA Nao), Assigned: mrbkap)

Tracking

({verified1.8.1})

Trunk
mozilla1.8.1
verified1.8.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

12 years ago
Yeah, the decompiler needs some love.
Assignee: general → mrbkap
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
Created attachment 233346 [details] [diff] [review]
Hack hack hack

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

Comment 3

12 years ago
Created attachment 233685 [details] [diff] [review]
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.
Attachment #233346 - Attachment is obsolete: true
Attachment #233685 - Flags: review?(brendan)
(Assignee)

Comment 4

12 years ago
Created attachment 233694 [details] [diff] [review]
Better
Attachment #233685 - Attachment is obsolete: true
Attachment #233694 - Flags: review?(brendan)
Attachment #233685 - Flags: review?(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
(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
(Assignee)

Comment 7

12 years ago
Created attachment 233702 [details] [diff] [review]
Best
Attachment #233694 - Attachment is obsolete: true
Attachment #233702 - Flags: review?(brendan)
Attachment #233694 - Flags: review?(brendan)
Comment on attachment 233702 [details] [diff] [review]
Best

r=me, thanks.

/be
Attachment #233702 - Flags: review?(brendan)
Attachment #233702 - Flags: review+
Attachment #233702 - Flags: approval1.8.1?
(Assignee)

Comment 9

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 336378

Comment 10

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

Flags: in-testsuite+
Comment on attachment 233702 [details] [diff] [review]
Best

a=beltzner on behalf of drivers for the mozilla_1_8_branch
Attachment #233702 - Flags: approval1.8.1? → approval1.8.1+

Comment 12

12 years ago
verified fixed 1.9 20060815 windows/mac(ppc|tel)/linux
Status: RESOLVED → VERIFIED
(Assignee)

Comment 13

12 years ago
Fixed on the 1.8 branch.
Keywords: fixed1.8.1
(Assignee)

Comment 14

12 years ago
I filed bug 348904 on the for-in issue.

Comment 15

12 years ago
verified fixed 1.8 20060818 windows/mac*/linuc
Keywords: fixed1.8.1 → verified1.8.1
You need to log in before you can comment on or make changes to this bug.