no-unnecessary-callback-wrapper: Allow x => x(x)

[andy-hanson]

PR checklist

• Addresses an existing issue: #0000
• New feature, bugfix, or enhancement
  • Includes tests
• Documentation update

Overview of change:

#2510 is too simple; there are cases of non-identifier calls that are still an unnecessary callback, like x => f()(x). We can solve #2509 by looking for uses of the parameters in the called expression, so that x => x(x) is still not a failure.

CHANGELOG.md entry:

[enhancement] no-unnecessary-callback-wrapper: Allow x => x(x)

[ajafff]

I disagree with some changes made here. (x) => foo()(x) is no unnecessary wrapper, because foo() could have side effects. It would change the semantics of the program if the callback is never called or called more than once.
Every CallExpression whose expression is not an identifier should be allowed.

+1 for allowing (x) => x(x)

[andy-hanson]

That's technically true... but that would be a pretty cruel place to hide a side effect. Most of the time code is better off evaluating foo() only once, instead of once per callback.

[ajafff]

but that would be a pretty cruel place to hide a side effect. Most of the time code is better off evaluating foo() only once, instead of once per callback.

Agreed, but that's definitely not the responsibility of this rule.

What if the arrow function is not used as callback. It's totally valid to write all functions as arrow function. If that's the case, side effects are expected and it's not just an "unnecessary wrapper"

[andy-hanson]

OK, I've reverted to only check for identifiers.

[ajafff]

this is the only line that really changed
please consider keeping the diff small and just add this line to the source as it is on master.
Btw. sorry for the rather big refactor and therefore unnecessary big diff in #2510 😕

[andy-hanson]

Updated changelog entry to reflect what this PR ended up actually doing.