Skip to content

Commit

Permalink
no-array-callback-reference: Check logical expressions, check terna…
Browse files Browse the repository at this point in the history
…ries deeply (#2289)
  • Loading branch information
fisker authored Feb 25, 2024
1 parent 78810a5 commit 231529a
Show file tree
Hide file tree
Showing 8 changed files with 519 additions and 58 deletions.
113 changes: 71 additions & 42 deletions rules/no-array-callback-reference.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
'use strict';
const {isParenthesized} = require('@eslint-community/eslint-utils');
const {isMethodCall} = require('./ast/index.js');
const {isNodeMatches, isNodeValueNotFunction} = require('./utils/index.js');
const {
isNodeMatches,
isNodeValueNotFunction,
isParenthesized,
getParenthesizedRange,
getParenthesizedText,
shouldAddParenthesesToCallExpressionCallee,
} = require('./utils/index.js');

const ERROR_WITH_NAME_MESSAGE_ID = 'error-with-name';
const ERROR_WITHOUT_NAME_MESSAGE_ID = 'error-without-name';
Expand All @@ -25,7 +31,7 @@ const iteratorMethods = new Map([
},
{
method: 'filter',
test: node => !(node.callee.object.type === 'Identifier' && node.callee.object.name === 'Vue'),
shouldIgnoreCallExpression: node => (node.callee.object.type === 'Identifier' && node.callee.object.name === 'Vue'),
ignore: [
'Boolean',
],
Expand Down Expand Up @@ -63,7 +69,7 @@ const iteratorMethods = new Map([
},
{
method: 'map',
test: node => !(node.callee.object.type === 'Identifier' && node.callee.object.name === 'types'),
shouldIgnoreCallExpression: node => (node.callee.object.type === 'Identifier' && node.callee.object.name === 'types'),
ignore: [
'String',
'Number',
Expand Down Expand Up @@ -104,35 +110,39 @@ const iteratorMethods = new Map([
ignore = [],
minParameters = 1,
returnsUndefined = false,
test,
shouldIgnoreCallExpression,
}) => [method, {
minParameters,
parameters,
returnsUndefined,
test(node) {
shouldIgnoreCallExpression(callExpression) {
if (
method !== 'reduce'
&& method !== 'reduceRight'
&& isAwaitExpressionArgument(node)
&& isAwaitExpressionArgument(callExpression)
) {
return false;
return true;
}

if (isNodeMatches(node.callee.object, ignoredCallee)) {
return false;
if (isNodeMatches(callExpression.callee.object, ignoredCallee)) {
return true;
}

if (node.callee.object.type === 'CallExpression' && isNodeMatches(node.callee.object.callee, ignoredCallee)) {
return false;
if (
callExpression.callee.object.type === 'CallExpression'
&& isNodeMatches(callExpression.callee.object.callee, ignoredCallee)
) {
return true;
}

const [callback] = node.arguments;

return shouldIgnoreCallExpression?.(callExpression) ?? false;
},
shouldIgnoreCallback(callback) {
if (callback.type === 'Identifier' && ignore.includes(callback.name)) {
return false;
return true;
}

return !test || test(node);
return false;
},
}]));

Expand Down Expand Up @@ -163,9 +173,14 @@ function getProblem(context, node, method, options) {
name,
method,
},
suggest: [],
};

if (node.type === 'YieldExpression' || node.type === 'AwaitExpression') {
return problem;
}

problem.suggest = [];

const {parameters, minParameters, returnsUndefined} = options;
for (let parameterLength = minParameters; parameterLength <= parameters.length; parameterLength++) {
const suggestionParameters = parameters.slice(0, parameterLength).join(', ');
Expand All @@ -178,16 +193,20 @@ function getProblem(context, node, method, options) {
},
fix(fixer) {
const {sourceCode} = context;
let nodeText = sourceCode.getText(node);
if (isParenthesized(node, sourceCode) || type === 'ConditionalExpression') {
nodeText = `(${nodeText})`;
let text = getParenthesizedText(node, sourceCode);

if (
!isParenthesized(node, sourceCode)
&& shouldAddParenthesesToCallExpressionCallee(node)
) {
text = `(${text})`;
}

return fixer.replaceText(
node,
return fixer.replaceTextRange(
getParenthesizedRange(node, sourceCode),
returnsUndefined
? `(${suggestionParameters}) => { ${nodeText}(${suggestionParameters}); }`
: `(${suggestionParameters}) => ${nodeText}(${suggestionParameters})`,
? `(${suggestionParameters}) => { ${text}(${suggestionParameters}); }`
: `(${suggestionParameters}) => ${text}(${suggestionParameters})`,
);
},
};
Expand All @@ -198,47 +217,57 @@ function getProblem(context, node, method, options) {
return problem;
}

function * getTernaryConsequentAndALternate(node) {
if (node.type === 'ConditionalExpression') {
yield * getTernaryConsequentAndALternate(node.consequent);
yield * getTernaryConsequentAndALternate(node.alternate);
return;
}

yield node;
}

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => ({
CallExpression(node) {
* CallExpression(callExpression) {
if (
!isMethodCall(node, {
!isMethodCall(callExpression, {
minimumArguments: 1,
maximumArguments: 2,
optionalCall: false,
optionalMember: false,
computed: false,
})
|| node.callee.property.type !== 'Identifier'
|| callExpression.callee.property.type !== 'Identifier'
) {
return;
}

const methodNode = node.callee.property;
const methodNode = callExpression.callee.property;
const methodName = methodNode.name;
if (!iteratorMethods.has(methodName)) {
return;
}

const [callback] = node.arguments;

if (
callback.type === 'FunctionExpression'
|| callback.type === 'ArrowFunctionExpression'
// Ignore all `CallExpression`s include `function.bind()`
|| callback.type === 'CallExpression'
|| isNodeValueNotFunction(callback)
) {
const options = iteratorMethods.get(methodName);
if (options.shouldIgnoreCallExpression(callExpression)) {
return;
}

const options = iteratorMethods.get(methodName);
for (const callback of getTernaryConsequentAndALternate(callExpression.arguments[0])) {
if (
callback.type === 'FunctionExpression'
|| callback.type === 'ArrowFunctionExpression'
// Ignore all `CallExpression`s include `function.bind()`
|| callback.type === 'CallExpression'
|| options.shouldIgnoreCallback(callback)
|| isNodeValueNotFunction(callback)
) {
continue;
}

if (!options.test(node)) {
return;
yield getProblem(context, callback, methodName, options);
}

return getProblem(context, callback, methodName, options);
},
});

Expand Down
1 change: 1 addition & 0 deletions rules/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module.exports = {
isValueNotUsable: require('./is-value-not-usable.js'),
needsSemicolon: require('./needs-semicolon.js'),
shouldAddParenthesesToMemberExpressionObject: require('./should-add-parentheses-to-member-expression-object.js'),
shouldAddParenthesesToCallExpressionCallee: require('./should-add-parentheses-to-call-expression-callee.js'),
shouldAddParenthesesToAwaitExpressionArgument: require('./should-add-parentheses-to-await-expression-argument.js'),
singular: require('./singular.js'),
toLocation: require('./to-location.js'),
Expand Down
1 change: 0 additions & 1 deletion rules/utils/is-node-value-not-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ const impossibleNodeTypes = new Set([
const mostLikelyNotNodeTypes = new Set([
'AssignmentExpression',
'AwaitExpression',
'LogicalExpression',
'NewExpression',
'TaggedTemplateExpression',
'ThisExpression',
Expand Down
22 changes: 22 additions & 0 deletions rules/utils/should-add-parentheses-to-call-expression-callee.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';

/**
Check if parentheses should be added to a `node` when it's used as `callee` of `CallExpression`.
@param {Node} node - The AST node to check.
@returns {boolean}
*/
function shouldAddParenthesesToCallExpressionCallee(node) {
return node.type === 'SequenceExpression'
|| node.type === 'YieldExpression'
|| node.type === 'ArrowFunctionExpression'
|| node.type === 'ConditionalExpression'
|| node.type === 'AssignmentExpression'
|| node.type === 'LogicalExpression'
|| node.type === 'BinaryExpression'
|| node.type === 'UnaryExpression'
|| node.type === 'UpdateExpression'
|| node.type === 'NewExpression';
}

module.exports = shouldAddParenthesesToCallExpressionCallee;
63 changes: 49 additions & 14 deletions test/no-array-callback-reference.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -265,25 +265,16 @@ test({
),

// Need parenthesized

invalidTestCase({
code: 'foo.map(a ? b : c)',
code: 'foo.map(a || b)',
method: 'map',
suggestions: [
'foo.map((element) => (a ? b : c)(element))',
'foo.map((element, index) => (a ? b : c)(element, index))',
'foo.map((element, index, array) => (a ? b : c)(element, index, array))',
'foo.map((element) => (a || b)(element))',
'foo.map((element, index) => (a || b)(element, index))',
'foo.map((element, index, array) => (a || b)(element, index, array))',
],
}),
// Note: `await` is not handled, not sure if this is needed
// invalidTestCase({
// code: `foo.map(await foo())`,
// method: 'map',
// suggestions: [
// `foo.map(async (accumulator, element) => (await foo())(accumulator, element))`,
// `foo.map(async (accumulator, element, index) => (await foo())(accumulator, element, index))`,
// `foo.map(async (accumulator, element, index, array) => (await foo())(accumulator, element, index, array))`
// ]
// }),

// Actual messages
{
Expand Down Expand Up @@ -444,3 +435,47 @@ test({
}),
],
});

// Ternaries
test.snapshot({
valid: [
'foo.map(_ ? () => {} : _ ? () => {} : () => {})',
'foo.reduce(_ ? () => {} : _ ? () => {} : () => {})',
'foo.every(_ ? Boolean : _ ? Boolean : Boolean)',
'foo.map(_ ? String : _ ? Number : Boolean)',
],
invalid: [
outdent`
foo.map(
_
? String // This one should be ignored
: callback
);
`,
outdent`
foo.forEach(
_
? callbackA
: _
? callbackB
: callbackC
);
`,
// Needs parentheses
// Some of them was ignored since we know they are not callback function
outdent`
async function * foo () {
foo.map((0, bar));
foo.map(yield bar);
foo.map(yield* bar);
foo.map(() => bar);
foo.map(bar &&= baz);
foo.map(bar || baz);
foo.map(bar + bar);
foo.map(+ bar);
foo.map(++ bar);
foo.map(new Function(''));
}
`,
],
});
Loading

0 comments on commit 231529a

Please sign in to comment.