linter: false positive in unicorn/no-useless-spread

[camc314]

the following code proiduces an error on the ...

const test2 = [...new Set(test || [])];

https://oxc-project.github.io/oxc/playground/?code=3YCAAICogICAgICAgICxG0qZRraXVswdRc9QqqX01RSLQj6CIL60mOY7cTm3PXoTnCmrrStudD9%2FdpNkgA%3D%3D

this is incorrect as new Set() returns a Set where as [...new Set(test || [])] returns an array.

looks like this regressed in #4791 ?

[DonIsaac]

It did come from there, but it was intentional. The thought is that most code written like this doesn't necessarily intending to cast a set into an array, and this instead indicates a bug.

Could we suggest that users use Array.from() to cast iterables into arrays in these cases?

[camc314]

hmm to me, this feels like quite a common case? a user wants to remove any duplicate elements from the array.

maybe set is special here and has different semantics?

[DonIsaac]

I'm ok with changing this behavior if such usage is commonplace. Did you run into this because of a linter diagnostic in one of your codebases? Do you have examples of this being done elsewhere?

[camc314]

yeah we've got it in a bunch of places in our internal codebases.

searching GitHub also reveals ~155k results:

https://github.com/search?q=%22%5B...new+Set%22+AND+%28language%3AJavaScript+OR+language%3ATypeScript+%29&type=code

searching google for "remove duplicate values from array", and this seems to be the main solution (some solutions suggest .filter and .includes which feels painfully slow)

Ref:

• https://builtin.com/software-engineering-perspectives/remove-duplicates-from-array-javascript (2nd suggestion)
• https://stackoverflow.com/questions/9229645/remove-duplicate-values-from-js-array
• https://www.geeksforgeeks.org/how-to-remove-duplicate-elements-from-javascript-array/
• https://dev.to/fpaghar/remove-duplicates-ways-from-an-array-in-javascript-3k1d

[DonIsaac]

👀 guess we're changing this then

[DonIsaac]

I'm currently moving apartments, but I'll address this when I have time. I have a particular solution in mind.

[camc314]

does this seem like a reasonable use case for ... and new Set? maybe we could have an option that turns this behaviour off

[DonIsaac]

This does seem reasonable and used quite frequently, based on the links you sent. I'm ok with removing it straight up. Thoughts?

[camc314]

sounds good, yeah i think it's gonna be a common use case - we shouldn't report on it

[craftedsystems]

Please remove this as it is just wrong. :)

[danielfrenda]

Another thing to point out is pairing this with the prefer-spread rule is annoying.
It seems eslint's no-useless-spread already accounts for this and wont report on [...new Set( )]
Which then makes an // eslint ignore a pain, where its needed for oxlint but not for eslint, which autofixes the unused ignore directive away causing oxlint to fail