1167845 - Refactor a bunch of assignment/increment/decrement parsing, with the aim of providing a Parser::isValidSimpleAssignmentTarget method usable outside the parser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

As part of making constant-folding happen by kind and not arity, I want to be able to assert that various operations (increment/decrement were the initial motivation, but this patch pointso out assignment and other contexts) are still structurally valid after folding.  For this, I really need a Parser::isValidSimpleAssignmentTarget method for assertions.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

This is kind of large-ish for ~little gain, but I do get my method at the end.  Also, I get rid of some very hairy current code that uses full/syntax partial specializations -- one definition for full/syntax now, for a number of methods.  (Note: checkDestructuring will abortIfSyntaxParser, so that bit of parsing code is 100% fine to be in the syntax-parsing instantiation of that one method.)  I don't have much confidence in that code being fully consistent in all cases with respect to error messages and the like, to be completely honest.

Oh, and as I write this I remember I got rid of the isOp assertions on PNK_CALL nodes that are assignment targets but didn't split that into a separate patch.  I could readd them if desired, but my auditing of Parser::memberExpr verified that as the complete list.  And as you noted, it seems extraordinarily unlikely anyone's mutating the op outside that method, without mutating kind to go along with it.

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8609732 [details] [diff] [review]
Patch

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

It's really nice to have isValidSimpleAssignmentTarget() as one thing. I remember missing each spot to ad SUPERPROP and SUPERELEM in turn before having to find them as tests failed.both

::: js/src/frontend/FullParseHandler.h
@@ +76,5 @@
> +    }
> +
> +    bool isFunctionCall(ParseNode* node) {
> +        // Note that super() is *not* included in this check.
> +        return node->isKind(PNK_CALL);

Nor is it |new foo(...)|, right? Do we not allow setcall on construction sites, or something? That would be great, since setcall is legacy nonsense.

@@ +749,5 @@
>      }
>      bool isCall(ParseNode* pn) {
>          return pn->isKind(PNK_CALL);
>      }
> +    PropertyName* isDottedProperty(ParseNode* pn) {

nit: is... is a strange name here, kinda, since it doesn't return bool?

::: js/src/frontend/Parser.cpp
@@ +6376,5 @@
> +            // specialized, in the very weird "for (var [x] = i in o) ..."
> +            // case. See bug 558633.
> +            //
> +            // XXX Is this necessary with the changes in bug 1164741?  This is
> +            //     likely removable now.

What would it take to find this out? I certainly am not able to evaluate that, either.

@@ +6533,5 @@
> +template <typename ParseHandler>
> +bool
> +Parser<ParseHandler>::reportIfArgumentsEvalTarget(Node target)
> +{
> +    PropertyName* name = handler.isName(target);

again, I would prefer maybeName(), but I admit it's preexisting.

::: js/src/frontend/SyntaxParseHandler.h
@@ +419,5 @@
> +        // Note: |super.apply(...)| is a special form that calls an "apply"
> +        // method retrieved from one value, but using a *different* value as
> +        // |this|.  It's not really eligible for the funapply/funcall
> +        // optimizations as they're currently implemented (assuming a single
> +        // value is used for both retrieval and |this|).

I guess there's no better place to put this comment, but if I was just scrolling through the file, I would be confused what that comment has to do with this method. Only because I'm reading this patch, and I saw that we use isDottedProperty there, did I understand the relevance.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0cd6ec2fc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/79466a3087b1

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Eric Faust [:efaust] from comment #2)
> > +    bool isFunctionCall(ParseNode* node) {
> > +        // Note that super() is *not* included in this check.
> > +        return node->isKind(PNK_CALL);
> 
> Nor is it |new foo(...)|, right? Do we not allow setcall on construction
> sites, or something? That would be great, since setcall is legacy nonsense.

Nope, assigning to construction was never allowed.  Given "function call" seems clearly to mean *calling* and not construction to me, I didn't call it out here.

> > +    PropertyName* isDottedProperty(ParseNode* pn) {
> 
> nit: is... is a strange name here, kinda, since it doesn't return bool?

Renamed to maybe* in a followup patch.

> ::: js/src/frontend/Parser.cpp
> @@ +6376,5 @@
> > +            // specialized, in the very weird "for (var [x] = i in o) ..."
> > +            // case. See bug 558633.
> > +            //
> > +            // XXX Is this necessary with the changes in bug 1164741?  This is
> > +            //     likely removable now.
> 
> What would it take to find this out? I certainly am not able to evaluate
> that, either.

I'm going to investigate this fairly soon as part of for-loop scoping adjustments.

> ::: js/src/frontend/SyntaxParseHandler.h
> @@ +419,5 @@
> > +        // Note: |super.apply(...)| is a special form that calls an "apply"
> > +        // method retrieved from one value, but using a *different* value as
> > +        // |this|.  It's not really eligible for the funapply/funcall
> > +        // optimizations as they're currently implemented (assuming a single
> > +        // value is used for both retrieval and |this|).
> 
> I guess there's no better place to put this comment, but if I was just
> scrolling through the file, I would be confused what that comment has to do
> with this method. Only because I'm reading this patch, and I saw that we use
> isDottedProperty there, did I understand the relevance.

Yeah, a better method name, specific to this exact use case, would be preferable.  But I don't have good ideas, so left alone.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out so I could cleanly back out bug 1141865 for bustage. Feel free to reland this as soon as I reopen the tree.

Comment 6

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/186226d9a118
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e379d6d6e8

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b429a52c6135
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13e7c53ea2e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b429a52c6135
https://hg.mozilla.org/mozilla-central/rev/a13e7c53ea2e