Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from Nov 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/rules/no-useless-spread.md
Expand Up @@ -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:

Expand Down Expand Up @@ -65,6 +66,14 @@ function * foo() {
}
```

```js
function foo(bar) {
return [
...bar.map(x => x * 2),
];
}
```

## Pass

```js
Expand Down Expand Up @@ -116,3 +125,9 @@ function * foo() {
yield * anotherGenerator();
}
```

```js
function foo(bar) {
return bar.map(x => x * 2);
}
```
160 changes: 117 additions & 43 deletions rules/no-useless-spread.js
Expand Up @@ -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([
Expand All @@ -26,7 +36,7 @@ const uselessSpreadInListSelector = matches([
'NewExpression > SpreadElement.arguments > ArrayExpression.argument',
]);

const iterableToArraySelector = [
const singleArraySpreadSelector = [
'ArrayExpression',
'[elements.length=1]',
'[elements.0.type="SpreadElement"]',
Expand All @@ -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',
Expand All @@ -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();
Expand Down Expand Up @@ -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) {
Expand All @@ -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),
};
},
};
Expand Down
64 changes: 63 additions & 1 deletion test/no-useless-spread.mjs
Expand Up @@ -9,7 +9,6 @@ test.snapshot({
'const array = [[]]',
'const array = [{}]',
'const object = ({...[]})',
'const array = [...[].map(x => x)]',
'foo([])',
'foo({})',
'new Foo([])',
Expand Down Expand Up @@ -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: [
Expand Down