788261 - Split PNK_FUNCTION into multiple parse node kinds

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

PNK_FUNCTIONNS for nullary nodes representing the E4X function:: namespace
PNK_FUNCTIONEXPR for function expressions
PNK_FUNCTIONDECL for function declarations

Maybe there should be more of them, but this is a decent start.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - PNK_FUNCTIONNS, v1

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - PNK_FUNCTION{EXPR,DECL}, v1

js> parse("function f(){}")
(STATEMENTLIST [(FUNCTIONDECL (ARGSBODY [(STATEMENTLIST [])]))])
js> parse("(function f(){})")
(STATEMENTLIST [(SEMI (FUNCTIONEXPR (ARGSBODY [(STATEMENTLIST [])])))])

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 658209 [details] [diff] [review]
Part 2 - PNK_FUNCTION{EXPR,DECL}, v1

Review of attachment 658209 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice to see ParseNode subclassed into StatementNode and ExpressionNode so that the AST wasn't one undistinguished mass of nodes.  (And every node kind would have appropriately typed fields/accessors, of course.)  The function-declaration, function-expression dichotomy would be much clearer and more obvious if we had that.  You can slip that change into this patch easily enough, right?

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6052,1 @@
>          ok = EmitFunc(cx, bce, pn);

Hm, I wonder how much effort EmitFunc does that's decl-specific versus expr-specific...

@@ +6077,5 @@
>              // 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.
>               

Remove this trailing whitespace while you're here?

@@ +6082,5 @@
>              // Currently this is used only for functions, as compile-as-we go
>              // mode for scripts does not allow separate emitter passes.
>              for (ParseNode *pn2 = pnchild; pn2; pn2 = pn2->pn_next) {
> +                JS_ASSERT(!pn2->isKind(PNK_FUNCTIONEXPR));
> +                JS_ASSERT(!pn2->isKind(PNK_FUNCTIONNS));

Either of these would be wrapped in a PNK_SEMI, right?

::: js/src/frontend/ParseNode.h
@@ +215,5 @@
>   * Label        Variant     Members
>   * -----        -------     -------
>   * <Definitions>
> + * PNK_FUNCTIONDECL,
> + * PNK_FUNCTIONEXPR

A function expression is a <Definitions>?

::: js/src/frontend/Parser.cpp
@@ +957,5 @@
>      pn->pn_dflags |= dn->pn_dflags & PND_USE2DEF_FLAGS;
>      pn->dn_uses = dn;
>  
>      /*
> +     * A PNK_FUNCTIONDECL node must be a definition, so convert shadowed

"must be"?  It seems to me it more *is* a definition, by definition (pun intended), tautologically.  Is this leading clause even necessary any more?  I think maybe it should just be removed.

@@ +1500,5 @@
>      JS_ASSERT_IF(kind == Statement, funName);
>  
>      /* Make a TOK_FUNCTION node. */
> +    ParseNode *pn;
> +    pn = FunctionNode::create(kind == Statement ? PNK_FUNCTIONDECL : PNK_FUNCTIONEXPR, this);

ParseNode *pn =
    FunctionNode::create(....);

@@ +2241,5 @@
>                pn->isOp(JSOP_FUNCALL) || pn->isOp(JSOP_FUNAPPLY));
>      if (!parser->reportStrictModeError(pn, msg))
>          return false;
>  
> +    if (pn->isGeneratorExpr()) {

I am going to semi-assume this is a reasonable change.  It looks reasonable -- doesn't seem to me that you should need to consult funboxen to determine generator-ness -- but I'm not familiar with this quite enough to confidently stamp it.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 658209 [details] [diff] [review]
Part 2 - PNK_FUNCTION{EXPR,DECL}, v1

Review of attachment 658209 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6052,1 @@
>          ok = EmitFunc(cx, bce, pn);

Not much, AFAICT.

::: js/src/frontend/Parser.cpp
@@ +2241,5 @@
>                pn->isOp(JSOP_FUNCALL) || pn->isOp(JSOP_FUNAPPLY));
>      if (!parser->reportStrictModeError(pn, msg))
>          return false;
>  
> +    if (pn->isGeneratorExpr()) {

I don't know either, but this plus the removal of the JSOP_CALL case in FoldConstants.cpp makes me wonder if we can avoid passing the |inGenexpLambda| parameter to FoldConstants().

Comment 5

[Nicholas Nethercote [inactive]]

(Oh, my comments are in response to Waldo's comments at the same points in the code.  They make more sense when you view the code via Splinter.)

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> It would be nice to see ParseNode subclassed into StatementNode and
> ExpressionNode so that the AST wasn't one undistinguished mass of nodes. 
> (And every node kind would have appropriately typed fields/accessors, of
> course.)  The function-declaration, function-expression dichotomy would be
> much clearer and more obvious if we had that.  You can slip that change into
> this patch easily enough, right?

Sure thing.

> >          ok = EmitFunc(cx, bce, pn);
> 
> Hm, I wonder how much effort EmitFunc does that's decl-specific versus
> expr-specific...

Not much, but at least there's the difference in how the function name is a scope in the expression (function x(){return x;}) compared to the declaration.

> Remove this trailing whitespace while you're here?

done

> @@ +6082,5 @@
> >              // Currently this is used only for functions, as compile-as-we go
> >              // mode for scripts does not allow separate emitter passes.
> >              for (ParseNode *pn2 = pnchild; pn2; pn2 = pn2->pn_next) {
> > +                JS_ASSERT(!pn2->isKind(PNK_FUNCTIONEXPR));
> > +                JS_ASSERT(!pn2->isKind(PNK_FUNCTIONNS));
> 
> Either of these would be wrapped in a PNK_SEMI, right?

Well, no, the purpose of those assertions was to check that any function node that gets here is properly labeled as a FUNCTIONDECL -- clearly FUNCTIONEXPR would be just plain wrong. If one of these failed, it would mean that this changeset did something wrong in Parser.cpp; a node is being given the wrong kind.

I really only intended to run the test with these assertions once, not leave them in. They are clearly kind of silly, especially the FUNCTIONNS one. Removed.

> >   * <Definitions>
> > + * PNK_FUNCTIONDECL,
> > + * PNK_FUNCTIONEXPR
> 
> A function expression is a <Definitions>?

Oh, good point. Moved it down to <Expressions>, like this:

+ * PNK_FUNCTIONEXPR function  The same fields as PNK_FUNCTIONDECL above.

> ::: js/src/frontend/Parser.cpp
> @@ +957,5 @@
> >      pn->pn_dflags |= dn->pn_dflags & PND_USE2DEF_FLAGS;
> >      pn->dn_uses = dn;
> >  
> >      /*
> > +     * A PNK_FUNCTIONDECL node must be a definition, so convert shadowed
> 
> "must be"?  It seems to me it more *is* a definition, by definition (pun
> intended), tautologically.  Is this leading clause even necessary any more? 
> I think maybe it should just be removed.

Instead, I changed it to:

     * A PNK_FUNCTIONDECL node is always a definition, and two definition nodes
     * in the same scope can't define the same name, so convert shadowed
     * function declarations into nops. [...]

I think it's worth explaining why we do this instead of just ignoring the duplicates and letting a later phase cope.

> ParseNode *pn =
>     FunctionNode::create(....);

Happily.

> @@ +2241,5 @@
> >                pn->isOp(JSOP_FUNCALL) || pn->isOp(JSOP_FUNAPPLY));
> >      if (!parser->reportStrictModeError(pn, msg))
> >          return false;
> >  
> > +    if (pn->isGeneratorExpr()) {
> 
> I am going to semi-assume this is a reasonable change.  It looks reasonable
> -- doesn't seem to me that you should need to consult funboxen to determine
> generator-ness -- but I'm not familiar with this quite enough to confidently
> stamp it.

Yeah. I looked and couldn't find anything, and we have to sweep out the cruft sometime.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb7d4d469355
https://hg.mozilla.org/mozilla-central/rev/04eb40b6fdd0

Comment 8

[Nicholas Nethercote [inactive]]

I backed out part 2 because it was blocking my work in bug 790485.  I figure it won't be much extra work for you to combine your fix with the original patch when the time comes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/94e50524191d

Comment 9

[Ed Morley [:emorley]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> I backed out part 2 because it was blocking my work in bug 790485.  I figure
> it won't be much extra work for you to combine your fix with the original
> patch when the time comes.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/94e50524191d

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/94e50524191d

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 11

[Jan de Mooij [:jandem]]

10 years later, we now have a FunctionNode containing a FunctionSyntaxKind enum class. I don't think this bug will go anywhere so let's close it.