`no-useless-spread` it is not safe to remove spread in `[...foo.concat(bar)]`

[magic-akari]

eslint-plugin-unicorn/test/no-useless-spread.mjs

Line 259 in 2b469be

'[...foo.concat(bar)]',

Note

The concat() method preserves empty slots if any of the source arrays is sparse.

via MDN

const foo = new Array(5);
const bar = new Array(5);

const cat = [...foo.concat(bar)];
// now we got a new array with undefined x 10
// but foo.concat(bar) is empty x 10.
// It's not what we want.

[sindresorhus]

Sparse arrays are an anti-pattern, so we're not going to change the auto-fix, but we should document the issue with sparse arrays, so people can choose to turn off the rule if they want to use sparse arrays.

[bensaufley]

So, one way to initialize a non-sparse array is [...new Array(10)]. This is valid JS, and in this scenario, the spread is not useless: it has a meaningful effect. I definitely understand there are different ways to do that (I use Array.from({ length: 10 }, () => …) now), but the above is valid JS that does not result in a sparse array. The fix is unsafe in this scenario. It seems like, at a minimum, the rule should be marked as a potentially unsafe fix.

Interestingly, eslint's docs list new Array(23) as a "correct" example under no-sparse-arrays. So even if a user says "I don't want to use sparse arrays", enables that rule, and thus thinks no-useless-spread is safe, they may run into this particular issue.

Linters are here to help us write better code, but if you only accept good code to lint in the first place, I feel like that's missing the point of why people use a linter. If the opinion of the linter is

[fregante]

if you only accept good code to lint in the first place, I feel like that's missing the point of why people use a linter

You have a good point here, but I think a lot of unicorn rules build on each other. For example unicorn/no-new-array already rewrites it:

- [...new Array(10)]
+ [...Array.from({length: 10})]

Then unicorn/no-useless-spread rewrites it again:

- [...Array.from({length: 10})]
+ Array.from({length: 10})

$ xo
  index.js:25:1
  ✖  25:1   Unnecessarily cloning an array.                                         unicorn/no-useless-spread
  ✖  25:5   Do not use new Array().                                                 unicorn/no-new-array

For your code, this is already being auto-fixed correctly by xo --fix (which runs the whole recommended list of unicorn)

- [...new Array(10)]
+ Array.from({length: 10})

I can see the same happening for OP's code:

- const foo = new Array(5);
- const bar = new Array(5);
+ const foo = Array.from({length: 5});
+ const bar = Array.from({length: 5});

- const cat = [...foo.concat(bar)];
+ const cat = foo.concat(bar);

With further advice on the concat, not autofixed:

  ✖  28:17  Prefer the spread operator over Array#concat(…).                               unicorn/prefer-spread

The result is the same, cat has 10 undefined items.

In short, the advice to "turn off the rule if they want to use sparse arrays" seems valid here.

[fregante]

So, one way to initialize a non-sparse array is [...new Array(10)]

If this is a common pattern, it could be detected and autofixed to Array.from({length: 10}) as part of no-new-array, because the intent is clear and the rule is still valid.

For a generic [...foo.concat(bar)] it's hard to give the same advice because the rule assumes you have no "bad code" in your codebase already, thanks to other rules.

If you think about it, even eslint makes a lot of assumptions on code safety, for example it assumes you don't rewrite .toString() and other anti-patterns.