828466 - Remove some ParseNode morphing across types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Let's just say morphing is kind of crazy.

ParseNode::become() is barely used; it can go.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: part 2, v1

Separate, because I'm not sure about it. I don't see why this hack would be necessary. I can't come up with code that compiles, and then round-trips incorrectly due to the EmptyStatement being removed. This, for example:

    a: for (;;) {
        a: ;
        for (;;) { break a; }
    }

would round-trip incorrectly, except that it's not legal JS. The duplicate label triggers a SyntaxError.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: part 3, v1

What the heck, have another.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 699856 [details] [diff] [review]
v1

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

::: js/src/frontend/FoldConstants.cpp
@@ +196,5 @@
> +    //
> +    // pnp points to a ParseNode pointer. This must be the only pointer that
> +    // points to the parse node being replaced. The replacement, *pn, is
> +    // unchanged except for its pn_next pointer; updating that is necessary if
> +    // *pn's new parent is a list node.

Erm, shouldn't this be outside the method, as a whole-method comment?

@@ +435,5 @@
>  
>          /* Don't fold a parenthesized call expression. See bug 537673. */
> +        ParseNode **listp = &pn->pn_head;
> +        if ((pn->isKind(PNK_CALL) || pn->isKind(PNK_NEW)) && (*listp)->isInParens())
> +            listp = &(*listp)->pn_next;

This has to be pretty close to the worst of the Augean stables here.  Right?  I hope?  Pretty please?

Okay, probably not.

::: js/src/frontend/FoldConstants.h
@@ +18,1 @@
>                bool inCond = false);

This method needs a comment to explain why we're passing ParseNode**, and to demonstrate how to use it.  It's understandable if you consult all the uses, but that really shouldn't be necessary!

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 699872 [details] [diff] [review]
part 2, v1

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

...I have no idea at all about this.  Throw a fuzzer at it, have it find any problems that might exist with this.  rs=me

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 700619 [details] [diff] [review]
part 3, v1

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

::: js/src/frontend/ParseNode.cpp
@@ +263,5 @@
>      return parser->new_<ParseNode>(kind, JSOP_NOP, arity, tok.pos);
>  }
>  
>  ParseNode *
> +ParseNode::append(ParseNodeKind kind, JSOp op, ParseNode *left, ParseNode *right, Parser *parser)

Can't this return ListNode* now?

@@ +278,3 @@
>          ParseNode *pn1 = left->pn_left, *pn2 = left->pn_right;
> +        list = parser->new_<ListNode>(kind, op);
> +        list->pn_pos = left->pn_pos;

It would be more consistent with the other ParseNode subclass constructors to set the position as part of the constructor.  That said, I'm not 100% certain what it would make sense to pass in here, exactly.  Should ListNode perhaps take in a single node to use as its sole child, after initial construction, and derive begin/end from that?  That seems best to me, but maybe you have other thoughts on this.

::: js/src/frontend/ParseNode.h
@@ +1021,5 @@
>  #endif
>  };
>  
>  struct ListNode : public ParseNode {
> +    ListNode(ParseNodeKind kind, JSOp op)

struct ListNode : public ParseNode
{

@@ +1022,5 @@
>  };
>  
>  struct ListNode : public ParseNode {
> +    ListNode(ParseNodeKind kind, JSOp op)
> +        : ParseNode(kind, op, PN_LIST)

Should be indented two spaces fewer.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Erm, shouldn't this be outside the method, as a whole-method comment?

Duh. Yes, of course. Done.

No comment on the Augean stables.

> ::: js/src/frontend/FoldConstants.h
> @@ +18,1 @@
> >                bool inCond = false);
> 
> This method needs a comment to explain why we're passing ParseNode**, and to
> demonstrate how to use it.  It's understandable if you consult all the uses,
> but that really shouldn't be necessary!

OK, done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> >  ParseNode *
> > +ParseNode::append(ParseNodeKind kind, JSOp op, ParseNode *left, ParseNode *right, Parser *parser)
> 
> Can't this return ListNode* now?

Yes; I'll change the type in a subsequent patch.

> [...] Should ListNode
> perhaps take in a single node to use as its sole child, after initial
> construction, and derive begin/end from that?  That seems best to me, but
> maybe you have other thoughts on this.

All right. Changed.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: part 1, v2

carrying forward Waldo's r+.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: part 2, v2

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 3, v2

Fuzzers, please give this stack of patches a spin.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: consolidated patch

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 702612 [details] [diff] [review]
consolidated patch

I didn't find anything particularly wrong here after a night of fuzzing - tentatively marking feedback+.

Comment 13

[Christian Holler (:decoder)]

Comment on attachment 702612 [details] [diff] [review]
consolidated patch

Same here :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc14e5835254
https://hg.mozilla.org/mozilla-central/rev/f36ab43c5b1e
https://hg.mozilla.org/mozilla-central/rev/6501ad647c04