1472211 - Update SuperProperty evaluation semantics per spec PR

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

See spec PR https://github.com/tc39/ecma262/pull/1207.

Filing now to have a reference link for the test262 update (test262 already has a test for the new proposed semantics).

Comment 1

[André Bargull [:anba]]

Attachment: bug1472211.patch

Update emitSuperElemOperands() to first emit code to retrieve the this-value and then evaluate the property key per the latest spec changes. A similar change was necessary for emitDeleteElement(), but additionally also added missing byte code instructions for |delete super.prop| and |delete super[elem]|.

And while there:
- Remove the unused JSOp parameter from emitSuperPropOp(), because it's always JSOP_GETPROP_SUPER. And then remove the "Op" suffix from the method name, because the method no longer accepts a JSOp argument. 
- Perform the same change for emitSuperElemOp.
- Remove the unused |EmitElemOption::Set| enum member.

Update 'non262/class/superPropDelete.js' to test the new behaviour and add 'non262/class/superElemDelete.js' to test the case when |ToPropertyKey(elem)| in |delete super[elem]| has side-effects.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8993464 [details] [diff] [review]
bug1472211.patch

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

I am entirely un-confident in my memory/recollection/understanding of the semantics of super-accesses.  Looks okay to me, but I could easily have missed something and probably did.  f+ from me and forwarding to someone else for a second look.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2092,5 @@
>      MOZ_ASSERT(pn->isKind(ParseNodeKind::Elem) && pn->as<PropertyByValue>().isSuper());
>  
>      // The ordering here is somewhat screwy. We need to evaluate the propval
>      // first, by spec. Do a little dance to not emit more than one JSOP_THIS.
>      // Since JSOP_THIS might throw in derived class constructors, we cannot

This comment is out of date now, isn't it?

@@ +2113,1 @@
>          // We need another |this| on top, also

"We need a second |this| that will be consumed during computation of the value of the property.  (The original |this| is passed to the call.)"

or somesuch.

@@ +2154,4 @@
>  {
>      EmitElemOption opts = EmitElemOption::Get;
>      if (isCall)
>          opts = EmitElemOption::Call;

I'd prefer these lines be unified to a ternary.

@@ +6649,5 @@
>      MOZ_ASSERT(node->isKind(ParseNodeKind::DeleteProp));
>      MOZ_ASSERT(node->isArity(PN_UNARY));
>  
>      ParseNode* propExpr = node->pn_kid;
>      MOZ_ASSERT(propExpr->isKind(ParseNodeKind::Dot));

If there are going to be multiple as<> calls on this, let's just have

  PropertyAccess& propExpr = node->pn_kid->as<PropertyAccess>();

which incidentally directly depends on the kind-check, so you can have just the one line instead of two.

@@ +6654,5 @@
>  
>      if (propExpr->as<PropertyAccess>().isSuper()) {
> +        // Still have to calculate this and the base, even though we are are
> +        // going to throw unconditionally, as calculating this or the base
> +        // could also throw.

Perhaps

"""
The expression |delete super.foo;| has to evaluate |super.foo|, which could throw if |this| hasn't yet been set by a |super(...)| call, before throwing a ReferenceError for attempting to delete a Reference that IsSuperReference.
"""

IMO the wording you have (we had before) is jargony and not very much in the vein of what the code being compiled would actually look like/be doing.

@@ +6666,5 @@
> +            return false;
> +
> +        // Another wrinkle: Balance the stack from the emitter's point of view.
> +        // Execution will not reach here, as the last bytecode threw.
> +        return emit1(JSOP_POP);

We might stand in a new bug to add new opcodes like JSOP_UNREACHABLE_POP and JSOP_UNREACHABLE_POP2 or such for these things, that we could make assert or something when executed.  Something better than just aligning the stack and falling into whatever follows.

@@ +6684,5 @@
>      MOZ_ASSERT(elemExpr->isKind(ParseNodeKind::Elem));
>  
>      if (elemExpr->as<PropertyByValue>().isSuper()) {
>          // Still have to calculate everything, even though we're gonna throw
>          // since it may have side effects

A variation on the other suggested comment would be nice here.

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8993464 [details] [diff] [review]
bug1472211.patch

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

Looks good :)

Comment 4

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #2)
> @@ +6666,5 @@
> > +            return false;
> > +
> > +        // Another wrinkle: Balance the stack from the emitter's point of view.
> > +        // Execution will not reach here, as the last bytecode threw.
> > +        return emit1(JSOP_POP);
> 
> We might stand in a new bug to add new opcodes like JSOP_UNREACHABLE_POP and
> JSOP_UNREACHABLE_POP2 or such for these things, that we could make assert or
> something when executed.  Something better than just aligning the stack and
> falling into whatever follows.
> 

Hmm, I'm a bit hesitant to spend precious byte code space for this. (A single JSOP_UNREACHABLE with an operand to consume a variable number of stack operands may acceptable, but spending two byte codes is likely too much.) I assume the current code is (mis)using JSOP_POP/JSOP_POP2 simply because that seemed safer than directly adjusting |BytecodeEmitter::stackDepth|? And probably because it doesn't matter if we emit more byte code for an always error case which won't ever happen in real code.

Comment 5

[André Bargull [:anba]]

Attachment: bug1472211-part1-implement-spec-change.patch

Updated patch per review comments, carrying r+.

Comment 6

[André Bargull [:anba]]

Attachment: bug1472211-part2-reorder-operands.patch

Let's also change the operands of the JSOP_SETELEM_SUPER and JSOP_GETELEM_SUPER byte codes, so we don't need to emit JSOP_SWAP to reorder the operands on the stack. (And for the call-case we now only need to emit JSOP_DUP instead of JSOP_DUPAT.)

Drive-by change: Improve BytecodeEmitter::emitElemIncDec() to use JSOP_UNPICK instead of multiple JSOP_PICK byte codes.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8996457 [details] [diff] [review]
bug1472211-part2-reorder-operands.patch

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

Thanks!

::: js/src/jit/BaselineIC.cpp
@@ +638,5 @@
>  }
>  
>  static bool
>  DoGetElemSuperFallback(JSContext* cx, BaselineFrame* frame, ICGetElem_Fallback* stub_,
> +                       HandleValue lhs, HandleValue rhs, HandleValue receiver,

looks like we have several inconsistent kinds of orders between lhs/receiver/key across functions.
it would be nice if we can use single order across all functions, but of course it's for other bug.

Comment 8

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> looks like we have several inconsistent kinds of orders between
> lhs/receiver/key across functions.
> it would be nice if we can use single order across all functions, but of
> course it's for other bug.

Agreed, neither the parameter order nor the parameter names are consistent across the engine for the various implementations of [[Get]], [[Set]], and so forth. For example when having to decide which of the following names is the best choice for the "property key" argument, I think it's clear that "key" as a parameter name is more expressive than "rhs" or "propval". :-D

I'll file a bug.

Comment 9

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dafa3f50d04bf9bc793cab64e8caf4653b907dc

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d750528120
Part 1: Reorder super-property evaluation order per latest spec change. r=arai, f=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ca0190aa09
Part 2: Reorder operands in Super-Elem bytecode operations. r=arai

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c7d750528120
https://hg.mozilla.org/mozilla-central/rev/12ca0190aa09