Skip to content

Commit d364d67

Browse files
fiskerfisker
authored
no-array-method-this-argument: Fix false positives (#1386)
1 parent e41d1c7 commit d364d67

8 files changed

+133
-100
lines changed

‎rules/no-array-callback-reference.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,8 @@ function getProblem(context, node, method, options) {
133133

134134
const ignoredFirstArgumentSelector = [
135135
notFunctionSelector('arguments.0'),
136+
// Ignore all `CallExpression`s include `function.bind()`
137+
'[arguments.0.type!="CallExpression"]',
136138
'[arguments.0.type!="FunctionExpression"]',
137139
'[arguments.0.type!="ArrowFunctionExpression"]'
138140
].join('');

‎rules/no-array-method-this-argument.js‎

Lines changed: 63 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
'use strict';
22
const {hasSideEffect} = require('eslint-utils');
3-
const {methodCallSelector} = require('./selectors/index.js');
3+
const {methodCallSelector, notFunctionSelector} = require('./selectors/index.js');
44
const {removeArgument} = require('./fix/index.js');
55
const {getParentheses, getParenthesizedText} = require('./utils/parentheses.js');
66
const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object.js');
7+
const {isNodeMatches} = require('./utils/is-node-matches.js');
78

89
const ERROR = 'error';
910
const SUGGESTION_BIND = 'suggestion-bind';
@@ -14,19 +15,61 @@ const messages = {
1415
[SUGGESTION_BIND]: 'Use a bound function.'
1516
};
1617

17-
const selector = methodCallSelector({
18-
names: [
19-
'every',
20-
'filter',
21-
'find',
22-
'findIndex',
23-
'flatMap',
24-
'forEach',
25-
'map',
26-
'some'
27-
],
28-
length: 2
29-
});
18+
const ignored = [
19+
'lodash.every',
20+
'_.every',
21+
'underscore.every',
22+
23+
'lodash.filter',
24+
'_.filter',
25+
'underscore.filter',
26+
'Vue.filter',
27+
28+
'lodash.find',
29+
'_.find',
30+
'underscore.find',
31+
32+
'lodash.findIndex',
33+
'_.findIndex',
34+
'underscore.findIndex',
35+
36+
'lodash.flatMap',
37+
'_.flatMap',
38+
39+
'lodash.forEach',
40+
'_.forEach',
41+
'React.Children.forEach',
42+
'Children.forEach',
43+
44+
'lodash.map',
45+
'_.map',
46+
'underscore.map',
47+
'React.Children.map',
48+
'Children.map',
49+
'jQuery.map',
50+
'$.map',
51+
52+
'lodash.some',
53+
'_.some',
54+
'underscore.some'
55+
];
56+
57+
const selector = [
58+
methodCallSelector({
59+
names: [
60+
'every',
61+
'filter',
62+
'find',
63+
'findIndex',
64+
'flatMap',
65+
'forEach',
66+
'map',
67+
'some'
68+
],
69+
length: 2
70+
}),
71+
notFunctionSelector('arguments.0')
72+
].join('');
3073

3174
function removeThisArgument(callExpression, sourceCode) {
3275
return fixer => removeArgument(fixer, callExpression.arguments[1], sourceCode);
@@ -63,7 +106,12 @@ const create = context => {
63106

64107
return {
65108
[selector](callExpression) {
66-
const method = callExpression.callee.property.name;
109+
const {callee} = callExpression;
110+
if (isNodeMatches(callee, ignored)) {
111+
return;
112+
}
113+
114+
const method = callee.property.name;
67115
const [callback, thisArgument] = callExpression.arguments;
68116

69117
const problem = {

‎rules/selectors/not-function.js‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
'use strict';
2+
const not = require('./negation.js');
23

34
// AST Types:
45
// https://github.com/eslint/espree/blob/master/lib/ast-node-types.js#L18
@@ -18,17 +19,26 @@ const impossibleNodeTypes = [
1819
const mostLikelyNotNodeTypes = [
1920
'AssignmentExpression',
2021
'AwaitExpression',
21-
'CallExpression',
2222
'LogicalExpression',
2323
'NewExpression',
2424
'TaggedTemplateExpression',
2525
'ThisExpression'
2626
];
2727

28-
const notFunctionSelector = node => [
29-
...[...impossibleNodeTypes, ...mostLikelyNotNodeTypes].map(type => `[${node}.type!="${type}"]`),
30-
`:not([${node}.type="Identifier"][${node}.name="undefined"])`
31-
].join('');
28+
const notFunctionSelector = node => not([
29+
[...impossibleNodeTypes, ...mostLikelyNotNodeTypes].map(type => `[${node}.type="${type}"]`),
30+
`[${node}.type="Identifier"][${node}.name="undefined"]`,
31+
[
32+
`[${node}.type="CallExpression"]`,
33+
not([
34+
`[${node}.callee.type="MemberExpression"]`,
35+
`[${node}.callee.optional!=true]`,
36+
`[${node}.callee.computed!=true]`,
37+
`[${node}.callee.property.type="Identifier"]`,
38+
`[${node}.callee.property.name="bind"]`
39+
].join(''))
40+
].join('')
41+
]);
3242

3343
module.exports = {
3444
notFunctionSelector

‎test/no-array-callback-reference.mjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ test({
9696

9797
// Exclude await expressions
9898
...simpleMethods.map(method => `(async () => await foo.${method}(bar))()`),
99+
'foo.map(function (a) {}.bind(bar))',
99100

100101
// #813
101102
outdent`

‎test/no-array-method-this-argument.mjs‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {getTester} from './utils/test.mjs';
1+
import {getTester, parsers} from './utils/test.mjs';
22

33
const {test} = getTester(import.meta);
44

@@ -14,7 +14,24 @@ test.snapshot({
1414
'array.map(() => {},)',
1515
'array.map(() => {}, ...thisArgument)',
1616
'array.map(...() => {}, thisArgument)',
17-
'array.map(() => {}, thisArgument, extraArgument)'
17+
'array.map(() => {}, thisArgument, extraArgument)',
18+
// Ignored
19+
'lodash.every(array, () => {})',
20+
'lodash.find(array, () => {})',
21+
'jQuery.map(array, () => {})',
22+
'$.map(array, () => {})',
23+
'React.Children.map(children, () => {})',
24+
'Children.map(children, () => {})',
25+
'React.Children.forEach(children, () => {})',
26+
'Children.forEach(children, () => {})',
27+
'Vue.filter("capitalize", () => {})',
28+
// `jQuery.find` and `jQuery.filter` don't accept second argument
29+
'$( "li" ).filter( ":nth-child(2n)" ).css( "background-color", "red" );',
30+
'$( "li.item-ii" ).find( "li" ).css( "background-color", "red" );',
31+
// First argument is not function
32+
'array.map(new Callback, thisArgument)',
33+
'array.map(1, thisArgument)',
34+
'async () => array.map(await callback, thisArgument)'
1835
],
1936
invalid: [
2037
'array.every(() => {}, thisArgument)',
@@ -40,12 +57,15 @@ test.snapshot({
4057
'array.map(callback, (0, thisArgument))',
4158
'array.map(function () {}, thisArgument)',
4259
'array.map(function callback () {}, thisArgument)',
43-
'array.map(new Callback, thisArgument)',
44-
'array.map(1, thisArgument)',
45-
'async () => array.map(await callback, thisArgument)',
46-
'array.map((new Callback), thisArgument)',
47-
'array.map(new Callback(), thisArgument)',
48-
'array.map( (( callback )), (( thisArgument )),)',
60+
{
61+
code: 'array.map( foo as bar, (( thisArgument )),)',
62+
parser: parsers.typescript
63+
},
64+
{
65+
code: 'array.map( (( foo as bar )), (( thisArgument )),)',
66+
parser: parsers.typescript
67+
},
68+
'array.map( (( 0, callback )), (( thisArgument )),)',
4969
// This callback is actually arrow function, but we don't know
5070
'array.map((0, () => {}), thisArgument)',
5171
// This callback is a bound function, but we don't know

‎test/snapshots/no-array-method-this-argument.mjs.md‎

Lines changed: 16 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -235,114 +235,60 @@ Generated by [AVA](https://avajs.dev).
235235
`
236236

237237
## Invalid #5
238-
1 | array.map(new Callback, thisArgument)
238+
1 | array.map( foo as bar, (( thisArgument )),)
239239

240240
> Error 1/1
241241
242242
`␊
243-
> 1 | array.map(new Callback, thisArgument)␊
244-
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
243+
> 1 | array.map( foo as bar, (( thisArgument )),)␊
244+
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
245245
␊
246246
--------------------------------------------------------------------------------␊
247247
Suggestion 1/2: Remove the second argument.␊
248-
1 | array.map(new Callback)␊
248+
1 | array.map( foo as bar,)␊
249249
␊
250250
--------------------------------------------------------------------------------␊
251251
Suggestion 2/2: Use a bound function.␊
252-
1 | array.map((new Callback).bind(thisArgument))␊
252+
1 | array.map( (foo as bar).bind((( thisArgument ))),)␊
253253
`
254254

255255
## Invalid #6
256-
1 | array.map(1, thisArgument)
256+
1 | array.map( (( foo as bar )), (( thisArgument )),)
257257

258258
> Error 1/1
259259
260260
`␊
261-
> 1 | array.map(1, thisArgument)␊
262-
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
261+
> 1 | array.map( (( foo as bar )), (( thisArgument )),)␊
262+
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
263263
␊
264264
--------------------------------------------------------------------------------␊
265265
Suggestion 1/2: Remove the second argument.␊
266-
1 | array.map(1)␊
266+
1 | array.map( (( foo as bar )),)␊
267267
␊
268268
--------------------------------------------------------------------------------␊
269269
Suggestion 2/2: Use a bound function.␊
270-
1 | array.map((1).bind(thisArgument))␊
270+
1 | array.map( (( foo as bar )).bind((( thisArgument ))),)␊
271271
`
272272

273273
## Invalid #7
274-
1 | async () => array.map(await callback, thisArgument)
274+
1 | array.map( (( 0, callback )), (( thisArgument )),)
275275

276276
> Error 1/1
277277
278278
`␊
279-
> 1 | async () => array.map(await callback, thisArgument)␊
280-
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
279+
> 1 | array.map( (( 0, callback )), (( thisArgument )),)␊
280+
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
281281
␊
282282
--------------------------------------------------------------------------------␊
283283
Suggestion 1/2: Remove the second argument.␊
284-
1 | async () => array.map(await callback)␊
284+
1 | array.map( (( 0, callback )),)␊
285285
␊
286286
--------------------------------------------------------------------------------␊
287287
Suggestion 2/2: Use a bound function.␊
288-
1 | async () => array.map((await callback).bind(thisArgument))␊
288+
1 | array.map( (( 0, callback )).bind((( thisArgument ))),)␊
289289
`
290290

291291
## Invalid #8
292-
1 | array.map((new Callback), thisArgument)
293-
294-
> Error 1/1
295-
296-
`␊
297-
> 1 | array.map((new Callback), thisArgument)␊
298-
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
299-
␊
300-
--------------------------------------------------------------------------------␊
301-
Suggestion 1/2: Remove the second argument.␊
302-
1 | array.map((new Callback))␊
303-
␊
304-
--------------------------------------------------------------------------------␊
305-
Suggestion 2/2: Use a bound function.␊
306-
1 | array.map((new Callback).bind(thisArgument))␊
307-
`
308-
309-
## Invalid #9
310-
1 | array.map(new Callback(), thisArgument)
311-
312-
> Error 1/1
313-
314-
`␊
315-
> 1 | array.map(new Callback(), thisArgument)␊
316-
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
317-
␊
318-
--------------------------------------------------------------------------------␊
319-
Suggestion 1/2: Remove the second argument.␊
320-
1 | array.map(new Callback())␊
321-
␊
322-
--------------------------------------------------------------------------------␊
323-
Suggestion 2/2: Use a bound function.␊
324-
1 | array.map(new Callback().bind(thisArgument))␊
325-
`
326-
327-
## Invalid #10
328-
1 | array.map( (( callback )), (( thisArgument )),)
329-
330-
> Error 1/1
331-
332-
`␊
333-
> 1 | array.map( (( callback )), (( thisArgument )),)␊
334-
| ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊
335-
␊
336-
--------------------------------------------------------------------------------␊
337-
Suggestion 1/2: Remove the second argument.␊
338-
1 | array.map( (( callback )),)␊
339-
␊
340-
--------------------------------------------------------------------------------␊
341-
Suggestion 2/2: Use a bound function.␊
342-
1 | array.map( (( callback )).bind((( thisArgument ))),)␊
343-
`
344-
345-
## Invalid #11
346292
1 | array.map((0, () => {}), thisArgument)
347293

348294
> Error 1/1
@@ -360,7 +306,7 @@ Generated by [AVA](https://avajs.dev).
360306
1 | array.map((0, () => {}).bind(thisArgument))␊
361307
`
362308

363-
## Invalid #12
309+
## Invalid #9
364310
1 | array.map(callback.bind(foo), thisArgument)
365311

366312
> Error 1/1

‎test/snapshots/no-array-method-this-argument.mjs.snap‎

-128 Bytes
Binary file not shown.

‎test/utils/snapshot-rule-tester.mjs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ function normalizeTests(tests) {
5959

6060
const additionalProperties = getAdditionalProperties(
6161
testCase,
62-
['code', 'options', 'filename', 'parserOptions']
62+
['code', 'options', 'filename', 'parserOptions', 'parser']
6363
);
6464

6565
if (additionalProperties.length > 0) {
@@ -72,9 +72,15 @@ function normalizeTests(tests) {
7272
}
7373

7474
function getVerifyConfig(ruleId, testerConfig, testCase) {
75-
const {options, parserOptions} = testCase;
75+
const {
76+
options,
77+
parserOptions,
78+
parser = testerConfig.parser
79+
} = testCase;
80+
7681
return {
7782
...testerConfig,
83+
parser,
7884
parserOptions: {
7985
...testerConfig.parserOptions,
8086
...parserOptions

0 commit comments

Comments
 (0)