Skip to content

Commit 96587f2

Browse files
fiskerfisker
authored
Add no-array-method-this-argument rule (#1357)
1 parent f2f5235 commit 96587f2

10 files changed

+638
-0
lines changed

‎docs/rules/no-array-method-this-argument.md‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# Disallow using the `this` argument in array methods
2+
3+
The rule forbids using the `thisArg` argument in array methods:
4+
5+
- If the callback is an arrow function or a bound function, the `thisArg` won't affect it.
6+
- If you intent to use a custom `this` in the callback, it‘s better to use the variable directly or use `callback.bind(thisArg)`.
7+
8+
This rule checks following array methods accepts `thisArg`:
9+
10+
- [`Array#every()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/every)
11+
- [`Array#filter()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/filter)
12+
- [`Array#find()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/find)
13+
- [`Array#findIndex()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/findIndex)
14+
- [`Array#flatMap()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/flatMap)
15+
- [`Array#forEach()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/forEach)
16+
- [`Array#map()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/map)
17+
- [`Array#some()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/some)
18+
19+
This rule is fixable when the callback is an arrow function and the `thisArg` argument has no side effect.
20+
21+
## Fail
22+
23+
```js
24+
const foo = bar.find(element => isUnicorn(element), baz);
25+
```
26+
27+
```js
28+
const foo = bar.map(function (element) => {
29+
return this.unicorn(element);
30+
}, baz);
31+
```
32+
33+
## Pass
34+
35+
```js
36+
const foo = bar.find(element => isUnicorn(element));
37+
```
38+
39+
```js
40+
const foo = bar.map(function (element) => {
41+
return baz.unicorn(element);
42+
});
43+
```
44+
45+
```js
46+
const foo = bar.map(function (element) => {
47+
return this.unicorn(element);
48+
}.bind(baz));
49+
```

‎index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ module.exports = {
5555
'unicorn/no-abusive-eslint-disable': 'error',
5656
'unicorn/no-array-callback-reference': 'error',
5757
'unicorn/no-array-for-each': 'error',
58+
'unicorn/no-array-method-this-argument': 'error',
5859
'unicorn/no-array-push-push': 'error',
5960
'unicorn/no-array-reduce': 'error',
6061
'unicorn/no-console-spaces': 'error',

‎readme.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ Configure it in `package.json`.
5252
"unicorn/no-abusive-eslint-disable": "error",
5353
"unicorn/no-array-callback-reference": "error",
5454
"unicorn/no-array-for-each": "error",
55+
"unicorn/no-array-method-this-argument": "error",
5556
"unicorn/no-array-push-push": "error",
5657
"unicorn/no-array-reduce": "error",
5758
"unicorn/no-console-spaces": "error",
@@ -156,6 +157,7 @@ Each rule has emojis denoting:
156157
| [no-abusive-eslint-disable](docs/rules/no-abusive-eslint-disable.md) | Enforce specifying rules to disable in `eslint-disable` comments. | ✅ | | |
157158
| [no-array-callback-reference](docs/rules/no-array-callback-reference.md) | Prevent passing a function reference directly to iterator methods. | ✅ | | 💡 |
158159
| [no-array-for-each](docs/rules/no-array-for-each.md) | Prefer `for…of` over `Array#forEach(…)`. | ✅ | 🔧 | |
160+
| [no-array-method-this-argument](docs/rules/no-array-method-this-argument.md) | Disallow using the `this` argument in array methods. | ✅ | 🔧 | 💡 |
159161
| [no-array-push-push](docs/rules/no-array-push-push.md) | Enforce combining multiple `Array#push()` into one call. | ✅ | 🔧 | 💡 |
160162
| [no-array-reduce](docs/rules/no-array-reduce.md) | Disallow `Array#reduce()` and `Array#reduceRight()`. | ✅ | | |
161163
| [no-console-spaces](docs/rules/no-console-spaces.md) | Do not use leading/trailing space between `console.log` parameters. | ✅ | 🔧 | |

‎rules/fix/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,6 @@
22

33
module.exports = {
44
appendArgument: require('./append-argument.js'),
5+
removeArgument: require('./remove-argument.js'),
56
switchNewExpressionToCallExpression: require('./switch-new-expression-to-call-expression.js')
67
};

‎rules/fix/remove-argument.js‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
'use strict';
2+
const {isCommaToken} = require('eslint-utils');
3+
const {getParentheses} = require('../utils/parentheses.js');
4+
5+
function removeArgument(fixer, node, sourceCode) {
6+
const callExpression = node.parent;
7+
const index = callExpression.arguments.indexOf(node);
8+
const parentheses = getParentheses(node, sourceCode);
9+
const firstToken = parentheses[0] || node;
10+
const lastToken = parentheses[parentheses.length - 1] || node;
11+
12+
let [start] = firstToken.range;
13+
let [, end] = lastToken.range;
14+
15+
if (index !== 0) {
16+
start = sourceCode.getTokenBefore(firstToken).range[0];
17+
}
18+
19+
// If the removed argument is the only argument, the trailing comma must be removed too
20+
/* istanbul ignore next: Not reachable for now */
21+
if (callExpression.arguments.length === 1) {
22+
const tokenAfter = sourceCode.getTokenBefore(lastToken);
23+
if (isCommaToken(tokenAfter)) {
24+
end = tokenAfter[1];
25+
}
26+
}
27+
28+
return fixer.replaceTextRange([start, end], '');
29+
}
30+
31+
module.exports = removeArgument;

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

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
'use strict';
2+
const {hasSideEffect} = require('eslint-utils');
3+
const {methodCallSelector} = require('./selectors/index.js');
4+
const {removeArgument} = require('./fix/index.js');
5+
const {getParentheses, getParenthesizedText} = require('./utils/parentheses.js');
6+
const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object.js');
7+
8+
const ERROR = 'error';
9+
const SUGGESTION_BIND = 'suggestion-bind';
10+
const SUGGESTION_REMOVE = 'suggestion-remove';
11+
const messages = {
12+
[ERROR]: 'Do not use the `this` argument in `Array#{{method}}()`.',
13+
[SUGGESTION_REMOVE]: 'Remove the second argument.',
14+
[SUGGESTION_BIND]: 'Use a bound function.'
15+
};
16+
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+
});
30+
31+
function removeThisArgument(callExpression, sourceCode) {
32+
return fixer => removeArgument(fixer, callExpression.arguments[1], sourceCode);
33+
}
34+
35+
function useBoundFunction(callExpression, sourceCode) {
36+
return function * (fixer) {
37+
yield removeThisArgument(callExpression, sourceCode)(fixer);
38+
39+
const [callback, thisArgument] = callExpression.arguments;
40+
41+
const callbackParentheses = getParentheses(callback, sourceCode);
42+
const isParenthesized = callbackParentheses.length > 0;
43+
const callbackLastToken = isParenthesized ?
44+
callbackParentheses[callbackParentheses.length - 1] :
45+
callback;
46+
if (
47+
!isParenthesized &&
48+
shouldAddParenthesesToMemberExpressionObject(callback, sourceCode)
49+
) {
50+
yield fixer.insertTextBefore(callbackLastToken, '(');
51+
yield fixer.insertTextAfter(callbackLastToken, ')');
52+
}
53+
54+
const thisArgumentText = getParenthesizedText(thisArgument, sourceCode);
55+
// `thisArgument` was a argument, no need add extra parentheses
56+
yield fixer.insertTextAfter(callbackLastToken, `.bind(${thisArgumentText})`);
57+
};
58+
}
59+
60+
/** @param {import('eslint').Rule.RuleContext} context */
61+
const create = context => {
62+
const sourceCode = context.getSourceCode();
63+
64+
return {
65+
[selector](callExpression) {
66+
const method = callExpression.callee.property.name;
67+
const [callback, thisArgument] = callExpression.arguments;
68+
69+
const problem = {
70+
node: thisArgument,
71+
messageId: ERROR,
72+
data: {method}
73+
};
74+
75+
const thisArgumentHasSideEffect = hasSideEffect(thisArgument, sourceCode);
76+
const isArrowCallback = callback.type === 'ArrowFunctionExpression';
77+
78+
if (isArrowCallback) {
79+
if (thisArgumentHasSideEffect) {
80+
problem.suggest = [
81+
{
82+
messageId: SUGGESTION_REMOVE,
83+
fix: removeThisArgument(callExpression, sourceCode)
84+
}
85+
];
86+
} else {
87+
problem.fix = removeThisArgument(callExpression, sourceCode);
88+
}
89+
90+
return problem;
91+
}
92+
93+
problem.suggest = [
94+
{
95+
messageId: SUGGESTION_REMOVE,
96+
fix: removeThisArgument(callExpression, sourceCode)
97+
},
98+
{
99+
messageId: SUGGESTION_BIND,
100+
fix: useBoundFunction(callExpression, sourceCode)
101+
}
102+
];
103+
104+
return problem;
105+
}
106+
};
107+
};
108+
109+
module.exports = {
110+
create,
111+
meta: {
112+
type: 'suggestion',
113+
docs: {
114+
description: 'Disallow using the `this` argument in array methods.'
115+
},
116+
fixable: 'code',
117+
messages,
118+
hasSuggestions: true
119+
}
120+
};

‎rules/utils/should-add-parentheses-to-member-expression-object.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ function shouldAddParenthesesToMemberExpressionObject(node, sourceCode) {
2121
case 'TemplateLiteral':
2222
case 'ThisExpression':
2323
case 'ArrayExpression':
24+
case 'FunctionExpression':
2425
return false;
2526
case 'NewExpression':
2627
return !isNewExpressionWithParentheses(node, sourceCode);

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import {getTester} from './utils/test.mjs';
2+
3+
const {test} = getTester(import.meta);
4+
5+
// Arrow functions
6+
test.snapshot({
7+
valid: [
8+
'array.unknownMethod(() => {}, thisArgument)',
9+
'new array.map(() => {}, thisArgument)',
10+
'array.map?.(() => {}, thisArgument)',
11+
'array?.map(() => {}, thisArgument)',
12+
// More or less arguments
13+
'array.map()',
14+
'array.map(() => {},)',
15+
'array.map(() => {}, ...thisArgument)',
16+
'array.map(...() => {}, thisArgument)',
17+
'array.map(() => {}, thisArgument, extraArgument)'
18+
],
19+
invalid: [
20+
'array.every(() => {}, thisArgument)',
21+
'array.filter(() => {}, thisArgument)',
22+
'array.find(() => {}, thisArgument)',
23+
'array.findIndex(() => {}, thisArgument)',
24+
'array.flatMap(() => {}, thisArgument)',
25+
'array.forEach(() => {}, thisArgument)',
26+
'array.map(() => {}, thisArgument)',
27+
// Comma
28+
'array.map(() => {}, thisArgument,)',
29+
'array.map(() => {}, (0, thisArgument),)',
30+
// Side effect
31+
'array.map(() => {}, thisArgumentHasSideEffect())'
32+
]
33+
});
34+
35+
// Non-arrow functions
36+
test.snapshot({
37+
valid: [],
38+
invalid: [
39+
'array.map(callback, thisArgument)',
40+
'array.map(callback, (0, thisArgument))',
41+
'array.map(function () {}, thisArgument)',
42+
'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 )),)',
49+
// This callback is actually arrow function, but we don't know
50+
'array.map((0, () => {}), thisArgument)',
51+
// This callback is a bound function, but we don't know
52+
'array.map(callback.bind(foo), thisArgument)'
53+
]
54+
});

0 commit comments

Comments
 (0)