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

prefer-spread: Check String#split('') #1489

Merged
merged 3 commits into from
Aug 16, 2021
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
16 changes: 15 additions & 1 deletion docs/rules/prefer-spread.md
@@ -1,4 +1,4 @@
# Prefer the spread operator over `Array.from(…)`, `Array#concat(…)` and `Array#slice()`
# Prefer the spread operator over `Array.from(…)`, `Array#concat(…)`, `Array#slice()` and `String#split('')`

Enforces the use of [the spread operator (`...`)](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax) over

Expand All @@ -18,6 +18,12 @@ Enforces the use of [the spread operator (`...`)](https://developer.mozilla.org/

Variables named `arrayBuffer`, `blob`, `buffer`, `file`, and `this` are ignored.

- `String#split('')`

Split a string into an array of characters.

Note: [The suggestion fix may get different result](https://stackoverflow.com/questions/4547609/how-to-get-character-array-from-a-string/34717402#34717402).

This rule is partly fixable.

## Fail
Expand All @@ -34,6 +40,10 @@ const array = array1.concat(array2);
const copy = array.slice();
```

```js
const characters = string.split('');
```

## Pass

```js
Expand All @@ -52,6 +62,10 @@ const tail = array.slice(1);
const copy = [...array];
```

```js
const characters = [...string];
```

## With the `unicorn/no-useless-spread` rule

Some cases are fixed using extra spread syntax. Therefore we recommend enabling the [`unicorn/no-useless-spread`](./no-useless-spread.md) rule to fix it.
Expand Down
2 changes: 1 addition & 1 deletion readme.md
Expand Up @@ -229,7 +229,7 @@ Each rule has emojis denoting:
| [prefer-reflect-apply](docs/rules/prefer-reflect-apply.md) | Prefer `Reflect.apply()` over `Function#apply()`. | ✅ | 🔧 | |
| [prefer-regexp-test](docs/rules/prefer-regexp-test.md) | Prefer `RegExp#test()` over `String#match()` and `RegExp#exec()`. | ✅ | 🔧 | |
| [prefer-set-has](docs/rules/prefer-set-has.md) | Prefer `Set#has()` over `Array#includes()` when checking for existence or non-existence. | ✅ | 🔧 | 💡 |
| [prefer-spread](docs/rules/prefer-spread.md) | Prefer the spread operator over `Array.from(…)`, `Array#concat(…)` and `Array#slice()`. | ✅ | 🔧 | 💡 |
| [prefer-spread](docs/rules/prefer-spread.md) | Prefer the spread operator over `Array.from(…)`, `Array#concat(…)`, `Array#slice()` and `String#split('')`. | ✅ | 🔧 | 💡 |
| [prefer-string-replace-all](docs/rules/prefer-string-replace-all.md) | Prefer `String#replaceAll()` over regex searches with the global flag. | | 🔧 | |
| [prefer-string-slice](docs/rules/prefer-string-slice.md) | Prefer `String#slice()` over `String#substr()` and `String#substring()`. | ✅ | 🔧 | |
| [prefer-string-starts-ends-with](docs/rules/prefer-string-starts-ends-with.md) | Prefer `String#startsWith()` & `String#endsWith()` over `RegExp#test()`. | ✅ | 🔧 | 💡 |
Expand Down
58 changes: 54 additions & 4 deletions rules/prefer-spread.js
Expand Up @@ -15,18 +15,22 @@ const {
const ERROR_ARRAY_FROM = 'array-from';
const ERROR_ARRAY_CONCAT = 'array-concat';
const ERROR_ARRAY_SLICE = 'array-slice';
const ERROR_STRING_SPLIT = 'string-split';
const SUGGESTION_CONCAT_ARGUMENT_IS_SPREADABLE = 'argument-is-spreadable';
const SUGGESTION_CONCAT_ARGUMENT_IS_NOT_SPREADABLE = 'argument-is-not-spreadable';
const SUGGESTION_CONCAT_TEST_ARGUMENT = 'test-argument';
const SUGGESTION_CONCAT_SPREAD_ALL_ARGUMENTS = 'spread-all-arguments';
const SUGGESTION_USE_SPREAD = 'use-spread';
const messages = {
[ERROR_ARRAY_FROM]: 'Prefer the spread operator over `Array.from(…)`.',
[ERROR_ARRAY_CONCAT]: 'Prefer the spread operator over `Array#concat(…)`.',
[ERROR_ARRAY_SLICE]: 'Prefer the spread operator over `Array#slice()`.',
[ERROR_STRING_SPLIT]: 'Prefer the spread operator over `String#split(\'\')`.',
[SUGGESTION_CONCAT_ARGUMENT_IS_SPREADABLE]: 'First argument is an `array`.',
[SUGGESTION_CONCAT_ARGUMENT_IS_NOT_SPREADABLE]: 'First argument is not an `array`.',
[SUGGESTION_CONCAT_TEST_ARGUMENT]: 'Test first argument with `Array.isArray(…)`.',
[SUGGESTION_CONCAT_SPREAD_ALL_ARGUMENTS]: 'Spread all unknown arguments`.',
[SUGGESTION_USE_SPREAD]: 'Use `...` operator.',
};

const arrayFromCallSelector = [
Expand Down Expand Up @@ -67,6 +71,11 @@ const ignoredSliceCallee = [
'this',
];

const stringSplitCallSelector = methodCallSelector({
method: 'split',
argumentsLength: 1,
});

const isArrayLiteral = node => node.type === 'ArrayExpression';
const isArrayLiteralHasTrailingComma = (node, sourceCode) => {
if (node.elements.length === 0) {
Expand Down Expand Up @@ -281,7 +290,7 @@ function fixArrayFrom(node, sourceCode) {
};
}

function fixSlice(node, sourceCode) {
function methodCallToSpread(node, sourceCode) {
return function * (fixer) {
// Fixed code always starts with `[`
if (needsSemicolon(sourceCode.getTokenBefore(node), sourceCode, '[')) {
Expand All @@ -291,7 +300,7 @@ function fixSlice(node, sourceCode) {
yield fixer.insertTextBefore(node, '[...');
yield fixer.insertTextAfter(node, ']');

// The array is already accessing `.slice`, there should not any case need add extra `()`
// The array is already accessing `.slice` or `.split`, there should not any case need add extra `()`

yield * removeMethodCall(fixer, node, sourceCode);
};
Expand Down Expand Up @@ -418,8 +427,49 @@ const create = context => {
return {
node: node.callee.property,
messageId: ERROR_ARRAY_SLICE,
fix: fixSlice(node, sourceCode),
fix: methodCallToSpread(node, sourceCode),
};
},
[stringSplitCallSelector](node) {
const [separator] = node.arguments;
if (!isLiteralValue(separator, '')) {
return;
}

const string = node.callee.object;
const staticValue = getStaticValue(string, context.getScope());
let hasSameResult = false;
if (staticValue) {
const {value} = staticValue;

if (typeof value !== 'string') {
return;
}

const resultBySplit = value.split('');
const resultBySpread = [...value];

hasSameResult = resultBySplit.length === resultBySpread.length
&& resultBySplit.every((character, index) => character === resultBySpread[index]);
}

const problem = {
node: node.callee.property,
messageId: ERROR_STRING_SPLIT,
};

if (hasSameResult) {
problem.fix = methodCallToSpread(node, sourceCode);
} else {
problem.suggest = [
{
messageId: SUGGESTION_USE_SPREAD,
fix: methodCallToSpread(node, sourceCode),
},
];
}

return problem;
},
};
};
Expand All @@ -429,7 +479,7 @@ module.exports = {
meta: {
type: 'suggestion',
docs: {
description: 'Prefer the spread operator over `Array.from(…)`, `Array#concat(…)` and `Array#slice()`.',
description: 'Prefer the spread operator over `Array.from(…)`, `Array#concat(…)`, `Array#slice()` and `String#split(\'\')`.',
},
fixable: 'code',
messages,
Expand Down
43 changes: 43 additions & 0 deletions test/prefer-spread.mjs
Expand Up @@ -339,3 +339,46 @@ test.snapshot({
'array.slice(0.00, )',
],
});

// `String#slice('')`
test.snapshot({
valid: [
'new foo.split("")',
'split("")',
'string[split]("")',
'string.split',
'string.split(1)',
'string.split(..."")',
'string.split(...[""])',
'string.split("" + "")',
'string.split(0)',
'string.split(false)',
'string.split(undefined)',
'string.split(0n)',
'string.split(null)',
'string.split(/""/)',
'string.split(``)',
'const EMPTY_STRING = ""; string.split(EMPTY_STRING)',
'string.split("", limit)',
'"".split(string)',
'string.split()',
'string.notSplit("")',
'const notString = 0; notString.split("")',
],
invalid: [
'"string".split("")',
'"string".split(\'\')',
'unknown.split("")',
'const characters = "string".split("")',
'(( (( (( "string" )).split ))( (("")) ) ))',
// Semicolon
outdent`
bar()
foo.split("")
`,
'unknown.split("")',
// Not result the same
'"🦄".split("")',
'const {length} = "🦄".split("")',
],
});
9 changes: 9 additions & 0 deletions test/run-rules-on-codebase/lint.mjs
Expand Up @@ -70,6 +70,15 @@ const eslint = new ESLint({
'unicorn/prefer-module': 'off',
},
},
{
files: [
'rules/prefer-spread.js',
],
rules: {
// TODO[xo@>=0.45.0]: Enable this rule when `xo` updated `eslint-plugin-unicorn`
'unicorn/prefer-spread': 'off',
},
},
],
},
});
Expand Down
137 changes: 137 additions & 0 deletions test/snapshots/prefer-spread.mjs.md
Expand Up @@ -2251,3 +2251,140 @@ Generated by [AVA](https://avajs.dev).
> 1 | array.slice(0.00, )␊
| ^^^^^ Prefer the spread operator over \`Array#slice()\`.␊
`

## Invalid #1
1 | "string".split("")

> Output

`␊
1 | [..."string"]␊
`

> Error 1/1

`␊
> 1 | "string".split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
`

## Invalid #2
1 | "string".split('')

> Output

`␊
1 | [..."string"]␊
`

> Error 1/1

`␊
> 1 | "string".split('')␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
`

## Invalid #3
1 | unknown.split("")

> Error 1/1

`␊
> 1 | unknown.split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
--------------------------------------------------------------------------------␊
Suggestion 1/1: Use \`...\` operator.␊
1 | [...unknown]␊
`

## Invalid #4
1 | const characters = "string".split("")

> Output

`␊
1 | const characters = [..."string"]␊
`

> Error 1/1

`␊
> 1 | const characters = "string".split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
`

## Invalid #5
1 | (( (( (( "string" )).split ))( (("")) ) ))

> Output

`␊
1 | (( [...(( (( "string" )) ))] ))␊
`

> Error 1/1

`␊
> 1 | (( (( (( "string" )).split ))( (("")) ) ))␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
`

## Invalid #6
1 | bar()
2 | foo.split("")

> Error 1/1

`␊
1 | bar()␊
> 2 | foo.split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
--------------------------------------------------------------------------------␊
Suggestion 1/1: Use \`...\` operator.␊
1 | bar()␊
2 | ;[...foo]␊
`

## Invalid #7
1 | unknown.split("")

> Error 1/1

`␊
> 1 | unknown.split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
--------------------------------------------------------------------------------␊
Suggestion 1/1: Use \`...\` operator.␊
1 | [...unknown]␊
`

## Invalid #8
1 | "🦄".split("")

> Error 1/1

`␊
> 1 | "🦄".split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
--------------------------------------------------------------------------------␊
Suggestion 1/1: Use \`...\` operator.␊
1 | [..."🦄"]␊
`

## Invalid #9
1 | const {length} = "🦄".split("")

> Error 1/1

`␊
> 1 | const {length} = "🦄".split("")␊
| ^^^^^ Prefer the spread operator over \`String#split('')\`.␊
--------------------------------------------------------------------------------␊
Suggestion 1/1: Use \`...\` operator.␊
1 | const {length} = [..."🦄"]␊
`
Binary file modified test/snapshots/prefer-spread.mjs.snap
Binary file not shown.