Bug 410649: function statement and destructuring parameter name clash now favours the function. r,a=brendan

js/src/jsemit.c

@@ -3928,6 +3928,14 @@ GettableNoteForNextOp(JSCodeGenerator *cg)
 }
 #endif
 
+/* Top-level named functions need a nop for decompilation. */
+static JSBool
+EmitFunctionDefNop(JSContext *cx, JSCodeGenerator *cg, uintN index)
+{
+    return js_NewSrcNote2(cx, cg, SRC_FUNCDEF, (ptrdiff_t)index) >= 0 &&
+           js_Emit1(cx, cg, JSOP_NOP) >= 0;
+}
+
 JSBool
 js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
 {
@@ -3962,9 +3970,9 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
     switch (pn->pn_type) {
       case TOK_FUNCTION:
       {
+        JSFunction *fun;
         void *cg2mark;
         JSCodeGenerator *cg2;
-        JSFunction *fun;
         uintN slot;
 
 #if JS_HAS_XML_SUPPORT
@@ -3975,6 +3983,21 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
         }
 #endif
 
+        fun = GET_FUNCTION_PRIVATE(cx, pn->pn_funpob->object);
+        if (fun->u.i.script) {
+            /*
+             * This second pass is needed to emit JSOP_NOP with a source note
+             * for the already-emitted function. See comments in the TOK_LC
+             * case.
+             */
+            JS_ASSERT(pn->pn_op == JSOP_NOP);
+            JS_ASSERT(cg->treeContext.flags & TCF_IN_FUNCTION);
+            JS_ASSERT(pn->pn_index != (uint32) -1);
+            if (!EmitFunctionDefNop(cx, cg, pn->pn_index))
+                return JS_FALSE;
+            break;
+        }
+
         /* Generate code for the function's body. */
         cg2mark = JS_ARENA_MARK(&cx->tempPool);
         JS_ARENA_ALLOCATE_TYPE(cg2, JSCodeGenerator, &cx->tempPool);
@@ -3987,7 +4010,6 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
                              pn->pn_pos.begin.lineno);
         cg2->treeContext.flags = (uint16) (pn->pn_flags | TCF_IN_FUNCTION);
         cg2->treeContext.maxScopeDepth = pn->pn_sclen;
-        fun = GET_FUNCTION_PRIVATE(cx, pn->pn_funpob->object);
         cg2->treeContext.fun = fun;
         cg2->parent = cg;
         if (!js_EmitFunctionScript(cx, cg2, pn->pn_body)) {
@@ -4023,52 +4045,41 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
             break;
         }
 
-        /* Top-level named functions need a nop for decompilation. */
-        noteIndex = js_NewSrcNote2(cx, cg, SRC_FUNCDEF, (ptrdiff_t)index);
-        if (noteIndex < 0 ||
-            js_Emit1(cx, cg, JSOP_NOP) < 0) {
-            return JS_FALSE;
-        }
-
         /*
-         * Top-levels also need a prolog op to predefine their names in the
+         * For a script we emit the code as we parse. Thus the bytecode for
-         * variable object, or if local, to fill their stack slots.
+         * top-level functions should go in the prolog to predefine their
+         * names in the variable object before the already-generated main code
+         * is executed. This extra work for top-level scripts is not necessary
+         * when we emit the code for a function. It is fully parsed prior to
+         * invocation of the emitter and calls to js_EmitTree for function
+         * definitions can be scheduled before generating the rest of code.
          */
-        CG_SWITCH_TO_PROLOG(cg);
-
         if (!(cg->treeContext.flags & TCF_IN_FUNCTION)) {
+            CG_SWITCH_TO_PROLOG(cg);
+
             /*
-             * Emit JSOP_CLOSURE for eval code to do less checks when
+             * Emit JSOP_CLOSURE for eval code to do fewer checks when
              * instantiating top-level functions in the non-eval case.
              */
             JS_ASSERT(!cg->treeContext.topStmt);
             op = (cx->fp->flags & JSFRAME_EVAL) ? JSOP_CLOSURE : JSOP_DEFFUN;
             EMIT_INDEX_OP(op, index);
+            CG_SWITCH_TO_MAIN(cg);
+
+            /* Emit NOP for the decompiler. */
+            if (!EmitFunctionDefNop(cx, cg, index))
+                return JS_FALSE;
         } else {
 #ifdef DEBUG
             JSLocalKind localKind =
 #endif
                 js_LookupLocal(cx, cg->treeContext.fun, fun->atom, &slot);
             JS_ASSERT(localKind == JSLOCAL_VAR || localKind == JSLOCAL_CONST);
-
+            JS_ASSERT(pn->pn_index == (uint32) -1);
-            /*
+            pn->pn_index = index;
-             * If this local function is declared in a body block induced
-             * by let declarations, reparent fun->object to the compiler-
-             * created body block object, so that JSOP_DEFLOCALFUN clones
-             * that block into the runtime scope chain.
-             */
-            stmt = cg->treeContext.topStmt;
-            if (stmt && stmt->type == STMT_BLOCK &&
-                stmt->down && stmt->down->type == STMT_BLOCK &&
-                (stmt->down->flags & SIF_SCOPE)) {
-                JS_ASSERT(STOBJ_GET_CLASS(stmt->down->u.blockObj) ==
-                          &js_BlockClass);
-                OBJ_SET_PARENT(cx, fun->object, stmt->down->u.blockObj);
-            }
             if (!EmitSlotIndexOp(cx, JSOP_DEFLOCALFUN, slot, index, cg))
                 return JS_FALSE;
         }
-        CG_SWITCH_TO_MAIN(cg);
         break;
       }
 
@@ -5158,6 +5169,27 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
         }
 
         js_PushStatement(&cg->treeContext, &stmtInfo, STMT_BLOCK, top);
+        if (pn->pn_extra & PNX_FUNCDEFS) {
+            /*
+             * This block contains top-level function definitions. To ensure
+             * that we emit the bytecode defining them prior the rest of code
+             * in the block we use a separate pass over functions. During the
+             * main pass later the emitter will add JSOP_NOP with source notes
+             * for the function to preserve the original functions position
+             * when decompiling.
+             *
+             * Currently this is used only for functions, as compile-as-we go
+             * mode for scripts does not allow separate emitter passes.
+             */
+            JS_ASSERT(cg->treeContext.flags & TCF_IN_FUNCTION);
+            for (pn2 = pn->pn_head; pn2; pn2 = pn2->pn_next) {
+                if (pn2->pn_type == TOK_FUNCTION) {
+                    JS_ASSERT(pn2->pn_op == JSOP_NOP);
+                    if (!js_EmitTree(cx, cg, pn2))
+                        return JS_FALSE;
+                }
+            }
+        }
         for (pn2 = pn->pn_head; pn2; pn2 = pn2->pn_next) {
             if (!js_EmitTree(cx, cg, pn2))
                 return JS_FALSE;

js/src/jsinterp.c

@@ -4717,51 +4717,17 @@ interrupt:
              */
             slot = GET_VARNO(pc);
 
-            JS_ASSERT(!fp->blockChain);
+            parent = js_GetScopeChain(cx, fp);
-            if (!(fp->flags & JSFRAME_POP_BLOCKS)) {
+            if (!parent) {
-                /*
+                ok = JS_FALSE;
-                 * If the compiler-created function object (obj) is scoped by a
+                goto out;
-                 * let-induced body block, temporarily update fp->blockChain so
-                 * that js_GetScopeChain will clone the block into the runtime
-                 * scope needed to parent the function object's clone.
-                 */
-                parent = OBJ_GET_PARENT(cx, obj);
-                if (parent && OBJ_GET_CLASS(cx, parent) == &js_BlockClass)
-                    fp->blockChain = parent;
-                parent = js_GetScopeChain(cx, fp);
-            } else {
-                /*
-                 * We have already emulated JSOP_ENTERBLOCK for the enclosing
-                 * body block, for a prior JSOP_DEFLOCALFUN in the prolog,  so
-                 * we just load fp->scopeChain into parent.
-                 *
-                 * In typical execution scenarios, the prolog bytecodes that
-                 * include this JSOP_DEFLOCALFUN run, then come main bytecodes
-                 * including JSOP_ENTERBLOCK for the outermost (body) block.
-                 * JSOP_ENTERBLOCK will detect that it need not do anything if
-                 * the body block was entered above due to a local function.
-                 * Finally the matching JSOP_LEAVEBLOCK runs.
-                 *
-                 * If the matching JSOP_LEAVEBLOCK for the body block does not
-                 * run for some reason, the body block will be properly "put"
-                 * (via js_PutBlockObject) by the PutBlockObjects call at the
-                 * bottom of js_Interpret.
-                 */
-                parent = fp->scopeChain;
-                JS_ASSERT(OBJ_GET_CLASS(cx, parent) == &js_BlockClass);
-                JS_ASSERT(OBJ_GET_PROTO(cx, parent) == OBJ_GET_PARENT(cx, obj));
-                JS_ASSERT(OBJ_GET_CLASS(cx, OBJ_GET_PARENT(cx, parent))
-                          == &js_CallClass);
             }
 
-            /* If re-parenting, store a clone of the function object. */
+            SAVE_SP_AND_PC(fp);
-            if (OBJ_GET_PARENT(cx, obj) != parent) {
+            obj = js_CloneFunctionObject(cx, obj, parent);
-                SAVE_SP_AND_PC(fp);
+            if (!obj) {
-                obj = js_CloneFunctionObject(cx, obj, parent);
+                ok = JS_FALSE;
-                if (!obj) {
+                goto out;
-                    ok = JS_FALSE;
-                    goto out;
-                }
             }
 
             fp->vars[slot] = OBJECT_TO_JSVAL(obj);
@@ -5547,23 +5513,12 @@ interrupt:
              */
             if (fp->flags & JSFRAME_POP_BLOCKS) {
                 JS_ASSERT(!fp->blockChain);
-
+                obj = js_CloneBlockObject(cx, obj, fp->scopeChain, fp);
-                /*
+                if (!obj) {
-                 * Check whether JSOP_DEFLOCALFUN emulated JSOP_ENTERBLOCK for
+                    ok = JS_FALSE;
-                 * the body block in order to correctly scope the local cloned
+                    goto out;
-                 * function object it creates.
-                 */
-                parent = fp->scopeChain;
-                if (OBJ_GET_PROTO(cx, parent) == obj) {
-                    JS_ASSERT(OBJ_GET_CLASS(cx, parent) == &js_BlockClass);
-                } else {
-                    obj = js_CloneBlockObject(cx, obj, parent, fp);
-                    if (!obj) {
-                        ok = JS_FALSE;
-                        goto out;
-                    }
-                    fp->scopeChain = obj;
                 }
+                fp->scopeChain = obj;
             } else {
                 JS_ASSERT(!fp->blockChain ||
                           OBJ_GET_PARENT(cx, obj) == fp->blockChain);

js/src/jsparse.c

@@ -84,6 +84,14 @@
 #include "jsdhash.h"
 #endif
 
+/*
+ * Asserts to verify assumptions behind pn_ macros.
+ */
+JS_STATIC_ASSERT(offsetof(JSParseNode, pn_u.name.atom) ==
+                 offsetof(JSParseNode, pn_u.apair.atom));
+JS_STATIC_ASSERT(offsetof(JSParseNode, pn_u.name.slot) ==
+                 offsetof(JSParseNode, pn_u.lexical.slot));
+
 /*
  * JS parsers, from lowest to highest precedence.
  *
@@ -1106,6 +1114,9 @@ FunctionDef(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc,
     pn = NewParseNode(cx, ts, PN_FUNC, tc);
     if (!pn)
         return NULL;
+#ifdef DEBUG
+    pn->pn_index = (uint32) -1;
+#endif
 
     /* Scan the optional function name into funAtom. */
     ts->flags |= TSF_KEYWORD_IS_NAME;
@@ -1442,6 +1453,7 @@ Statements(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
 
     CHECK_RECURSION();
 
+    JS_ASSERT(CURRENT_TOKEN(ts).type == TOK_LC);
     pn = NewParseNode(cx, ts, PN_LIST, tc);
     if (!pn)
         return NULL;
@@ -1453,8 +1465,11 @@ Statements(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
         ts->flags |= TSF_OPERAND;
         tt = js_PeekToken(cx, ts);
         ts->flags &= ~TSF_OPERAND;
-        if (tt <= TOK_EOF || tt == TOK_RC)
+        if (tt <= TOK_EOF || tt == TOK_RC) {
+            if (tt == TOK_ERROR)
+                return NULL;
             break;
+        }
         pn2 = Statement(cx, ts, tc);
         if (!pn2) {
             if (ts->flags & TSF_EOF)
@@ -1462,10 +1477,21 @@ Statements(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
             return NULL;
         }
 
-        /* Detect a function statement for the TOK_LC case in Statement. */
+        if (pn2->pn_type == TOK_FUNCTION) {
-        if (pn2->pn_type == TOK_FUNCTION && !AT_TOP_LEVEL(tc))
+            /*
-            tc->flags |= TCF_HAS_FUNCTION_STMT;
+             * PNX_FUNCDEFS notifies the emitter that the block contains top-
-
+             * level function definitions that should be processed before the
+             * rest of nodes.
+             *
+             * TCF_HAS_FUNCTION_STMT is for the TOK_LC case in Statement. It
+             * is relevant only for function definitions not at top-level,
+             * which we call function statements.
+             */
+            if (AT_TOP_LEVEL(tc))
+                pn->pn_extra |= PNX_FUNCDEFS;
+            else
+                tc->flags |= TCF_HAS_FUNCTION_STMT;
+        }
         PN_APPEND(pn, pn2);
     }
 
@@ -1478,10 +1504,6 @@ Statements(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
         pn = tc->blockNode;
     tc->blockNode = saveBlock;
 
-    ts->flags &= ~TSF_OPERAND;
-    if (tt == TOK_ERROR)
-        return NULL;
-
     pn->pn_pos.end = CURRENT_TOKEN(ts).pos.end;
     return pn;
 }

js/src/jsparse.h

@@ -282,6 +282,7 @@ struct JSParseNode {
             JSParseNode *body;          /* TOK_LC list of statements */
             uint16      flags;          /* accumulated tree context flags */
             uint16      sclen;          /* maximum scope chain length */
+            uint32      index;          /* emitter's index */
         } func;
         struct {                        /* list of next-linked nodes */
             JSParseNode *head;          /* first node in list */
@@ -331,6 +332,7 @@ struct JSParseNode {
 #define pn_body         pn_u.func.body
 #define pn_flags        pn_u.func.flags
 #define pn_sclen        pn_u.func.sclen
+#define pn_index        pn_u.func.index
 #define pn_head         pn_u.list.head
 #define pn_tail         pn_u.list.tail
 #define pn_count        pn_u.list.count
@@ -362,7 +364,8 @@ struct JSParseNode {
 #define PNX_XMLROOT     0x20            /* top-most node in XML literal tree */
 #define PNX_GROUPINIT   0x40            /* var [a, b] = [c, d]; unit list */
 #define PNX_NEEDBRACES  0x80            /* braces necessary due to closure */
-
+#define PNX_FUNCDEFS   0x100            /* contains top-level function
+                                           statements */
 /*
  * Move pn2 into pn, preserving pn->pn_pos and pn->pn_offset and handing off
  * any kids in pn2->pn_u, by clearing pn2.

js/src/jsxdrapi.h

@@ -202,7 +202,7 @@ JS_XDRFindClassById(JSXDRState *xdr, uint32 id);
  * before deserialization of bytecode.  If the saved version does not match
  * the current version, abort deserialization and invalidate the file.
  */
-#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 17)
+#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 18)
 
 /*
  * Library-private functions.