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

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

Open
shulaoda opened this issue Oct 16, 2024 · 10 comments · May be fixed by #6655
Open

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

shulaoda opened this issue Oct 16, 2024 · 10 comments · May be fixed by #6655
Labels
A-linter Area - Linter C-bug Category - Bug good first issue Experience Level - Good for newcomers

Comments

@shulaoda
Copy link
Contributor

shulaoda commented Oct 16, 2024

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
@shulaoda shulaoda added A-linter Area - Linter C-bug Category - Bug labels Oct 16, 2024
@DonIsaac
Copy link
Collaborator

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

@camc314
Copy link
Collaborator

camc314 commented Oct 16, 2024

empty items vs undefined

Screenshot 2024-10-16 at 08 58 54

@magic-akari
Copy link
Collaborator

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
Copy link
Collaborator

camc314 commented Oct 16, 2024

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

@bensaufley
Copy link

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.

@DonIsaac DonIsaac removed their assignment Oct 16, 2024
@DonIsaac DonIsaac added the good first issue Experience Level - Good for newcomers label Oct 16, 2024
@magic-akari
Copy link
Collaborator

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 sindresorhus/eslint-plugin-unicorn#2480 (comment)

@bensaufley
Copy link

bensaufley commented Oct 17, 2024

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
Copy link
Contributor Author

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
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-bug Category - Bug good first issue Experience Level - Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants