Closed Bug 571452 Opened 14 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect, P2)

Other Branch
x86
macOS
defect

Tracking

()

RESOLVED INVALID

People

(Reporter: gal, Unassigned)

References

Details

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."
Summary: TM: → 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 → 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
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
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.
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.
* 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.
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.
Priority: -- → P2
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.
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)
No longer blocks: harmony:proxy
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
Closed: 12 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.