Bug 553778: Don't orphan placeholder definition nodes when a real definition is found. r=brendan
authorJim Blandy <jimb@mozilla.com>
Wed, 10 Nov 2010 13:18:15 -0800
changeset 57762 8c59a5bf187ba33ddf01f756e1acf5a438fd6434
parent 57761 6ecdb8a8a4955a999a530bf50b1f996a7b372bea
child 57763 bfe06159bb2f4bd75866dd60fbb0db1bc5b18caa
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersbrendan
bugs553778
milestone2.0b8pre
Bug 553778: Don't orphan placeholder definition nodes when a real definition is found. r=brendan When we incorporate an inner function's lexdeps into our own lexdeps and decls tables, always create a fresh definition node for an identifier we don't have an entry for yet, and turn the inner definition node into a use of that definition, to ensure that references to those definitions from TOK_UPVARS nodes properly resolve to the outer definitions that capture them. This patch also changes MakePlaceholder to initialize the new node's type and op. Normally, JSParseNode::create initializes them from the current token, but that creates a fragile dependency of placeholder construction on lexing state, and is not actually what two out of (now) three call sites want.
js/src/jsparse.cpp
js/src/tests/js1_8_5/regress/jstests.list
js/src/tests/js1_8_5/regress/regress-553778.js
--- a/js/src/jsparse.cpp
+++ b/js/src/jsparse.cpp
@@ -1423,16 +1423,18 @@ MakePlaceholder(JSParseNode *pn, JSTreeC
     if (!ale)
         return NULL;
 
     JSDefinition *dn = (JSDefinition *)NameNode::create(pn->pn_atom, tc);
     if (!dn)
         return NULL;
 
     ALE_SET_DEFN(ale, dn);
+    dn->pn_type = TOK_NAME;
+    dn->pn_op = JSOP_NOP;
     dn->pn_defn = true;
     dn->pn_dflags |= PND_PLACEHOLDER;
     return ale;
 }
 
 static bool
 Define(JSParseNode *pn, JSAtom *atom, JSTreeContext *tc, bool let = false)
 {
@@ -2046,16 +2048,17 @@ Parser::markFunArgs(JSFunctionBox *funbo
                          */
                         afunbox = funbox;
                         uintN calleeLevel = lexdep->pn_cookie.level();
                         uintN staticLevel = afunbox->level + 1U;
                         while (staticLevel != calleeLevel) {
                             afunbox = afunbox->parent;
                             --staticLevel;
                         }
+                        JS_ASSERT(afunbox->level + 1U == calleeLevel);
                         afunbox->node->setFunArg();
                     } else {
                        afunbox = lexdep->pn_funbox;
                     }
                     queue.push(afunbox);
 
                     /*
                      * Walk over nested functions again, now that we have
@@ -2604,68 +2607,80 @@ LeaveFunction(JSParseNode *fn, JSTreeCon
              * by eval or with, to safely statically bind globals (see bug 561923).
              */
             if (funtc->callsEval() ||
                 (outer_ale && tc->innermostWith &&
                  ALE_DEFN(outer_ale)->pn_pos < tc->innermostWith->pn_pos)) {
                 DeoptimizeUsesWithin(dn, fn->pn_pos);
             }
 
-            JSDefinition *outer_dn;
-
             if (!outer_ale)
                 outer_ale = tc->lexdeps.lookup(atom);
-            if (outer_ale) {
-                /*
-                 * Insert dn's uses list at the front of outer_dn's list.
+            if (!outer_ale) {
+                /* 
+                 * Create a new placeholder for our outer lexdep. We could simply re-use
+                 * the inner placeholder, but that introduces subtleties in the case where
+                 * we find a later definition that captures an existing lexdep. For
+                 * example:
+                 *
+                 *   function f() { function g() { x; } let x; }
                  *
-                 * Without loss of generality or correctness, we allow a dn to
-                 * be in inner and outer lexdeps, since the purpose of lexdeps
-                 * is one-pass coordination of name use and definition across
-                 * functions, and if different dn's are used we'll merge lists
-                 * when leaving the inner function.
+                 * Here, g's TOK_UPVARS node lists the placeholder for x, which must be
+                 * captured by the 'let' declaration later, since 'let's are hoisted.
+                 * Taking g's placeholder as our own would work fine. But consider:
                  *
-                 * The dn == outer_dn case arises with generator expressions
-                 * (see CompExprTransplanter::transplant, the PN_FUNC/PN_NAME
-                 * case), and nowhere else, currently.
+                 *   function f() { x; { function g() { x; } let x; } }
+                 *
+                 * Here, the 'let' must not capture all the uses of f's lexdep entry for
+                 * x, but it must capture the x node referred to from g's TOK_UPVARS node.
+                 * Always turning inherited lexdeps into uses of a new outer definition
+                 * allows us to handle both these cases in a natural way.
                  */
-                outer_dn = ALE_DEFN(outer_ale);
-
-                if (dn != outer_dn) {
-                    JSParseNode **pnup = &dn->dn_uses;
-                    JSParseNode *pnu;
-
-                    while ((pnu = *pnup) != NULL) {
-                        pnu->pn_lexdef = outer_dn;
-                        pnup = &pnu->pn_link;
-                    }
-
-                    /*
-                     * Make dn be a use that redirects to outer_dn, because we
-                     * can't replace dn with outer_dn in all the pn_namesets in
-                     * the AST where it may be. Instead we make it forward to
-                     * outer_dn. See JSDefinition::resolve.
-                     */
-                    *pnup = outer_dn->dn_uses;
-                    outer_dn->dn_uses = dn;
-                    outer_dn->pn_dflags |= dn->pn_dflags & ~PND_PLACEHOLDER;
-                    dn->pn_defn = false;
-                    dn->pn_used = true;
-                    dn->pn_lexdef = outer_dn;
-
-                    /* Mark the outer dn as escaping. */
+                outer_ale = MakePlaceholder(dn, tc);
+            }
+
+            JSDefinition *outer_dn = ALE_DEFN(outer_ale);
+
+            /*
+             * Insert dn's uses list at the front of outer_dn's list.
+             *
+             * Without loss of generality or correctness, we allow a dn to
+             * be in inner and outer lexdeps, since the purpose of lexdeps
+             * is one-pass coordination of name use and definition across
+             * functions, and if different dn's are used we'll merge lists
+             * when leaving the inner function.
+             *
+             * The dn == outer_dn case arises with generator expressions
+             * (see CompExprTransplanter::transplant, the PN_FUNC/PN_NAME
+             * case), and nowhere else, currently.
+             */
+            if (dn != outer_dn) {
+                JSParseNode **pnup = &dn->dn_uses;
+                JSParseNode *pnu;
+
+                while ((pnu = *pnup) != NULL) {
+                    pnu->pn_lexdef = outer_dn;
+                    pnup = &pnu->pn_link;
                 }
-            } else {
-                /* Add an outer lexical dependency for ale's definition. */
-                outer_ale = tc->lexdeps.add(tc->parser, atom);
-                if (!outer_ale)
-                    return false;
-                outer_dn = ALE_DEFN(ale);
-                ALE_SET_DEFN(outer_ale, outer_dn);
-            }
+
+                /*
+                 * Make dn be a use that redirects to outer_dn, because we
+                 * can't replace dn with outer_dn in all the pn_namesets in
+                 * the AST where it may be. Instead we make it forward to
+                 * outer_dn. See JSDefinition::resolve.
+                 */
+                *pnup = outer_dn->dn_uses;
+                outer_dn->dn_uses = dn;
+                outer_dn->pn_dflags |= dn->pn_dflags & ~PND_PLACEHOLDER;
+                dn->pn_defn = false;
+                dn->pn_used = true;
+                dn->pn_lexdef = outer_dn;
+            }
+
+            /* Mark the outer dn as escaping. */
             outer_dn->pn_dflags |= PND_CLOSED;
         }
 
         if (funtc->lexdeps.count - foundCallee != 0) {
             JSParseNode *body = fn->pn_body;
 
             fn->pn_body = NameSetNode::create(tc);
             if (!fn->pn_body)
@@ -4772,20 +4787,16 @@ RebindLets(JSParseNode *pn, JSTreeContex
                     }
                 }
 
                 ale = tc->lexdeps.lookup(pn->pn_atom);
                 if (!ale) {
                     ale = MakePlaceholder(pn, tc);
                     if (!ale)
                         return NULL;
-
-                    JSDefinition *dn = ALE_DEFN(ale);
-                    dn->pn_type = TOK_NAME;
-                    dn->pn_op = JSOP_NOP;
                 }
                 LinkUseToDef(pn, ALE_DEFN(ale), tc);
             }
         }
         break;
 
       case PN_NAMESET:
         RebindLets(pn->pn_tree, tc);
@@ -8627,18 +8638,16 @@ Parser::primaryExpr(TokenKind tt, JSBool
                      * is not a left parenthesis.
                      *
                      * If the definition eventually parsed into dn is not a
                      * function, this flag won't hurt, and if we do parse a
                      * function with pn's name, then the PND_FUNARG flag is
                      * necessary for safe context->display-based optimiza-
                      * tion of the closure's static link.
                      */
-                    JS_ASSERT(PN_TYPE(dn) == TOK_NAME);
-                    JS_ASSERT(dn->pn_op == JSOP_NOP);
                     if (tokenStream.peekToken() != TOK_LP)
                         dn->pn_dflags |= PND_FUNARG;
                 }
             }
 
             JS_ASSERT(dn->pn_defn);
             LinkUseToDef(pn, dn, tc);
 
--- a/js/src/tests/js1_8_5/regress/jstests.list
+++ b/js/src/tests/js1_8_5/regress/jstests.list
@@ -6,16 +6,17 @@ script regress-541255-1.js
 script regress-541255-2.js
 script regress-541255-3.js
 script regress-541255-4.js
 script regress-541455.js
 script regress-546615.js
 script regress-551763-0.js
 script regress-551763-1.js
 script regress-551763-2.js
+script regress-553778.js
 script regress-555246-0.js
 script regress-555246-1.js
 script regress-559438.js
 script regress-560101.js
 script regress-560998-1.js
 script regress-560998-2.js
 script regress-563210.js
 script regress-563221.js
new file mode 100644
--- /dev/null
+++ b/js/src/tests/js1_8_5/regress/regress-553778.js
@@ -0,0 +1,16 @@
+/*
+ * This should compile without triggering the following assertion:
+ * 
+ * Assertion failure: cg->fun->u.i.skipmin <= skip, at ../jsemit.cpp:2346
+ */
+
+function f() {
+    function g() {
+        function h() {
+            g; x;
+        }
+        var [x] = [];
+    }
+}
+
+reportCompare(true, true);