Better infer the name of constant-like function expressions

[ericelliott]

See this gist for details.

The un-optimized function body gets moved as-is into a parameter that's passed into a wrapper function so that the function can be wrapped with a named function declaration...

I wonder if other ES6 things are broken by name wrappers, too?

Expected:

Tail calls should still be optimized.

[RReverser]

Function assigned to variable, property or somewhere else is not kind of things that can be safely optimized since those can be easily, incl. conditionally, reassigned in random places. Tracking all those places would be almost equal to interpreting entire codebase which would be painfully slow, so this is left as-is. Power of transpilers is kinda limited :)

[sebmck]

This is intentional. You'd need to deopt on reassignments to the binding anyway which would confuse the shit out of people when it doesn't work. This doesn't really make it smarter, it just makes it less dumb. Which is fine but the cost of additional magic isn't worth it.

[ericelliott]

You're saying you can't optimize the function that gets passed into the named wrapper? That would not rely on the outer assignment... would it?

[sebmck]

No, it could be optimised but it will lead people to believe that Babel is smarter than it is.

For example:

var a = b;

function b() {
  return a();
}

Could be handled and detected as a recursive tail call but then people will think it's smart as shit and start throwing dynamic code at it and expect it to work.

[ericelliott]

hmm.. this case is also broken for const -- which can't be reassigned.

[sebmck]

Not broken :) Just not implemented. Up for a PR?

[ericelliott]

it will lead people to believe that Babel is smarter than it is.

How could it possibly be more confusing than silently failing to optimize the tail call, resulting in exploding stack frames? =)

[ericelliott]

I know that transpilers have their limits, but spec not implemented = broken from the user's perspective. ;)

[ericelliott]

I'm not sure I have time for a PR, but if you leave it to me, I'll optimize the function that gets passed into the wrapper, which you've already said you don't want.

If I'm gonna fix something, I'll fix it right. ;)

[sebmck]

If I'm gonna fix something, I'll fix it right. ;)

Why are you being so coarse?

[ericelliott]

Sorry, I meant it in a playful tone. I'm not trying to be insulting.

I find the current behavior very surprising, and I would consider the conditional reassignment to be an edge-case and an anti-pattern, anyway. =)

In other words, if the decision were up to me, I'd succeed for the common case and fail for the edge case (and mention the limitation in the docs).

[sebmck]

No worries. “Tail call recursion optimisation only works for named functions" is easier to explain than “tail call recursion optimisation only works on named functions and on locally bound constant inferred named functions."

On Tue, Apr 28, 2015 at 12:12 PM, Eric Elliott notifications@github.com
wrote:

Sorry, I meant it in a playful tone. I'm not trying to be insulting.
I find the current behavior very surprising, and I would consider the conditional reassignment to be an edge-case and an anti-pattern, anyway. =)

In other words, if the decision were up to me, I'd succeed for the common case and fail for the edge case (and mention the limitation in the docs).

Reply to this email directly or view it on GitHub:
#1367 (comment)

[ericelliott]

Fair enough. I guess if I don't have time to commit a sensible PR, I don't have time to complain, either. ;)

Cheers!

[sebmck]

Actually, this should automatically work if the spec.functionName transformer checks if the left side of a variable declaration is constant-like (ie. never reassigned) and forces the id on the function expression. Reopening to track it.

[sebmck]

The spec.functionName transformer is now aware of "constant-like" function variable declarations. I've also made it rename variables instead of doing the bullshit function wrapper.

[sebmck]

Released as of 5.2.0, thanks!

[ericelliott]

Yay! That's a great start, but the method literal form still does the bullshit function wrapper, which falls on its face:

> babel-node --version
5.2.17

const repeater = {
    name: "Repeater",
    types: [
      { f: 'function' },
      { n: 'number' }
    ],
    repeat: function (f, n) {
        if (typeof f === 'function') {
          f();
        } else {
          throw new Error('repeat: A Function is required.');
        }
        if (!n) {
          return;
        }

        n -= 1;
        return repeat(f, n);
    }
};

repeater.repeat(function repeat() {console.log('foo');}, 10);

Compiles to:

'use strict';

var repeater = {
  name: 'Repeater',
  types: [{ f: 'function' }, { n: 'number' }],
  repeat: (function (_repeat) {
    function repeat(_x, _x2) {
      return _repeat.apply(this, arguments);
    }

    repeat.toString = function () {
      return _repeat.toString();
    };

    return repeat;
  })(function (f, n) {
    if (typeof f === 'function') {
      f();
    } else {
      throw new Error('repeat: A Function is required.');
    }
    if (!n) {
      return;
    }

    n -= 1;
    return repeat(f, n);
  })
};

repeater.repeat(function repeat() {
  console.log('foo');
}, 10);

Output:

foo
ReferenceError: repeat is not defined

[sebmck]

@ericelliott Nope, that's correct behaviour. Object literal concise methods don't create a lexical binding like normal named function expressions.

The function wrapper is only used in one scenario, and that's where you reference a binding that isn't actually bound to the current file, this means it's exposed as a global and you need to explicitly reference it by the same identifier for it to work since you obviously can't rename it.

[ericelliott]

Object literal concise methods don't create a lexical binding like normal named function expressions.

Wait... WHAT? Could you link me to where it says that in the spec, 'cause I must be reading it wrong:

"4. Let scope be the running execution context’s LexicalEnvironment." - https://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-definemethod

Are you trying to tell me that this is correct behavior, too? (Note that it's NOT a concise method so presumably, your response does not apply):

const repeater = {
    name: "Repeater",
    types: [
      { f: 'function' },
      { n: 'number' }
    ],
    repeat: function (f, n) {
        if (typeof f === 'function') {
          f();
        } else {
          throw new Error('repeat: A Function is required.');
        }
        if (!n) {
          return;
        }

        n -= 1;
        return repeat(f, n);
    }
};

repeater.repeat(function repeat() {console.log('foo');}, 10);

Output:

foo
ReferenceError: repeat is not defined

I'm really confused about how you could think this is spec-compliant behavior. It seems pretty clear to me that it's a bug.

[RReverser]

Are you trying to tell me that this is correct behavior, too?

Actually yes - why do you think repeat has something to refer to in that context?

[sebmck]

@ericelliott

Wait... WHAT? Could you link me to where it says that in the spec, 'cause I must be reading it wrong:

@getify did an entire article on it which was spawned from the discussion about why Babel inferred function names in the first place.

Are you trying to tell me that this is correct behavior, too? (Note that it's NOT a concise method so presumably, your response does not apply):

Where do you think repeat is coming from? You have not defined it.

[sebmck]

@ericelliott Unsure why you referenced step 4, it says nothing about the PropertyName creating a local binding.

[ericelliott]

Actually yes - why do you think repeat has something to refer to in that context?

In ES5 it wouldn't. But in ES6, for all function expressions, regardless of context, whether it's let, const, var, or method: this:

myFunc = function (f, n) {

is equivalent to this:

myFunc = function myFunc () {

And if you disagree with that assertion, instead of just saying, "no, you're wrong", please quote and link to the text in the spec that says different.

@ericelliott Unsure why you referenced step 4, it says nothing about the PropertyName creating a local binding.

I referenced step 4 because concise methods should conform to Runtime Semantics: DefineMethod.

[RReverser]

is equivalent to this

It's not. Why do you think so? It would be a backward-incompatible change which could break some code.

And if you disagree with that assertion, instead of just saying, "no, you're wrong", please quote and link to the text in the spec that says different.

If you say Babel is not following the spec, please quote the part of spec which proves that.

[ericelliott]

Function name inference is just that, name inference. It just not create bindings unless you explicitly name them.

Isn't this exactly what I'm trying to say? You are the one who brought up bindings.

You don't need a binding to infer a function name, right?

[sebmck]

No, you're the one saying that repeat in this code snippet refers to the function expression. Please see the arrows.

const repeater = {
    name: "Repeater",
    types: [
      { f: 'function' },
      { n: 'number' }
    ],
    repeat:
    function (f, n) { <--- This function expression is **not** named
        if (typeof f === 'function') {
          f();
        } else {
          throw new Error('repeat: A Function is required.');
        }
        if (!n) {
          return;
        }

        n -= 1;
        return repeat(f, n); <-- `repeat` is not defined.
    }
};

repeater.repeat(function repeat() {console.log('foo');}, 10);

[sebmck]

Here are some more examples:

var foo = "foo";
var bar = "bar";

var obj = {
  foo: function () {
    return foo;
  },

  bar() {
    return bar;
  }
};

console.log(obj.foo()); // result is "foo", it does not refer to the function
console.log(obj.bar()); // result is "bar", it does not refer to the function

[ericelliott]

Isn't it spelled out that the method name is inferred by the spec here, or am I missing something?

PropertyDefinition : PropertyName : AssignmentExpression
Let propKey be the result of evaluating PropertyName.
ReturnIfAbrupt(propKey).
Let exprValueRef be the result of evaluating AssignmentExpression.
Let propValue be GetValue(exprValueRef).
ReturnIfAbrupt(propValue).
If IsAnonymousFunctionDefinition(AssignmentExpression) is true, then
Let hasNameProperty be HasOwnProperty(propValue, "name").
ReturnIfAbrupt(hasNameProperty).
If hasNameProperty is false, perform SetFunctionName(propValue, propKey).
Assert: enumerable is true.
Return CreateDataPropertyOrThrow(object, propKey, propValue).

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-setfunctionname

[RReverser]

No, it just tells to set internal name property of function to the same string as property key is. Nothing about bindings, so

But in ES6, for all function expressions, regardless of context, whether it's let, const, var, or method: this:

myFunc = function (f, n) {

is equivalent to this:

myFunc = function myFunc () {

is still untrue.

[ericelliott]

I think we can all agree that what the spec definitely does not say is that you should create a "bullshit function wrapper" (not my words) around function expressions as Babel currently does for methods:

var repeater = {
  name: 'Repeater',
  types: [{ f: 'function' }, { n: 'number' }],
  repeat: (function (_repeat) { // <--- WTF IS THIS FOR? vvv
    function repeat(_x, _x2) {
      return _repeat.apply(this, arguments);
    }

    repeat.toString = function () {
      return _repeat.toString();
    };

    return repeat;
  })(function (f, n) { // <--- WTF IS THIS FOR? ^^^
    if (typeof f === 'function') {
      f();
    } else {
      throw new Error('repeat: A Function is required.');
    }
    if (!n) {
      return;
    }

    n -= 1;
    return repeat(f, n);
  })
};

[RReverser]

a bullshit function wrapper

@ericelliott Let's be nice, ok? Making statements in offensive manner doesn't make them any better or any more true.

[sebmck]

You are missing something. SetFunctionName does not create a local binding with that inferred function name. It sets the function name, that's it. It is not the same as a named function expression.

Here are the steps that initialize the local binding for named function expressions:

14.1.20 Runtime Semantics: Evaluation

1. FunctionExpression : function BindingIdentifier ( FormalParameters ) { FunctionBody }
2. If the function code for FunctionExpression is strict mode code, let strict be true. Otherwise let strict be false.
3. Let runningContext be the running execution context’s Lexical Environment.
4. Let funcEnv be NewDeclarativeEnvironment(runningContext ).
5. Let envRec be funcEnv’s EnvironmentRecord.
6. Let name be StringValue of BindingIdentifier.
7. Perform envRec.CreateImmutableBinding(name).
8. Let closure be FunctionCreate(Normal, FormalParameters, FunctionBody, funcEnv, strict).
9. Perform MakeConstructor(closure).
10. Perform SetFunctionName(closure, name).
11. Perform envRec.InitializeBinding(name, closure).
12. Return NormalCompletion(closure).

Note steps 7 and 11 which don't exist for inferred function names.

I think we can all agree that what the spec definitely does not say is that you should create a bullshit function wrapper around function expressions as Babel currently does for methods:

Dude, drop the attitude. You're being extremely foul and nasty and are making this an extremely hostile discussion.

[ericelliott]

I'm sorry, I'm not trying to be hostile, I'm just trying to understand how you could possibly think that method assignment function expressions should be treated any differently than any other function expression.

RE: "bullshit function wrapper" <--- I am not the one who named it that.

RE: "definitely does not say" <--- this isn't attitude, AFAIK, it's a simple statement of fact.

Back to business:

"Let name be StringValue of BindingIdentifier."

In the context of an object literal, is BindingIdentifier not the same thing as the left hand identifier in the literal? If you read it differently, could you please explain why you read it differently? again, referring to specific lines in the spec that I can read in order to try to get on the same page with you?

[ericelliott]

Please forgive my lack of communication skills and walk me through the logic that leads you to believe this:

SetFunctionName does not create a local binding with that inferred function name.

...because we're reading the same specification, and I'm coming to a very different understanding of it.

[sebmck]

@ericelliott

RE: "bullshit function wrapper" <--- I am not the one who named it that.

You seem to have gotten offended because I called it a "bullshit function wrapper". Not sure why.

RE: "definitely does not say" <--- this isn't attitude, AFAIK, it's a simple statement of fact.

Not going to have this philosophical argument about programming semantics and layers of abstraction and what justifies following semantics and what doesn't.

In the context of an object literal, is BindingIdentifier not the same thing as the left hand identifier in the literal? If you read it differently, could you please explain why you read it differently? again, referring to specific lines in the spec that I can read in order to try to get on the same page with you?

BindingIdentifier is an identifier that creates a binding. The key of a property and name of a concise method are not binding identifiers, they do not create any kind of bindings.

The steps I quoted are purely for named function expressions, their semantics do not matter since we are not talking about named function expressions. I only quoted them because you're confusing the semantics of named function expressions with that of function name inference.

Function name inference in ES6 does not create bindings. I assume you obviously know what a binding is but if not it's something that creates a "declaration".

var foo = function () {};

foo is a BindingIdentifier since it creates a binding that we can reference with the same name, in this case, it's foo.

Your ultimate mistake is assuming that foo in the following example creates a binding of any kind:

var bar = {
  foo: function () {}
};

It does not. foo will not refer to that function expression anywhere in that file. It's true that bar.foo.name will be "foo". That is completely irrelevant to bindings.

If you bring up your previous comment of:

You are the one who brought up bindings.

No, you were when you implied that function name inference created bindings, it does not. Plain and simple. You keep on asking us to quote steps when you can't quote something that doesn't exist :) Feel free to quote the steps that lead you to the impression that you have and I'll gladly point you in the right direction. To go back over the methods that you have quoted so far:

PropertyDefinition : PropertyName : AssignmentExpression

No mention of creating a binding.

4. Let scope be the running execution context’s LexicalEnvironment.

This has nothing to do with bindings and is simply setting the scope to the current environment record which is completely irrelevant to magically creating bindings based on function name inference.

[ericelliott]

I think we're getting lost in irrelevant details. Here's my logic. Please point out my errors.

I assert:

1. prop names are set by concise methods -- this does not depend on lexical binding
2. SetFunctionName is called for concise methods, passing in the prop name as the value
3. SetFunctionName does lead to local binding at method evaluation time

Evidence: prop names are set by concise methods, SetFunctionName is called with prop name as value:

Runtime Semantics: PropertyDefinitionEvaluation

With parameters object and enumerable.

See also: 12.2.5.9, 14.4.13, B.3.1

MethodDefinition : PropertyName ( StrictFormalParameters ) { FunctionBody }
Let methodDef be DefineMethod of MethodDefinition with argument object.
ReturnIfAbrupt(methodDef).
Perform SetFunctionName(methodDef.[[closure]], methodDef.[[key]]). <-- prop name passed as value
Let desc be the Property Descriptor{[[Value]]: methodDef.[[closure]], [[Writable]]: true, [[Enumerable]]: enumerable, [[Configurable]]: true}.
Return DefinePropertyOrThrow(object, methodDef.[[key]], desc).

Evidence: SetFunctionName does lead to local binding at method evaluation time

This works in every major browser & Node:

var a = {
  bar: function foo() {
    console.log(typeof foo);
  }
};

Output:

> a.bar();
function

But Babel does this:

const repeater = {
    name: "Repeater",
    types: [
      { f: 'function' },
      { n: 'number' }
    ],
    repeat (f, n) {
        if (typeof f === 'function') {
          f();
        } else {
          throw new Error('repeat: A Function is required.');
        }
        if (!n) {
          return;
        }

        n -= 1;
        return repeat(f, n);
    }
};

repeater.repeat(function () {console.log('foo');}, 10);

foo
ReferenceError: repeat is not defined

Again, I ask, what am I missing?

Last time, you answered:

SetFunctionName does not create a local binding with that inferred function name

This doesn't explain anything to me, because as I read the spec, concise methods don't have to infer anything. The prop name is the function name, and the function name is available in the function's environment.

[sebmck]

1. SetFunctionName does lead to local binding at method evaluation time

This is where you're wrong.

9.2.11 SetFunctionName (F, name, prefix)

1. Assert: F is an extensible object that does not have a name own property.
2. Assert: Type(name) is either Symbol or String.
3. Assert: If prefix was passed then Type(prefix) is String.
4. If Type(name) is Symbol, then
   • Let description be name’s [[Description]] value.
   • b. If description is undefined, let name be the empty String.
   • c. Else, let name be the concatenation of "[", description, and "]".
5. If prefix was passed, then
   • Let name be the concatenation of prefix, code unit 0x0020 (SPACE), and name.
6. Return DefinePropertyOrThrow(F, "name", PropertyDescriptor{[[Value]]: name, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true}).
7. Assert: the result is never an abrupt completion.

All SetFunctionName does is literally set the name property on the function.

As I mentioned before in this comment, the reason that a foo binding exists in the following:

var a = {
  bar: function foo() {
    console.log(typeof foo);
  }
};

is because of steps 7 and 11, not because of step 10 in:

14.1.20 Runtime Semantics: Evaluation

1. FunctionExpression : function BindingIdentifier ( FormalParameters ) { FunctionBody }
2. If the function code for FunctionExpression is strict mode code, let strict be true. Otherwise let strict be false.
3. Let runningContext be the running execution context’s Lexical Environment.
4. Let funcEnv be NewDeclarativeEnvironment(runningContext ).
5. Let envRec be funcEnv’s EnvironmentRecord.
6. Let name be StringValue of BindingIdentifier.
7. Perform envRec.CreateImmutableBinding(name).
8. Let closure be FunctionCreate(Normal, FormalParameters, FunctionBody, funcEnv, strict).
9. Perform MakeConstructor(closure).
10. Perform SetFunctionName(closure, name).
11. Perform envRec.InitializeBinding(name, closure).
12. Return NormalCompletion(closure).

[ericelliott]

So your argument is that this evaluation signature:

function BindingIdentifier ( FormalParameters ) { FunctionBody }

... does not apply at all because foo in:

const a = {
  foo() {
  }
}

... is not interpreted as a BindingIdentifier... and the value of the name property in a.foo.name is not made available as an identifier at a.foo() runtime evaluation.

Hence: foo () {} in the object literal is not equivalent to foo: function foo() {} in every important ES5 implementation. Even though foo.name evaluates to foo.

We're on the same page so far?

I assert that in practice in all current ES5 engines, a function's name serves two purposes:

1. To provide an identifier for call stack output.
2. To provide a handle to use for reference / recursion inside the function at runtime.

And in ES6, the spirit of the fn.name property is just a way to introspect the value of that name.

And that the spirit of the concise method is to provide the same semantics for methods, complete with a name that follows the cowpath semantics.

I also assert that if that's not how you interpret the spec, you're ignoring the context of the origins of the function name property. You are attempting to follow the letter of the spec, but not the spirit of the spec, and I suspect other implementors will interpret it differently from you, which will cause further confusion.

I also assert that ignoring that context will astonish users of Babel.

I've run out of time today to dig around deeper inside the guts of the spec to discover whether or not I've missed anything that would help my argument, but rest assured, I will return better armed to make my case -- and if we decide that the spec is indeed written as you suggest it is, I'll make my best effort to get the spec changed. I know it's probably far too late for ES6, but maybe we can fix it for ES7 so user's don't get frustrated as I have been, and several others I know of (including Kyle Simpson who authored an O'Reilly book on the subject of the ES6 spec).

Thank you for your patience.

[monsanto]

Your interpretation of the spec is simply wrong, I'm not sure what you are talking about with respect to "context of the origins of the name property". You can try this in latest Chrome or iojs 2.0 if you want, as shorthand method syntax has moved out of V8 staging:

> var obj = { foo() { console.log(foo) } }
undefined
> var obj2 = { foo: function foo() { console.log(foo) } }
undefined
> obj.foo()
ReferenceError: foo is not defined
    at Object.foo (repl:1:33)
    at repl:1:5
> obj2.foo()
[Function: foo]

[ericelliott]

Well damn. I'm still quite confident that this will be a source of bugs and frustration for developers who are trying to build apps with it. I'm not the first to notice this issue, and I won't be the last.

It may be time to move this to ES Discuss so we can think about sorting it out in ES7. Thanks for putting up with me. I really appreciate it.

[jayphelps]

I've been bitten by this but when I figured out why I said, oh well that actually makes sense.

[ericelliott]

Yeah, makes sense after my hand-held tour through the ES6 spec, at least. );

But I believe this post is still relevant:

#1367 (comment)

Can somebody explain to me why we need that function wrapper?

[loganfsmyth]

@ericelliott This blog post does a decent job of explaining this whole issue, including your spec complaints and addressing your question: http://blog.getify.com/es6-concise-methods-lexical-or-not/

By adding the infered function name to the method function expression, it adds the name binding to the function (as established above). But that could break the code since it might be relying on an outer definition of that particular identifier. The wrapper function avoids that issue by wrapping the function in a named function that is defined in a separate scope. That way the function still has the proper outer scoping, which also having a name.

[ericelliott]

@loganfsmyth

Thanks. =) I guess I didn't see much point in the name if it couldn't be used for recursion, but hey, named callstacks are cool, too.

The problem is, I always want the name inside the function, just to avoid the confusion of finding that it's inconsistently missing in the concise method, even though the name is available in every other form that looks like a function expression. I might just disable concise methods in my ESLint config for this reason.

Which totally sucks, 'cause I love the idea of them. );

[loganfsmyth]

To me, method syntax implies that the function is a really attached to one particular object in a more OO style, so I'd expect a recursive call to be done via this.repeater() (which disables TCO).

What are the "every other form" you mean?

[ericelliott]

To me, method syntax implies that the function is a really attached to one particular object in a more OO style, so I'd expect a recursive call to be done via this.repeater() (which disables TCO).

Yeah, that "disables TCO" is a bummer. );

const repeat = function (f, n) {
  if (typeof f === 'function') {
    f();
  } else {
    throw new Error('repeat: A Function is required.');
  }
  if (!n) {
    return;
  }

  n -= 1;
  return repeat(f, n);
};

repeat(function () {console.log('foo');}, 10); // Works great. Yay!

The same holds for let, var... in these forms, const repeat = function (f, n) { is functionally equivalent to ES5's var repeat = function repeat (f, n) {, which led me to assume that the concise method syntax would behave the same way, where repeat () { would be equivalent to repeat: function repeat() {.

And that's the assumption that led me to read the spec wrong. I was looking at function BindingIdentifier ( FormalParameters ) { FunctionBody } and making the incorrect assumption that the logic there applied to concise methods. );

sigh

I will admit defeat, and simply add concise methods to my "bad parts" collection. Or maybe I can come up with a crazy lint rule that will look for references to the method name inside the method and flag it as an error. That way we could catch it at build time and still use concise methods for everything else. =)

[monsanto]

Or maybe I can come up with a crazy lint rule that will look for references to the method name inside the method and flag it as an error.

use the optional transformer validation.undeclaredVariableCheck

[ericelliott]

Thanks! That catches the error, but doesn't explain why it's undefined or how to fix it. I still think it's worth having a dedicated rule. =)

[ericelliott]

Now there's a custom ESLint rule for this issue. 👍

[getify]

FYI: that eslint rule appears to completely misunderstand the issue with lexical name bindings and inferences. I would avoid it until the misunderstanding and incorrect logic are corrected.