no-useless-spread: Check cloning inline arrays (#1980)

sindresorhus · Nov 20, 2022 · 5d90d73 · 5d90d73
1 parent 51d5254
commit 5d90d73
Show file tree

Hide file tree

Showing 5 changed files with 571 additions and 44 deletions.
diff --git a/docs/rules/no-useless-spread.md b/docs/rules/no-useless-spread.md
@@ -12,6 +12,7 @@
   - Spread an array literal as elements of an array literal
   - Spread an array literal as arguments of a call or a `new` call
   - Spread an object literal as properties of an object literal
+  - Use spread syntax to clone an array created inline
 
 - The following builtins accept an iterable, so it's unnecessary to convert the iterable to an array:
 
@@ -65,6 +66,14 @@ function * foo() {
 }
 ```
 
+```js
+function foo(bar) {
+	return [
+		...bar.map(x => x * 2),
+	];
+}
+```
+
 ## Pass
 
 ```js
@@ -116,3 +125,9 @@ function * foo() {
 	yield * anotherGenerator();
 }
 ```
+
+```js
+function foo(bar) {
+	return bar.map(x => x * 2);
+}
+```
diff --git a/rules/no-useless-spread.js b/rules/no-useless-spread.js
@@ -6,17 +6,27 @@ const {
 	methodCallSelector,
 } = require('./selectors/index.js');
 const typedArray = require('./shared/typed-array.js');
-const {removeParentheses, fixSpaceAroundKeyword} = require('./fix/index.js');
+const {
+	removeParentheses,
+	fixSpaceAroundKeyword,
+	addParenthesizesToReturnOrThrowExpression,
+} = require('./fix/index.js');
+const isOnSameLine = require('./utils/is-on-same-line.js');
+const {
+	isParenthesized,
+} = require('./utils/parentheses.js');
 
 const SPREAD_IN_LIST = 'spread-in-list';
 const ITERABLE_TO_ARRAY = 'iterable-to-array';
 const ITERABLE_TO_ARRAY_IN_FOR_OF = 'iterable-to-array-in-for-of';
 const ITERABLE_TO_ARRAY_IN_YIELD_STAR = 'iterable-to-array-in-yield-star';
+const CLONE_ARRAY = 'clone-array';
 const messages = {
 	[SPREAD_IN_LIST]: 'Spread an {{argumentType}} literal in {{parentDescription}} is unnecessary.',
 	[ITERABLE_TO_ARRAY]: '`{{parentDescription}}` accepts iterable as argument, it\'s unnecessary to convert to an array.',
 	[ITERABLE_TO_ARRAY_IN_FOR_OF]: '`for…of` can iterate over iterable, it\'s unnecessary to convert to an array.',
 	[ITERABLE_TO_ARRAY_IN_YIELD_STAR]: '`yield*` can delegate iterable, it\'s unnecessary to convert to an array.',
+	[CLONE_ARRAY]: 'Unnecessarily cloning an array.',
 };
 
 const uselessSpreadInListSelector = matches([
@@ -26,7 +36,7 @@ const uselessSpreadInListSelector = matches([
 	'NewExpression > SpreadElement.arguments > ArrayExpression.argument',
 ]);
 
-const iterableToArraySelector = [
+const singleArraySpreadSelector = [
 	'ArrayExpression',
 	'[elements.length=1]',
 	'[elements.0.type="SpreadElement"]',
@@ -49,12 +59,47 @@ const uselessIterableToArraySelector = matches([
 			methodCallSelector({object: 'Object', method: 'fromEntries', argumentsLength: 1}),
 		]),
 		' > ',
-		`${iterableToArraySelector}.arguments:first-child`,
+		`${singleArraySpreadSelector}.arguments:first-child`,
 	].join(''),
-	`ForOfStatement > ${iterableToArraySelector}.right`,
-	`YieldExpression[delegate=true] > ${iterableToArraySelector}.argument`,
+	`ForOfStatement > ${singleArraySpreadSelector}.right`,
+	`YieldExpression[delegate=true] > ${singleArraySpreadSelector}.argument`,
 ]);
 
+const uselessArrayCloneSelector = [
+	`${singleArraySpreadSelector} > .elements:first-child > .argument`,
+	matches([
+		// Array methods returns a new array
+		methodCallSelector([
+			'concat',
+			'copyWithin',
+			'filter',
+			'flat',
+			'flatMap',
+			'map',
+			'slice',
+			'splice',
+		]),
+		// `String#split()`
+		methodCallSelector('split'),
+		// `Object.keys()` and `Object.values()`
+		methodCallSelector({object: 'Object', methods: ['keys', 'values'], argumentsLength: 1}),
+		// `await Promise.all()` and `await Promise.allSettled`
+		[
+			'AwaitExpression',
+			methodCallSelector({
+				object: 'Promise',
+				methods: ['all', 'allSettled'],
+				argumentsLength: 1,
+				path: 'argument',
+			}),
+		].join(''),
+		// `Array.from()`, `Array.of()`
+		methodCallSelector({object: 'Array', methods: ['from', 'of']}),
+		// `new Array()`
+		newExpressionSelector('Array'),
+	]),
+].join('');
+
 const parentDescriptions = {
 	ArrayExpression: 'array literal',
 	ObjectExpression: 'object literal',
@@ -81,6 +126,59 @@ function getCommaTokens(arrayExpression, sourceCode) {
 	});
 }
 
+function * unwrapSingleArraySpread(fixer, arrayExpression, sourceCode) {
+	const [
+		openingBracketToken,
+		spreadToken,
+		thirdToken,
+	] = sourceCode.getFirstTokens(arrayExpression, 3);
+
+	// `[...value]`
+	//  ^
+	yield fixer.remove(openingBracketToken);
+
+	// `[...value]`
+	//   ^^^
+	yield fixer.remove(spreadToken);
+
+	const [
+		commaToken,
+		closingBracketToken,
+	] = sourceCode.getLastTokens(arrayExpression, 2);
+
+	// `[...value]`
+	//              ^
+	yield fixer.remove(closingBracketToken);
+
+	// `[...value,]`
+	//              ^
+	if (isCommaToken(commaToken)) {
+		yield fixer.remove(commaToken);
+	}
+
+	/*
+	```js
+	function foo() {
+		return [
+			...value,
+		];
+	}
+	```
+	*/
+	const {parent} = arrayExpression;
+	if (
+		(parent.type === 'ReturnStatement' || parent.type === 'ThrowStatement')
+		&& parent.argument === arrayExpression
+		&& !isOnSameLine(openingBracketToken, thirdToken)
+		&& !isParenthesized(arrayExpression, sourceCode)
+	) {
+		yield * addParenthesizesToReturnOrThrowExpression(fixer, parent, sourceCode);
+		return;
+	}
+
+	yield * fixSpaceAroundKeyword(fixer, arrayExpression, sourceCode);
+}
+
 /** @param {import('eslint').Rule.RuleContext} context */
 const create = context => {
 	const sourceCode = context.getSourceCode();
@@ -138,15 +236,15 @@ const create = context => {
 							continue;
 						}
 
-						// `call([foo, , bar])`
-						//             ^ Replace holes with `undefined`
+						// `call(...[foo, , bar])`
+						//               ^ Replace holes with `undefined`
 						yield fixer.insertTextBefore(commaToken, 'undefined');
 					}
 				},
 			};
 		},
-		[uselessIterableToArraySelector](array) {
-			const {parent} = array;
+		[uselessIterableToArraySelector](arrayExpression) {
+			const {parent} = arrayExpression;
 			let parentDescription = '';
 			let messageId = ITERABLE_TO_ARRAY;
 			switch (parent.type) {
@@ -173,42 +271,18 @@ const create = context => {
 			}
 
 			return {
-				node: array,
+				node: arrayExpression,
 				messageId,
 				data: {parentDescription},
-				* fix(fixer) {
-					if (parent.type === 'ForOfStatement') {
-						yield * fixSpaceAroundKeyword(fixer, array, sourceCode);
-					}
-
-					const [
-						openingBracketToken,
-						spreadToken,
-					] = sourceCode.getFirstTokens(array, 2);
-
-					// `[...iterable]`
-					//  ^
-					yield fixer.remove(openingBracketToken);
-
-					// `[...iterable]`
-					//   ^^^
-					yield fixer.remove(spreadToken);
-
-					const [
-						commaToken,
-						closingBracketToken,
-					] = sourceCode.getLastTokens(array, 2);
-
-					// `[...iterable]`
-					//              ^
-					yield fixer.remove(closingBracketToken);
-
-					// `[...iterable,]`
-					//              ^
-					if (isCommaToken(commaToken)) {
-						yield fixer.remove(commaToken);
-					}
-				},
+				fix: fixer => unwrapSingleArraySpread(fixer, arrayExpression, sourceCode),
+			};
+		},
+		[uselessArrayCloneSelector](node) {
+			const arrayExpression = node.parent.parent;
+			return {
+				node: arrayExpression,
+				messageId: CLONE_ARRAY,
+				fix: fixer => unwrapSingleArraySpread(fixer, arrayExpression, sourceCode),
 			};
 		},
 	};

diff --git a/test/no-useless-spread.mjs b/test/no-useless-spread.mjs
@@ -9,7 +9,6 @@ test.snapshot({
 		'const array = [[]]',
 		'const array = [{}]',
 		'const object = ({...[]})',
-		'const array = [...[].map(x => x)]',
 		'foo([])',
 		'foo({})',
 		'new Foo([])',
@@ -241,6 +240,69 @@ test.snapshot({
 	],
 });
 
+// Cloning an immediate array
+test.snapshot({
+	valid: [
+		'[...not.array]',
+		'[...not.array()]',
+		'[...array.unknown()]',
+		'[...Object.notReturningArray(foo)]',
+		'[...NotObject.keys(foo)]',
+		'[...Int8Array.from(foo)]',
+		'[...Int8Array.of()]',
+		'[...new Int8Array(3)]',
+		'[...Promise.all(foo)]',
+		'[...Promise.allSettled(foo)]',
+		'[...await Promise.all(foo, extraArgument)]',
+	],
+	invalid: [
+		'[...foo.concat(bar)]',
+		'[...foo.copyWithin(-2)]',
+		'[...foo.filter(bar)]',
+		'[...foo.flat()]',
+		'[...foo.flatMap(bar)]',
+		'[...foo.map(bar)]',
+		'[...foo.slice(1)]',
+		'[...foo.splice(1)]',
+		'[...foo.split("|")]',
+		'[...Object.keys(foo)]',
+		'[...Object.values(foo)]',
+		'[...Array.from(foo)]',
+		'[...Array.of()]',
+		'[...new Array(3)]',
+		'[...await Promise.all(foo)]',
+		'[...await Promise.allSettled(foo)]',
+		outdent`
+			function foo(bar) {
+				return[...Object.keys(bar)];
+			}
+		`,
+		outdent`
+			function foo(bar) {
+				return[
+					...Object.keys(bar)
+				];
+			}
+		`,
+		outdent`
+			function foo(bar) {
+				return[
+					...(
+						Object.keys(bar)
+					)
+				];
+			}
+		`,
+		outdent`
+			function foo(bar) {
+				return([
+					...Object.keys(bar)
+				]);
+			}
+		`,
+	],
+});
+
 test.babel({
 	valid: [],
 	invalid: [