[prefer-tacit] May cause source of error

[jens-duttke]

Bug Report

The documentation of prefer-tacit states:

| Generally there's no reason to wrap a function with a callback wrapper if it's directly called anyway.

But this is not correct.

Steps to reproduce

Imagine you are using (a third-party library which provides) a function shorten:

function shorten (text) {
	return text.substr(0, 2);
}

Now you want to use Array.prototype.map() to transform an array of strings to a version with shortened strings:

console.log(['Hello', 'World'].map((text) => shorten(text)));
// -> ["He", "Wo"]

Since this is reported as "Potentially unnecessary function wrapper" by prefer-tacit, you change it to:

console.log(['Hello', 'World'].map(shorten));
// ["He", "Wo"]

Same result, so everything is fine.

Later the author of shorten decides to add a second, optional parameter:

function shorten (text, length = 2) {
	return text.substr(0, length);
}

Now, the "optimized" code, returns a totally different result, because the second parameter of map is the index:

console.log(['Hello', 'World'].map(shorten));
// ["", "W"]

This could result in hard-to-find bugs.

Proposed changes

I would expect the documentation to clearly point out this problem, rather than pretending it's the same functionality.
And that this rule will not be added to the recommend and lite configs in the next major release (#171)

[RebeccaStevens]

The documentation probably should be updated to highlight this issue.

However, the rule itself is smart enough to not mark the following as a violation if shorten is typed as taking more than 1 parameter.

['Hello', 'World'].map((text) => shorten(text));

In JS world where there are no types, the rule's fixer is disabled by default and only set to the "warn" severity.

The only issue comes when the function shorten is modified after the fact. @jonaskello Do you think this is reason enough to not add the rule to the recommended and lite configs?

I have also been thinking that it might be best to change the fixer to a suggestion.

[jens-duttke]

| The only issue comes when the function shorten is modified after the fact.

This is the main problem - the result is code which changes it's behaviour unintentionally.

I just found this article by Jake Archibald who describes the exact same problem:
https://jakearchibald.com/2021/function-callback-risks/
Maybe it could be linked in the documentation of the rule.

[jonaskello]

Well, this seems like a tough problem to solve automatically. If you enable the rule and follow it blindly you could run into situations like the one described here. I would say the developer must understand the risks at each call-site and make a decision.

Documenting the risks more would of course be a good step. I have not used suggestions in eslint but it seems like they may be able to provide more descriptions to the developer before applying the fix? In that case I think that could also be a good thing. A text like "Switch to point-free style. Be aware that future changes to the function's signature may cause unintended behavior. See docs, [link to docs]" would be helpful when applying the fix.

In case the developer decides that he don't want to use point-free style, perhaps because it is a third-party function, then it should also be possible to avoid having the rule flagging the case. The rule seem to have the ignorePattern option but I'm not sure how it works for this particular rule?

If possible I think having the fixer display two suggestions would be a good solution, something like this:

• Replace with point-free style. Be aware that future changes to the function's signature may cause unintended behavior. See docs, [link to docs]
• Change to follow ignorePattern so that this usage of non-point-free style will be ignored.

[RebeccaStevens]

I think you're right @jens-duttke, with the way the rule currently is, the rule should be opt-in rather than enabled by default.

I wonder if we could add a setting so that the rule would ignore functions that come from an external source (though this might be difficult to do as "what defines an external source?" - we'd need to get the user to specify (by default it would be any import that doesn't start with "./" or "../")).

With such a setting enabled, the rule could then be used in conjunction with rules like unicorn/no-array-callback-reference without error.

[RebeccaStevens]

The fixer has been replaced with a suggestion so there are no automatic changes that could break things.