571452 - proxy construct trap coerces to object but should faithfully return primitives instead (per spec)

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

jorendorff: "My reading of the straw-man spec is that JSProxy::construct and JSProxyHandler::construct should not take a 'receiver' argument at all.

The JSClass::construct hook is not the same thing as the constructTrap for
function proxies, so we're probably not implementing the straw-man
correctly. JSClass::construct only virtualizes the function-calling step of
constructing a new object (specifically, step 8 of the [[Construct]] internal
method described in ES5 13.2.2). I think the constructTrap is supposed to
virtualize the entire [[Construct]] method.

This further raises the question of whether the straw-man intends that |new x|
can be a primitive value if x happens to be a function proxy. I imagine not; it
should be brought up in the committee."

Comment 1

[Andreas Gal :gal]

Alright, so looking at the spec and the implementation, I am not sure this is really a bug.

js> var p = Proxy.createFunction(({ getPropertyDescriptor: function() { return ({}); } }), function() { return 5; }, function() { return 5; });
js> new p()                                                                                                                                     
5
js> typeof (new p())
"number"

jorendorff, can you explain to me again what exactly you are concerned about?

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The new expression should always produce an object or throw

Comment 3

[Andreas Gal :gal]

Yeah, thats why I posted the code above. I have to censor primitive return values, but that has nothing to do with the Class construct hook. The issue above is all I could think of wrt non-compliance.

Comment 4

[Jason Orendorff [:jorendorff]]

Andreas, I think you're right that the summary is too dire. JSClass::construct is sufficient; the actual issues are:

1. We're passing a this-argument to the construct trap. The straw-man says "The this-binding of the constructTrap is undefined." I think that means it should behave like this:

    var a, p;

    p = Proxy.createFunction({getPropertyDescriptor: function(){return {};}},
                             function () {},
                             function () { a = this; return {};});
    new p();
    assertEq(a === this);  // the global object

    p = Proxy.createFunction({getPropertyDescriptor: function(){return {};}},
                             function () {},
                             function () { "use strict";
                                           a = this; return {};});
    new p();
    assertEq(a === undefined);

Currently we fail both assertions. This is what I meant by "JSProxy::construct and JSProxyHandler::construct shouldn't take a 'receiver' argument."

2. I think the spec says that what we do in comment 1 is correct. The committee probably hasn't discussed that; they might decide on the behavior shaver wants. Or not. I don't know.

Comment 5

[Andreas Gal :gal]

Ok, we have consensus then. Some stuff needs to be hashed out here. I hope Tom and Mark can chime in. But this is minor surgery, and the general use of the construct JSClass hook seems viable.

Comment 6

[Mark S. Miller]

* What strawman spec is comment 1 referring to?

* Step 6 for [[Construct]] at <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics#detailed_semantics_of_additional_behavior_for_function_proxies> is

   6. Return ToObject(result).

which would give the invariant Mike Shaver asks for in comment 2. However, on reflection, I'm not sure this is good. We've already removed from ES5/strict (and therefore from ES6) other cases where primitive values are implicitly wrapped. It would be a shame to lose ground on that. I agree it should be brought up in committee.

* Regarding the this-binding of the constructTrap, the two assertions in comment 4 must pass. Anything else violates step 5 of that same spec and is clearly a bug.

Comment 7

[Brendan Eich [:brendan]]

Hi Mark, thanks for commenting.

(In reply to comment #6)
> * What strawman spec is comment 1 referring to?

I'm sure the non-strawman harmony:proxies proposal was meant ;-).

> * Step 6 for [[Construct]] at
> <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics#detailed_semantics_of_additional_behavior_for_function_proxies>
> is
> 
>    6. Return ToObject(result).
> 
> which would give the invariant Mike Shaver asks for in comment 2. However, on
> reflection, I'm not sure this is good. We've already removed from ES5/strict
> (and therefore from ES6) other cases where primitive values are implicitly
> wrapped. It would be a shame to lose ground on that.

I agree. If we do add value types, one plausible basis is proxies. These would not want to produce a non-primitive from new.

> I agree it should be brought up in committee.

Could you please mail John Neumann about adding it to the agenda? Thanks.

> * Regarding the this-binding of the constructTrap, the two assertions in
> comment 4 must pass. Anything else violates step 5 of that same spec and is
> clearly a bug.

Absolutely.

/be

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sorry, I wasn't objecting because I don't personally like producing primitives from proxies -- though I think that doing so via the |new| operator is pretty confusing -- but rather because ES5 seems to require it:

ES5 11.2.2 says that [[Construct]] produces the value, and Table 9 in 8.6.2 says that [[Construct]]'s domain is "SpecOp(a List of any) → Object" with the description stating that it "[c]reates an object."

I wasn't proposing that it automatically wrap primitives, but rather that a proxy hook returning a primitive for construct would trigger an exception.  If we can make proxies obey different rules, that's more than fine with me, I just didn't know that was in the scope of the design.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Furthermore, as regards value-types-atop-proxies, this would only matter if one tried to [[Construct]] on a proxy-based-value-type-instance which then produces another primitive which was therefore *not* a proxy-based-value-type-instance (whew!), which would certainly seem an unusual departure from what we permit on instances of the current primitive types.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Furthermore, as regards value-types-atop-proxies, this would only matter if one
> tried to [[Construct]] on a proxy-based-value-type-instance which then produces
> another primitive which was therefore *not* a proxy-based-value-type-instance
> (whew!), which would certainly seem an unusual departure from what we permit on
> instances of the current primitive types.

Not if we let proxies customize typeof-type and otherwise implement value types. But I agree this is in the future. Throwing for now is prudent.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to comment #6)
> * What strawman spec is comment 1 referring to?

http://wiki.ecmascript.org/doku.php?id=harmony:proxies#semantics

I wasn't aware of the new page.

Comment 12

[Tom Van Cutsem]

At the last TC39 meeting, I believe we had consensus that "new FunctionProxy()" should return whatever the function proxy's "construct" trap returns (so no coercion to Object, no wrapping of primitives).

I updated the [[Construct]] trap semantics at <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics#detailed_semantics_of_additional_behavior_for_function_proxies> accordingly.

Comment 13

[Andreas Gal :gal]

JS_REQUIRES_STACK bool
InvokeConstructor(JSContext *cx, const CallArgs &argsRef)
{

...
    /* Check the return value and if it's primitive, force it to be obj. */
    if (args.rval().isPrimitive()) {
        if (callee->getClass() != &js_FunctionClass) {
            /* native [[Construct]] returning primitive is error */
            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
                                 JSMSG_BAD_NEW_RESULT,
                                 js_ValueToPrintableString(cx, args.rval()));
            return false;
        }
        args.rval().setObject(*obj);
    }

we coerce pretty deep down in the layering. We don't know here its a proxy. That needs to be fixed.

Comment 14

[David Bruant]

Marking this bug as invalid since Proxy.createFunction is not relevant in the direct proxies design.
Feel free to change the status if I'm mistaken.