Implement Function.prototype.bind from the ECMAScript 4 spec. Relevant spec: bind ( fn, thisArg=…, ...args ) Description The static bind method takes arguments fn, thisArg, and optionally some args. Returns The bind method returns a Function object that accepts some arguments moreargs and which calls fn with thisArg as the this object and the values of args and moreargs as actual arguments. Implementation static public function bind(method: Callable, thisObj: Object, ...args) helper::bind(method, thisObj, args); static helper function bind(method, thisObj, args) { return function (...moreargs) method.apply(thisObj, args.concat(moreargs)); } The implementation should also include the static Function.bind as well.

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?. If this should be a blocker, please mark accordingly.

Shouldn't this block bug445494 the ES5 tracking bug rather than the JS1.9 tracking bug which is blocked by the ES5 tracking bug?

Is it possible to add this to upcoming 1.9.2? ES5 is now finalized, so it seems safe to start implementing Function#bind as described in 15.3.4.5.

(In reply to comment #3) > Is it possible to add this to upcoming 1.9.2? ES5 is now finalized, so it seems > safe to start implementing Function#bind as described in 15.3.4.5. 1.9.2 is all but done; it'll show up in the next big-enough release. ES5 is not all there in 1.9.2; this is not the last detail, without which we have a fatally flawed Fabergé egg. Ajax libs have their own bind impls for Firefox 3.6 as for 3.5 and other browsers. /be

> 1.9.2 is all but done; it'll show up in the next big-enough release. Good to know. Thanks. > ES5 is not all there in 1.9.2; this is not the last detail, without which we > have a fatally flawed Fabergé egg [...] I'm aware of SpiderMonkey's ES5 tracking ticket (https://bugzilla.mozilla.org/show_bug.cgi?id=445494), but thought that something like `Function.prototype.bind` would be a good candidate for addition to 1.9.2+ > Ajax libs have their own bind impls for Firefox 3.6 as for 3.5 and other browsers. Yes, as a developer of one of such libs (Prototype.js), I would be glad to see native implementation replace custom one when available, similar to something like `String.prototype.trim` — part of ES5, but made available in SpiderMonkey as a custom extension. In Prototype.js we use String#trim to improve performance of relevant helper method (String#strip); it would be nice to have fast native Function#bind as well.

CCing luke (who will end up reviewing the changes to function invocation that I'll be making here)...

I'm beginning to stab at this now. Because of the way I'm changing how one invokes a function I'm ending up having to change a fair number of places where natives reenter the engine. After that come modifications to the interpreter, then the tracer. Work proceeds well, just a bit of a slog to change all callers in this way, and to work out exactly how to make the parts interlock (also while remaining forward-thinking about ES5 primitive-this and other forthcoming innovations). There will be yet more fun with fatvals here, but I'm optimistic that ongoing work here will be productive even while fatvals remain unlanded -- some trouble certainly, but I think it will be okay. (Far worse would have been merging with the stack-layout rewrite of a few months ago, but that rewrite actually makes much of this patch a good deal easier.) Mind turned to mush this late at night, time to get some sleep; back tomorrow for a bit to work on this, hopefully to finish up at least the first cut before heading out for a slightly lengthened weekend...

Without code in js_NewBoundFunction it's hard to be sure about the design here, but the patch is overlarge due to lots of cleanup changes as well as invoke-path changes that are more than cleanup. Some of the cleanup looks good but it isn't all about bind. Separate bugs unless it is all required somehow. What is not clear is why a big patch is needed. JSFUN_BOUND_METHOD on a custom invoker function that carries the target and extra args via the two reserved slots set aside for embeddings such as XPConnect should be sufficient, without pervasive changes. What am I missing? /be

The cleanup changes are necessary for the invoke-path work to be sound; exposing a raw vp would make it much more difficult to be confident in runtime safety. (I've commented on this before; see bug 540706 comment 26.) The invoke path permits bound functions to be efficient, and as you note much of it is good change anyway. I don't think it will be substantially more work to do it right the first time than to do it quickly but stupidly, and if we took the shortcut now I believe we would do the harder work pretty soon after anyway. It might perhaps be possible one could do this with a native function, but one notable wrinkle is that you can also have bound constructor-like behavior. We were moving away from having fast natives be constructable last I remembered (if we haven't yet made it impossible), and I suspect we don't want to take the slow native path for these function.

(In reply to comment #9) > The cleanup changes are necessary for the invoke-path work to be sound; > exposing a raw vp would make it much more difficult to be confident in runtime > safety. (I've commented on this before; see bug 540706 comment 26.) Yeah, but we've had raw pointers forever, why is this bug the one in which to clean things up? It's not about logical soundness. > The > invoke path permits bound functions to be efficient, At what cost to the unbound normal case? It's early days to optimize bind but if you can pull it off without slowing down the cases all web JS currently use, then maybe. Yet it is more code change / risk than an ES5 newcomer like bind seems to require, and the existing Ajax library binds are very slow, even though they are used a lot w/o perf complaints rising above the "make V8 raytrace faster" (its perf paths are not dominated by bind). > and as you note much of it is good change anyway. How about: as I note it's a separate pile of work for a separate bug. Please don't paraphrase me selectively. My whole point in writing that comment was that while the cleanup may be good (haven't reviewed it, just skimmed it), it is not necessary to implement bind. > I don't think it will be substantially more work to do > it right the first time than to do it quickly but stupidly, and if we took the > shortcut now I believe we would do the harder work pretty soon after anyway. That's not clear given the schedule. It's also the case that you're doing the cleanup before the actual guts of the bind implementation, which has design choices that need review. So you are putting the cart before the horse, at least in this single bug, and doing the cleanup bug first is out of priority order, which means schedule risk. > It might perhaps be possible one could do this with a native function, but one > notable wrinkle is that you can also have bound constructor-like behavior. So? Native functions can be callable or constructable, and forward to the target function via the appropriate call or construct helper. Or so it seems -- again is there a problem using an invoker/constructor "bind wrapper"? I don't see how given that this is exactly what self-hosted Ajax library binds do. > We > were moving away from having fast natives be constructable last I remembered > (if we haven't yet made it impossible), and I suspect we don't want to take the > slow native path for these function. Why not? All native constructors must be slow natives already (to get a frame distinguished by the JSFRAME_CONSTRUCTING flag, for callable-or-constructable functions such as Date). Ajax library self-hosted bind is much slower yet, so avoid optimizing if you can -- measuring beats suspecting any day of the week. Sorry to be so picky, but you wrote a lot of words defending without addressing substantive points I raised. Here's another substantive point raised but not yet addressed: why are you using dslots for anything in the bound function, instead of the two reserved fslots? Sorry if I missed a reason, just asking! /be

Whatever work I had done here previously has since become so bitrotted that it was easier to throw it all away and start from scratch. As far as previous arguments go, analogizing from strict-arguments work suggests pushing bind-specialness into callers is the better idea. This is atop a bunch of other patches, most notably bug 514581 for the [[ThrowTypeError]] bits. I'm going to have a huge mess of patches (in more ways than one) to land once that gets reviewed.

The loop in fun_hasInstance is, I think, the neatest thing that this code happens to test. Or maybe it's the callee tests that depend on step 5 of 15.3.4.5.1/2. :-) Either way, good stuff.

Comment on attachment 465591 [details] [diff] [review] 1: Patch >+++ b/js/src/jsapi.h >@@ -473,12 +473,10 @@ extern JS_PUBLIC_DATA(jsid) JSID_VOID; > > /* Function flags, set in JSFunctionSpec and passed to JS_NewFunction etc. */ > #define JSFUN_LAMBDA 0x08 /* expressed, not declared, function */ >-#define JSFUN_BOUND_METHOD 0x40 /* bind this to fun->object's parent */ > #define JSFUN_HEAVYWEIGHT 0x80 /* activation requires a Call object */ > > #define JSFUN_DISJOINT_FLAGS(f) ((f) & 0x0f) Looks like JSFUN_DISJOINT_FLAGS can be removed now, from a quick grep. Or perhaps it should be used! Was its use lost somehow? Separate bug, wanted to raise it in case anyone knows off the top of their head. >@@ -1840,7 +1849,7 @@ fun_resolve(JSContext *cx, JSObject *obj > > PropertyOp getter, setter; > uintN attrs = JSPROP_PERMANENT; >- if (fun->inStrictMode()) { >+ if (fun->inStrictMode() || obj->isBoundFunction()) { > JSObject *throwTypeError = GetThrowTypeError(obj); > > getter = CastAsPropertyOp(throwTypeError); Context ended here, and IIRC, this is based on the poison pill patch that jimb reviewed (not yet landed). ES5 does not want an error for caller accessed from a bound function object, it just wants the default [[Get]] behavior (so you'd get undefined). Does this change do that? >+namespace { >+JSBool >+CallOrConstructBoundFunction(JSContext *cx, uintN argc, Value *vp); >+} >+ >+inline bool >+JSObject::isBoundFunction() const >+{ >+ return isFunction() && >+ getFunctionPrivate()->u.n.native == reinterpret_cast<Native>(CallOrConstructBoundFunction); Use Valueify, not equivalent reinterpret_cast. >+inline bool >+JSObject::initBoundFunction(JSContext *cx, JSObject *target, const Value &thisArg, >+ const Value *args, uintN argslen) >+{ >+ JS_ASSERT(isBoundFunction()); >+ >+ if (!js_EnsureReservedSlots(cx, this, 3 + argslen)) Right now, function objects have two free fslots reserved in case an embedding wants to store dispatch type-info in them for native functions the embedding creates (so you can have a generic native function referenced by multiple native function objects, where the generic code dispatches based on the type-info -- XPConnect among many others does this). But for a function object we create, those two fslots are just sitting there, unused. What's more, the parent slot, which implements as the ES3/5 [[Scope]] internal property, is not needed (ES5 15.3.4.5 NOTE). So there are three fixed members of JSObject available. These should be used, not wasted, independent of the following point. The patch for bug 558451 requires invariant first ad-hoc property slot for a given class, so allocating reserved dslots is about to be verboten for any object that can grow ad-hoc properties, including bound function objects. The issue is the property caching, PICs, and trace guards all need to see the empty shape for an unextended function instance and predict the slot to allocate based only on that shape and the add-property program point. But bind is sometimes (often? can't tell) used without pre-arguments, so there's no need for dslots in such cases. I'll deal with another js_EnsureReservedSlots call if you land ahead of bug 558451, but I wanted to write this in case you can see a better way to store the pre-args. >+ if (!constructing) { >+ /* FIXME Pass boundThis directly without boxing! */ >+ JSObject *boundThisObj; >+ if (boundThis.isObject()) >+ boundThisObj = &boundThis.toObject(); >+ else if (!js_ValueToObjectOrNull(cx, boundThis, &boundThisObj)) Use boundThis.toObjectOrNull not js_ValueToObjectOrNull. >+ JSObject *parent = vp[0].toObject().getGlobal(); Spec says no [[Scope]], but we do need a parent (global objects have null parent, any other null-parent objects are prone to be confused for globals -- not sure where this sits, mrbkap and jorendorff know all, but it's safe to say we need a non-null parent). But as proposed above, you could store the target in the bound function object's parent member. I think that would lead to the correct global. If one calls otherWindow.Function.prototype.bind.call(myFunction, myThisArg), then ignoring the requirement that otherWindow be same-origin to myFunction's global in order for such code to work in the first place, should the returned bound function object really be parented by otherWindow? Again ES5 can't care because (apart from not treating muliple global objects, except by the [[Scope]] internal property of function objects added in ES3) it specs no [[Scope]] for bound function objects. So in theory parent can be the target function object. Can you try that? >+ >+ /* Step 4. */ >+ JSObject *proto; >+ if (!js_GetClassPrototype(cx, parent, JSProto_Function, &proto)) >+ return false; proto is unused after this point. >+ /* Steps 15-16. */ >+ uintN length = 0; >+ if (target->isFunction()) { >+ uintN nargs = target->getFunctionPrivate()->nargs; >+ if (nargs > argslen) >+ length = nargs - argslen; >+ } >+ >+ /* Step 4-6, 10-11. */ >+ JSAtom *name = target->isFunction() ? target->getFunctionPrivate()->atom : NULL; >+ JSObject *f = >+ js_NewFunction(cx, NULL, reinterpret_cast<Native>(CallOrConstructBoundFunction), length, Valueify not reinterpret_cast. This should allow the call to wrap more naturally instead of breaking after =. >+ JSFUN_FAST_NATIVE | JSFUN_FAST_NATIVE_CTOR, parent, name); >+ if (!f) >+ return false; Minus'ing per your preferred review protocol, but it's looking good apart from the above. /be

(In reply to comment #13) Will get to this ASAP, just wanted to say first: > Minus'ing per your preferred review protocol, but it's looking good apart from > the above. Excellent, much appreciated.

(In reply to comment #13) > > #define JSFUN_DISJOINT_FLAGS(f) ((f) & 0x0f) > > Looks like JSFUN_DISJOINT_FLAGS can be removed now, from a quick grep. Or > perhaps it should be used! Was its use lost somehow? Separate bug, wanted to > raise it in case anyone knows off the top of their head. Looks like it's 1.8-branch detritus: http://mxr.mozilla.org/mozilla/source/js/src/jsapi.c#4279 I'll remove it here, doesn't seem worth the trouble of a new bug for a single line of removal. > >@@ -1840,7 +1849,7 @@ fun_resolve(JSContext *cx, JSObject *obj > > > > PropertyOp getter, setter; > > uintN attrs = JSPROP_PERMANENT; > >- if (fun->inStrictMode()) { > >+ if (fun->inStrictMode() || obj->isBoundFunction()) { > > JSObject *throwTypeError = GetThrowTypeError(obj); > > > > getter = CastAsPropertyOp(throwTypeError); > > Context ended here, and IIRC, this is based on the poison pill patch that > jimb reviewed (not yet landed). ES5 does not want an error for caller > accessed from a bound function object, it just wants the default [[Get]] > behavior (so you'd get undefined). Does this change do that? That doesn't seem to be the case to me. 15.3.4.5 step 20 says to define a "caller" property on the bound function with [[Get]]: [[ThrowTypeError]] and [[Set]]: [[ThrowTypeError]]. This code seems correct to me. Although, this is quite interesting: it looks to me as though 15.3.5.4 says that, in addition to .caller throwing on strict mode functions, .caller on non-strict mode functions *which would ordinarily return a strict mode function* is supposed to throw as well. I filed bug 588251 on that. > >+ return isFunction() && > >+ getFunctionPrivate()->u.n.native == reinterpret_cast<Native>(CallOrConstructBoundFunction); > > Use Valueify, not equivalent reinterpret_cast. That's not how Valueify works; I think it's only supposed to interconvert between an internal js:: type and an external JS type, or js::Value/jsval. reinterpret_cast is what I want here, right, Luke? Certainly just using Valueify gives a compile error. > Right now, function objects have two free fslots reserved in case an > embedding wants to store dispatch type-info in them for native functions the > embedding creates (so you can have a generic native function referenced by > multiple native function objects, where the generic code dispatches based on > the type-info -- XPConnect among many others does this). > > But for a function object we create, those two fslots are just sitting > there, unused. > > What's more, the parent slot, which implements as the ES3/5 [[Scope]] > internal property, is not needed (ES5 15.3.4.5 NOTE). > > So there are three fixed members of JSObject available. These should be used, > not wasted, independent of the following point. I dislike the implicit punning on the slots' contents here, but I guess that's what is-bound assertions are supposed to catch. Really wanting object subtypes... > The patch for bug 558451 requires invariant first ad-hoc property slot for > a given class, so allocating reserved dslots is about to be verboten for any > object that can grow ad-hoc properties, including bound function objects. > The issue is the property caching, PICs, and trace guards all need to see > the empty shape for an unextended function instance and predict the slot to > allocate based only on that shape and the add-property program point. > > But bind is sometimes (often? can't tell) used without pre-arguments, so > there's no need for dslots in such cases. > > I'll deal with another js_EnsureReservedSlots call if you land ahead of bug > 558451, but I wanted to write this in case you can see a better way to > store the pre-args. JSFunction::u could be extended, but that's about all I see. And then you have an unnecessary JSFunction in a one-to-one correspondence with the object that's the bound function. But maybe that's the right tradeoff to make. > >+ JSObject *parent = vp[0].toObject().getGlobal(); > > Spec says no [[Scope]], but we do need a parent (global objects have null > parent, any other null-parent objects are prone to be confused for globals > -- not sure where this sits, mrbkap and jorendorff know all, but it's safe > to say we need a non-null parent). But as proposed above, you could store > the target in the bound function object's parent member. I think that would > lead to the correct global. If one calls > otherWindow.Function.prototype.bind.call(myFunction, myThisArg), then > ignoring the requirement that otherWindow be same-origin to myFunction's > global in order for such code to work in the first place, should the > returned bound function object really be parented by otherWindow? > > Again ES5 can't care because (apart from not treating muliple global > objects, except by the [[Scope]] internal property of function objects added > in ES3) it specs no [[Scope]] for bound function objects. > > So in theory parent can be the target function object. Can you try that? Will take a look at it. Although, I think jorendorff/mrbkap probably want to know about this, too, and think through whether it breaks anything or makes anything weird.

(In reply to comment #15) > > >+ return isFunction() && > > >+ getFunctionPrivate()->u.n.native == reinterpret_cast<Native>(CallOrConstructBoundFunction); > > > > Use Valueify, not equivalent reinterpret_cast. > > That's not how Valueify works; I think it's only supposed to interconvert > between an internal js:: type and an external JS type, or js::Value/jsval. > reinterpret_cast is what I want here, right, Luke? Certainly just using > Valueify gives a compile error. See jsvalue.h: js::Native Valueify(JSNative n) { return (js::Native)n; } The idea is for Jsvalify/Valueify to go between anything that differs only by jsval/js::Value, precisely to avoid widespread reinterpret_cast'ing.

One might even say that Jsvalify and Valueify are inverse natrual transformations between the functors Jsval, Value : V --> SM, for V the cartesian-closed category of values and functions over values and SM, the cartesian-closed category for SpiderMonkey ;-)

Yes, certainly. However, this instance is converting a fast-native-signatured function into a Native -- that's not something Valueify is supposed to do, as I understand it.

Slow natives are going away; write something that will break when I merge with the slow-native-removal patch, e.g.: js::Native JsvalifyFastToSlowNative(JSFastNative)

I am uneasy about abusing the parent field in the way you suggest, but I can't think of anything that can go wrong. The spec says to put the target function on the stack, so things like, say scope-chain access will get the target function's scope chain and not the bogo-chain from the bound function. That's the nearest I can think of how this goes bad. We still could do a special JSFunction for bound functions, and drop the bound arguments in there -- but if you can work with this, this is still the easiest fix, so I'm sticking with it for now.

(In reply to comment #20) > Created attachment 467785 [details] [diff] [review] > 1: Patch > > I am uneasy about abusing the parent field in the way you suggest, It was only recently when bug 579957 was fixed that parent became a member to pack things on 32 bits better. But the longer term fix, which we may get soon (fx4 timeframe, this month or next) wants parent optional, so not declared in the base JSObject type. So it is free space for now, and a dedicated member in a better future. > but I can't think of anything that can go wrong. Me neither. Doesn't mean something won't but the space efficiency is still worth it and we'll just have to fix anything uncovered by this that would need fixing for optional parent anyway. > The spec says to put the target function > on the stack, so things like, say scope-chain access will get the target > function's scope chain and not the bogo-chain from the bound function. That's > the nearest I can think of how this goes bad. They should lead to the same global in the same compartment, right? > We still could do a special JSFunction for bound functions, and drop the bound > arguments in there -- but if you can work with this, this is still the easiest > fix, so I'm sticking with it for now. Agreed we want object subtypes, not just interface but implementation types. I think we can do it either by making fslots exist only for some well-know base(r) types (Object, Array), or by putting fslots at the end, always. Then the approach bhackett wants in bug 584917 of pointing dslots at fslots if they are big enough still works, the tracing JIT can still constant fold fslots offsets and avoid the dslots load, etc. -- all provided the class is guarded (shape guard covers class too) so as to compute the right offsetof for the first fslot. /be

(In reply to comment #21) > > The spec says to put the target function > > on the stack, so things like, say scope-chain access will get the target > > function's scope chain and not the bogo-chain from the bound function. > > That's the nearest I can think of how this goes bad. > > They should lead to the same global in the same compartment, right? I disclaim much familiarity with the compartment mechanism, but I believe the wrapping mechanism would/should produce same-compartment-ness. As for same global, I don't think so. If the target's from another window from Function.prototype.bind, you'd get the target's global. If it's from the same window, you'd get Function.prototype.bind's global. But as for how that difference would be observable (in script, without JSAPI use), or would affect execution, I don't know.

(In reply to comment #22) > > They should lead to the same global ... Same global implies same compartment, let's focus on that. > As for same > global, I don't think so. If the target's from another window from > Function.prototype.bind, you'd get the target's global. I was talking about the target function and bound function having the same global as last object on their scope chains. Function.prototype.bind can come from anywhere accessible (assume no security bugs ;-). Its scope is not important, since it is just a handle on the built-in bind functionality that sets the bound function's scope. This could matter for tracing bound functions. It may not be observable except by deoptimization. /be

Comment on attachment 467785 [details] [diff] [review] 1: Patch >+JSObject::initBoundFunction(JSContext *cx, const Value &thisArg, >+ const Value *args, uintN argslen) >+{ >+ JS_ASSERT(isFunction()); >+ JS_ASSERT(getFunctionPrivate()->isBound()); >+ >+ if (!js_EnsureReservedSlots(cx, this, argslen)) >+ return false; Call this below only if argslen != 0. >+ >+ fslots[JSSLOT_BOUND_FUNCTION_THIS] = thisArg; >+ fslots[JSSLOT_BOUND_FUNCTION_ARGS_COUNT].setPrivateUint32(argslen); >+ if (argslen > 0) { Nit: argslen != 0 for unsigned tests. >+JSObject::getBoundFunctionTarget() const >+{ >+ JS_ASSERT(isFunction()); >+ JS_ASSERT(getFunctionPrivate()->isBound()); >+ >+ /* Bound functions abuse |parent| to store their target function. */ >+ return getParent(); It'd be more on-target to say parent abuses JSObject by being non-optional, but maybe the thing to note here is that parent should be optional, and well-typed C++ data members should be supported in subtypes of JSObject that are nevertheless GC-allocated and have flexible fixed slots. Or don't say anything ;-). But I understand why you want to comment this call! The place where parent is set, the call to NewFunction, seems like it needs a comment too >+ JSObject *boundThisObj; >+ if (boundThis.isObjectOrNull()) >+ boundThisObj = boundThis.toObjectOrNull(); >+ else if (!js_ValueToObjectOrNull(cx, boundThis, &boundThisObj)) >+ return false; I just wrote code like this, considering only control-flow, and I'm thinking of revising as we've done elsewhere, to avoid the lumpy appearance and appearance of if(A)B;else return (easy to miss the else-if when scanning just left prefix of each line, reading for indentation and return/break/continue/goto): .. if (boundThis.isObjectOrNull()) { .. boundThisObj = boundThis.toObjectOrNull(); .. } else { .. if (!js_ValueToObjectOrNull(cx, boundThis, &boundThisObj)) .. return false; .. } What do you think? >+ /* Step 4-6, 10-11. */ >+ JSAtom *name = target->isFunction() ? target->getFunctionPrivate()->atom : NULL; >+ JSObject *f = >+ js_NewFunction(cx, NULL, FastNativeToNative(CallOrConstructBoundFunction), length, >+ JSFUN_FAST_NATIVE | JSFUN_FAST_NATIVE_CTOR, target, name); >+ if (!f) >+ return false; ES5 happens to use F (capitalized, italicized) for the metavariable but this is not spec code, and you're not able to italicize. And (my point) house style doesn't want capitalized variable names. So use the canonical name, funobj. >+ inline bool >+ initBoundFunction(JSContext *cx, const js::Value &thisArg, >+ const js::Value *args, uintN argslen); No need to wrap after type just cuz args wrapped, but I see why you did that. Kinda like the NewFunction call layout in the previous cited hunk. r=me with argslen conditioning the js_EnsureReservedSlots call, and style nits picked. /be

(In reply to comment #24) > It'd be more on-target to say parent abuses JSObject by being non-optional, > but maybe the thing to note here is that parent should be optional, and > well-typed C++ data members should be supported in subtypes of JSObject that > are nevertheless GC-allocated and have flexible fixed slots. Or don't say > anything ;-). At the moment, the code flouts the tradition that an object created on behalf of a built-in function from one window has a parent from that window. That's what seems to need commenting, to me. If we commented on parent being vestigial here, we arguably should do the same in many other places which create an object with a mostly-vestigial parent, but I don't think such comments would tell us anything we don't already know. It seems an almost entirely safe assumption that when parent goes away, someone will have to rewrite this code at the very least because the compiler complained. For that reason at the very least it doesn't seem necessary to say anything about parent not being necessary here. > The place where parent is set, the call to NewFunction, seems like it needs > a comment too Good call, added one. > >+ JSObject *boundThisObj; > >+ if (boundThis.isObjectOrNull()) > >+ boundThisObj = boundThis.toObjectOrNull(); > >+ else if (!js_ValueToObjectOrNull(cx, boundThis, &boundThisObj)) > >+ return false; > > I just wrote code like this, considering only control-flow, and I'm thinking > of revising as we've done elsewhere, to avoid the lumpy appearance and > appearance of if(A)B;else return (easy to miss the else-if when scanning > just left prefix of each line, reading for indentation and > return/break/continue/goto): > > .. if (boundThis.isObjectOrNull()) { > .. boundThisObj = boundThis.toObjectOrNull(); > .. } else { > .. if (!js_ValueToObjectOrNull(cx, boundThis, &boundThisObj)) > .. return false; > .. } > > What do you think? I agree. Usually I do end up writing it this way, in fact; it didn't occur to me to do so here, for some reason. I'd push this now (and am sorely tempted to do so regardless, just to get it to interested developers, Narcissus folk among them), but the tests still aren't reviewed yet -- mind doing that so I can push? ;-)

Comment on attachment 465593 [details] [diff] [review] 2: Tests (Stealing review with permission.) Nice tests, good way to learn Function.prototype.bind! >+ assertEq((function() { return this; }).bind(undefined), undefined); Discussed on IRC: assertEq(function() { "use strict"; return this; }).bind(undefined)(), undefined) >+/* >+ * 1. Let Target be the this value. >+ * 2. If IsCallable(Target) is false, throw a TypeError exception. >+ */ >+expectThrowTypeError(function() { bind.call(null); }); ... I guess its not ES5, but it might be future-good to throw in some callable proxies here and in the other tests. >+/* >+ * 3. Let A be a new (possibly empty) internal list of all of the argument >+ * values provided after thisArg (arg1, arg2 etc), in order. >+ * 4. Let F be a new native ECMAScript object . >+ * 5. Set all the internal methods, except for [[Get]], of F as specified in 8.12. >+ * 6. Set the [[Get]] internal property of F as specified in 15.3.5.4. >+ * 7. Set the [[TargetFunction]] internal property of F to Target. >+ * 8. Set the [[BoundThis]] internal property of F to the value of thisArg. >+ * 9. Set the [[BoundArgs]] internal property of F to A. >+ */ >+// throughout Throughout? >+// retch >+var br = Object.create(null, { length: { value: 0 } }); >+try >+{ >+ br = bind.call(/a/g, /a/g, "aaaa"); >+} >+catch (e) { /* nothing */ } >+assertEq(br.length, 0); Is it your intent that either path produces br.length == 0? Future-proofing against the pending removal of the [[Call]] of regexps?

(In reply to comment #26) > I guess its not ES5, but it might be future-good to throw in some callable > proxies here and in the other tests. Added a couple lines to scripted-proxies.js for this -- not attempting to be exhaustive, I don't know proxy semantics well enough to really cover it all. > Throughout? As in, these steps (rather, what modifications they make to the bound function) are tested throughout the remaining tests. > Is it your intent that either path produces br.length == 0? Future-proofing > against the pending removal of the [[Call]] of regexps? Yes. If regular expressions are not callable, then the length will be 0. If regular expressions are callable, then the bound function that is produced must have a length of 0. Either way the implementation might behave (any test not in an extensions/ directory is supposed to pass on any spec-compliant engine, and if it doesn't that's a bug), per spec, we test for conformance and accept.

http://hg.mozilla.org/tracemonkey/rev/5782b1ce64a0 http://hg.mozilla.org/tracemonkey/rev/56931614ba2c

...and a followup for a browser-only problem (global object in browser needs unwrapping before testing for callability): http://hg.mozilla.org/tracemonkey/rev/7680aecfb0c6 I spent the several hours babysitting this into the tree writing documentation, since it needed to be done and I wasn't in the mood/groove to write code for other bugs. What I have is here: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/bind I'm basically satisfied with what I have, although I admit to some uncertainty about the wisdom of including an only-half-working partial implementation. Given that the method (in one form or another) has been widely implemented forever, tho, one more partial implementation seems unlikely to do much harm, and it may well do some good in encouraging people to put the new method to good use where they might have felt constrained by lack of cross-browser support. I'm also not certain about the wisdom of letting the construct-with-varargs trick out of the bag, but I've seen at least one other independent discovery of the trick, and doubtless others will think of it too. Again here, explicitly pointing it out does offer the chance to point out the reasons why the trick isn't advisable, perhaps counterbalancing some of the "evil" in revealing a technique that might dog us performance-wise in the future. Leaving the d-d-n keyword so this can be linked from more places than just the Function reference page...

Hum: https://mootools.lighthouseapp.com/projects/2706/tickets/963-functionprototypebind-broken-in-chrome-beta https://mootools.lighthouseapp.com/projects/2706/tickets/822 http://code.google.com/p/chromium/issues/detail?id=52842 The fallout from this bug may not have landed yet, but if it becomes necessary, we can take it to followup bugs.

\m/ http://hg.mozilla.org/mozilla-central/rev/5782b1ce64a0

Fabian added this to the New in JavaScript 1.8.5 page. Question: Would this be something that would be useful in extensions that want to be able to avoid the complexities of how "this" can vary depending on the context from which their callbacks get executed? If so, I can add mentions of this function in appropriate documents.

Yes, I think so -- basically it's useful calling callbacks that require a particular this, such as with setTimeout, setInterval, addEventListener, and third-party callback-based APIs.

I've added mentions of function.prototype.bind() to these places: https://developer.mozilla.org/en/JavaScript/Reference/Operators/Special_Operators/this_Operator https://developer.mozilla.org/en/DOM/element.addEventListener https://developer.mozilla.org/en/DOM/window.setTimeout

(In reply to comment #34) > https://developer.mozilla.org/en/DOM/element.addEventListener I'm not sure if addEventListener should mention bind: o = {}; function f() 0; f.bind(o) == f.bind(o) /* false */ You can't remove the listener you just added without some extra tracking: listeners[eventName] = f.bind(o); addEventListener(eventName, listeners[eventName], false); ... function onUnload() { for (let [name, listener] in Iterator(listeners)) removeEventListeners(name, listener, false);

You could add caveats. It's intended for the case of an object literal with an embedded function that you then want to pass in -- bound -- as the event listener. That seems like a reasonably common use in moderately complex code these days. But fundamentally, yes, a bound function != its target function, and addEventListener requires you to keep a reference to the listener around to remove it. Maybe it's a footgun API, but I don't think that's the fault of bound functions, and I do think it's still probably worth noting for addEventListener.