Last Comment Bug 571452 - proxy construct trap coerces to object but should faithfully return primitives instead (per spec)
: proxy construct trap coerces to object but should faithfully return primitive...
Status: RESOLVED INVALID
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: P2 normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: harmony:proxies
  Show dependency treegraph
 
Reported: 2010-06-11 01:00 PDT by Andreas Gal :gal
Modified: 2012-09-17 06:30 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Andreas Gal :gal 2010-06-11 01:00:49 PDT
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 2010-06-11 12:37:34 PDT
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) 2010-06-11 12:49:47 PDT
The new expression should always produce an object or throw
Comment 3 Andreas Gal :gal 2010-06-11 12:55:19 PDT
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] 2010-06-11 13:40:48 PDT
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 2010-06-11 13:44:41 PDT
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 2010-06-11 14:34:41 PDT
* 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] 2010-06-11 15:53:23 PDT
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) 2010-06-11 16:13:31 PDT
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) 2010-06-11 16:15:15 PDT
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] 2010-06-11 16:19:55 PDT
(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] 2010-06-12 04:41:22 PDT
(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 2010-08-04 05:05:07 PDT
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 2010-09-29 12:12:29 PDT
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 2012-09-17 06:30:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.