Feature Request: expose `ignoredCallee` to `no-array-callback-reference` rule

[Cherry]

This rule is awesome, but with some older code in AngularJS, we find ourselves having to inline disable unicorn/no-array-callback-reference regularly where our code uses things like Angular.forEach. There are of course much more modern alternatives to this nowadays, but configuration is always helpful.

Rather than maintain this list privately, I think it would make sense to expose in the rule configuration so users can configure it for their own needs. Something like:

"unicorn/no-array-callback-reference": ["error", {
	"ignoredCallee": [
		"Promise",
		"React.children",
		"Children",
		"lodash",
		"underscore",
		"_",
		"Async",
		"async",
		"Angular"
	]
}]

[sindresorhus]

The problem with exposing the list is that everyone will be too lazy to contribute additions and instead just ignore it in their own config. As long as it's something generally useful, I think we should encourage people to submit to our ignore list instead.

[Cherry]

Thanks for the reply! That makes sense, though if we had our own internal library that exposed a forEach function, having a way to tweak that list for our specific use-case would be beneficial.

Adding https://docs.angularjs.org/api/ng/function/angular.forEach to the list would resolve our immediate use-case though.

[sindresorhus]

That makes sense, though if we had our own internal library that exposed a forEach function, having a way to tweak that list for our specific use-case would be beneficial.

Sure, and if someone presents an use-case like that, we will expose an option, but I would prefer to wait until such time.

[Cherry]

Here's an example with an internal queue system we have that I ran into today after upgrading some other repos:

const queues = {};
queues.a = {
	name: 'a',
	find: function(filter, callback){
		// look in job queue, find job
		// callback with job
	}
};

const filter = {some: 'filter'};
queues.a.find(filter, console.log); // this throws a `unicorn/no-array-callback-reference` error

It's a contrived example here and in reality is more complex and exists across multiple different files, etc. but should illustrate the need for such a config.

Another example that triggers an error when doing database queries with RethinkDB and rethinkdbdash:

r.table('table').filter(someFunc)...

[sindresorhus]

queues.a.find(filter, console.log);

The rule is just doing its job here though.

This would be safer and more readable:

queues.a.find(filter, job => {
	console.log(job);
});

r.table('table').filter(someFunc)...

Same as above. Why would you need to ignore this and not Array#filter?

---

How would you ignore queues.a.find(filter, console.log); and r.table('table').filter(someFunc) if there was an ignore option? ['a', 'table'] feels a bit too general.

[Cherry]

This would be safer and more readable:

queues.a.find(filter, job => {
	console.log(job);
});

That still throws a unicorn/no-array-callback-reference - it's the first argument to the find function, not the console.log causing the linting issue - that was just an example. Since we wrote the find and filter functions/objects ourselves, we can be sure there are no side effects. In this example case above, filter is an object and not even a function that could have any side-effects.

Same as above. Why would you need to ignore this and not Array#filter?

This is a function, or object, etc. passed directly to RethinkDB, and evaluated in the DB engine itself. It's not really a direct comparison to Array#filter in my opinion, as once again it's not always a function:

const queryFilter = {foo: 'bar'};
r.table('table').filter(queryFilter) // throws a `unicorn/no-array-callback-reference`

How would you ignore queues.a.find(filter, console.log); and r.table('table').filter(someFunc) if there was an ignore option? ['a', 'table'] feels a bit too general.

I'm not directly familiar with ESLint's AST or how this could be done, but being able to ignore r.table(...).filter for example, and queues[...].find would be ideal in this example.

[sindresorhus]

That still throws a unicorn/no-array-callback-reference - it's the first argument to the find function, not the console.log causing the linting issue

I think we could detect this case by only triggering the error if there's one argument (since the thisArgument on the actual array methods is rarely used).

Since we wrote the find and filter functions/objects ourselves, we can be sure there are no side effects.

That's not always the case though. Programmers are often too optimistic about their ability to reason about code and side effects. For example, if you start using this .find method across files, it can be hard to remember all the places it's called.

This is a function, or object, etc. passed directly to RethinkDB, and evaluated in the DB engine itself. It's not really a direct comparison to Array#filter in my opinion, as once again it's not always a function:

If the object literal is in the same scope, we might be able to detect it.

I'm not directly familiar with ESLint's AST or how this could be done, but being able to ignore r.table(...).filter for example, and queues[...].find would be ideal in this example.

I think it should only work on the "callee", so it would be r.table and queues. Not sure it's worth inventing our own DSL for just this option though. Alternatively, we could make the syntax be esquery, although that's a bit harder for users to deal with.

[Cherry]

I think we could detect this case by only triggering the error if there's one argument (since the thisArgument on the actual array methods is rarely used).

That would be ideal.

That's not always the case though. Programmers are often too optimistic about their ability to reason about code and side effects. For example, if you start using this .find method across files, it can be hard to remember all the places it's called.

That's very fair. I think this would be a project-by-project configuration thing in such a case.

If the object literal is in the same scope, we might be able to detect it.

That would be awesome. I feel like this rule throwing an error when passing anything but a function is definitely a bug.

I think it should only work on the "callee", so it would be r.table and queues. Not sure it's worth inventing our own DSL for just this option though. Alternatively, we could make the syntax be esquery, although that's a bit harder for users to deal with.

Something like that sounds reasonable. Thanks so much for working through this with me.

[tmtron]

Another use-case where we want to disable this rule is for methods that use type predicates

// note, that we use a type predicate: input is T 
const isNotNull = <T>(input: T|null): input is T => input !== null;

// type is string[]
const arrOfString = [null, ''].filter(isNotNull);
// type is (string | null)[]
const arrOfStringOrNull = [null, ''].filter(i => isNotNull(i))pescript

Playground example

So in this case we may want to deactivate the check for all uses of the isNotNull function.

[Alster]

Another use-case where we want to disable this rule is for methods that use type predicates

// note, that we use a type predicate: input is T 
const isNotNull = <T>(input: T|null): input is T => input !== null;

// type is string[]
const arrOfString = [null, ''].filter(isNotNull);
// type is (string | null)[]
const arrOfStringOrNull = [null, ''].filter(i => isNotNull(i))pescript

Playground example

So in this case we may want to deactivate the check for all uses of the isNotNull function.

Yes, i have the same trouble in project where widely used type assertions like you mentioned. As a solution, maybe rule can detect that function is type assertion, and pass it. I write this comment just to support this topic)