At the last tc39 meeting, agreement was reached to add an invoke trap. As this will remove a serious performance hazard for Shumway, we should implement it sooner, rather than later. The spec hasn't yet been changed to include the trap, but I doubt there'll be too much wiggle room in how to implement this. For now, here's the meeting notes: https://mail.mozilla.org/pipermail/es-discuss/2013-June/030958.html And this is the relevant part of them: ============= Proxy Invoke Trap and wrong |this|-binding on built-in methods AWB: with current default behavior of “get”, “Caretaker” will break on built-ins such as Date, because the |this| binding is by default set to the proxy, so the Date built-in method will not find the correct private state. ARB: Same issue with binary methods ... STH: We should add invoke trap but not change the object model MM: Pleasant to have. Separate from private state. AWB: used to think this was an issue with proxies, but convinced that it’s an API issue: we need to provide default handlers that do the right thing, and which users can subclass. In particular, want a handler that, on forwarding, rebinds |this| to the target. STH: If you want to proxy a Date method the underlying `this` needs to be a non wrapped Date object. TVC: previously proposed a Handler API that defines derived traps and fundamental traps, allows you to subclass and inherit correct behavior for derived traps. Can be used as the basis. AWB/TVC: invoke trap would make it easier to control |this|-binding DH: Never liked breaking the semantics of [[Get]] + [[Call]] TVC: there already exist invoke-only properties on platforms with __noSuchMethod__ AWB: For a [[Call]] it might be important to control `this` but by the time the [[Call]] is happening you do not know what `this` to use. DH: ActionScript has a proxy and they do have an invoke trap. BM: The most common action is to invoke a method. ? : we already gave up on the |this| invariant for accessors: in ES5, if obj.x is a getter, |this| will always be bound to obj in the getter. With proxies this is no longer true. AI(AWB, TVC): Add spec for invoke. Tom and Allen to work out details of a Handler API that accommodates both “caretaker” (aka forwarding) and “virtual object” use cases. Consensus: Add invoke trap. ==============

wip version, mostly working. Still has one issue in that it weirdly destroys the current scope (or something) if a get operation is attempted on an indirect proxy without a [[get]] trap. I'll have to investigate that. This is exposed by un-commenting line 69 in the test file, whereupon `reportCompare` isn't found anymore. Other than that, works in the interpreter and the jits, although I'm not sure the implementation is ideal at all.

Forgot to deal with JSOP_CALLELEM, this version adds support for it. This passes all tests and jit-tests except for: tests/js1_8_5/extensions/scripted-proxies.js tests/js1_8_5/regress/regress-566914.js jit-test/tests/jaeger/bug588338.js These tests check that JSOP_CALLPROP works for functions returned from the [[get]] trap. The patch doesn't do that, anymore, as it seems like methods should exclusively be dealt with by [[invoke]]. I'm not sure we can do that, however, and what the final spec is going to say about it. Another option would be to always use the [[get]] trap if [[invoke]] isn't set. Regarding the aforementioned issue with non-trapped gets: that turns out to be pre-existing; I'll open a new bug for it.

(In reply to Till Schneidereit [:till] from comment #2) > This passes all tests and jit-tests except for: > > tests/js1_8_5/extensions/scripted-proxies.js > tests/js1_8_5/regress/regress-566914.js > jit-test/tests/jaeger/bug588338.js > > These tests check that JSOP_CALLPROP works for functions returned from the > [[get]] trap. The patch doesn't do that, anymore, as it seems like methods > should exclusively be dealt with by [[invoke]]. I'm not sure we can do that, > however, and what the final spec is going to say about it. Another option > would be to always use the [[get]] trap if [[invoke]] isn't set. I don't understand the issue you're describing here, but it looks that something spec folks will be interested in. Can you bring that to es-discuss? > Regarding the aforementioned issue with non-trapped gets: that turns out to > be pre-existing; I'll open a new bug for it. When you open the bug, can you make it block bug 703537 please (I try to make sure all proxy-related bugs block 703537 so that they can all be found in one place). Thanks :-)

Please get in touch with whoever's specifying direct proxies and talk through this in some detail. Specifically: 1. What happens when we call a method on an object whose [[Prototype]] is a proxy with an invoke trap? var p = new Proxy({}, {invoke: ...}); var q = Object.create(p); q.f(); Does this trigger p's invoke trap? 2. What happens when a proxy with an invoke trap is used in a with-block? with (p) f(); 3. When exactly do we check for an invoke trap, and when do we call it? How does that affect 11.2.3? https://people.mozilla.com/~jorendorff/es6-draft.html#sec-11.2.3 In an expression like p.f(), do we still evaluate "p.f" as in 11.2.3 step 1?

(In reply to Till Schneidereit [:till] from comment #2) > These tests check that JSOP_CALLPROP works for functions returned from the > [[get]] trap. The patch doesn't do that, anymore, as it seems like methods > should exclusively be dealt with by [[invoke]]. I'm not sure we can do that, > however, and what the final spec is going to say about it. Please take this to the direct proxies champion as well. I think most likely the spec will say that "get" traps are not called in this case. We'd have to change the tests. But ask someone who knows the answer!

I might or might not have forgotten to add the tests in the last two versions ...

Comment on attachment 760099 [details] [diff] [review] Implement invoke trap for scripted proxies. v3 Yeah, so this all works very different from how I thought it did: an [[Invoke]] trap doesn't return a function. Instead, it does the invocation itself, returning the result. Also, it builds on the introduction of [[Invoke]] as a derived trap in the MOP itself, which we should probably add as an abstraction in our object handling, too. This all makes a lot more sense, but also makes the implementation a bit more difficult. Oh well.

This should be *much* closer. At least, it works as I expect it to and passes the tests I could come up with. I'm fairly annoyed at the amount of code needed to implement this, so any suggestions for decreasing that would be very welcome.

Forgot to deal with the invoke trap not being defined, but the target having a function with the right id. Hereby rectified.

Comment on attachment 761001 [details] [diff] [review] Implement invoke trap for scripted proxies. v5 Review of attachment 761001 [details] [diff] [review]: ----------------------------------------------------------------- After discussing with till on irc, I'm clearing r? for now. Actual draft spec text is coming soon. A few early comments: ::: js/src/ion/BaselineIC.cpp @@ +5431,5 @@ > if (!obj) > return false; > > if (obj->getOps()->getProperty) { > + if (op == JSOP_CALLPROP && JS_UNLIKELY(obj->isProxy())) { I think the invoke trap is also meant to be called in cases like with (proxy) { method(); } ::: js/src/jsproxy.cpp @@ +659,5 @@ > JS_ASSERT(name == cx->names().has || > name == cx->names().hasOwn || > name == cx->names().get || > name == cx->names().set || > + name == cx->names().invoke || If you agree that we can leave indirect proxies as-is, it isn't necessary to add this line. @@ +970,5 @@ > } > > bool > +ScriptedIndirectProxyHandler::invoke(JSContext *cx, HandleObject proxy, HandleObject receiver, > + HandleId id, unsigned argc, Value *argv, MutableHandleValue vp) I prefer not to change the behavior of indirect proxies in any way. They're legacy. We should instead preserve the existing behavior, I guess by making this delegate to get().

Changes things so that for indirect proxies, the [[Get]] trap is called. I guess we could introduce an IsDirectProxy function and get rid of the entire invokeTrampoline creation for them, but I don't think it's worth it. Also removes some cruft that wasn't really necessary. Regarding the `with (proxy){ method() }` thing: we don't currently call the [[Get]] trap in that situation, either. I'm not sure we should and don't know where in the spec I would look for clarification. I'd advocate landing this without `with`-support and doing that for all relevant traps in a follow-up bug.

All nice and green on try: https://tbpl.mozilla.org/?tree=Try&rev=018455a7e32e (Never mind the known ggc-orange)

Gentle review ping. I'd really like to get this into 25.

Rebased. Jorendorff, do you think there's any chance of getting a review for this soon-ish? I'd very much like to get it into 26, as it would be of great help for Shumway.

(In reply to Till Schneidereit [:till] from comment #14) > Rebased. Jorendorff, do you think there's any chance of getting a review for > this soon-ish? I'd very much like to get it into 26, as it would be of great > help for Shumway. I'm reviewing now.

Comment on attachment 792054 [details] [diff] [review] Implement invoke trap for scripted proxies. This looks pretty great. I tried so hard to find a bug in InvokeTrampoline, but only caught one tiny thing. In jsapi.cpp, JS_CallFunctionName: > RootedValue v(cx); > RootedId id(cx, AtomToId(atom)); >- if (!JSObject::getGeneric(cx, obj, obj, id, &v)) >- return false; >+ if (IsProxy(obj)) { >+ if (!Proxy::invoke(cx, obj, obj, id, &v)) >+ return false; >+ } else { >+ if (!JSObject::getGeneric(cx, obj, obj, id, &v)) >+ return false; >+ } > > RootedValue rv(cx); > if (!Invoke(cx, ObjectOrNullValue(obj), v, argc, argv, &rv)) > return false; What we want here is not another special case, but rather the most generic possible thing: JSAtom *atom = Atomize(cx, name, strlen(name)); if (!atom) return false; - RootedValue v(cx); RootedId id(cx, AtomToId(atom)); - if (!JSObject::getGeneric(cx, obj, obj, id, &v)) - return false; - RootedValue rv(cx); - if (!Invoke(cx, ObjectOrNullValue(obj), v, argc, argv, &rv)) + if (!objArg->callMethod(cx, id, argc, argv, &rv)) return false; *rval = rv; return true; In jsobj.cpp, JSObject::callMethod(): >- if (!JSObject::getGeneric(cx, obj, obj, id, &fval)) >- return false; >+ if (IsProxy(obj)) { >+ if (!Proxy::invoke(cx, obj, obj, id, &fval)) >+ return false; >+ } else { >+ if (!JSObject::getGeneric(cx, obj, obj, id, &fval)) >+ return false; >+ } > return Invoke(cx, ObjectValue(*obj), fval, argc, argv, vp); > } We can avoid the trampoline penalty here. Like this: { + if (IsProxy(obj)) + return Proxy::invoke(cx, obj, id, argc, argv, vp); if (!JSObject::getGeneric(cx, obj, obj, id, &fval)) return false; return Invoke(cx, ObjectValue(*obj), fval, argc, argv, vp); } Add the appropriate Proxy::invoke signature that simply calls the handler's invoke trap (with prototype hoodoo, if bholley insists). (See my comment on InvokeTrampoline below.) In jsproxy.cpp, ScriptedDirectProxyHandler::invoke(): >+ RootedPropertyName trapName(cx, cx->names().invoke); >+ if (!JSObject::getProperty(cx, handler, handler, trapName, &trap)) >+ return false; cx->names() is rooted already. Pass it directly, please, like all the other methods do. >+ // step 7 >+ JSObject *argArray = (argc == 0) ? NewDenseEmptyArray(cx) : NewDenseCopiedArray(cx, argc, argv); Please do not check for argc == 0. Just call NewDenseCopiedArray(). Also change the call site in ArrayFromCallArgs that does the same silly thing. And track down whoever wrote that and shame them for me. (Hint: it was added in revision d0c3168c3c47, apparently authored by one "Jason Orendorff"...) Maybe I'm missing the point of this, but in that case let's talk about it before you check this in. :) >+ Value args[] = { >+ ObjectOrNullValue(target), >+ value, >+ argArrayVal, >+ ObjectOrNullValue(receiver) >+ }; >+ return Invoke(cx, ObjectValue(*handler), trap, 3, args, vp); argc should be 4. And add a test that would have detected this bug! >+static bool >+InvokeTrampoline(JSContext *cx, unsigned argc, JS::Value *vp) { Mm. The existence of this trampoline thingy is pretty gross. :-\ Oh well. >+ JS_ASSERT(fun->getExtendedSlot(0).isObject()); >+ RootedObject receiver(cx, &fun->getExtendedSlot(0).toObject()); The assertion is redundant with the .toObject() call. >+ RootedValue idVal(cx, fun->getExtendedSlot(1)); >+ RootedId id(cx); >+ if (!ValueToId<CanGC>(cx, idVal, &id)) >+ return false; I think everything *after* this point should be refactored into the new Proxy::invoke() method that actually fires the invoke trap. So InvokeTrampoline would get to this point and then tail-call Proxy::invoke(). >+ BaseProxyHandler *handler = GetProxyHandler(proxy); >+ bool own; >+ if (!handler->hasPrototype()) { >+ own = true; >+ } else { >+ if (!handler->hasOwn(cx, proxy, id, &own)) >+ return false; >+ } >+ if (!own) { >+ INVOKE_ON_PROTOTYPE(cx, handler, proxy, >+ JSObject::getGeneric(cx, proto, receiver, id, args.rval())); >+ } I'm pretty confused about this part; I defer to bholley on whether or not this is a good idea; but it should be disabled for scripted proxies anyway, I suppose? >+bool >+Proxy::invoke(JSContext *cx, HandleObject proxy, HandleObject receiver, HandleId id, >+ MutableHandleValue vp) Please rename this to Proxy::getInvokeTrampoline or something. >+ if (!IsScriptedDirectProxy(proxy)) >+ return Proxy::get(cx, proxy, receiver, id, vp); Filed bug 911252 to remove this special case. In tests/ecma_6/Proxy/invoke-trap.js: >\ No newline at end of file Add one?

Comment on attachment 792054 [details] [diff] [review] Implement invoke trap for scripted proxies. Review of attachment 792054 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.cpp @@ +2134,5 @@ > + value, > + argArrayVal, > + ObjectOrNullValue(receiver) > + }; > + return Invoke(cx, ObjectValue(*handler), trap, 3, args, vp); This should use InvokeArgs and the Invoke that goes with it. Eventually we want to get rid of this overload, so at least let's not add more uses of it.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17) > This should use InvokeArgs and the Invoke that goes with it. Eventually we > want to get rid of this overload, so at least let's not add more uses of it. All the other ScriptedDirectProxyHandler methods do this. I say leave it. Waldo, I sympathize with the desire to have one less thing in the codebase. But it is really just not productive to go about it this way (specifically, by reading all new code and asking other contributors for revisions). Or so it seems to me. If you want to get rid of this Invoke overload, please do it and change existing code. I'll review!

I'm just skimming bug comments (an interesting subset of them) as usual, not reading all new code/patches. Sometimes things stick out, sometimes they don't. I think we should always be looking to move in the right direction, and be willing to make and accept suggestions for minor changes in that pursuit. But if you really don't want to start it now, okay, it's not that important.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19) > I'm just skimming bug comments (an interesting subset of them) as usual, not > reading all new code/patches. Sometimes things stick out, sometimes they > don't. That's why it's not an effective way to achieve the desired change. You won't catch them all. Catching some fraction of new call sites doesn't help you remove the signature you don't like. It only taxes people doing work you find interesting.

Please read through this and consider adding tests for these things before pushing. - `for (x of obj)` triggers an [[Invoke]] on obj. Currently it does obj.iterator(), so test that; it'll change to @@iterator soon. - Then, if the iterator is a proxy, the implicit iterator.next() method call also triggers an [[Invoke]]. - Similarly, in an ES6 generator, yield* X, where X.iterator() returns a proxy P, can trigger P.[[Invoke]] calling either P.next(val) or P.throw(exc). - Object.prototype.toLocaleString uses [[Invoke]] to call this.toString(). - Array.prototype.toLocaleString uses [[Invoke]] to call e.toLocaleString() on each element e. (This already works, since it uses JSObject::callMethod; just needs a test.) - If the global object's prototype chain has a proxy on it, calling functionThatDoesNotExist() should fire the proxy's invoke trap. - I noticed you have some commented-out tests for with-blocks. If you want to defer that, add a FIXME comment and file the follow-up bug. - `with (obj) f()` should trigger obj.[[Invoke]] for "f", if obj has a property "f", regardless of whether it's callable. - `with (primitive) f()` should, too, if a proxy has been hacked into the relevant prototype object's prototype chain. For example: String.prototype.__proto__ = new Proxy(target, handler); "ponies".foo(); // triggers handler.invoke() - `with (obj) eval(code)` is direct eval if obj.eval === global.eval and obj is an ordinary object, but not if obj is a scripted proxy (with or without an invoke trap). - But the implicit obj.toString() call in ("" + obj) does *not* use [[Invoke]]. - Nor do implicit calls to .valueOf(). - Nor do implicit calls to .toJSON(). - Nor do the calls to accessor functions caused by getting and setting accessor properties. - Nor do any Proxy handler method calls. For example, in `new Proxy(target, handler).x`, the trap call is like `handler.[[Get]]("get").[[Call]](...)`, not handler.[[Invoke]](handler, "get", ...). (All this is according to the 5 September 2013 draft, rev 18.)

I asked about some of the last few items on that list, the ones that *don't* use [[Invoke]] (.toString(), .valueOf(), .toJSON(), and Proxy handler method calls) and Allen's looking into it. The spec may change. http://esdiscuss.org/topic/invoke-and-implicit-method-calls

Re-posted to es-discuss to try to figure out what the conclusion was. Nothing changed in the spec, and I believe the discussion established that any changes will be minor. Till, is anything else still blocking this? Note that bug 923765 made a change in this area. (I'm not sure but I think the getprop/callprop IC stuff for proxies is all-new since you originally posted this...)

After long discussions on es-discuss[1,2], this has been removed from the latest ES6 spec draft. [1]: https://mail.mozilla.org/pipermail/es-discuss/2013-September/thread.html#33276 [2]: https://mail.mozilla.org/pipermail/es-discuss/2013-October/thread.html#34243