1130811 - Remove ParseNode arity and imply it by ParseNodeKind

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Arity-generic ParseNode handling is quite error-prone in the face of the many parse node structures.  Implying structure based upon parse node kind, and having a particular kind always have a particular structure, is much clearer and simpler to reason about.

For the most part, kind implies arity already.  But there are at least a few places where that's not true.  We'll have to split up those kinds first to fix this bug.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove the unused PNK_ELSE

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Actually use PNK_CATCHLIST so that PNK_CATCH isn't used for multiple arities

I seem to recall introducing PNK_CATCHLIST.  I'm not sure why it was thought necessary to stop using it.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Remove PNK_FINALLY as unused

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Split out of PNK_LET (used for let declarations, deprecated let blocks, and deprecated let expressions) additional PNK_LETBLOCK and PNK_LETEXPR kinds

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Examine nodes kind-wise when deciding whether a node contains a hoisted declaration

The first of the arity-wise inspection removals, and the only one completely finished so far.

I'm not certain what the best way to review this is, given it must examine the structure/uses of every single kind to be truly diligent about things.  I suppose perhaps the best way is to examine the sole use case for this function, then reason from there through every kind.  That seems like it should largely work without too much fuss -- save in those cases where the parse tree doesn't follow the obvious/predictable design, mostly because of block scoping inducing PNK_LEXICALSCOPE deviations into the tree, and so on.  It's fortunate this operation is only looking for variable declarations, and those being statements, this code need never recur *into* an expression node.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Always use list nodes (albeit in some circumstances with only two elements), and never binary nodes, to represent various binary operations

Interpreting nodes by kind and not arity is impossible when a kind can have multiple arities.  This is true of a slew of left-associative binary operations that can be either binary or list.  I'm unsure why, given the recursion concern, they weren't just made lists *all* the time.  A list of two occupies just as much space as a binary node.  Conceivably (I haven't checked) a binary or list node might require one more or fewer field to write to initialize, but we're really grasping at straws by this point.

So just make these binary-or-list nodes *always* lists.  asm.js's desire for "binary" nodes is a confounding factor that required the introduction of a gazillion more helper functions; I elected to have that case keep creating two-element nodes, just make them two-element *lists* instead.  Didn't seem to affect the complexity of the asm.js algorithms.

Making these only lists *did* affect complexity a bunch in other places in the engine -- mostly by removing special-case handling that duplicated the effect of a two-element list, if it were possible to create such a list.  The EmitTree code is the funkiest bit of this.  Sadly, removing dead code there and reindenting the existing list code had the effect of creating a rather mangled diff.  I'll post a -w diff after this, which might help.

Note that newBinaryOrAppend was used for the left-associative ops and for assignment *only*.  And in the assignment case, we were always appending different kinds of node to each other.  So that really should have just been, and is post-patch, newBinary.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Whitespace-ignoring version of attachment 8560967

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Remove the unused foldConstants bool from both ParseHandler classes

A cleanup building atop the previous patch, split out for simplicity.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8560952 [details] [diff] [review]
Actually use PNK_CATCHLIST so that PNK_CATCH isn't used for multiple arities

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

::: js/src/frontend/ParseNode.h
@@ +286,4 @@
>   *                          pn_kid3:  update expr after second ';' or nullptr
>   * PNK_THROW    unary       pn_op: JSOP_THROW, pn_kid: exception
>   * PNK_TRY      ternary     pn_kid1: try block
> + *                          pn_kid2: null or PNK_CATCHLIST list

Did you accidentally delete the pn_kid3 comment for PNK_TRY?

::: js/src/frontend/Parser.cpp
@@ +5633,4 @@
>      if (!tokenStream.getToken(&tt))
>          return null();
>      if (tt == TOK_CATCH) {
> +        catchList = handler.newCatchList();

Why add a new method? handler.newList(PNK_CATCHLIST) is perfectly readable to me.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8560954 [details] [diff] [review]
Split out of PNK_LET (used for let declarations, deprecated let blocks, and deprecated let expressions) additional PNK_LETBLOCK and PNK_LETEXPR kinds

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ -7205,3 @@
>        case PNK_LET:
> -      case PNK_CONST:
> -        MOZ_ASSERT_IF(pn->isKind(PNK_CONST), !pn->isArity(PN_BINARY));

Nice, getting rid of this assert.

::: js/src/frontend/Parser.cpp
@@ +3679,5 @@
>  
>              /*
>               * If this is really an expression in let statement guise, then we
> +             * need to wrap the PNK_LETEXPR node in a PNK_SEMI node so that we
> +             * pop the return value of the expression.

So gross.

@@ +3710,4 @@
>      handler.setLexicalScopeBody(block, expr);
>      PopStatementPC(tokenStream, pc);
>  
> +    TokenPos letPos(begin, pos().end);

Oh that's a nice change.

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8560956 [details] [diff] [review]
Examine nodes kind-wise when deciding whether a node contains a hoisted declaration

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

It's hard for me to make a confident statement about completeness of the new ContainsHoistedDeclaration. It looks reasonable otherwise though.

r=me with the "why does this case need to recur" question below answered.

::: js/src/frontend/FoldConstants.cpp
@@ +80,5 @@
> +        *result = false;
> +        return true;
> +
> +      // ContainsHoistedDeclaration is only called on nested nodes, so any
> +      // instance of this must be a function sub-statement extension.  We don't

I don't understand "function sub-statement extension". You mean function statements not at body-level?

@@ +119,5 @@
> +        *result = false;
> +        return true;
> +
> +      // Statements possibly containing hoistable declarations in the left half
> +      // only.

Here, left half means the body of the do { ... }? A comment saying so would be nice.

@@ +124,5 @@
> +      case PNK_DOWHILE:
> +        return ContainsHoistedDeclaration(cx, node->pn_left, result);
> +
> +      // Statements possibly containing hoistable declarations in the right
> +      // half only.

And here, right half here meaning the body of the loop, right?

@@ +268,5 @@
> +            // for (target of ...), where only target may introduce hoisted
> +            // declarations.
> +            //
> +            // Either way, any declaration present in target is either the
> +            // first kid, *or* it's been hoisted into an ancestral PNK_SEQ.

Typo: kid

@@ +274,5 @@
> +
> +            ParseNode *decl = loopHead->pn_kid1;
> +            if (decl && decl->isKind(PNK_VAR)) {
> +                if (!ContainsHoistedDeclaration(cx, decl, result))
> +                    return false;

Why does this case need to recur instead of just setting *result = true?

@@ +302,5 @@
> +
> +        if (expr->isKind(PNK_STATEMENTLIST))
> +            return ListContainsHoistedDeclaration(cx, &node->pn_expr->as<ListNode>(), result);
> +
> +        MOZ_CRASH("unexpected expr kind");

Nit: Maybe MOZ_ASSERT(expr->isKind(PNK_STATEMENTLIST)); instead of checking both PNK_FOR and PNK_STATEMENTLIST?

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8560967 [details] [diff] [review]
Always use list nodes (albeit in some circumstances with only two elements), and never binary nodes, to represent various binary operations

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

::: js/src/frontend/FoldConstants.cpp
@@ -888,5 @@
> -      case PNK_RSHASSIGN:
> -      case PNK_URSHASSIGN:
> -      case PNK_MULASSIGN:
> -      case PNK_DIVASSIGN:
> -      case PNK_MODASSIGN:

Where did all these go?  Did you mean to add them as cases where do_binary_op used to be?

::: js/src/frontend/ParseNode.cpp
@@ +256,5 @@
> +        // The asm.js specification is written in ECMAScript grammar terms that
> +        // specify *only* a binary tree.  While it's possible to implement the
> +        // asm.js spec to act upon n-ary lists as created below, it's a royal
> +        // pain.  So for asm.js, form a binary tree of lists exactly as
> +        // ECMAScript would.

Seems nice to have the comment right before the "useAsmOrInsideUseAsm" test instead of after.  It looks like you could also add !pc->useAsmOrInsideUseAsm() to the conjunct list below which would reduce nesting and fix the comment at the expense of a multi-line condition.

Comment 13

[Jeff Walden [:Waldo]]

More to come, don't close me bro.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f37bded46028
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa5eafb19ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/079473f453ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f28e0ce04e93
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a850bc4ea8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b560b5afe0c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b00f60dbd69
https://hg.mozilla.org/integration/mozilla-inbound/rev/97e200fe37d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fed536bd689

(In reply to Shu-yu Guo [:shu] from comment #9)
> Did you accidentally delete the pn_kid3 comment for PNK_TRY?

Yes, nice catch.  http://rimshot.vorb.is/

> Why add a new method? handler.newList(PNK_CATCHLIST) is perfectly readable
> to me.

A named method is somewhat more greppable, in my experience, than a newList use with a particular kind constant.  Searching for a kind will turn up all sorts of hits; searching for the method that creates a list of such will hit far fewer things.

(In reply to Shu-yu Guo [:shu] from comment #11)
> I don't understand "function sub-statement extension". You mean function
> statements not at body-level?

Yes.  I changed the wording a bit harder to hopefully be clearer about this.  Also I'm not 100% sure, on ES6 double-check, about function statements being allowed in blocks, so I punted a little on that language.

> > +      // Statements possibly containing hoistable declarations in the left half
> > +      // only.
> 
> Here, left half means the body of the do { ... }? A comment saying so would
> be nice.

Added comments clarifying left/right as in the ParseNode fields, not in source-order AST terms.

> > +      // Statements possibly containing hoistable declarations in the right
> > +      // half only.
> 
> And here, right half here meaning the body of the loop, right?

Yeah.  Did same again.

> > +            // for (target of ...), where only target may introduce hoisted
> > +            // declarations.
> > +            //
> > +            // Either way, any declaration present in target is either the
> > +            // first kid, *or* it's been hoisted into an ancestral PNK_SEQ.
> 
> Typo: kid

That's not a typo, it's referring to the first kid of the forin/forof.  But I tweaked the text some to maybe more reduce the possibility of misreading.

> > +            ParseNode *decl = loopHead->pn_kid1;
> > +            if (decl && decl->isKind(PNK_VAR)) {
> > +                if (!ContainsHoistedDeclaration(cx, decl, result))
> > +                    return false;
> 
> Why does this case need to recur instead of just setting *result = true?

Because dumb.  Fixt.

> > +        if (expr->isKind(PNK_STATEMENTLIST))
> > +            return ListContainsHoistedDeclaration(cx, &node->pn_expr->as<ListNode>(), result);
> > +
> > +        MOZ_CRASH("unexpected expr kind");
> 
> Nit: Maybe MOZ_ASSERT(expr->isKind(PNK_STATEMENTLIST)); instead of checking
> both PNK_FOR and PNK_STATEMENTLIST?

Good call.  Did that one or two other places as well when landing.

(In reply to Luke Wagner [:luke] from comment #12)

> > -      case PNK_RSHASSIGN:
> > -      case PNK_URSHASSIGN:
> > -      case PNK_MULASSIGN:
> > -      case PNK_DIVASSIGN:
> > -      case PNK_MODASSIGN:
> 
> Where did all these go?  Did you mean to add them as cases where
> do_binary_op used to be?

So actually these were just stupid, and so was PNK_ADDASSIGN (also removed).  The prior ordering had these all flow to the same case as PNK_ADD, for list concatenation.  But.  Addition at least was binary-or-list, legitimately.  These assignment nodes were *always* binary.  And the binary optimization only kicked in if left and right were number nodes that could be folded.  The left half of a compound assignment ain't gonna be a number node.  So these were all just really stupidly casing to do nothing.  I removed the lot of them.

> @@ +256,5 @@
> > +        // The asm.js specification is written in ECMAScript grammar terms that
> > +        // specify *only* a binary tree.  While it's possible to implement the
> > +        // asm.js spec to act upon n-ary lists as created below, it's a royal
> > +        // pain.  So for asm.js, form a binary tree of lists exactly as
> > +        // ECMAScript would.
> 
> Seems nice to have the comment right before the "useAsmOrInsideUseAsm" test
> instead of after.

I now have comments on both sides, actually, after more tweaking.

> It looks like you could also add !pc->useAsmOrInsideUseAsm() to the
> conjunct list below which would reduce nesting and fix the comment at
> the expense of a multi-line condition.

It wouldn't just be a multiline condition, tho.  I think this would fit into the scenario jorendorff's been noting recently where grouping two dissimilar conditions together results in less-readable code.  The canonical example of that being something like

    if (foldConstants) {
        if (!FoldConstants(context, &pn, this))
            return null();
    }

where writing it this way

    if (foldConstants && !FoldConstants(context, &pn, this))
        return null();

would suggest one coherent condition -- not a condition guarding an operation logically distinct.  In the case we have here, the in-asmjs-code condition is logically distinct from the overall comparison made to determine whether to do a list-append.  So they should be separate, with the one if nested in the other.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f37bded46028
https://hg.mozilla.org/mozilla-central/rev/2fa5eafb19ee
https://hg.mozilla.org/mozilla-central/rev/079473f453ed
https://hg.mozilla.org/mozilla-central/rev/f28e0ce04e93
https://hg.mozilla.org/mozilla-central/rev/5a850bc4ea8f
https://hg.mozilla.org/mozilla-central/rev/b560b5afe0c4
https://hg.mozilla.org/mozilla-central/rev/3b00f60dbd69
https://hg.mozilla.org/mozilla-central/rev/97e200fe37d4

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Make addArrayElement infallible, 'cause it is

Spreading the review love around some, not because anyone has particular expertise in the areas targeted by most of these patches.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Eliminate an else-after-return when parsing ES7 array comprehensions

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Mark PNK_NOP nodes as not being definitions

Node cleanup right now, for PN_NULLARY nodes, cleans up such nodes "later" if the node is a definition.  The bit here is a leftover from the original PNK_FUNCTION, has no meaning for PNK_NOP, and is best removed to not forbid recycling this node.  A later patch in this bug will take advantage of this to more narrowly specify what PNK_NOP cleanup does.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Refactor node recycling code into arity-specific methods

We can share arity-specific handling here, but let's do it in methods that kind-wise cleanup calls.  Not by sharing literally the same code, making it hard to change handling for just one specific kind.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Start adding kind-specific handling for node recycling, eschewing arity-specific handling

Admittedly, I could just have all the nullary nodes call the nullary-handling method.  But when for a particular kind I *know* something about those nodes not implied by the kind, we should be taking advantage of that.  The code savings of further unifying nullary node handling that is almost the same is just not worth it, particularly over modifications that implicitly document node structure.  I can't tell you how many times, in working on other modifications to remove arity-checks, I've found this patch or others I've worked on useful in putting in writing what I knew about the structure of one kind or another.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Handle pushing unary nodes with a non-null kid, kindwise, when recycling

Again, it's useful to distinguish nodes with known structures (such as a single non-null child) for documentation purposes.  Might help perf slightly in rare cases, too, although that's far secondary.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Handle pushing binary-operation list nodes by kind when recycling

Belatedly realized I'd put in that default: case in the first patch adding all the kinds -- shouldn't have added it at all, but previously added, at least I can remove it quickly.

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Handle pushing assignment and compound assignment nodes by kind when recycling

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Handle various miscellaneous nodes by kind when recycling

Pretty sure I've dealt with the left != right nonsense by the end of these patches.  In the meantime, lol along the way as enlightenment dawns on me.

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Handle more miscellaneous nodes by kind when recycling

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Handle yield/yield* by kind when recycling

Read and weep.

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Handle a few more kinds, by kind, when recycling

Whoa, I understand for-loop AST structure!

Comment 27

[Jeff Walden [:Waldo]]

Attachment: When |obj[prop]| is folded into a new |obj.prop| node, convert the old |obj[prop]| node into a |prop;| ExpressionStatement, rather than mutating it into a PNK_ELEM whose halves are both quite unexpectedly null

This change lets us assert more about nodes that we recycle, wrt their structure and which fields are null.  (We don't recycle partially-constructed nodes -- such happen only in case of error, and our parser fails hard on the first error to happen.)  The sly reuse of PNK_SEMI here is a bit cute, but it gets the job done, better than leaving around a node with unexpectedly null halves.

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Handle a few more kinds, by kind, when recycling

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Handle a few more kinds, by kind, when recycling

Comment 30

[Jeff Walden [:Waldo]]

Attachment: When parsing an array that later turns out to be a legacy array comprehension, explicitly discard the array literal, rather than needlessly setting its kind to PNK_ARRAYCOMP and then implicitly dropping it on the floor

Comment 31

[Jeff Walden [:Waldo]]

Attachment: Handle PNK_ARRAYCOMP nodes by kind when recycling

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Handle import/export and lexicalscope nodes by kind, when recycling

The assertions here should hopefully make clear what structure is expected.  I'm not sure how 100% important this is right now given that import/export are turned off...somewhere...but this should be right according to the AST we generate for it, even if it's a syntax error somewhere else in the pipeline.

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Handle argsbody and seq nodes by kind, when recycling

PNK_SEQ at least has some narrowness to its use, that I might be able to assert something about here.  I think I started documenting it in some other patch I've landed, or am working on locally.  As I elaborate its uses, I'll come back and update this with comments and assertions along those lines.

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Convert the last ParseNode recycling arity-check to deal *only* with PNK_NAME, to give the right impression about when fallback happens

I convert PNK_NAME into multiple kinds/arities in later patches.

Comment 35

[Jeff Walden [:Waldo]]

Attachment: Remove the bizarre left-equals-right special case when recycling binary nodes

At this point atop all the other patches, the only place the special-case applies is for return nodes.  And return nodes definitely never have left-equals-right, unless both are null.  So the special case goes away.

I'm still relatively convinced this is detritus from PNK_SHORTHAND having the exact same node as both left and right halves, that was changed at some point because the RHS wasn't being noted as an assignment during destructuring.  But fortunately, at this point, I don't need to learn for sure!

Comment 36

[Jeff Walden [:Waldo]]

Attachment: Refactor some object-literal key:property and shorthand parsing code slightly

Use different handler methods to create normal property pairs and shorthands.  Now, any name token in an object literal that names a property of that object literal, is created by newObjectLiteralPropertyName.  That includes properties that are normal properties, shorthands, methods, generator properties -- everything.  These are all currently PNK_NAME with PN_NULLARY arity.  The next patch converts all of them -- without touching Parser.cpp at all! -- to a new kind.

Naming help with newObjectLiteralPropertyName appreciated, btw.  It's a fugly name, but it gets the job done.  Also keep in mind that harmonizing with the name of the kind (PNK_OBJECT_PROPERTY_NAME, next patch) would be nice.  Not sure I care too much, it's clear what it is, terseness isn't that important for something not used horrendously many places.

Comment 37

[Jeff Walden [:Waldo]]

Attachment: Split PNK_OBJECT_PROPERTY_NAME out of PNK_NAME, so that PNK_NAME is *always* a name reference and the other is *always* the identifier used to specify a property name in an object literal

And now we recycle nodes *totally* based on their kind, not arity.  Given I handle *every* kind in this switch, I think this may be the point where every kind has exactly one arity.  Which should make some of the other arity-conversion work go more smoothly.

Comment 38

[Shu-yu Guo [:shu]]

Comment on attachment 8563036 [details] [diff] [review]
Mark PNK_NOP nodes as not being definitions

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

This seems okay.

Comment 39

[Shu-yu Guo [:shu]]

Comment on attachment 8563037 [details] [diff] [review]
Refactor node recycling code into arity-specific methods

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

::: js/src/frontend/ParseNode.cpp
@@ +193,5 @@
> +PushBinaryNodeChildren(ParseNode *node, NodeStack *stack)
> +{
> +    MOZ_ASSERT(node->isArity(PN_BINARY) || node->isArity(PN_BINARY_OBJ));
> +
> +    // This is *probably* PNK_SHORTHAND, but that's not yet clear.

Nit: well that's not a very useful comment. Can the children be identical in the ternary case?

@@ +210,5 @@
> +
> +    if (node->isUsed())
> +        return PushResult::CleanUpLater;
> +    if (node->isDefn())
> +        return PushResult::CleanUpLater;

Nit: maybe node->isUsed() || node->isDefn()

@@ +253,2 @@
>        default:
> +        MOZ_CRASH("huh?");

Nit: make crash message less cryptic

Comment 40

[Shu-yu Guo [:shu]]

Comment on attachment 8563045 [details] [diff] [review]
Start adding kind-specific handling for node recycling, eschewing arity-specific handling

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

okay

Comment 41

[Shu-yu Guo [:shu]]

Comment on attachment 8563047 [details] [diff] [review]
Handle pushing unary nodes with a non-null kid, kindwise, when recycling

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

r=me with comment addressed

::: js/src/frontend/ParseNode.cpp
@@ +199,3 @@
>  
>      return PushResult::Recyclable;
>  }

I don't really see the need for this separate function. Could you assert pn->pn_kid in the switch below instead?

Comment 42

[Shu-yu Guo [:shu]]

Comment on attachment 8563060 [details] [diff] [review]
Handle pushing binary-operation list nodes by kind when recycling

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

::: js/src/frontend/ParseNode.cpp
@@ -336,5 @@
>  
> -        /*
> -         * Binary operators.
> -         * These must be in the same order as TOK_OR and friends in TokenStream.h.
> -         */

Should we maintain the order comment? Why must these be the same order? Micro-optimizing for tableswitch or something?

Comment 43

[Shu-yu Guo [:shu]]

Comment on attachment 8563061 [details] [diff] [review]
Handle pushing assignment and compound assignment nodes by kind when recycling

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

::: js/src/frontend/ParseNode.cpp
@@ +208,5 @@
> +    stack->push(node->pn_left);
> +    stack->push(node->pn_right);
> +
> +    return PushResult::Recyclable;
> +}

Again, I don't really feel like we need a separate function. An assert does fine for readability.

Comment 44

[Shu-yu Guo [:shu]]

Comment on attachment 8563070 [details] [diff] [review]
Handle more miscellaneous nodes by kind when recycling

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

::: js/src/frontend/ParseNode.cpp
@@ +349,5 @@
> +        stack->push(pn->pn_kid1);
> +        stack->push(pn->pn_kid2);
> +        if (pn->pn_kid3)
> +            stack->push(pn->pn_kid3);
> +        return PushResult::Recyclable;

Why no PushTernaryNodeChildren? Because these cases all have different nullability requirements? I'd rather have a general function and assert the non-nullness of the children that must be there. The others we will leave alone, like an absent father might.

Comment 45

[Shu-yu Guo [:shu]]

Comment on attachment 8563075 [details] [diff] [review]
Handle yield/yield* by kind when recycling

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

::: js/src/frontend/ParseNode.cpp
@@ +337,5 @@
> +      // internal goop: a name reference to the invisible '.generator' local
> +      // variable, or an assignment of a PNK_GENERATOR node to the '.generator'
> +      // local, for a synthesized, prepended initial yield.  Yum!
> +      case PNK_YIELD_STAR:
> +      case PNK_YIELD: {

Nit: unnecessary bracing

Comment 46

[Shu-yu Guo [:shu]]

Comment on attachment 8563077 [details] [diff] [review]
Handle a few more kinds, by kind, when recycling

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

::: js/src/frontend/ParseNode.cpp
@@ +365,5 @@
>  
> +      // For for-in and for-of, the first child is any declaration present in
> +      // the for-loop (and null if not).  The second child is the expression or
> +      // pattern assigned every loop, and the third child is the expression
> +      // looped over.

It's not clear to me what "the expression looped over" is referring to. I think an example |for (e1 in e2)| and saying what maps to what would be much clearer.

@@ +367,5 @@
> +      // the for-loop (and null if not).  The second child is the expression or
> +      // pattern assigned every loop, and the third child is the expression
> +      // looped over.
> +      case PNK_FORIN:
> +      case PNK_FOROF: {

Nit: unnecessary bracing

Comment 47

[Shu-yu Guo [:shu]]

Comment on attachment 8563078 [details] [diff] [review]
When |obj[prop]| is folded into a new |obj.prop| node, convert the old |obj[prop]| node into a |prop;| ExpressionStatement, rather than mutating it into a PNK_ELEM whose halves are both quite unexpectedly null

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

r=me even though this patch is wrong without the freeTree call, as it's a trivial fix that I trust Waldo to make.

::: js/src/frontend/FoldConstants.cpp
@@ +1126,5 @@
> +            // can free the |"prop"| node and |obj["prop"]| nodes -- but not
> +            // the |obj| node now a sub-node of |expr|.  Mutate |pn| into a
> +            // node with |"prop"| as its child so that our |pn| doesn't have a
> +            // necessarily-weird structure (say, by nulling out |pn->pn_left|
> +            // only).

I'd add an additional note about freeTree here doing basic sanity checks on AST node structure, thus this weird mutation.

@@ +1130,5 @@
> +            // only).
> +            pn->setKind(PNK_SEMI);
> +            pn->setArity(PN_UNARY);
> +            pn->pn_expr = pn2;
> +

Forgot the freeTree call.

Comment 48

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4cb45a4588
https://hg.mozilla.org/integration/mozilla-inbound/rev/008a003c15c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8353fc755046
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9bc8928f54
https://hg.mozilla.org/integration/mozilla-inbound/rev/823e4436a2ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb48b124f67b
https://hg.mozilla.org/integration/mozilla-inbound/rev/af3054673b35
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c56cf464f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd630ae8bbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d93b1ffb2ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/28cbfff0dd9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/03eda399201b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a411bde0705

And a try-run of that for good measure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e36e35320a0

(In reply to Shu-yu Guo [:shu] from comment #39)
> > +    // This is *probably* PNK_SHORTHAND, but that's not yet clear.
> 
> Nit: well that's not a very useful comment. Can the children be identical in
> the ternary case?

By the end of all the patches here, this has gone away, so I'm not fussing with this.  I'm pretty sure it's a use of PNK_SHORTHAND from the time when the two halves there were the same node, at this point, but no way am I bothering to investigate with it clearly not being possible by the end.  :-)

> Nit: maybe node->isUsed() || node->isDefn()

Changed, but this disappears by the end of the patch sequence anyway, so meh.  (Rather, there's only one caller of this method, and so I'm inlining it into that call in a later patch, with your implicit r=shu because that's basically what you ask for in one or another of these comments.  :-) )

> > +        MOZ_CRASH("huh?");
> 
> Nit: make crash message less cryptic

This also disappears by the end of the patch stack, so again not much point in modifying it.

(In reply to Shu-yu Guo [:shu] from comment #41)
> I don't really see the need for this separate function. Could you assert
> pn->pn_kid in the switch below instead?

It's usefully self-documenting as to structure, including children's optionality.  I do the same thing a few other places in this patch-stack, and I expect to do the same thing in other patch-stacks dealing with other places that examine arities.  Continuing into dealing with other arity-switch code, I can't tell you how useful it's been to be able to consult very-precise specifications of ParseNode structure like this to address other places.

Going forward, the idea of sharing code or cases, when the cases in question aren't siblings of each other in the grammar -- for example, all the non-base-case expansions of UnaryExpression -- simply isn't going to be sound.  PNK_SEMI is a statement; PNK_PLUS is an expression.  The former has to store additional info beyond simply its child (namely, pn_prologue).  It's total happenstance that any structure is shared.  Thus, it makes complete sense for these to be different.  I plan to leave this as-is.

(In reply to Shu-yu Guo [:shu] from comment #42)
> > -        /*
> > -         * Binary operators.
> > -         * These must be in the same order as TOK_OR and friends in TokenStream.h.
> > -         */
> 
> Should we maintain the order comment? Why must these be the same order?
> Micro-optimizing for tableswitch or something?

No reason at all for this.  This was just a comment copied over from the original source material, namely the higher-order macro to list all the ParseNodeKinds.  It has relevance there because of operations that require ordering of the kinds, but it has none at all here.  A good compiler will convert this to a table-based switch as SpiderMonkey itself does, and I'm 99.999% sure the order of the cases won't matter if they're dense enough (and they are).

(In reply to Shu-yu Guo [:shu] from comment #43)
> @@ +208,5 @@
> > +    stack->push(node->pn_left);
> > +    stack->push(node->pn_right);
> > +
> > +    return PushResult::Recyclable;
> > +}
> 
> Again, I don't really feel like we need a separate function.

By the end of patches rewriting recycling, this function has one caller, so I have a patch queued to inline it at that one location.

(In reply to Shu-yu Guo [:shu] from comment #44)
> Why no PushTernaryNodeChildren? Because these cases all have different
> nullability requirements? I'd rather have a general function and assert the
> non-nullness of the children that must be there. The others we will leave
> alone, like an absent father might.

The different nullability details for the various ternary nodes are precisely what make it so valuable to handle them with distinct code.  Simply having different prefatory assertions doesn't make it as clear as having explicit ifs and tests per child.

(In reply to Shu-yu Guo [:shu] from comment #45)
> Nit: unnecessary bracing

I can't remember if I've usually been bracing in these or not.  I think I may have, to most clearly delineate the code that executes for a series of cases.  I can do a pass at the end to adjust bracing, if it matters that bracing here only occur when it's truly necessary.

(In reply to Shu-yu Guo [:shu] from comment #46)
> It's not clear to me what "the expression looped over" is referring to. I
> think an example |for (e1 in e2)| and saying what maps to what would be much
> clearer.

Adjusted wording some, added an illustrative example.

> Nit: unnecessary bracing

Same comment as before.

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff4cb45a4588
https://hg.mozilla.org/mozilla-central/rev/008a003c15c9
https://hg.mozilla.org/mozilla-central/rev/8353fc755046
https://hg.mozilla.org/mozilla-central/rev/0a9bc8928f54
https://hg.mozilla.org/mozilla-central/rev/823e4436a2ff
https://hg.mozilla.org/mozilla-central/rev/cb48b124f67b
https://hg.mozilla.org/mozilla-central/rev/af3054673b35
https://hg.mozilla.org/mozilla-central/rev/d6c56cf464f5
https://hg.mozilla.org/mozilla-central/rev/7fd630ae8bbf
https://hg.mozilla.org/mozilla-central/rev/6d93b1ffb2ce
https://hg.mozilla.org/mozilla-central/rev/28cbfff0dd9a
https://hg.mozilla.org/mozilla-central/rev/03eda399201b
https://hg.mozilla.org/mozilla-central/rev/8a411bde0705

Comment 50

[Jeff Walden [:Waldo]]

Attachment: Add more argument-sanity-checking assertions and a missed null-check to FullParseHandler

Comment 51

[Jeff Walden [:Waldo]]

Attachment: Handle PNK_ARRAYCOMP nodes by kind when recycling, revised

I could have sworn I try'd this, but no.  When I ran tests against the prior version, non-legacy array comprehensions failed horribly with it.  This fixes the issue.

The PNK_STATEMENTLIST oddment is from an extremely extensive set of assertions I tried doing, verifying practically the entire substructure under PNK_ARRAYCOMP.  Generally the internal structure, underneath either PNK_ARRAYCOMP or the PNK_LEXICALSCOPE is:

(for forhead-node (for forhead-node ultimate-node))

with as many nested fors as needed.  And, for non-legacy comprehensions, arbitrary interleaving of ifs.  The ultimate node is typically either

(semi PNK_YIELD-node)

or

(arraypush expr)

of

(if condition consequent #null)

The "issue" is when the condition is constantly non-truthy.  In that case, FoldConstants replaces the entire if with an empty PNK_STATEMENTLIST.  My assertions hadn't expected that, then discovered it happening.  But because we're not doing a prepare-for-mutations right now, we're just dropping the condition and consequent nodes on the floor.  For explicitness, and because it really should be the norm, we should explicitly indicate we're going to be mutating the node in-place.  This will have the nice side effect of recycling the condition and consequent nodes immediately.

Oh, those "extremely extensive" assertions?  I ended up dropping them when I discovered that for/if can interleave in non-legacy comprehensions but not in legacy:

  [for (x of [1, 2, 3]) if (x < 3) for (y of [3, 4, 5]) [x, y]] // okay

There's some similarity of structure, but not enough to reuse enough code to make it fly, alas.  I'd have to have separate loops verifying the whole substructure for each, and 100+ lines of structure verification seemed slightly excessive.

Comment 52

[Shu-yu Guo [:shu]]

Comment on attachment 8564375 [details] [diff] [review]
Add more argument-sanity-checking assertions and a missed null-check to FullParseHandler

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

::: js/src/frontend/FullParseHandler.h
@@ +779,5 @@
>      ParseNode *catchpn = newTernary(PNK_CATCH, catchName, catchGuard, catchBody);
>      if (!catchpn)
>          return false;
>  
> +    letBlock->pn_expr = catchpn;

Why this transposition?

Comment 53

[Jeff Walden [:Waldo]]

Comment on attachment 8564375 [details] [diff] [review]
Add more argument-sanity-checking assertions and a missed null-check to FullParseHandler

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

Not sure offhand how well this patch can move closer to the bottom (top?) of my patch stack to land without waiting on other reviews here.  Might give it a try, tho I have reviews to do, and this isn't that important, so I may just wait for now.

::: js/src/frontend/FullParseHandler.h
@@ +272,5 @@
>      }
>  
>      void addArrayElement(ParseNode *literal, ParseNode *element) {
> +        MOZ_ASSERT(literal->isKind(PNK_ARRAY));
> +        MOZ_ASSERT(literal->isArity(PN_LIST));

More testing locally reveals that, improbably, this method is called with |literal->isKind(PNK_CALLSITEOBJ)| and also I think with template strings, too -- non-array kinds both.  It happens that the nodes passed in when adding to a callsiteobj are definitely constant, so I inlined this method into those call sites with assertions of node constancy to avoid the conditional pn_xflags modification.  (This lets me assert harder in places that these kinds don't have any extra flags set in them, more clearly confining the exact field values for the relevant kinds.)

@@ +779,5 @@
>      ParseNode *catchpn = newTernary(PNK_CATCH, catchName, catchGuard, catchBody);
>      if (!catchpn)
>          return false;
>  
> +    letBlock->pn_expr = catchpn;

General preference for fully initializing things before hooking them up to some other object -- in this case, just a matter of style, tho.

Comment 54

[Eric Faust [:efaust]]

Comment on attachment 8563032 [details] [diff] [review]
Make addArrayElement infallible, 'cause it is

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

Suuuure does.

Comment 55

[Eric Faust [:efaust]]

Comment on attachment 8563091 [details] [diff] [review]
When parsing an array that later turns out to be a legacy array comprehension, explicitly discard the array literal, rather than needlessly setting its kind to PNK_ARRAYCOMP and then implicitly dropping it on the floor

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

Seems OK. The position you provide might be different, but seems more sane. r=me

::: js/src/frontend/Parser.cpp
@@ +7021,5 @@
> +
> +    uint32_t arrayBegin = handler.getPosition(array).begin;
> +    uint32_t blockid = array->pn_blockid;
> +
> +    ParseNode *bodyExpr = array->pn_head;

This is better than last(), and equivalent, because pn_count == 1, I assume.

@@ +7026,5 @@
>      array->pn_count = 0;
>      array->pn_tail = &array->pn_head;
>      *array->pn_tail = nullptr;
>  
> +    handler.freeTree(array);

Look at us being all responsible...

Comment 56

[Eric Faust [:efaust]]

Comment on attachment 8563089 [details] [diff] [review]
Handle a few more kinds, by kind, when recycling

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

See comment below. r=me.

::: js/src/frontend/ParseNode.cpp
@@ +327,5 @@
>        case PNK_FOR:
>          return PushBinaryNodeChildren(pn, stack);
>  
> +      // PNK_WITH is PN_BINARY_OBJ -- that is, PN_BINARY with the addition of
> +      // pn_binary_obj as the StaticWithObject, not relevant to this function.

Is it clearer if this reads "with the addition of the StaticWithObject as pn_binary_obj"? I agree that noting which extra field is relevant is the right thing to emphasize. Maybe I just don't like "as"? At any rate, it took me two scans to parse this comment, and I already know what PN_BINARY_OBJ means.

Comment 57

[Eric Faust [:efaust]]

Comment on attachment 8563105 [details] [diff] [review]
Handle import/export and lexicalscope nodes by kind, when recycling

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

::: js/src/frontend/ParseNode.cpp
@@ +388,5 @@
> +      case PNK_EXPORT_FROM: {
> +        MOZ_ASSERT(pn->isArity(PN_BINARY));
> +        MOZ_ASSERT(pn->isKind(PNK_IMPORT)
> +                   ? pn->pn_left->isKind(PNK_IMPORT_SPEC_LIST)
> +                   : pn->pn_left->isKind(PNK_EXPORT_SPEC_LIST));

Nit: This might be even clearer as a pair of MOZ_ASSERT_IFs

@@ +515,4 @@
>  
>        case PNK_LABEL:
>        case PNK_DOT:
> +      case PNK_LEXICALSCOPE:

Logically, LEXICALSCOPE isn't a NAME at all. I guess we leverage the blockid stuff in the union to store the data about the scoped region (which does live in pn_expr). :(

Comment 58

[Eric Faust [:efaust]]

Comment on attachment 8563115 [details] [diff] [review]
Remove the bizarre left-equals-right special case when recycling binary nodes

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

(O.o'). O...kay. The removed code certainly looks terrifying.

Comment 59

[Eric Faust [:efaust]]

Comment on attachment 8563117 [details] [diff] [review]
Refactor some object-literal key:property and shorthand parsing code slightly

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

This is much better than newBinary by hand.

::: js/src/frontend/Parser.cpp
@@ -8069,5 @@
>                      report(ParseError, false, null(), JSMSG_BAD_PROP_ID);
>                      return null();
>                  }
> -                if (!abortIfSyntaxParser())
> -                    return null();

So weird that this was ever here...I guess classes will have a very odd looking one of these when global lexicals gets fixed. Maybe some long-lost good reason.

@@ +8079,3 @@
>                      return null();
> +
> +                if (!handler.addShorthand(literal, propname, nameExpr))

Is there some reason we can't do this as a PNK_COLON node? What has to be magically special about shorthands? Scoping?

Comment 60

[Eric Faust [:efaust]]

Comment on attachment 8563123 [details] [diff] [review]
Split PNK_OBJECT_PROPERTY_NAME out of PNK_NAME, so that PNK_NAME is *always* a name reference and the other is *always* the identifier used to specify a property name in an object literal

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

Cool.

::: js/src/frontend/BytecodeEmitter.cpp
@@ -1701,4 @@
>  {
>      MOZ_ASSERT(pn->isKind(PNK_NAME));
>  
> -    MOZ_ASSERT_IF(pn->isKind(PNK_FUNCTION), pn->isBound());

bottom implies everything.

Comment 61

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7e45daf893
https://hg.mozilla.org/integration/mozilla-inbound/rev/a38231711605
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a230d1ff77
https://hg.mozilla.org/integration/mozilla-inbound/rev/293f7e3a1cb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb65bb77555
https://hg.mozilla.org/integration/mozilla-inbound/rev/56acd8644a3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62587c1fa13
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e2fa416f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb17e51e21c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c804d608a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5f2a894a8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8333a0bfa579
https://hg.mozilla.org/integration/mozilla-inbound/rev/52debad69c67
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01c44107a30

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c7e45daf893
https://hg.mozilla.org/mozilla-central/rev/a38231711605
https://hg.mozilla.org/mozilla-central/rev/d4a230d1ff77
https://hg.mozilla.org/mozilla-central/rev/293f7e3a1cb1
https://hg.mozilla.org/mozilla-central/rev/efb65bb77555
https://hg.mozilla.org/mozilla-central/rev/56acd8644a3f
https://hg.mozilla.org/mozilla-central/rev/f62587c1fa13
https://hg.mozilla.org/mozilla-central/rev/74e2fa416f62
https://hg.mozilla.org/mozilla-central/rev/fb17e51e21c2
https://hg.mozilla.org/mozilla-central/rev/75c804d608a2
https://hg.mozilla.org/mozilla-central/rev/5d5f2a894a8b
https://hg.mozilla.org/mozilla-central/rev/8333a0bfa579
https://hg.mozilla.org/mozilla-central/rev/52debad69c67
https://hg.mozilla.org/mozilla-central/rev/d01c44107a30

Comment 63

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

The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?

Comment 64

[Jeff Walden [:Waldo]]

This got done in other bugs recently.  I'm not sure if arity as concept itself is super-desirable, but that's arguably afield of at least only deriving the arity-related handling directly/solely from kind.