From 057c6a842a55fbbf59db5c422f943c770568ce3d Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 6 Jan 2021 19:19:36 +0100 Subject: [PATCH] tools: add ESLint rule no-array-destructuring Iterating over arrays should be avoided because it relies on user-mutable global methods (`Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`), we should instead use other alternatives. This commit adds a rule that disallow array destructuring syntax in favor of object destructuring syntax. Note that you can ignore this rule if you are using the array destructuring syntax over a safe iterable, or actually want to iterate over a user-provided object. PR-URL: https://github.com/nodejs/node/pull/36818 Reviewed-By: Rich Trott --- lib/.eslintrc.yaml | 1 + .../test-eslint-no-array-destructuring.js | 141 ++++++++++++++++++ tools/eslint-rules/no-array-destructuring.js | 83 +++++++++++ 3 files changed, 225 insertions(+) create mode 100644 test/parallel/test-eslint-no-array-destructuring.js create mode 100644 tools/eslint-rules/no-array-destructuring.js diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 05142865e12802..2cc2660157191f 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -28,6 +28,7 @@ rules: # Custom rules in tools/eslint-rules node-core/lowercase-name-for-primitive: error node-core/non-ascii-character: error + node-core/no-array-destructuring: error node-core/prefer-primordials: - error - name: Array diff --git a/test/parallel/test-eslint-no-array-destructuring.js b/test/parallel/test-eslint-no-array-destructuring.js new file mode 100644 index 00000000000000..be59ee69309755 --- /dev/null +++ b/test/parallel/test-eslint-no-array-destructuring.js @@ -0,0 +1,141 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +common.skipIfEslintMissing(); + +const { RuleTester } = require('../../tools/node_modules/eslint'); +const rule = require('../../tools/eslint-rules/no-array-destructuring'); + +const USE_OBJ_DESTRUCTURING = + 'Use object destructuring instead of array destructuring.'; +const USE_ARRAY_METHODS = + 'Use primordials.ArrayPrototypeSlice to avoid unsafe array iteration.'; + +new RuleTester({ + parserOptions: { ecmaVersion: 2021 }, + env: { es6: true } +}) + .run('no-array-destructuring', rule, { + valid: [ + 'const first = [1, 2, 3][0];', + 'const {0:first} = [1, 2, 3];', + '({1:elem} = array);', + 'function name(param, { 0: key, 1: value },) {}', + ], + invalid: [ + { + code: 'const [Array] = args;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'const {0:Array} = args;' + }, + { + code: 'const [ , res] = args;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'const { 1:res} = args;', + }, + { + code: '[, elem] = options;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: '({ 1:elem} = options);', + }, + { + code: 'const {values:[elem]} = options;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'const {values:{0:elem}} = options;', + }, + { + code: '[[[elem]]] = options;', + errors: [ + { message: USE_OBJ_DESTRUCTURING }, + { message: USE_OBJ_DESTRUCTURING }, + { message: USE_OBJ_DESTRUCTURING }, + ], + output: '({0:[[elem]]} = options);', + }, + { + code: '[, ...rest] = options;', + errors: [{ message: USE_ARRAY_METHODS }], + }, + { + code: 'for(const [key, value] of new Map);', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'for(const {0:key, 1:value} of new Map);', + }, + { + code: 'let [first,,,fourth] = array;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'let {0:first,3:fourth} = array;', + }, + { + code: 'let [,second,,fourth] = array;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'let {1:second,3:fourth} = array;', + }, + { + code: 'let [ ,,,fourth ] = array;', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'let { 3:fourth } = array;', + }, + { + code: 'let [,,,fourth, fifth,, minorFall, majorLift,...music] = arr;', + errors: [{ message: USE_ARRAY_METHODS }], + }, + { + code: 'function map([key, value]) {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'function map({0:key, 1:value}) {}', + }, + { + code: 'function map([key, value],) {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'function map({0:key, 1:value},) {}', + }, + { + code: '(function([key, value]) {})', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: '(function({0:key, 1:value}) {})', + }, + { + code: '(function([key, value] = [null, 0]) {})', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: '(function({0:key, 1:value} = [null, 0]) {})', + }, + { + code: 'function map([key, ...values]) {}', + errors: [{ message: USE_ARRAY_METHODS }], + }, + { + code: 'function map([key, value], ...args) {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'function map({0:key, 1:value}, ...args) {}', + }, + { + code: 'async function map([key, value], ...args) {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'async function map({0:key, 1:value}, ...args) {}', + }, + { + code: 'async function* generator([key, value], ...args) {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'async function* generator({0:key, 1:value}, ...args) {}', + }, + { + code: 'function* generator([key, value], ...args) {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'function* generator({0:key, 1:value}, ...args) {}', + }, + { + code: 'const cb = ([key, value], ...args) => {}', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'const cb = ({0:key, 1:value}, ...args) => {}', + }, + { + code: 'class name{ method([key], ...args){} }', + errors: [{ message: USE_OBJ_DESTRUCTURING }], + output: 'class name{ method({0:key}, ...args){} }', + }, + ] + }); diff --git a/tools/eslint-rules/no-array-destructuring.js b/tools/eslint-rules/no-array-destructuring.js new file mode 100644 index 00000000000000..81667e698fbaad --- /dev/null +++ b/tools/eslint-rules/no-array-destructuring.js @@ -0,0 +1,83 @@ +/** + * @fileoverview Iterating over arrays should be avoided because it relies on + * user-mutable global methods (`Array.prototype[Symbol.iterator]` + * and `%ArrayIteratorPrototype%.next`), we should instead use + * other alternatives. This file defines a rule that disallow + * array destructuring syntax in favor of object destructuring + * syntax. Note that you can ignore this rule if you are using + * the array destructuring syntax over a safe iterable, or + * actually want to iterate over a user-provided object. + * @author aduh95 + */ +'use strict'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +const USE_OBJ_DESTRUCTURING = + 'Use object destructuring instead of array destructuring.'; +const USE_ARRAY_METHODS = + 'Use primordials.ArrayPrototypeSlice to avoid unsafe array iteration.'; + +const findComma = (sourceCode, elements, i, start) => { + if (i === 0) + return sourceCode.getTokenAfter(sourceCode.getTokenByRangeStart(start)); + + let element; + const originalIndex = i; + while (i && !element) { + element = elements[--i]; + } + let token = sourceCode.getTokenAfter( + element ?? sourceCode.getTokenByRangeStart(start) + ); + for (; i < originalIndex; i++) { + token = sourceCode.getTokenAfter(token); + } + return token; +}; +const createFix = (fixer, sourceCode, { range: [start, end], elements }) => [ + fixer.replaceTextRange([start, start + 1], '{'), + fixer.replaceTextRange([end - 1, end], '}'), + ...elements.map((node, i) => + (node === null ? + fixer.remove(findComma(sourceCode, elements, i, start)) : + fixer.insertTextBefore(node, i + ':')) + ), +]; +const arrayPatternContainsRestOperator = ({ elements }) => + elements?.find((node) => node?.type === 'RestElement'); + +module.exports = { + meta: { + type: 'suggestion', + fixable: 'code', + schema: [], + }, + create(context) { + const sourceCode = context.getSourceCode(); + + return { + ArrayPattern(node) { + const hasRest = arrayPatternContainsRestOperator(node); + context.report({ + message: hasRest ? USE_ARRAY_METHODS : USE_OBJ_DESTRUCTURING, + node: hasRest || node, + fix: hasRest ? + undefined : + (fixer) => [ + ...(node.parent.type === 'AssignmentExpression' ? + [ + fixer.insertTextBefore(node.parent, '('), + fixer.insertTextAfter(node.parent, ')'), + ] : + []), + ...createFix(fixer, sourceCode, node), + ], + }); + + }, + }; + }, +};