Skip to content

Commit 231529a

Browse files
fiskerfisker
authored
no-array-callback-reference: Check logical expressions, check ternaries deeply (#2289)
1 parent 78810a5 commit 231529a

8 files changed

+519
-58
lines changed

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

+71-42
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
'use strict';
2-
const {isParenthesized} = require('@eslint-community/eslint-utils');
32
const {isMethodCall} = require('./ast/index.js');
4-
const {isNodeMatches, isNodeValueNotFunction} = require('./utils/index.js');
3+
const {
4+
isNodeMatches,
5+
isNodeValueNotFunction,
6+
isParenthesized,
7+
getParenthesizedRange,
8+
getParenthesizedText,
9+
shouldAddParenthesesToCallExpressionCallee,
10+
} = require('./utils/index.js');
511

612
const ERROR_WITH_NAME_MESSAGE_ID = 'error-with-name';
713
const ERROR_WITHOUT_NAME_MESSAGE_ID = 'error-without-name';
@@ -25,7 +31,7 @@ const iteratorMethods = new Map([
2531
},
2632
{
2733
method: 'filter',
28-
test: node => !(node.callee.object.type === 'Identifier' && node.callee.object.name === 'Vue'),
34+
shouldIgnoreCallExpression: node => (node.callee.object.type === 'Identifier' && node.callee.object.name === 'Vue'),
2935
ignore: [
3036
'Boolean',
3137
],
@@ -63,7 +69,7 @@ const iteratorMethods = new Map([
6369
},
6470
{
6571
method: 'map',
66-
test: node => !(node.callee.object.type === 'Identifier' && node.callee.object.name === 'types'),
72+
shouldIgnoreCallExpression: node => (node.callee.object.type === 'Identifier' && node.callee.object.name === 'types'),
6773
ignore: [
6874
'String',
6975
'Number',
@@ -104,35 +110,39 @@ const iteratorMethods = new Map([
104110
ignore = [],
105111
minParameters = 1,
106112
returnsUndefined = false,
107-
test,
113+
shouldIgnoreCallExpression,
108114
}) => [method, {
109115
minParameters,
110116
parameters,
111117
returnsUndefined,
112-
test(node) {
118+
shouldIgnoreCallExpression(callExpression) {
113119
if (
114120
method !== 'reduce'
115121
&& method !== 'reduceRight'
116-
&& isAwaitExpressionArgument(node)
122+
&& isAwaitExpressionArgument(callExpression)
117123
) {
118-
return false;
124+
return true;
119125
}
120126

121-
if (isNodeMatches(node.callee.object, ignoredCallee)) {
122-
return false;
127+
if (isNodeMatches(callExpression.callee.object, ignoredCallee)) {
128+
return true;
123129
}
124130

125-
if (node.callee.object.type === 'CallExpression' && isNodeMatches(node.callee.object.callee, ignoredCallee)) {
126-
return false;
131+
if (
132+
callExpression.callee.object.type === 'CallExpression'
133+
&& isNodeMatches(callExpression.callee.object.callee, ignoredCallee)
134+
) {
135+
return true;
127136
}
128137

129-
const [callback] = node.arguments;
130-
138+
return shouldIgnoreCallExpression?.(callExpression) ?? false;
139+
},
140+
shouldIgnoreCallback(callback) {
131141
if (callback.type === 'Identifier' && ignore.includes(callback.name)) {
132-
return false;
142+
return true;
133143
}
134144

135-
return !test || test(node);
145+
return false;
136146
},
137147
}]));
138148

@@ -163,9 +173,14 @@ function getProblem(context, node, method, options) {
163173
name,
164174
method,
165175
},
166-
suggest: [],
167176
};
168177

178+
if (node.type === 'YieldExpression' || node.type === 'AwaitExpression') {
179+
return problem;
180+
}
181+
182+
problem.suggest = [];
183+
169184
const {parameters, minParameters, returnsUndefined} = options;
170185
for (let parameterLength = minParameters; parameterLength <= parameters.length; parameterLength++) {
171186
const suggestionParameters = parameters.slice(0, parameterLength).join(', ');
@@ -178,16 +193,20 @@ function getProblem(context, node, method, options) {
178193
},
179194
fix(fixer) {
180195
const {sourceCode} = context;
181-
let nodeText = sourceCode.getText(node);
182-
if (isParenthesized(node, sourceCode) || type === 'ConditionalExpression') {
183-
nodeText = `(${nodeText})`;
196+
let text = getParenthesizedText(node, sourceCode);
197+
198+
if (
199+
!isParenthesized(node, sourceCode)
200+
&& shouldAddParenthesesToCallExpressionCallee(node)
201+
) {
202+
text = `(${text})`;
184203
}
185204

186-
return fixer.replaceText(
187-
node,
205+
return fixer.replaceTextRange(
206+
getParenthesizedRange(node, sourceCode),
188207
returnsUndefined
189-
? `(${suggestionParameters}) => { ${nodeText}(${suggestionParameters}); }`
190-
: `(${suggestionParameters}) => ${nodeText}(${suggestionParameters})`,
208+
? `(${suggestionParameters}) => { ${text}(${suggestionParameters}); }`
209+
: `(${suggestionParameters}) => ${text}(${suggestionParameters})`,
191210
);
192211
},
193212
};
@@ -198,47 +217,57 @@ function getProblem(context, node, method, options) {
198217
return problem;
199218
}
200219

220+
function * getTernaryConsequentAndALternate(node) {
221+
if (node.type === 'ConditionalExpression') {
222+
yield * getTernaryConsequentAndALternate(node.consequent);
223+
yield * getTernaryConsequentAndALternate(node.alternate);
224+
return;
225+
}
226+
227+
yield node;
228+
}
229+
201230
/** @param {import('eslint').Rule.RuleContext} context */
202231
const create = context => ({
203-
CallExpression(node) {
232+
* CallExpression(callExpression) {
204233
if (
205-
!isMethodCall(node, {
234+
!isMethodCall(callExpression, {
206235
minimumArguments: 1,
207236
maximumArguments: 2,
208237
optionalCall: false,
209238
optionalMember: false,
210239
computed: false,
211240
})
212-
|| node.callee.property.type !== 'Identifier'
241+
|| callExpression.callee.property.type !== 'Identifier'
213242
) {
214243
return;
215244
}
216245

217-
const methodNode = node.callee.property;
246+
const methodNode = callExpression.callee.property;
218247
const methodName = methodNode.name;
219248
if (!iteratorMethods.has(methodName)) {
220249
return;
221250
}
222251

223-
const [callback] = node.arguments;
224-
225-
if (
226-
callback.type === 'FunctionExpression'
227-
|| callback.type === 'ArrowFunctionExpression'
228-
// Ignore all `CallExpression`s include `function.bind()`
229-
|| callback.type === 'CallExpression'
230-
|| isNodeValueNotFunction(callback)
231-
) {
252+
const options = iteratorMethods.get(methodName);
253+
if (options.shouldIgnoreCallExpression(callExpression)) {
232254
return;
233255
}
234256

235-
const options = iteratorMethods.get(methodName);
257+
for (const callback of getTernaryConsequentAndALternate(callExpression.arguments[0])) {
258+
if (
259+
callback.type === 'FunctionExpression'
260+
|| callback.type === 'ArrowFunctionExpression'
261+
// Ignore all `CallExpression`s include `function.bind()`
262+
|| callback.type === 'CallExpression'
263+
|| options.shouldIgnoreCallback(callback)
264+
|| isNodeValueNotFunction(callback)
265+
) {
266+
continue;
267+
}
236268

237-
if (!options.test(node)) {
238-
return;
269+
yield getProblem(context, callback, methodName, options);
239270
}
240-
241-
return getProblem(context, callback, methodName, options);
242271
},
243272
});
244273

‎rules/utils/index.js

+1
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ module.exports = {
4545
isValueNotUsable: require('./is-value-not-usable.js'),
4646
needsSemicolon: require('./needs-semicolon.js'),
4747
shouldAddParenthesesToMemberExpressionObject: require('./should-add-parentheses-to-member-expression-object.js'),
48+
shouldAddParenthesesToCallExpressionCallee: require('./should-add-parentheses-to-call-expression-callee.js'),
4849
shouldAddParenthesesToAwaitExpressionArgument: require('./should-add-parentheses-to-await-expression-argument.js'),
4950
singular: require('./singular.js'),
5051
toLocation: require('./to-location.js'),

‎rules/utils/is-node-value-not-function.js

-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ const impossibleNodeTypes = new Set([
1919
const mostLikelyNotNodeTypes = new Set([
2020
'AssignmentExpression',
2121
'AwaitExpression',
22-
'LogicalExpression',
2322
'NewExpression',
2423
'TaggedTemplateExpression',
2524
'ThisExpression',

‎rules/utils/should-add-parentheses-to-call-expression-callee.js

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
'use strict';
2+
3+
/**
4+
Check if parentheses should be added to a `node` when it's used as `callee` of `CallExpression`.
5+
6+
@param {Node} node - The AST node to check.
7+
@returns {boolean}
8+
*/
9+
function shouldAddParenthesesToCallExpressionCallee(node) {
10+
return node.type === 'SequenceExpression'
11+
|| node.type === 'YieldExpression'
12+
|| node.type === 'ArrowFunctionExpression'
13+
|| node.type === 'ConditionalExpression'
14+
|| node.type === 'AssignmentExpression'
15+
|| node.type === 'LogicalExpression'
16+
|| node.type === 'BinaryExpression'
17+
|| node.type === 'UnaryExpression'
18+
|| node.type === 'UpdateExpression'
19+
|| node.type === 'NewExpression';
20+
}
21+
22+
module.exports = shouldAddParenthesesToCallExpressionCallee;

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

+49-14
Original file line numberDiff line numberDiff line change
@@ -265,25 +265,16 @@ test({
265265
),
266266

267267
// Need parenthesized
268+
268269
invalidTestCase({
269-
code: 'foo.map(a ? b : c)',
270+
code: 'foo.map(a || b)',
270271
method: 'map',
271272
suggestions: [
272-
'foo.map((element) => (a ? b : c)(element))',
273-
'foo.map((element, index) => (a ? b : c)(element, index))',
274-
'foo.map((element, index, array) => (a ? b : c)(element, index, array))',
273+
'foo.map((element) => (a || b)(element))',
274+
'foo.map((element, index) => (a || b)(element, index))',
275+
'foo.map((element, index, array) => (a || b)(element, index, array))',
275276
],
276277
}),
277-
// Note: `await` is not handled, not sure if this is needed
278-
// invalidTestCase({
279-
// code: `foo.map(await foo())`,
280-
// method: 'map',
281-
// suggestions: [
282-
// `foo.map(async (accumulator, element) => (await foo())(accumulator, element))`,
283-
// `foo.map(async (accumulator, element, index) => (await foo())(accumulator, element, index))`,
284-
// `foo.map(async (accumulator, element, index, array) => (await foo())(accumulator, element, index, array))`
285-
// ]
286-
// }),
287278

288279
// Actual messages
289280
{
@@ -444,3 +435,47 @@ test({
444435
}),
445436
],
446437
});
438+
439+
// Ternaries
440+
test.snapshot({
441+
valid: [
442+
'foo.map(_ ? () => {} : _ ? () => {} : () => {})',
443+
'foo.reduce(_ ? () => {} : _ ? () => {} : () => {})',
444+
'foo.every(_ ? Boolean : _ ? Boolean : Boolean)',
445+
'foo.map(_ ? String : _ ? Number : Boolean)',
446+
],
447+
invalid: [
448+
outdent`
449+
foo.map(
450+
_
451+
? String // This one should be ignored
452+
: callback
453+
);
454+
`,
455+
outdent`
456+
foo.forEach(
457+
_
458+
? callbackA
459+
: _
460+
? callbackB
461+
: callbackC
462+
);
463+
`,
464+
// Needs parentheses
465+
// Some of them was ignored since we know they are not callback function
466+
outdent`
467+
async function * foo () {
468+
foo.map((0, bar));
469+
foo.map(yield bar);
470+
foo.map(yield* bar);
471+
foo.map(() => bar);
472+
foo.map(bar &&= baz);
473+
foo.map(bar || baz);
474+
foo.map(bar + bar);
475+
foo.map(+ bar);
476+
foo.map(++ bar);
477+
foo.map(new Function(''));
478+
}
479+
`,
480+
],
481+
});

0 commit comments

Comments
 (0)