Skip to content

Commit

Permalink
no-array-method-this-argument: Check Array.from() (#2262)
Browse files Browse the repository at this point in the history
  • Loading branch information
@fisker
fisker authored Jan 18, 2024
1 parent 19ffd54 commit 797caee
Show file tree
Hide file tree
Showing 4 changed files with 427 additions and 88 deletions.
188 changes: 112 additions & 76 deletions rules/no-array-method-this-argument.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ const {isNodeMatches} = require('./utils/is-node-matches.js');
const {isNodeValueNotFunction} = require('./utils/index.js');
const {isMethodCall} = require('./ast/index.js');

const ERROR = 'error';
const ERROR_PROTOTYPE_METHOD = 'error-prototype-method';
const ERROR_STATIC_METHOD = 'error-static-method';
const SUGGESTION_BIND = 'suggestion-bind';
const SUGGESTION_REMOVE = 'suggestion-remove';
const messages = {
[ERROR]: 'Do not use the `this` argument in `Array#{{method}}()`.',
[ERROR_PROTOTYPE_METHOD]: 'Do not use the `this` argument in `Array#{{method}}()`.',
[ERROR_STATIC_METHOD]: 'Do not use the `this` argument in `Array.{{method}}()`.',
[SUGGESTION_REMOVE]: 'Remove the second argument.',
[SUGGESTION_BIND]: 'Use a bound function.',
};
Expand Down Expand Up @@ -70,107 +72,141 @@ const ignored = [
'underscore.some',
];

function removeThisArgument(callExpression, sourceCode) {
return fixer => removeArgument(fixer, callExpression.arguments[1], sourceCode);
function removeThisArgument(thisArgumentNode, sourceCode) {
return fixer => removeArgument(fixer, thisArgumentNode, sourceCode);
}

function useBoundFunction(callExpression, sourceCode) {
function useBoundFunction(callbackNode, thisArgumentNode, sourceCode) {
return function * (fixer) {
yield removeThisArgument(callExpression, sourceCode)(fixer);
yield removeThisArgument(thisArgumentNode, sourceCode)(fixer);

const [callback, thisArgument] = callExpression.arguments;

const callbackParentheses = getParentheses(callback, sourceCode);
const callbackParentheses = getParentheses(callbackNode, sourceCode);
const isParenthesized = callbackParentheses.length > 0;
const callbackLastToken = isParenthesized
? callbackParentheses.at(-1)
: callback;
: callbackNode;
if (
!isParenthesized
&& shouldAddParenthesesToMemberExpressionObject(callback, sourceCode)
&& shouldAddParenthesesToMemberExpressionObject(callbackNode, sourceCode)
) {
yield fixer.insertTextBefore(callbackLastToken, '(');
yield fixer.insertTextAfter(callbackLastToken, ')');
}

const thisArgumentText = getParenthesizedText(thisArgument, sourceCode);
const thisArgumentText = getParenthesizedText(thisArgumentNode, sourceCode);
// `thisArgument` was a argument, no need add extra parentheses
yield fixer.insertTextAfter(callbackLastToken, `.bind(${thisArgumentText})`);
};
}

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => {
const {sourceCode} = context;

return {
CallExpression(callExpression) {
if (
!isMethodCall(callExpression, {
methods: [
'every',
'filter',
'find',
'findLast',
'findIndex',
'findLastIndex',
'flatMap',
'forEach',
'map',
'some',
],
argumentsLength: 2,
optionalCall: false,
optionalMember: false,
})
|| isNodeMatches(callExpression.callee, ignored)
|| isNodeValueNotFunction(callExpression.arguments[0])
) {
return;
}

const {callee} = callExpression;
const method = callee.property.name;
const [callback, thisArgument] = callExpression.arguments;

const problem = {
node: thisArgument,
messageId: ERROR,
data: {method},
};

const thisArgumentHasSideEffect = hasSideEffect(thisArgument, sourceCode);
const isArrowCallback = callback.type === 'ArrowFunctionExpression';

if (isArrowCallback) {
if (thisArgumentHasSideEffect) {
problem.suggest = [
{
messageId: SUGGESTION_REMOVE,
fix: removeThisArgument(callExpression, sourceCode),
},
];
} else {
problem.fix = removeThisArgument(callExpression, sourceCode);
}

return problem;
}
function getProblem({
sourceCode,
callExpression,
callbackNode,
thisArgumentNode,
messageId,
}) {
const problem = {
node: thisArgumentNode,
messageId,
data: {
method: callExpression.callee.property.name,
},
};

const isArrowCallback = callbackNode.type === 'ArrowFunctionExpression';
if (isArrowCallback) {
const thisArgumentHasSideEffect = hasSideEffect(thisArgumentNode, sourceCode);
if (thisArgumentHasSideEffect) {
problem.suggest = [
{
messageId: SUGGESTION_REMOVE,
fix: removeThisArgument(callExpression, sourceCode),
},
{
messageId: SUGGESTION_BIND,
fix: useBoundFunction(callExpression, sourceCode),
fix: removeThisArgument(thisArgumentNode, sourceCode),
},
];
} else {
problem.fix = removeThisArgument(thisArgumentNode, sourceCode);
}

return problem;
}

return problem;
problem.suggest = [
{
messageId: SUGGESTION_REMOVE,
fix: removeThisArgument(thisArgumentNode, sourceCode),
},
};
{
messageId: SUGGESTION_BIND,
fix: useBoundFunction(callbackNode, thisArgumentNode, sourceCode),
},
];

return problem;
}

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => {
const {sourceCode} = context;

// Prototype methods
context.on('CallExpression', callExpression => {
if (
!isMethodCall(callExpression, {
methods: [
'every',
'filter',
'find',
'findLast',
'findIndex',
'findLastIndex',
'flatMap',
'forEach',
'map',
'some',
],
argumentsLength: 2,
optionalCall: false,
optionalMember: false,
})
|| isNodeMatches(callExpression.callee, ignored)
|| isNodeValueNotFunction(callExpression.arguments[0])
) {
return;
}

return getProblem({
sourceCode,
callExpression,
callbackNode: callExpression.arguments[0],
thisArgumentNode: callExpression.arguments[1],
messageId: ERROR_PROTOTYPE_METHOD,
});
});

// `Array.from()`
context.on('CallExpression', callExpression => {
if (
!isMethodCall(callExpression, {
object: 'Array',
method: 'from',
argumentsLength: 3,
optionalCall: false,
optionalMember: false,
})
|| isNodeValueNotFunction(callExpression.arguments[1])
) {
return;
}

return getProblem({
sourceCode,
callExpression,
callbackNode: callExpression.arguments[1],
thisArgumentNode: callExpression.arguments[2],
messageId: ERROR_STATIC_METHOD,
});
});
};

/** @type {import('eslint').Rule.RuleModule} */
Expand Down
37 changes: 36 additions & 1 deletion test/no-array-method-this-argument.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,26 @@ test.snapshot({
'new array.map(() => {}, thisArgument)',
'array.map?.(() => {}, thisArgument)',
'array?.map(() => {}, thisArgument)',
'Array.unknownMethod(iterableOrArrayLike, () => {}, thisArgument)',
'new Array.from(iterableOrArrayLike, () => {}, thisArgument)',
'Array.from?.(iterableOrArrayLike, () => {}, thisArgument)',
'Array?.from(iterableOrArrayLike, () => {}, thisArgument)',
'NotArray.from(iterableOrArrayLike, () => {}, thisArgument)',

// More or less arguments
'array.map()',
'array.map(() => {},)',
'array.map(() => {}, ...thisArgument)',
'array.map(...() => {}, thisArgument)',
'array.map(() => {}, thisArgument, extraArgument)',
'Array.from()',
'Array.from(iterableOrArrayLike)',
'Array.from(iterableOrArrayLike, () => {},)',
'Array.from(iterableOrArrayLike, () => {}, ...thisArgument)',
'Array.from(iterableOrArrayLike, ...() => {}, thisArgument)',
'Array.from(...iterableOrArrayLike, () => {}, thisArgument)',
'Array.from(iterableOrArrayLike, () => {}, thisArgument, extraArgument)',

// Ignored
'lodash.every(array, () => {})',
'lodash.find(array, () => {})',
Expand All @@ -33,10 +47,13 @@ test.snapshot({
// `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
// Callback argument is not function
'array.map(new Callback, thisArgument)',
'array.map(1, thisArgument)',
'async () => array.map(await callback, thisArgument)',
'Array.from(iterableOrArrayLike, new Callback, thisArgument)',
'Array.from(iterableOrArrayLike, 1, thisArgument)',
'Array.from(iterableOrArrayLike, await callback, thisArgument)',
],
invalid: [
'array.every(() => {}, thisArgument)',
Expand All @@ -48,11 +65,14 @@ test.snapshot({
'array.flatMap(() => {}, thisArgument)',
'array.forEach(() => {}, thisArgument)',
'array.map(() => {}, thisArgument)',
'Array.from(iterableOrArrayLike, () => {}, thisArgument)',
// Comma
'array.map(() => {}, thisArgument,)',
'array.map(() => {}, (0, thisArgument),)',
'Array.from(iterableOrArrayLike, () => {}, thisArgument,)',
// Side effect
'array.map(() => {}, thisArgumentHasSideEffect())',
'Array.from(iterableOrArrayLike, () => {}, thisArgumentHasSideEffect())',
],
});

Expand All @@ -61,21 +81,36 @@ test.snapshot({
valid: [],
invalid: [
'array.map(callback, thisArgument)',
'Array.from(iterableOrArrayLike, callback, thisArgument)',
'array.map(callback, (0, thisArgument))',
'Array.from(iterableOrArrayLike, callback, (0, thisArgument))',
'array.map(function () {}, thisArgument)',
'Array.from(iterableOrArrayLike, function () {}, thisArgument)',
'array.map(function callback () {}, thisArgument)',
'Array.from(iterableOrArrayLike, function callback () {}, thisArgument)',
{
code: 'array.map( foo as bar, (( thisArgument )),)',
parser: parsers.typescript,
},
{
code: 'Array.from(iterableOrArrayLike, foo as bar, (( thisArgument )),)',
parser: parsers.typescript,
},
{
code: 'array.map( (( foo as bar )), (( thisArgument )),)',
parser: parsers.typescript,
},
{
code: 'Array.from(iterableOrArrayLike, (( foo as bar )), (( thisArgument )),)',
parser: parsers.typescript,
},
'array.map( (( 0, callback )), (( thisArgument )),)',
'Array.from(iterableOrArrayLike, (( 0, callback )), (( thisArgument )),)',
// This callback is actually arrow function, but we don't know
'array.map((0, () => {}), thisArgument)',
'Array.from(iterableOrArrayLike, (0, () => {}), thisArgument)',
// This callback is a bound function, but we don't know
'array.map(callback.bind(foo), thisArgument)',
'Array.from(iterableOrArrayLike, callback.bind(foo), thisArgument)',
],
});
Loading

0 comments on commit 797caee

Please sign in to comment.