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: #36818 Reviewed-By: Rich Trott <[email protected]>
nodejs · aduh95 · Mar 3, 2021 · Jan 6, 2021 · Jan 6, 2021 · Mar 3, 2021
commit 26288ff25ee59c2052a71d44378d4eb3bc616596
diff --git 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
@@ -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
@@ -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 <[email protected]>
+ */
+'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),
+            ],
+        });
+
+      },
+    };
+  },
+};