oxc-project / oxc

⚓ A collection of JavaScript tools written in Rust.
https://oxc.rs
MIT License
12.01k stars 434 forks source link

linter: unsafe fixer in `no-useless-spread` #6618

Open shulaoda opened 1 week ago

shulaoda commented 1 week ago

What version of Oxlint are you using?

0.9.10

What happened?

This change has side effects and is unsafe.

> new Array(3).map((item) => { console.log(item) })
> [...new Array(3)].map((item) => { console.log(item) })
undefined
undefined
undefined
DonIsaac commented 1 week ago

I need more details. Is the bottom one the original case? And did you mean the spread to cover the map as well?

camc314 commented 1 week ago

empty items vs undefined

Screenshot 2024-10-16 at 08 58 54
magic-akari commented 1 week ago

For all cases where n>0, new Array(n) always differs from [...new Array(n)]. Hence, I recommend disabling the check for [...new Array(n)].

camc314 commented 1 week ago

yeah we should disable fixer or suggest a fix of new Array(5).fill(undefined)

bensaufley commented 1 week ago

I mentioned this on the Discord - I used to use this method to initialize a new array and so this was in my codebase. I've since switched to Array.from({ length: 5 }, () => {/*…*/}), for whatever that's worth. That's another option.

magic-akari commented 6 days ago

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/2b469bee475a8f3f2767f4669864acdd89654017/test/no-useless-spread.mjs#L276

[...new Array(3)] appears as an invalid case in the test cases of eslint-plugin-unicorn, and I supposed this rule is based on different perspectives and assumptions.

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.

Originally posted by @sindresorhus in https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2480#issuecomment-2417777209

bensaufley commented 6 days ago

Interesting. I suppose I disagree – regardless of whether it's an antipattern, it's valid code. But OK, so, this appears to reveal an unrelated issue: I have not enabled any unicorn linters, or no-useless-spread.

I'm testing a migration from ESLint to oxlint. I don't use eslint-plugin-unicorn. So … why did this fix get applied? I used eslint --print-config to get a flat-ish version of my current config for use in oxlint, then I ran oxlint -c .oxlintrc.json --fix and I got this change.

shulaoda commented 6 days ago

But OK, so, this appears to reveal an unrelated issue: I have not enabled any unicorn linters, or no-useless-spread. then I ran oxlint -c .oxlintrc.json --fix and I got this change

This may be because it is recommended and enabled by default in oxlint.

bensaufley commented 6 days ago

Oh interesting, so rather than having a recommended preset that can be used, it just turns them on automatically and I have to go through and enumerate the ones I want to turn off? 🤔

bensaufley commented 6 days ago

I especially think that if this rule is on by default it should not make unsafe fixes.