1168992 - Make meta property lookups (super.prop, super[elem], new.target) compliant with estree spec in Reflect.parse

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Neither the superprop stuff, nor the newtarget work that is to land in bug 1141865 is Reflect.parse spec compliant. We should follow up on this and fix it.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Fix up property names for reflected class nodes

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Fix new.target reflection

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 3: Fix super.prop reflection

Comment 4

[Eric Faust [:efaust]]

Attachment: Part 4: Fix super[elem] reflection

Both this and part 3 are larger than they strictly "need" to be. They both attempt to remove the PNK_SUPER* Nodes altogether, and integrate that functionality into the existing PNK_DOT and PNK_ELEM. It's just cleaner this way.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8635574 [details] [diff] [review]
Part 2: Fix new.target reflection

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7991,5 @@
>              return false;
>          break;
>  
> +      case PNK_POSHOLDER:
> +        MOZ_CRASH("Should never try to emit PNK_POSHOLDER");

MOZ_ASSERT_UNREACHABLE

::: js/src/frontend/Parser.cpp
@@ +8759,5 @@
>      }
>  
>      if (!checkAllowedNestedSyntax(SharedContext::AllowedSyntax::NewTarget)) {
> +        reportWithOffset(ParseError, false, handler.getPosition(newHolder).begin,
> +                         JSMSG_BAD_NEWTARGET);

SyntaxParseHandler::getPosition doesn't work the way you want it to work -- it just returns the current token's position.  You want to save the |uint32_t begin| when you create |newHolder| and pass that directly.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8635575 [details] [diff] [review]
Part 3: Fix super.prop reflection

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2653,5 @@
>  
> +    if (isSuper) {
> +        if (!emitSuperPropLHS())                    // THIS OBJ
> +            return false;
> +        if (!emit1(JSOP_DUP2))                      // THIS OJB THIS OBJ

OBJ

@@ +4345,5 @@
> +            offset += 2;
> +        } else {
> +            if (!emitTree(lhs->expr()))
> +                return false;
> +            offset++;

Possibly += 1 for symmetry with above.

@@ +4510,5 @@
>        case PNK_DOT:
>        {
> +        JSOp setOp = lhs->as<PropertyAccess>().isSuper() ?
> +                       sc->strict() ? JSOP_STRICTSETPROP_SUPER : JSOP_SETPROP_SUPER :
> +                       sc->strict() ? JSOP_STRICTSETPROP : JSOP_SETPROP;

Uh, this nesting is weird.  Parenthesize the two arms of the outermost ternary, or something.

::: js/src/frontend/FullParseHandler.h
@@ +353,5 @@
>      ParseNode* newNewTarget(ParseNode* newHolder, ParseNode* targetHolder) {
>          return new_<BinaryNode>(PNK_NEWTARGET, JSOP_NOP, newHolder, targetHolder);
>      }
>      ParseNode* newPosHolder(const TokenPos& pos) {
>          return new_<NullaryNode>(PNK_POSHOLDER, pos);

Add

  ParseNode* newSuperBase(const TokenPos& pos, ExclusiveContext* cx) {
    ParseNode* node = new_<NullaryNode>(PNK_POSHOLDER, pos);
#ifdef DEBUG
    // Set the atom so that ParseHandler::isSuperBase can assert its value.
    if (node)
       node->pn_atom = cx->names().super;
#endif
    return node;
  }

@@ +706,5 @@
>  
> +    bool isSuperBase(ParseNode* node) {
> +        // Even though POSHOLDER is used in other places. It's unique enough to
> +        // answer this question where we care.
> +        return node->isKind(PNK_POSHOLDER);

Make this instead:

bool isSuperBase(ParseNode* node, ExclusiveContext* cx) {
  bool isPosHolder = node->isKind(PNK_POSHOLDER);
  MOZ_ASSERT_IF(isHolder, node->pn_atom == cx->names().super);
  return isPosHolder;
}

with the same comment, but add a bit about verifying the atom as a double-check of that.

::: js/src/frontend/NameFunctions.cpp
@@ +755,5 @@
>  
> +          case PNK_DOT:
> +            // Super prop nodes do not have a meaningful LHS
> +            if (cur->as<PropertyAccess>().isSuper())
> +                break;

Don't do fallthrough if these kinds are that different, IMO.

  case PNK_DOT:
    MOZ_ASSERT(cur->isArity(PN_NAME));
    if (cur->as<PropertyAccess>().hasSuperBase())
      break;
    if (!resolve(cur->expr(), prefix))
      return false;
    break;

::: js/src/frontend/ParseNode.cpp
@@ +1092,4 @@
>              fputc(' ', stderr);
>              DumpParseTree(expr(), indent + 2);
>              fputc(')', stderr);
>          }

Don't you want

  if (as<PropertyAccess>().hasSuperBase())
    fprintf(stderr, "super");
  else
    DumpParseTree(expr(), indent + 2);

instead of not printing the "expression" part at all?  Looks like right now this would print

  (. key

for super.key: imbalanced and weird.

::: js/src/frontend/ParseNode.h
@@ +1301,5 @@
>      PropertyName& name() const {
>          return *pn_u.name.atom->asPropertyName();
>      }
> +
> +    bool isSuper() const {

hasSuperBase, please.  And double-check the pn_atom as well.

::: js/src/frontend/Parser.cpp
@@ +8002,5 @@
>                      handler.setOp(lhs, JSOP_SPREADNEW);
>              }
>          }
>      } else if (tt == TOK_SUPER) {
> +        lhs = handler.newPosHolder(pos());

newSuperBase

::: js/src/frontend/SyntaxParseHandler.h
@@ +135,5 @@
> +
> +        // Currently used in both property accesses involving super, and new.target
> +        // This node is necessary to determine if the LHS of a property access is
> +        // super related.
> +        NodePosHolder

This is only used for super-bases, so rename this to NodeSuperBase.

@@ +280,5 @@
>      Node newSuperElement(Node expr, const TokenPos& pos) {
>          return NodeSuperElement;
>      }
>      Node newNewTarget(Node newHolder, Node targetHolder) { return NodeGeneric; }
> +    Node newPosHolder(const TokenPos& pos) { return NodePosHolder; }

Use newSuperBase(const TokenPos&, ExclusiveContext* cx) instead of newPosHolder for super.

@@ +438,5 @@
>  
> +    bool isSuperBase(Node pn) {
> +        // While NodePosHolder is used in other places than just as super-base,
> +        // it is unique enough for our purposes.
> +        return pn == NodePosHolder;

Use |node| instead of |pn|, and add the |ExclusiveContext* cx| argument.  And the comment can die at this point, too.

::: js/src/jsreflect.cpp
@@ +3017,5 @@
>        {
>          MOZ_ASSERT(pn->pn_pos.encloses(pn->pn_expr->pn_pos));
>  
>          RootedValue expr(cx), id(cx);
>          RootedAtom pnAtom(cx, pn->pn_atom);

Rename this to |key| or |propname| or something, and move the id/key/propname definitions down next to the return statement.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8635578 [details] [diff] [review]
Part 4: Fix super[elem] reflection

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2749,5 @@
>  
>  bool
>  BytecodeEmitter::emitSuperElemOperands(ParseNode* pn, SuperElemOptions opts)
>  {
> +    MOZ_ASSERT(pn->isKind(PNK_ELEM) && pn->as<PropertyByValue>().isSuper());

Two separate assertions, so if you hit one/the other you know exactly *which* of these two conditions was wrong.

@@ +2888,5 @@
> +            return false;
> +    }
> +
> +    JSOp setOp = isSuper ? sc->strict() ? JSOP_STRICTSETELEM_SUPER : JSOP_SETELEM_SUPER
> +                         : sc->strict() ? JSOP_STRICTSETELEM : JSOP_SETELEM;

Parenthesize, and align ?: in the outer thing underneath the "i" in isSuper.

@@ +4494,5 @@
>        case PNK_ELEM:
>        {
> +        JSOp setOp = lhs->as<PropertyByValue>().isSuper() ?
> +                       sc->strict() ? JSOP_STRICTSETELEM_SUPER : JSOP_SETELEM_SUPER :
> +                       sc->strict() ? JSOP_STRICTSETELEM : JSOP_SETELEM;

More alignment/parentheses stuff.

::: js/src/frontend/NameFunctions.cpp
@@ +458,5 @@
>  
> +          case PNK_ELEM:
> +            MOZ_ASSERT(cur->isArity(PN_BINARY));
> +            if (!cur->as<PropertyByValue>().isSuper() && !resolve(cur->pn_left, prefix))
> +                return false;

if (!cur...) {
  if (!resolve(...))
    return false;
}

is the style preferred for when you have a condition guarding whether a fallible thing happens.

::: js/src/frontend/ParseNode.h
@@ +1323,5 @@
> +    }
> +
> +    bool isSuper() const {
> +        // Like PropertyAccess above, PNK_POSHOLDER is "good enough".
> +        return pn_left->isKind(PNK_POSHOLDER);

hasSuperBase, with the same sort of changes from the previous patch.

::: js/src/jsreflect.cpp
@@ +3035,5 @@
>        {
>          MOZ_ASSERT(pn->pn_pos.encloses(pn->pn_left->pn_pos));
>          MOZ_ASSERT(pn->pn_pos.encloses(pn->pn_right->pn_pos));
>  
>          RootedValue left(cx), right(cx);

Move right's definition down by the return.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff820964ea0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6155cc98258d
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e16ff50caa
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3a01796c02

Comment 9

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

I had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c77c5698e859 for apparently breaking reftests:

https://treeherder.mozilla.org/logviewer.html#?job_id=13628594&repo=mozilla-inbound

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a05bfd76c34c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14eb650f5be
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7c8a567707
https://hg.mozilla.org/integration/mozilla-inbound/rev/8534fa84c490

Comment 11

[Nigel Babu [:nigelb]]

Busted spidermonkey tests. Going to back this out.

Comment 12

[Nigel Babu [:nigelb]]

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/26951c3aa267
https://hg.mozilla.org/integration/mozilla-inbound/rev/857b2baa4312
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7324701130
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7515eddee8b

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5cc04f2339
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5f48b58d76
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab28f333e8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/333862eb2aa1

Comment 14

[Eric Faust [:efaust]]

Third time seems to have been the charm. Clearing ni?

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe5cc04f2339
https://hg.mozilla.org/mozilla-central/rev/7f5f48b58d76
https://hg.mozilla.org/mozilla-central/rev/1ab28f333e8a
https://hg.mozilla.org/mozilla-central/rev/333862eb2aa1