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

lcharlois-neotys · 2023-10-23T09:31:59Z

PR Checklist

Addresses an existing open issue: fixes #000
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Partially addresses #748.
Add a new rule that prevent spreading a function.

…ction`.

typescript-eslint · 2023-10-23T09:32:03Z

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 · 2023-10-23T09:32:04Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`4aed59b`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/655d9e8a567913000824c5de
😎 Deploy Preview	https://deploy-preview-7822--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 8 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (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 · 2023-10-23T09:33:51Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit bd83499. 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

✅ Successfully ran 19 targets

Sent with 💌 from NxCloud.

Josh-Cena

This rule's scope is too narrow. We should probably rename it to something like no-misused-spread, so it can be easily expanded to support spreading arrays and objects without any data properties.

StyleShit · 2023-11-21T19:40:12Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+  meta: {
+    docs: {
+      description: 'Disallow spread operator on function',
+      recommended: 'stylistic',


hmm
is it stylistic?

What will be your recommendation?
Should it be strict?

TBH I'm not sure what the guidelines are, let's wait for a maintainer

https://typescript-eslint.io/linting/configs - this is a functional, bug-catching rule so it wouldn't be 'stylistic'. It would be 'strict' for now to avoid a breaking change. In each new next major version we look at which rules might be promoted to 'recommended'.

packages/eslint-plugin/src/rules/no-misused-spread.ts

StyleShit · 2023-11-21T19:43:31Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+        'Spreading a function is almost always a mistake. Did you forgot to call the function?',
+    },
+    schema: [],
+    type: 'suggestion',


I think this is a "problem" rather than a "suggestion", WDYT?

IMO this is most of the time a problem, but untill today there is nothing warning the user that it is one.
So I guess this is an expected usage and I choose suggestion rather than problem.
I can change it if needed.

Not sure...
let's wait for a maintainer I guess?

It's a 'problem'. Anything that catches directly incorrect logic is a problem.

packages/eslint-plugin/src/rules/no-misused-spread.ts

StyleShit · 2023-11-21T19:57:50Z

packages/eslint-plugin/tests/rules/no-misused-spread.test.ts

+    `
+const a = () => ({ value: 33 });
+const b = ({ value: number }) => ({ value: value });
+    `,


where is the spread here?

I think it is a copy/paste from the previous in order to test another case, but honestly I don't remember since last time I worke on this PR.
I remove it.

StyleShit · 2023-11-21T20:00:49Z

packages/eslint-plugin/tests/rules/no-misused-spread.test.ts

maybe also add tests for spreading Map, Set, and a class instance?

EDIT: hmm, should we include a custom iterable as well? might be overkill, IDK

Do you mean in the valid part?

+1 on adding a test for a custom iterable :)

packages/eslint-plugin/src/rules/no-misused-spread.ts

StyleShit · 2023-11-21T20:04:35Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+  name: 'no-misused-spread',
+  meta: {
+    docs: {
+      description: 'Disallow spread operator on function',


should probably be updated to a more generic "no-spread" description

I've proposed a change.

StyleShit · 2023-11-21T20:08:59Z

If you've used some of @OliverJAsh's code from this comment, consider adding him as a co-author 😄

Co-authored-by: StyleShit <32631382+StyleShit@users.noreply.github.com>

lcharlois-neotys · 2023-11-22T05:14:45Z

If you've used some of @OliverJAsh's code from this comment, consider adding him as a co-author 😄

I will be happy. Could you tell me how to add co-author?
Is adding something in the PR description enough?

Co-authored-by: StyleShit <32631382+StyleShit@users.noreply.github.com>

StyleShit · 2023-11-22T14:26:12Z

If you've used some of @OliverJAsh's code from this comment, consider adding him as a co-author 😄

I will be happy. Could you tell me how to add co-author? Is adding something in the PR description enough?

You need to add "Co-authored-by: {{userName}}<{{userEmail}}>" in the commit description.

You can either do it on merge (but you're not gonna be the merger, so that's not an option), or add a commit with this description and it'll automatically be appended when the PR is merged.

See:
https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

StyleShit

looks good to me overall except for a small nitpicking, and maybe add the tests we've discussed earlier

anyway, I'm not a maintainer, just stumbled across this PR, so let's wait for one 😄

StyleShit · 2023-11-22T14:30:42Z

packages/eslint-plugin/src/configs/all.ts

@@ -97,6 +97,7 @@ export = {
    '@typescript-eslint/no-misused-new': 'error',
    '@typescript-eslint/no-misused-promises': 'error',
    '@typescript-eslint/no-mixed-enums': 'error',
+    '@typescript-eslint/no-misused-spread': 'error',


[Nit]
This line should probably need to be one line above

JoshuaKGoldberg

Looking good! Requesting expanding and a few small cleanups, but this is a great start 🙂

JoshuaKGoldberg · 2023-12-05T21:18:47Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+  meta: {
+    docs: {
+      description:
+        "Disallow spread operator that shouldn't be spread most of the time",


[Proofreading] "most of the time" is kind of an imprecise, vague statement. Users may not understand it.

Instead, let's speak to what this rule is specifically trying to avoid: spreads that don't do anything.

Suggested change

"Disallow spread operator that shouldn't be spread most of the time",

"Disallow using the spread operator on values that can't be spread",

JoshuaKGoldberg · 2023-12-05T21:20:31Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+    type: 'suggestion',
+  },
+  create: (context): TSESLint.RuleListener => {
+    const listener = (node: TSESTree.SpreadElement): void => {


[Style] Is there a reason you'd wanted to split this out instead of having it inline at the bottom of the function? Inline would mean an inferrable type for node:

return { SpreadElement(node): void { // ... } };

JoshuaKGoldberg · 2023-12-05T21:21:34Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+      const checker = services.program.getTypeChecker();
+
+      const tsNode = services.esTreeNodeToTSNodeMap.get(node.argument);
+      const type = checker.getTypeAtLocation(tsNode);


[Cleanup] We have a new thing for this 😄 https://typescript-eslint.io/blog/announcing-typescript-eslint-v6#type-checker-wrapper-apis

Suggested change

const checker = services.program.getTypeChecker();

const tsNode = services.esTreeNodeToTSNodeMap.get(node.argument);

const type = checker.getTypeAtLocation(tsNode);

const type = services.getTypeAtLocation(node.argument);

JoshuaKGoldberg · 2023-12-05T21:22:14Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+    },
+    messages: {
+      forbiddenFunctionSpread:
+        'Spreading a function is almost always a mistake. Did you forget to call the function?',


[Proofreading] Maybe...

Suggested change

'Spreading a function is almost always a mistake. Did you forget to call the function?',

'Spreading a function with no custom properties does nothing. Did you forget to call the function?',

JoshuaKGoldberg · 2023-12-05T21:23:18Z

packages/eslint-plugin/src/rules/no-misused-spread.ts

+      requiresTypeChecking: true,
+    },
+    messages: {
+      forbiddenFunctionSpread:


[Feature] If we add the rule as-is (only looking at functions), it'd be a breaking change to later add in Maps, Sets, etc. I think the rule should include all of what's requested in the issue to start.

LMK if any of them give you grief, happy to help!

JoshuaKGoldberg · 2023-12-29T18:31:09Z

👋 Ping @lcharlois-neotys, is this still something you have time for? No worries if not - I just don't want to leave it hanging.

lcharlois-neotys · 2024-01-03T09:05:33Z

👋 Ping @lcharlois-neotys, is this still something you have time for? No worries if not - I just don't want to leave it hanging.

Yes, I just need a bit of time to reread all the comments and do the changes.
I will try next week.

JoshuaKGoldberg · 2024-01-29T16:48:29Z

Closing this PR as it's been stale for a while without activity. cc @lcharlois-neotys - if you want to keep working on it, that'd be swell. But no worries if you don't have time. If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding a co-author attribution at the end of your PR description. Thanks! 😊

feat(eslint-plugin): [no-spread-function] Add new rule `no-spread-fun…

feb9556

…ction`.

lcharlois-neotys changed the title ~~feat(eslint-plugin): [no-spread-function] Add new rule no-spread-function~~ feat(eslint-plugin): [no-spread-function] add new rule no-spread-function Oct 23, 2023

lcharlois-neotys added 2 commits October 23, 2023 12:45

Fix lint & test issues

cc72332

Fix config tests

bcf64c3

Josh-Cena requested changes Oct 23, 2023

View reviewed changes

Rename rule from no-spread-function to no-misused-spread

f0b5507

lcharlois-neotys requested a review from Josh-Cena October 24, 2023 10:05

lcharlois-neotys changed the title ~~feat(eslint-plugin): [no-spread-function] add new rule no-spread-function~~ feat(eslint-plugin): [no-misused-spread] add new rule no-misused-spread Oct 25, 2023

Merge branch 'main' into add-no-spread-function-rule

bd83499

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Nov 10, 2023

StyleShit suggested changes Nov 21, 2023

View reviewed changes

packages/eslint-plugin/src/rules/no-misused-spread.ts Outdated Show resolved Hide resolved

StyleShit suggested changes Nov 21, 2023

View reviewed changes

Update packages/eslint-plugin/src/rules/no-misused-spread.ts

28eaaaa

Co-authored-by: StyleShit <32631382+StyleShit@users.noreply.github.com>

lcharlois-neotys and others added 8 commits November 22, 2023 06:14

Update packages/eslint-plugin/src/rules/no-misused-spread.ts

4377ed4

Co-authored-by: StyleShit <32631382+StyleShit@users.noreply.github.com>

Update packages/eslint-plugin/src/rules/no-misused-spread.ts

a8c6469

Co-authored-by: StyleShit <32631382+StyleShit@users.noreply.github.com>

Update packages/eslint-plugin/src/rules/no-misused-spread.ts

1f23aa7

Co-authored-by: StyleShit <32631382+StyleShit@users.noreply.github.com>

Remove useless test case.

b994177

Fix import and code after accepting suggestion

33ef9f3

Make rule description more generic.

34a9dbc

Change message id.

d84cf04

Fixing quotes in documentation.

4aed59b

lcharlois-neotys requested a review from StyleShit November 22, 2023 07:35

StyleShit reviewed Nov 22, 2023

View reviewed changes

JoshuaKGoldberg requested changes Dec 5, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 5, 2023

JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Dec 29, 2023

JoshuaKGoldberg removed the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jan 11, 2024

bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jan 27, 2024

JoshuaKGoldberg closed this Jan 29, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lcharlois-neotys commented Oct 23, 2023 •

edited by Josh-Cena

Loading

typescript-eslint bot commented Oct 23, 2023

netlify bot commented Oct 23, 2023 •

edited

Loading

nx-cloud bot commented Oct 23, 2023 •

edited

Loading

Josh-Cena left a comment •

edited

Loading

StyleShit Nov 21, 2023

lcharlois-neotys Nov 22, 2023

StyleShit Nov 22, 2023

JoshuaKGoldberg Dec 5, 2023

StyleShit Nov 21, 2023 •

edited

Loading

lcharlois-neotys Nov 22, 2023

StyleShit Nov 22, 2023 •

edited

Loading

JoshuaKGoldberg Dec 5, 2023

StyleShit Nov 21, 2023

lcharlois-neotys Nov 22, 2023

StyleShit Nov 21, 2023 •

edited

Loading

lcharlois-neotys Nov 22, 2023

StyleShit Nov 22, 2023

JoshuaKGoldberg Dec 5, 2023

StyleShit Nov 21, 2023

lcharlois-neotys Nov 22, 2023

StyleShit commented Nov 21, 2023 •

edited

Loading

lcharlois-neotys commented Nov 22, 2023

StyleShit commented Nov 22, 2023

StyleShit left a comment

StyleShit Nov 22, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg Dec 5, 2023

JoshuaKGoldberg Dec 5, 2023

JoshuaKGoldberg Dec 5, 2023

JoshuaKGoldberg Dec 5, 2023

JoshuaKGoldberg Dec 5, 2023

JoshuaKGoldberg commented Dec 29, 2023

lcharlois-neotys commented Jan 3, 2024

JoshuaKGoldberg commented Jan 29, 2024

	"Disallow spread operator that shouldn't be spread most of the time",
	"Disallow using the spread operator on values that can't be spread",

	'Spreading a function is almost always a mistake. Did you forget to call the function?',
	'Spreading a function with no custom properties does nothing. Did you forget to call the function?',

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

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

Conversation

lcharlois-neotys commented Oct 23, 2023 • edited by Josh-Cena Loading

PR Checklist

Overview

typescript-eslint bot commented Oct 23, 2023

netlify bot commented Oct 23, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Oct 23, 2023 • edited Loading

☁️ Nx Cloud Report

Josh-Cena left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit commented Nov 21, 2023 • edited Loading

lcharlois-neotys commented Nov 22, 2023

StyleShit commented Nov 22, 2023

StyleShit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Dec 29, 2023

lcharlois-neotys commented Jan 3, 2024

JoshuaKGoldberg commented Jan 29, 2024

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

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

lcharlois-neotys commented Oct 23, 2023 •

edited by Josh-Cena

Loading

netlify bot commented Oct 23, 2023 •

edited

Loading

nx-cloud bot commented Oct 23, 2023 •

edited

Loading

Josh-Cena left a comment •

edited

Loading

StyleShit Nov 21, 2023 •

edited

Loading

StyleShit Nov 22, 2023 •

edited

Loading

StyleShit Nov 21, 2023 •

edited

Loading

StyleShit commented Nov 21, 2023 •

edited

Loading