no-array-method-this-argument: Fix false positives (#1386)

sindresorhus · Jun 30, 2021 · d364d67 · d364d67
1 parent e41d1c7
commit d364d67
Show file tree

Hide file tree

Showing 8 changed files with 133 additions and 100 deletions.
diff --git a/rules/no-array-callback-reference.js b/rules/no-array-callback-reference.js
@@ -133,6 +133,8 @@ function getProblem(context, node, method, options) {
 
 const ignoredFirstArgumentSelector = [
 	notFunctionSelector('arguments.0'),
+	// Ignore all `CallExpression`s include `function.bind()`
+	'[arguments.0.type!="CallExpression"]',
 	'[arguments.0.type!="FunctionExpression"]',
 	'[arguments.0.type!="ArrowFunctionExpression"]'
 ].join('');

diff --git a/rules/no-array-method-this-argument.js b/rules/no-array-method-this-argument.js
@@ -1,9 +1,10 @@
 'use strict';
 const {hasSideEffect} = require('eslint-utils');
-const {methodCallSelector} = require('./selectors/index.js');
+const {methodCallSelector, notFunctionSelector} = require('./selectors/index.js');
 const {removeArgument} = require('./fix/index.js');
 const {getParentheses, getParenthesizedText} = require('./utils/parentheses.js');
 const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object.js');
+const {isNodeMatches} = require('./utils/is-node-matches.js');
 
 const ERROR = 'error';
 const SUGGESTION_BIND = 'suggestion-bind';
@@ -14,19 +15,61 @@ const messages = {
 	[SUGGESTION_BIND]: 'Use a bound function.'
 };
 
-const selector = methodCallSelector({
-	names: [
-		'every',
-		'filter',
-		'find',
-		'findIndex',
-		'flatMap',
-		'forEach',
-		'map',
-		'some'
-	],
-	length: 2
-});
+const ignored = [
+	'lodash.every',
+	'_.every',
+	'underscore.every',
+
+	'lodash.filter',
+	'_.filter',
+	'underscore.filter',
+	'Vue.filter',
+
+	'lodash.find',
+	'_.find',
+	'underscore.find',
+
+	'lodash.findIndex',
+	'_.findIndex',
+	'underscore.findIndex',
+
+	'lodash.flatMap',
+	'_.flatMap',
+
+	'lodash.forEach',
+	'_.forEach',
+	'React.Children.forEach',
+	'Children.forEach',
+
+	'lodash.map',
+	'_.map',
+	'underscore.map',
+	'React.Children.map',
+	'Children.map',
+	'jQuery.map',
+	'$.map',
+
+	'lodash.some',
+	'_.some',
+	'underscore.some'
+];
+
+const selector = [
+	methodCallSelector({
+		names: [
+			'every',
+			'filter',
+			'find',
+			'findIndex',
+			'flatMap',
+			'forEach',
+			'map',
+			'some'
+		],
+		length: 2
+	}),
+	notFunctionSelector('arguments.0')
+].join('');
 
 function removeThisArgument(callExpression, sourceCode) {
 	return fixer => removeArgument(fixer, callExpression.arguments[1], sourceCode);
@@ -63,7 +106,12 @@ const create = context => {
 
 	return {
 		[selector](callExpression) {
-			const method = callExpression.callee.property.name;
+			const {callee} = callExpression;
+			if (isNodeMatches(callee, ignored)) {
+				return;
+			}
+
+			const method = callee.property.name;
 			const [callback, thisArgument] = callExpression.arguments;
 
 			const problem = {

diff --git a/rules/selectors/not-function.js b/rules/selectors/not-function.js
@@ -1,4 +1,5 @@
 'use strict';
+const not = require('./negation.js');
 
 // AST Types:
 // https://github.com/eslint/espree/blob/master/lib/ast-node-types.js#L18
@@ -18,17 +19,26 @@ const impossibleNodeTypes = [
 const mostLikelyNotNodeTypes = [
 	'AssignmentExpression',
 	'AwaitExpression',
-	'CallExpression',
 	'LogicalExpression',
 	'NewExpression',
 	'TaggedTemplateExpression',
 	'ThisExpression'
 ];
 
-const notFunctionSelector = node => [
-	...[...impossibleNodeTypes, ...mostLikelyNotNodeTypes].map(type => `[${node}.type!="${type}"]`),
-	`:not([${node}.type="Identifier"][${node}.name="undefined"])`
-].join('');
+const notFunctionSelector = node => not([
+	[...impossibleNodeTypes, ...mostLikelyNotNodeTypes].map(type => `[${node}.type="${type}"]`),
+	`[${node}.type="Identifier"][${node}.name="undefined"]`,
+	[
+		`[${node}.type="CallExpression"]`,
+		not([
+			`[${node}.callee.type="MemberExpression"]`,
+			`[${node}.callee.optional!=true]`,
+			`[${node}.callee.computed!=true]`,
+			`[${node}.callee.property.type="Identifier"]`,
+			`[${node}.callee.property.name="bind"]`
+		].join(''))
+	].join('')
+]);
 
 module.exports = {
 	notFunctionSelector

diff --git a/test/no-array-callback-reference.mjs b/test/no-array-callback-reference.mjs
@@ -96,6 +96,7 @@ test({
 
 		// Exclude await expressions
 		...simpleMethods.map(method => `(async () => await foo.${method}(bar))()`),
+		'foo.map(function (a) {}.bind(bar))',
 
 		// #813
 		outdent`

diff --git a/test/no-array-method-this-argument.mjs b/test/no-array-method-this-argument.mjs
@@ -1,4 +1,4 @@
-import {getTester} from './utils/test.mjs';
+import {getTester, parsers} from './utils/test.mjs';
 
 const {test} = getTester(import.meta);
 
@@ -14,7 +14,24 @@ test.snapshot({
 		'array.map(() => {},)',
 		'array.map(() => {}, ...thisArgument)',
 		'array.map(...() => {}, thisArgument)',
-		'array.map(() => {}, thisArgument, extraArgument)'
+		'array.map(() => {}, thisArgument, extraArgument)',
+		// Ignored
+		'lodash.every(array, () => {})',
+		'lodash.find(array, () => {})',
+		'jQuery.map(array, () => {})',
+		'$.map(array, () => {})',
+		'React.Children.map(children, () => {})',
+		'Children.map(children, () => {})',
+		'React.Children.forEach(children, () => {})',
+		'Children.forEach(children, () => {})',
+		'Vue.filter("capitalize", () => {})',
+		// `jQuery.find` and `jQuery.filter` don't accept second argument
+		'$( "li" ).filter( ":nth-child(2n)" ).css( "background-color", "red" );',
+		'$( "li.item-ii" ).find( "li" ).css( "background-color", "red" );',
+		// First argument is not function
+		'array.map(new Callback, thisArgument)',
+		'array.map(1, thisArgument)',
+		'async () => array.map(await callback, thisArgument)'
 	],
 	invalid: [
 		'array.every(() => {}, thisArgument)',
@@ -40,12 +57,15 @@ test.snapshot({
 		'array.map(callback, (0, thisArgument))',
 		'array.map(function () {}, thisArgument)',
 		'array.map(function callback () {}, thisArgument)',
-		'array.map(new Callback, thisArgument)',
-		'array.map(1, thisArgument)',
-		'async () => array.map(await callback, thisArgument)',
-		'array.map((new Callback), thisArgument)',
-		'array.map(new Callback(), thisArgument)',
-		'array.map( (( callback )), (( thisArgument )),)',
+		{
+			code: 'array.map( foo as bar, (( thisArgument )),)',
+			parser: parsers.typescript
+		},
+		{
+			code: 'array.map( (( foo as bar )), (( thisArgument )),)',
+			parser: parsers.typescript
+		},
+		'array.map( (( 0, callback )), (( thisArgument )),)',
 		// This callback is actually arrow function, but we don't know
 		'array.map((0, () => {}), thisArgument)',
 		// This callback is a bound function, but we don't know

diff --git a/test/snapshots/no-array-method-this-argument.mjs.md b/test/snapshots/no-array-method-this-argument.mjs.md
@@ -235,114 +235,60 @@ Generated by [AVA](https://avajs.dev).
     `
 
 ## Invalid #5
-      1 | array.map(new Callback, thisArgument)
+      1 | array.map( foo as bar, (( thisArgument )),)
 
 > Error 1/1
 
     `␊
-    > 1 | array.map(new Callback, thisArgument)␊
-        |                         ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
+    > 1 | array.map( foo as bar, (( thisArgument )),)␊
+        |                           ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
     ␊
     --------------------------------------------------------------------------------␊
     Suggestion 1/2: Remove the second argument.␊
-      1 | array.map(new Callback)␊
+      1 | array.map( foo as bar,)␊
     ␊
     --------------------------------------------------------------------------------␊
     Suggestion 2/2: Use a bound function.␊
-      1 | array.map((new Callback).bind(thisArgument))␊
+      1 | array.map( (foo as bar).bind((( thisArgument ))),)␊
     `
 
 ## Invalid #6
-      1 | array.map(1, thisArgument)
+      1 | array.map( (( foo as bar )), (( thisArgument )),)
 
 > Error 1/1
 
     `␊
-    > 1 | array.map(1, thisArgument)␊
-        |              ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
+    > 1 | array.map( (( foo as bar )), (( thisArgument )),)␊
+        |                                 ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
     ␊
     --------------------------------------------------------------------------------␊
     Suggestion 1/2: Remove the second argument.␊
-      1 | array.map(1)␊
+      1 | array.map( (( foo as bar )),)␊
     ␊
     --------------------------------------------------------------------------------␊
     Suggestion 2/2: Use a bound function.␊
-      1 | array.map((1).bind(thisArgument))␊
+      1 | array.map( (( foo as bar )).bind((( thisArgument ))),)␊
     `
 
 ## Invalid #7
-      1 | async () => array.map(await callback, thisArgument)
+      1 | array.map( (( 0, callback )), (( thisArgument )),)
 
 > Error 1/1
 
     `␊
-    > 1 | async () => array.map(await callback, thisArgument)␊
-        |                                       ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
+    > 1 | array.map( (( 0, callback )), (( thisArgument )),)␊
+        |                                  ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
     ␊
     --------------------------------------------------------------------------------␊
     Suggestion 1/2: Remove the second argument.␊
-      1 | async () => array.map(await callback)␊
+      1 | array.map( (( 0, callback )),)␊
     ␊
     --------------------------------------------------------------------------------␊
     Suggestion 2/2: Use a bound function.␊
-      1 | async () => array.map((await callback).bind(thisArgument))␊
+      1 | array.map( (( 0, callback )).bind((( thisArgument ))),)␊
     `
 
 ## Invalid #8
-      1 | array.map((new Callback), thisArgument)
-
-> Error 1/1
-
-    `␊
-    > 1 | array.map((new Callback), thisArgument)␊
-        |                           ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
-    ␊
-    --------------------------------------------------------------------------------␊
-    Suggestion 1/2: Remove the second argument.␊
-      1 | array.map((new Callback))␊
-    ␊
-    --------------------------------------------------------------------------------␊
-    Suggestion 2/2: Use a bound function.␊
-      1 | array.map((new Callback).bind(thisArgument))␊
-    `
-
-## Invalid #9
-      1 | array.map(new Callback(), thisArgument)
-
-> Error 1/1
-
-    `␊
-    > 1 | array.map(new Callback(), thisArgument)␊
-        |                           ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
-    ␊
-    --------------------------------------------------------------------------------␊
-    Suggestion 1/2: Remove the second argument.␊
-      1 | array.map(new Callback())␊
-    ␊
-    --------------------------------------------------------------------------------␊
-    Suggestion 2/2: Use a bound function.␊
-      1 | array.map(new Callback().bind(thisArgument))␊
-    `
-
-## Invalid #10
-      1 | array.map( (( callback )), (( thisArgument )),)
-
-> Error 1/1
-
-    `␊
-    > 1 | array.map( (( callback )), (( thisArgument )),)␊
-        |                               ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
-    ␊
-    --------------------------------------------------------------------------------␊
-    Suggestion 1/2: Remove the second argument.␊
-      1 | array.map( (( callback )),)␊
-    ␊
-    --------------------------------------------------------------------------------␊
-    Suggestion 2/2: Use a bound function.␊
-      1 | array.map( (( callback )).bind((( thisArgument ))),)␊
-    `
-
-## Invalid #11
       1 | array.map((0, () => {}), thisArgument)
 
 > Error 1/1
@@ -360,7 +306,7 @@ Generated by [AVA](https://avajs.dev).
       1 | array.map((0, () => {}).bind(thisArgument))␊
     `
 
-## Invalid #12
+## Invalid #9
       1 | array.map(callback.bind(foo), thisArgument)
 
 > Error 1/1

diff --git a/test/snapshots/no-array-method-this-argument.mjs.snap b/test/snapshots/no-array-method-this-argument.mjs.snap
diff --git a/test/utils/snapshot-rule-tester.mjs b/test/utils/snapshot-rule-tester.mjs
@@ -59,7 +59,7 @@ function normalizeTests(tests) {
 
 			const additionalProperties = getAdditionalProperties(
 				testCase,
-				['code', 'options', 'filename', 'parserOptions']
+				['code', 'options', 'filename', 'parserOptions', 'parser']
 			);
 
 			if (additionalProperties.length > 0) {
@@ -72,9 +72,15 @@ function normalizeTests(tests) {
 }
 
 function getVerifyConfig(ruleId, testerConfig, testCase) {
-	const {options, parserOptions} = testCase;
+	const {
+		options,
+		parserOptions,
+		parser = testerConfig.parser
+	} = testCase;
+
 	return {
 		...testerConfig,
+		parser,
 		parserOptions: {
 			...testerConfig.parserOptions,
 			...parserOptions