Documentation incorrect about eslint-no-inferred-method-name

[ericelliott]

The documentation isn't worded correctly, but I believe the rule behaves as expected.

I intend to write a blog post that describes the issue more accurately, soon.

Thanks for pushing back RE: docs. We do need to clean this up.

👍

[johnstonbl01]

Hey Kyle. Before I address the issue, I just want to say how bizarre and humbling it is to have two of my JS heroes comment on one of my repos. I began learning JavaScript only 6 months ago, so I'm sure you can imagine how crazy this feels!

Back to the issue at hand - I will definitely review this tomorrow and the issue you linked in Babel to update the examples and introduction to be accurate. Thanks so much for taking the time to comment, and I apologize for the poorly worded documentation.

[ericelliott]

@getify It's easy to make this mistake, since typical function expressions can use the specified function name inside the function without a lexical binding. For example:

const bar = function foo () { console.log(typeof foo); };
bar(); // logs: 'function'

foo(); // no lexical 'foo' exists:
// ReferenceError: foo is not defined

@johnstonbl01 I've written up a gist that more accurately describes the issue using the repeat() example:

https://gist.github.com/ericelliott/a79a54e729f957b5debe

[getify]

👍

For the record:

1. I have not tried the rule myself, so I was only commenting on the documentation itself. If the code is correct and the documentation is wrong, my apologies for being too alarmist.
2. I am glad such a rule will exist, as it would seem to be helpful.
3. Apologies for my initially too-harsh tone. Overreaction on my part.
4. It should be a warning at most (and not an error), because this is valid if I wanted to do it, though you may very well want the linter to wag its finger at you for doing so:

function foo(x) {
   return obj.foo(x);
}

var obj = {
   foo(x) {
     if (x < 10) return foo(x * 2); // `foo` here comes from the function, not the concise method
     return x;
   }
};

obj.foo(4);  // 16

[johnstonbl01]

Hey guys. Below is the change I'm proposing to the docs. I spent some time this morning re-reading your statements here, and reviewing the Babel issue again. I think I'm on the same page now, and I apologize for not completely getting it. Indeed as Kyle mentioned, I thought the original issue was with the name inferrence and not necesarily the lexical name binding. That was my mistake. Luckily, on some level I did understand the issue with the code and I believe that the rule logic is sound. @ericelliott - let me know when you've completed that blog post, and I'll link to it as well in the overview.

Overview

In ES6, compact methods and unnamed function expression assignments within object literals do not create a lexical identification (name) binding that corresponds to the function name identifier for recursion or event binding. The compact method syntax will not be an appropriate option for these types of solutions, and a named function expression should be used instead. This custom ESLint rule will identify instances where a function name is being called and a lexical identifier is unavailable within a compact object literal.

More information on this can be found:

• eslint/eslint#2454
• babel/babel#1367
• ES6 Concise Methods: Lexical or Not?

Note - Tests are provided in the repo, but not necessary for installation or use of the rule.

Example Changes

Example 1

In the example below, 1 error is generated because foo is being called recursively when there is no lexical name binding for the foo function using the concise object literal syntax. See links above for in-depth discussion on this behavior.

const bar = {
  name: 'Bar',
  types: [
    { f: 'function' },
    { n: 'number' }
  ],
  foo (f, n) {   // this function will not have any lexical binding for recursive calls
    if (typeof f === 'function') {
      f();
    } else {
      throw new Error('foo: A Function is required.');
    }

    n -= 1;
    if (!n) {
      return undefined;
    }

    return foo(f, n);   // error on this line
  }
};

bar.foo(() => {console.log('baz');}, 3);
//baz
//ReferenceError: foo is not defined

Example 2

In this example, no errors are generated because the function expression explicitly defines a lexical identifier.

const bar = {
  name: 'Bar',
  types: [
    { f: 'function' },
    { n: 'number' }
  ],
  foo: function foo (f, n) {   // this function explicitly defines a lexical name for the method
    if (typeof f === 'function') {
      f();
    } else {
      throw new Error('foo: A Function is required.');
    }

    n -= 1;
    if (!n) {
      return undefined;
    }

    return foo(f, n);
  }
};

bar.foo(() => {console.log('baz');}, 3);
//baz
//baz
//baz

Updated Error Message

I propose changing the current error message:

{func} has no defined method name. Use syntax - foo: function foo {...}.

to

{func} has no lexical name binding. Use syntax - foo: function foo {...}.

If you think it should be something more descriptive, please let me know.

Error vs Warning

I'm happy to change the docs to recommend setting the value of the rule to 1 (for warning) rather than 2 (for error). However, the no-undef rule that's in ESLint by default will continue to flag these instances as an error. For consistency, I assumed that this rule should also show an error. If that assumption is incorrect and we think that it's better as a warning, I'm happy to change it.

Lastly - thanks to both of you for the help and guidance. It's really been a great learning experience.

@getify - No worries on the original tone. I knew where you were coming from. 👍

[getify]

Looks great to me!

I'm happy to change the docs to recommend setting the value of the rule to 1 (for warning) rather than 2 (for error)

I don't know if this is possible, but I'd suggest:

• It's an error if there's no lexical identifier foo in any of the lexical scopes. Can you definitively tell that the lexical identifier is completely missing?

  Example:

  var o = { foo(){ foo() } };   // should be an error!

• Otherwise, it's a warning if you use a lexical name that matches the concise method identifier, but the same lexical name already exists in the scope. Like in my previous example, you might want to do it... but I bet more times than not, it's a mistake. That's why I think it should be a warning instead of an error, in that specific case.

  Example:

  function foo() {}
  var o = { foo(){ foo() } };  // should be a warning!

[RReverser]

{func} has no lexical name binding. Use syntax - foo: function foo {...}.

One more proposal - why suggesting to completely change the way method is defined instead of showing warning/error on call itself and suggesting to change return foo(f, n) to return this.foo(f, n)? This makes more sense, since you're also passing context which in most cases is important for object methods, and with named function expression you're losing it.

[ericelliott]

@RReverser return this.foo() won't work if this changes, as is the case with .call() or .apply().

[RReverser]

@ericelliott It's much less likely to be mistake than when someone doesn't pass this at all by calling object method like regular named function. this is important, and ability to substitute different context by special APIs is not really an argument not to pass it at all.

[ericelliott]

@RReverser The argument here is which form is less likely to lead to code that doesn't work as expected. this is more error-prone than the alternative in this case.

[RReverser]

@ericelliott Do you suggest to also provide ESLint rule that restricts using this just because it can be error-prone?

[johnstonbl01]

Guys - let's keep it civil and on-topic, please.

@RReverser Please keep in mind that this is a custom rule, and there's no plan for this to be integrated into the standard ESLint rules. If you don't agree with the way it is written, then it's possible to create your own to use or not use this one at all. That's the great thing about ESLint, in my opinion.

@getify I think that should be possible, but I need to dig in and have a look. I'll push the README changes later today and get back to you on the warning / error functionality.

[RReverser]

@johnstonbl01 Of course I do understand that - I'm just pointing to other possible mistakes that this linting rule might accidentally push other devs towards (like not preserving context in object method when calling it recursively). Sorry if polluting the thread, I made my point so will stop here :)

[johnstonbl01]

Thanks, @RReverser . :)

[getify]

Regarding the this in the warning/error message... i'd say that only makes sense if this shows up anywhere in the function already, such as in reference to some other property/method. If so, then clearly it's a this aware method and the call in question should very likely be this.foo().

But if there's no other this reference in the function, inferring that it's a this-aware method (instead of just a function on an object namespace, as is the case with practically all modules) is too far a jump IMO. That sort of assumption is exactly the kind of thing that makes me disable such rules entirely in my linters.

[ericelliott]

I don't think the rule needs any logic changes.

We can adequately handle either case by changing the rule text:

{func} has no name binding. Use `foo: function foo {...}` or call with `this.foo`

For the sake of simplicity, let's assume the reader knows enough JS to figure out the rest.

[RReverser]

@ericelliott Sounds good to me!

[johnstonbl01]

Ok, guys. Here are the updates:

• Documentation overview updated to provide accurate description of the issue
• Documentation examples replaced with those mentioned above
• Error message updated to {func} has no lexical name binding. Use syntax 'foo: function foo {...}' or call with 'this.foo()'
• Tests updated to reflect new error message
• Updated a function name in rule code to be more descriptive
• Confirmed all tests pass after changes

Closing this issue for now - thanks for the input, everyone!