diff --git a/docs/rules/no-array-method-this-argument.md b/docs/rules/no-array-method-this-argument.md new file mode 100644 index 0000000000..863b12bd78 --- /dev/null +++ b/docs/rules/no-array-method-this-argument.md @@ -0,0 +1,49 @@ +# Disallow using the `this` argument in array methods + +The rule forbids using the `thisArg` argument in array methods: + +- If the callback is an arrow function or a bound function, the `thisArg` won't affect it. +- If you intent to use a custom `this` in the callback, itβ€˜s better to use the variable directly or use `callback.bind(thisArg)`. + +This rule checks following array methods accepts `thisArg`: + +- [`Array#every()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/every) +- [`Array#filter()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/filter) +- [`Array#find()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/find) +- [`Array#findIndex()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/findIndex) +- [`Array#flatMap()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/flatMap) +- [`Array#forEach()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/forEach) +- [`Array#map()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/map) +- [`Array#some()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Array/some) + +This rule is fixable when the callback is an arrow function and the `thisArg` argument has no side effect. + +## Fail + +```js +const foo = bar.find(element => isUnicorn(element), baz); +``` + +```js +const foo = bar.map(function (element) => { + return this.unicorn(element); +}, baz); +``` + +## Pass + +```js +const foo = bar.find(element => isUnicorn(element)); +``` + +```js +const foo = bar.map(function (element) => { + return baz.unicorn(element); +}); +``` + +```js +const foo = bar.map(function (element) => { + return this.unicorn(element); +}.bind(baz)); +``` diff --git a/index.js b/index.js index b255b56d5d..a71b6e07ef 100644 --- a/index.js +++ b/index.js @@ -55,6 +55,7 @@ module.exports = { 'unicorn/no-abusive-eslint-disable': 'error', 'unicorn/no-array-callback-reference': 'error', 'unicorn/no-array-for-each': 'error', + 'unicorn/no-array-method-this-argument': 'error', 'unicorn/no-array-push-push': 'error', 'unicorn/no-array-reduce': 'error', 'unicorn/no-console-spaces': 'error', diff --git a/readme.md b/readme.md index 38fbebf44f..3cb3304a38 100644 --- a/readme.md +++ b/readme.md @@ -52,6 +52,7 @@ Configure it in `package.json`. "unicorn/no-abusive-eslint-disable": "error", "unicorn/no-array-callback-reference": "error", "unicorn/no-array-for-each": "error", + "unicorn/no-array-method-this-argument": "error", "unicorn/no-array-push-push": "error", "unicorn/no-array-reduce": "error", "unicorn/no-console-spaces": "error", @@ -156,6 +157,7 @@ Each rule has emojis denoting: | [no-abusive-eslint-disable](docs/rules/no-abusive-eslint-disable.md) | Enforce specifying rules to disable in `eslint-disable` comments. | βœ… | | | | [no-array-callback-reference](docs/rules/no-array-callback-reference.md) | Prevent passing a function reference directly to iterator methods. | βœ… | | πŸ’‘ | | [no-array-for-each](docs/rules/no-array-for-each.md) | Prefer `for…of` over `Array#forEach(…)`. | βœ… | πŸ”§ | | +| [no-array-method-this-argument](docs/rules/no-array-method-this-argument.md) | Disallow using the `this` argument in array methods. | βœ… | πŸ”§ | πŸ’‘ | | [no-array-push-push](docs/rules/no-array-push-push.md) | Enforce combining multiple `Array#push()` into one call. | βœ… | πŸ”§ | πŸ’‘ | | [no-array-reduce](docs/rules/no-array-reduce.md) | Disallow `Array#reduce()` and `Array#reduceRight()`. | βœ… | | | | [no-console-spaces](docs/rules/no-console-spaces.md) | Do not use leading/trailing space between `console.log` parameters. | βœ… | πŸ”§ | | diff --git a/rules/fix/index.js b/rules/fix/index.js index 4ff3c351b6..58f4dd0176 100644 --- a/rules/fix/index.js +++ b/rules/fix/index.js @@ -2,5 +2,6 @@ module.exports = { appendArgument: require('./append-argument.js'), + removeArgument: require('./remove-argument.js'), switchNewExpressionToCallExpression: require('./switch-new-expression-to-call-expression.js') }; diff --git a/rules/fix/remove-argument.js b/rules/fix/remove-argument.js new file mode 100644 index 0000000000..90e90eb0c9 --- /dev/null +++ b/rules/fix/remove-argument.js @@ -0,0 +1,31 @@ +'use strict'; +const {isCommaToken} = require('eslint-utils'); +const {getParentheses} = require('../utils/parentheses.js'); + +function removeArgument(fixer, node, sourceCode) { + const callExpression = node.parent; + const index = callExpression.arguments.indexOf(node); + const parentheses = getParentheses(node, sourceCode); + const firstToken = parentheses[0] || node; + const lastToken = parentheses[parentheses.length - 1] || node; + + let [start] = firstToken.range; + let [, end] = lastToken.range; + + if (index !== 0) { + start = sourceCode.getTokenBefore(firstToken).range[0]; + } + + // If the removed argument is the only argument, the trailing comma must be removed too + /* istanbul ignore next: Not reachable for now */ + if (callExpression.arguments.length === 1) { + const tokenAfter = sourceCode.getTokenBefore(lastToken); + if (isCommaToken(tokenAfter)) { + end = tokenAfter[1]; + } + } + + return fixer.replaceTextRange([start, end], ''); +} + +module.exports = removeArgument; diff --git a/rules/no-array-method-this-argument.js b/rules/no-array-method-this-argument.js new file mode 100644 index 0000000000..daeeabc7bc --- /dev/null +++ b/rules/no-array-method-this-argument.js @@ -0,0 +1,120 @@ +'use strict'; +const {hasSideEffect} = require('eslint-utils'); +const {methodCallSelector} = require('./selectors/index.js'); +const {removeArgument} = require('./fix/index.js'); +const {getParentheses, getParenthesizedText} = require('./utils/parentheses.js'); +const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object.js'); + +const ERROR = 'error'; +const SUGGESTION_BIND = 'suggestion-bind'; +const SUGGESTION_REMOVE = 'suggestion-remove'; +const messages = { + [ERROR]: 'Do not use the `this` argument in `Array#{{method}}()`.', + [SUGGESTION_REMOVE]: 'Remove the second argument.', + [SUGGESTION_BIND]: 'Use a bound function.' +}; + +const selector = methodCallSelector({ + names: [ + 'every', + 'filter', + 'find', + 'findIndex', + 'flatMap', + 'forEach', + 'map', + 'some' + ], + length: 2 +}); + +function removeThisArgument(callExpression, sourceCode) { + return fixer => removeArgument(fixer, callExpression.arguments[1], sourceCode); +} + +function useBoundFunction(callExpression, sourceCode) { + return function * (fixer) { + yield removeThisArgument(callExpression, sourceCode)(fixer); + + const [callback, thisArgument] = callExpression.arguments; + + const callbackParentheses = getParentheses(callback, sourceCode); + const isParenthesized = callbackParentheses.length > 0; + const callbackLastToken = isParenthesized ? + callbackParentheses[callbackParentheses.length - 1] : + callback; + if ( + !isParenthesized && + shouldAddParenthesesToMemberExpressionObject(callback, sourceCode) + ) { + yield fixer.insertTextBefore(callbackLastToken, '('); + yield fixer.insertTextAfter(callbackLastToken, ')'); + } + + const thisArgumentText = getParenthesizedText(thisArgument, 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.getSourceCode(); + + return { + [selector](callExpression) { + const method = callExpression.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; + } + + problem.suggest = [ + { + messageId: SUGGESTION_REMOVE, + fix: removeThisArgument(callExpression, sourceCode) + }, + { + messageId: SUGGESTION_BIND, + fix: useBoundFunction(callExpression, sourceCode) + } + ]; + + return problem; + } + }; +}; + +module.exports = { + create, + meta: { + type: 'suggestion', + docs: { + description: 'Disallow using the `this` argument in array methods.' + }, + fixable: 'code', + messages, + hasSuggestions: true + } +}; diff --git a/rules/utils/should-add-parentheses-to-member-expression-object.js b/rules/utils/should-add-parentheses-to-member-expression-object.js index f8aa7100c3..4c8fae5ea6 100644 --- a/rules/utils/should-add-parentheses-to-member-expression-object.js +++ b/rules/utils/should-add-parentheses-to-member-expression-object.js @@ -21,6 +21,7 @@ function shouldAddParenthesesToMemberExpressionObject(node, sourceCode) { case 'TemplateLiteral': case 'ThisExpression': case 'ArrayExpression': + case 'FunctionExpression': return false; case 'NewExpression': return !isNewExpressionWithParentheses(node, sourceCode); diff --git a/test/no-array-method-this-argument.mjs b/test/no-array-method-this-argument.mjs new file mode 100644 index 0000000000..beb3d79884 --- /dev/null +++ b/test/no-array-method-this-argument.mjs @@ -0,0 +1,54 @@ +import {getTester} from './utils/test.mjs'; + +const {test} = getTester(import.meta); + +// Arrow functions +test.snapshot({ + valid: [ + 'array.unknownMethod(() => {}, thisArgument)', + 'new array.map(() => {}, thisArgument)', + 'array.map?.(() => {}, thisArgument)', + 'array?.map(() => {}, thisArgument)', + // More or less arguments + 'array.map()', + 'array.map(() => {},)', + 'array.map(() => {}, ...thisArgument)', + 'array.map(...() => {}, thisArgument)', + 'array.map(() => {}, thisArgument, extraArgument)' + ], + invalid: [ + 'array.every(() => {}, thisArgument)', + 'array.filter(() => {}, thisArgument)', + 'array.find(() => {}, thisArgument)', + 'array.findIndex(() => {}, thisArgument)', + 'array.flatMap(() => {}, thisArgument)', + 'array.forEach(() => {}, thisArgument)', + 'array.map(() => {}, thisArgument)', + // Comma + 'array.map(() => {}, thisArgument,)', + 'array.map(() => {}, (0, thisArgument),)', + // Side effect + 'array.map(() => {}, thisArgumentHasSideEffect())' + ] +}); + +// Non-arrow functions +test.snapshot({ + valid: [], + invalid: [ + 'array.map(callback, thisArgument)', + 'array.map(callback, (0, thisArgument))', + 'array.map(function () {}, thisArgument)', + 'array.map(function callback () {}, thisArgument)', + 'array.map(new Callback, thisArgument)', + 'array.map(1, thisArgument)', + 'async () => array.map(await callback, thisArgument)', + 'array.map((new Callback), thisArgument)', + 'array.map(new Callback(), thisArgument)', + 'array.map( (( callback )), (( thisArgument )),)', + // This callback is actually arrow function, but we don't know + 'array.map((0, () => {}), thisArgument)', + // This callback is a bound function, but we don't know + 'array.map(callback.bind(foo), thisArgument)' + ] +}); diff --git a/test/snapshots/no-array-method-this-argument.mjs.md b/test/snapshots/no-array-method-this-argument.mjs.md new file mode 100644 index 0000000000..797ce9da77 --- /dev/null +++ b/test/snapshots/no-array-method-this-argument.mjs.md @@ -0,0 +1,379 @@ +# Snapshot report for `test/no-array-method-this-argument.mjs` + +The actual snapshot is saved in `no-array-method-this-argument.mjs.snap`. + +Generated by [AVA](https://avajs.dev). + +## Invalid #1 + 1 | array.every(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.every(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.every(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#every()\`.␊ + ` + +## Invalid #2 + 1 | array.filter(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.filter(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.filter(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#filter()\`.␊ + ` + +## Invalid #3 + 1 | array.find(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.find(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.find(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#find()\`.␊ + ` + +## Invalid #4 + 1 | array.findIndex(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.findIndex(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.findIndex(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#findIndex()\`.␊ + ` + +## Invalid #5 + 1 | array.flatMap(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.flatMap(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.flatMap(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#flatMap()\`.␊ + ` + +## Invalid #6 + 1 | array.forEach(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.forEach(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.forEach(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#forEach()\`.␊ + ` + +## Invalid #7 + 1 | array.map(() => {}, thisArgument) + +> Output + + `␊ + 1 | array.map(() => {})␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.map(() => {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ` + +## Invalid #8 + 1 | array.map(() => {}, thisArgument,) + +> Output + + `␊ + 1 | array.map(() => {},)␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.map(() => {}, thisArgument,)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ` + +## Invalid #9 + 1 | array.map(() => {}, (0, thisArgument),) + +> Output + + `␊ + 1 | array.map(() => {},)␊ + ` + +> Error 1/1 + + `␊ + > 1 | array.map(() => {}, (0, thisArgument),)␊ + | ^^^^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ` + +## Invalid #10 + 1 | array.map(() => {}, thisArgumentHasSideEffect()) + +> Error 1/1 + + `␊ + > 1 | array.map(() => {}, thisArgumentHasSideEffect())␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/1: Remove the second argument.␊ + 1 | array.map(() => {})␊ + ` + +## Invalid #1 + 1 | array.map(callback, thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(callback, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(callback)␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map(callback.bind(thisArgument))␊ + ` + +## Invalid #2 + 1 | array.map(callback, (0, thisArgument)) + +> Error 1/1 + + `␊ + > 1 | array.map(callback, (0, thisArgument))␊ + | ^^^^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(callback)␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map(callback.bind((0, thisArgument)))␊ + ` + +## Invalid #3 + 1 | array.map(function () {}, thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(function () {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(function () {})␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map(function () {}.bind(thisArgument))␊ + ` + +## Invalid #4 + 1 | array.map(function callback () {}, thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(function callback () {}, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(function callback () {})␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map(function callback () {}.bind(thisArgument))␊ + ` + +## Invalid #5 + 1 | array.map(new Callback, thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(new Callback, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(new Callback)␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map((new Callback).bind(thisArgument))␊ + ` + +## Invalid #6 + 1 | array.map(1, thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(1, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(1)␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map((1).bind(thisArgument))␊ + ` + +## Invalid #7 + 1 | async () => array.map(await callback, thisArgument) + +> Error 1/1 + + `␊ + > 1 | async () => array.map(await callback, thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | async () => array.map(await callback)␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | async () => array.map((await callback).bind(thisArgument))␊ + ` + +## Invalid #8 + 1 | array.map((new Callback), thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map((new Callback), thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map((new Callback))␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map((new Callback).bind(thisArgument))␊ + ` + +## Invalid #9 + 1 | array.map(new Callback(), thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(new Callback(), thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(new Callback())␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map(new Callback().bind(thisArgument))␊ + ` + +## Invalid #10 + 1 | array.map( (( callback )), (( thisArgument )),) + +> Error 1/1 + + `␊ + > 1 | array.map( (( callback )), (( thisArgument )),)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map( (( callback )),)␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map( (( callback )).bind((( thisArgument ))),)␊ + ` + +## Invalid #11 + 1 | array.map((0, () => {}), thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map((0, () => {}), thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map((0, () => {}))␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map((0, () => {}).bind(thisArgument))␊ + ` + +## Invalid #12 + 1 | array.map(callback.bind(foo), thisArgument) + +> Error 1/1 + + `␊ + > 1 | array.map(callback.bind(foo), thisArgument)␊ + | ^^^^^^^^^^^^ Do not use the \`this\` argument in \`Array#map()\`.␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 1/2: Remove the second argument.␊ + 1 | array.map(callback.bind(foo))␊ + ␊ + --------------------------------------------------------------------------------␊ + Suggestion 2/2: Use a bound function.␊ + 1 | array.map(callback.bind(foo).bind(thisArgument))␊ + ` diff --git a/test/snapshots/no-array-method-this-argument.mjs.snap b/test/snapshots/no-array-method-this-argument.mjs.snap new file mode 100644 index 0000000000..0caad9d16d Binary files /dev/null and b/test/snapshots/no-array-method-this-argument.mjs.snap differ