The default bug view has changed. See this FAQ.

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

RESOLVED INVALID

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: gal, Unassigned)

Tracking

Other Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 years ago
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."
(Reporter)

Updated

7 years ago
Summary: TM: → TM:construct hook might not be sufficient to faithfully implement a proxy's construct trap
(Reporter)

Updated

7 years ago
Blocks: 546590
Summary: TM:construct hook might not be sufficient to faithfully implement a proxy's construct trap → TM: construct hook might not be sufficient to faithfully implement a proxy's construct trap
Summary: TM: construct hook might not be sufficient to faithfully implement a proxy's construct trap → JSClass construct hook might not be sufficient to faithfully implement a proxy's construct trap
(Reporter)

Comment 1

7 years ago
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?
The new expression should always produce an object or throw
(Reporter)

Comment 3

7 years ago
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.
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.
(Reporter)

Comment 5

7 years ago
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

7 years ago
* 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.
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
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.
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.
(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
(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

7 years ago
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.
(Reporter)

Updated

7 years ago
Priority: -- → P2
(Reporter)

Comment 13

7 years ago
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.
(Reporter)

Updated

7 years ago
Summary: JSClass construct hook might not be sufficient to faithfully implement a proxy's construct trap → proxy construct trap coerces to object but should faithfully return primitives instead (per spec)
Blocks: 694103
No longer blocks: 694103

Comment 14

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.