no-fn-reference-in-iterator false positives on non-array methods

[silverwind]

const obj = {filter: () => {}};
obj.filter([].join()); // error: Do not pass a function reference directly to an iterator method

.filter is not an iterator here. I think the rule confuses it with Array.prototype.filter and the false-positive seems specifically triggered by a CallExpression argument.

The autofix produces broken code for this case.

[himynameisdave]

I'm willing to take a stab at this, but I'm not sure where to start. Is it even possible to determine if filter (or any other array iterator method) is from the Array.prototype vs some other object?

[papb]

Hi @himynameisdave, may I give a suggestion?

One thing that seems quite unexpected to me is that:

the false-positive seems specifically triggered by a CallExpression argument.

The reason for my surprise is that my first guess for a cause of this bug would be the rule implementation just looking for .filter( calls without extra care if they are really from Array.prototype. However, if this was the correct explanation, false positives should happen more often than only specifically with a CallExpression.

So my first suggestion to you would be to generate several examples and see which ones really generate a false positive, in an attempt to have a better feeling of why this problem is happening, before diving into what you said:

determine if filter (or any other array iterator method) is from the Array.prototype vs some other object

[fisker]

Maybe we should just disable auto-fix on this rule #418 (comment)

[papb]

@fisker I agree that a 'more urgent' action is to disable the auto-fix, but we don't want the rule mistakenly reporting errors anyway.

[fisker]

I Don't think this is bug, and the auto-fix will be removed in #666

[axelvaindal]

I also have the issue with the following code:

const uid = "some uid";
const response = await source.find(uid);

where source.find is actually a regular method and not an iterator here.

[fisker]

We can't know source is not Array and uid is not a function.

So... I guess you'll have to use eslint-disable or

const response = await source.find("some uid"); // In this way we know argument is not a function

[papb]

Hmmm, but if the lint rule gives so many false positives like this, should it really exist?

[fisker]

I tried really hard to rewrite this rule , and you can see it did catch a lot of problems in #666 .

Big problem is we can't know if callee is an array , we can't know if argument is a function.

[fisker]

It's not auto-fixable now , means if we set to warn , your code won't be changed even with --fix, should we do that ?

[papb]

I tried really hard to rewrite this rule , and you can see it did catch a lot of problems in #666 .

Which problems? You mean like the following?

-	const superExpression = constructorBody.find(isSuperExpression);
+	const superExpression = constructorBody.find(body => isSuperExpression(body));

This was not a problem; I think one could argue the new version is more readable, but that's debatable...

[fisker]

That is exactly why this rule exists, it is problem.

[papb]

Are you sure? The isSuperExpression is not bound to any this context either way.

The real problem is something like arr.find(object.method) versus arr.find(x => object.method(x)). This is a real problem. But when a function is already unbounded, I don't think it is. Right?

[fisker]

No , it's not about binding, it's about function parameter changes . see https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/no-fn-reference-in-iterator.md

[fisker]

We don't even support foo.map(lib.fn) before #666 , you can check the code and PR description.

[papb]

Oh, of course 🤦‍♂️ I had some headaches in the past due to the function losing its binding context, that I forgot to think about the parameter changes. Sorry about that. You're right.

[papb]

should we do that ?

You mean set it to warn? I don't think so... Error is good... The problem is false positives, setting to warn would still make it annoying I think...

Is there a good way to let users know that some false positives are expected and encourage usage of eslint-disable if needed?

[axelvaindal]

In my case, the 'error' occurred while I was updating my xo version.
I disabled the rule for now, it was the only reported 'error' I had anyway.
As it was not related to filter, I figured it would be nice to report it as well in the opened issue, but right now I realize it would have probably be a false positive on any iterator like syntax.

Not sure how to fix the 'false' positive, but as long as it's not auto fixed, I'd assume the developers would know when or if this rule is worth keeping as an error in their codebase 🤔
Perhaps a warning when the rule is reported could direct the developers to set the rule as warn or off if they think it's a false positive.
Then again, a simple search/look is enough to find the result, so not sure it's worth the time 😄

[papb]

What if the linting error itself provides a hint such as:

If you think this is a false positive, disable this check with // eslint-disable-next-line unicorn/no-fn-reference-in-iterator

[fisker]

What if the linting error itself provides a hint such as:

If you think this is a false positive, disable this check with // eslint-disable-next-line unicorn/no-fn-reference-in-iterator

Good idea, PR welcome.

[papb]

Nice! I don't have time to do it though, sorry about that.

[ehacke]

This rule also triggers for most Bluebird.js use cases. (Bluebird.map, Bluebird.filter, etc).

Obviously I'll just disable to the rule globally in my case, but it seems like this rule has really a lot of false positives and may not be great to have enabled (neither warn nor error) by default.

Just my 2 cents.

[thinh105]

Anyone using MongoDB and Mongoose?

  const filterModel = {};
  const windows = await Tour.find(filterModel);

because it thought Mongo db.collection.find() Mongoose Model.find() as normal JS find() and raise the false positive!

Do not pass function filterModel directly to .find(…).eslintunicorn/no-fn-reference-in-iterator

My object fileterModel does not look like a function at all,

Make me so confuse @@

[fregante]

Same as:

• no-array-method-this-argument false positives on non-array methods #1394
• no-array-callback-reference false positives on non-array methods #1193

Main discusion:

• Rules improvements with the help of @typescript-eslint/parser #347