Skip to content

Commit

Permalink
no-useless-spread: Check cloning inline arrays (#1980)
Browse files Browse the repository at this point in the history
  • Loading branch information
@fisker
fisker committed Nov 20, 2022
1 parent 51d5254 commit 5d90d73
Show file tree
Hide file tree
Showing 5 changed files with 571 additions and 44 deletions.
15 changes: 15 additions & 0 deletions docs/rules/no-useless-spread.md
View file
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
View file
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
View file
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

0 comments on commit 5d90d73

Please sign in to comment.