848062 - Arrow functions should inherit 'this' lexically

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Ordinary functions bind the this-value provided by the caller to 'this'.

Arrow functions do not (following fat-arrow function in CoffeeScript). Instead 'this' in an arrow function gets the value from the enclosing scope. CoffeeScript explains this as "bind [the new function] to the current value of 'this', right on the spot". The way I think of it is, arrow functions *do not have 'this'* but rather you can refer to the 'this' provided by an enclosing scope, lexically. Same behavior either way.

Implementation plan:

Step 1 - When 'this' appears in an arrow function or generator-expression f, nested in a function g which has 'this', then g gets special prelude code to store the value of 'this' in an aliased slot, and f uses JSOP_GETALIASEDVAR to access it.

Step 2 - When 'this' appears in an arrow function or generator-expression f and no enclosing function has 'this', then the behavior of JSOP_THIS needs to change to get the correct object. You'd think this would be super stupid easy, just statically use the global object (outerized) instead of the StackFrame's this slot.

But no, the behavior of js::Execute, in selecting a 'this' object, is too dynamic; 'this' might be the global object or something else entirely. According to luke, we can walk the scope chain looking for an object that isVarObj(). Lame, but it's only needed in this one case.

Maybe we can bake the scope object into the JSScript instead. I don't know what that would break. Curious.

Comment 1

[Jason Orendorff [:jorendorff]]

Well, that was a stupid plan. It would have put hacks all over the engine. Even the debugger would have had to know about the plan.

Instead I'm just going to bind the arrow function at creation time using js_fun_bind.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1 - not working (Ion assertion)

I've tried to make this work in Ion, but I can't figure out how to pass the current thisValue from Ion code to C++. It's odd. The code for LCallDirectEval does exactly that, and I copied it all, from IonBuilder::jsop_eval() on down, but my copied code flunks a type-checking assertion. See the patch.

  $ gdb --args ./d-objdir/js --ion-eager -e 'function f() { return a => b; } f();'
  ...
  Assertion failure: mir->type() == MIRType_Value, at ion/x64/Lowering-x64.cpp:20
  20	    JS_ASSERT(mir->type() == MIRType_Value);
  (gdb) p mir->type()
  $1 = js::ion::MIRType_Object

Is it possible this direct eval code I'm copying is buggy, and we've only not noticed it because it never runs? It definitely can't run in --ion-eager mode; I'm not sure how to trigger it.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2 - works

This makes Ion just punt on arrow functions.

This could be better in several ways:
- make BC cope with it correctly
- only bind arrow functions that might use 'this'
- make Ion calls through bound functions fast

I can file follow-up bugs on anything anyone really wants to work on...

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 725866 [details] [diff] [review]
v2 - works

Review of attachment 725866 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ +2788,5 @@
> +        thisval = regs.fp()->thisValue();
> +        obj = js_fun_bind(cx, fun, thisval, NULL, 0);
> +        if (!obj)
> +            goto error;
> +    }

stubs::Lambda, which is used by JM (methodjit/StubCalls.cpp) also needs this logic; it is a copy paste of this interpreter case.  Also, the Ion stub js::Lambda (jsinterp.cpp) should at least be asserting if it sees an arrow function.

What I think should happen is that this logic goes in js::Lambda, which is then called by the interpreter case and stubs::Lambda.  It looks like ComputeThis will assert if the top frame begins an Ion activation.

@@ +3483,5 @@
> +    RootedObject clone(cx, CloneFunctionObjectIfNotSingleton(cx, fun, parent));
> +    if (!clone)
> +        return NULL;
> +    return js_fun_bind(cx, clone, thisv, NULL, 0);
> +}

This function is unused.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1255520f471b
https://hg.mozilla.org/mozilla-central/rev/ce50afadce07