feat(eslint-plugin): [no-misused-spread] add new rule

[StyleShit]

Closes #748

[typescript-eslint[bot]]

Thanks for the PR, @StyleShit!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

[netlify[bot]]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	af82cbb37a028d78723232e5d79cb9784ab67c52
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66c579e023f76c0008d8e90d
😎 Deploy Preview	https://deploy-preview-8509--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (🔴 down 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[nx-cloud[bot]]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit af82cbb37a028d78723232e5d79cb9784ab67c52. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

🟥 Failed Commands
nx test eslint-plugin --coverage=false

✅ Successfully ran 28 targets

• nx test type-utils --coverage=false
• nx test typescript-estree --coverage=false
• nx run types:build
• nx run-many --target=build --exclude website --exclude website-eslint
• nx test scope-manager --coverage=false
• nx test visitor-keys --coverage=false
• nx test typescript-eslint --coverage=false
• nx test utils --coverage=false
• nx test parser --coverage=false
• nx run-many --target=clean
• nx test rule-schema-to-typescript-types --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx run-many --target=lint --exclude eslint-plugin
• nx test visitor-keys
• nx test utils
• nx test typescript-estree
• nx test eslint-plugin-internal
• nx test typescript-eslint
• nx test type-utils
• nx test scope-manager
• nx test rule-schema-to-typescript-types
• nx test parser
• nx test ast-spec
• nx run-many --target=typecheck
• nx test ast-spec --coverage=false
• nx run integration-tests:test
• nx generate-configs

---

Sent with 💌 from NxCloud.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.27%. Comparing base (6250dab) to head (ab20e73).

:exclamation: Current head ab20e73 differs from pull request most recent head af82cbb

Please upload reports for the commit af82cbb to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8509      +/-   ##
==========================================
- Coverage   88.01%   87.27%   -0.75%     
==========================================
  Files         406      252     -154     
  Lines       13879    12363    -1516     
  Branches     4055     3899     -156     
==========================================
- Hits        12216    10790    -1426     
+ Misses       1354     1303      -51     
+ Partials      309      270      -39     

Flag	Coverage Δ
unittest	87.27% <100.00%> (-0.75%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...kages/eslint-plugin/src/rules/no-misused-spread.ts	100.00% <100.00%> (ø)

... and 346 files with indirect coverage changes

[StyleShit]

Not sure why the tests are failing, seems to pass locally

[JoshuaKGoldberg]

👋 Ping, just checking in @StyleShit - is this still something you have time to make progress on? Or is there anything you're waiting on us on?

[StyleShit]

@JoshuaKGoldberg

You pinged my right on the day I was able to work on it 😄

[StyleShit]

The tests are passing locally, not sure why it fails here

[StyleShit]

If we have separate options for allowStrings and allowIterables, we should have one for allowArrays too. And TBH, I don't think allowIterables is useful as an option because iterables have drastically different shapes and functions. Some iterables can be perfectly used as plain objects while other like Map and Set are basically useless, so I personally would never turn it on, neither can I imagine it not being a footgun to anyone else.

Yeah, I get your point here. So do you think we should stop reporting iterables unless they're of specific and known types (Set, Array, String, etc.)?

In contrast, you've said here that we should forbid spreading any iterables and allow certain types. So I'm not sure whether I understood you correctly 😅

Do allowClassDeclarations (which again I would strongly recommend renaming) and allowClassInstances have tests for built-in types such as new RegExp or new HTMLElement (which use a slightly different convention to allow declaration merging)? Are they intended to be supported?

There are no tests for these specific cases, but I've added tests for new Date IIRC. Seems like the rule reports these cases correctly BTW

Can you please explain (or point me to an explanation about) the difference in their declaration in the context of this rule? I'm not sure that I understand what you mean here, but it might explain why this happens(?)

[Josh-Cena]

Sorry I missed the last reply.

My main point on allowIterables is this:

so I personally would never turn it on, neither can I imagine it not being a footgun to anyone else.

i.e. it shouldn't exist as an option and iterables should be reported by default, with options to allow certain types.

Can you please explain (or point me to an explanation about) the difference in their declaration in the context of this rule?

Built-in classes are not declared as classes. They look like this:

interface WeakMapConstructor {
  new (): WeakMap;
  // ...
}

interface WeakMap {
  // ...
}

var WeakMap: WeakMapConstructor;

This approach allows versioned declarations to be merged.

[StyleShit]

i.e. it shouldn't exist as an option and iterables should be reported by default, with options to allow certain types.

So you're proposing to change the allowIterables option to accept an array of types rather than a boolean?

This approach allows versioned declarations to be merged.

Cool! thanks for the explanation 😄

[Josh-Cena]

So you're proposing to change the allowIterables option to accept an array of types rather than a boolean?

I think if we go down that path we should just have a generic allow array. Otherwise all the difference is that we also validate if the allowed type is also iterable, which doesn't appear necessary.

[StyleShit]

Otherwise all the difference is that we also validate if the allowed type is also iterable

Yeah, ok, makes sense!

@bradzacher @JoshuaKGoldberg, thoughts?

[JoshuaKGoldberg]

I agree with Josh. 🙂

[StyleShit]

@Josh-Cena @JoshuaKGoldberg

Added an allowForKnownSafeIterables option to align with allowForKnownSafePromises, WDYT?

[Josh-Cena]

Why allowForKnownSafeIterables instead of a generic allow array for any kind of type?

[StyleShit]

Why allowForKnownSafeIterables instead of a generic allow array for any kind of type?

Well, I need a condition for reporting a type before allowing it, right? which means something like:

if (shouldBeReported() && !isAllowed()) {
	report();
}

I mean... what will be the flags for reporting types if there is a generic "allow" option? wouldn't it flag basically anything?

if (! isAllowed()) {
	report();
}

Unless I'm missing something here and/or not understanding you correctly

[Josh-Cena]

I meant—currently, the rule looks like this:

if (type.isString()) report();
else if (type.isFunction()) report();
else ...
else if (type.isIterable() && !allowForKnownSafeIterables.includes(type)) report();

I'm saying why can't it be:

if (allow.includes(type)) return;
if (type.isString()) report();
else if (type.isFunction()) report();
...

This is not going to be much heavier since we have to query the type for every spread anyway. This is going to allow us to ignore only certain classes or certain class instances, in addition to certain iterable types.

Put another way, it could also look like this:

function maybeReport(type) {
  if (allow.includes(type)) return;
  report();
}

if (type.isString()) maybeReport(type);
else if (type.isFunction()) maybeReport(type);
...

[StyleShit]

Oh, I see, good point!

implemented, and it simplified the code a little bit