Skip to content

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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions packages/eslint-plugin/docs/rules/no-misused-spread.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
---
description: "Disallow spread operator that shouldn't be spread most of the time."
---

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/no-misused-spread** for documentation.

Spreading a function is almost always a mistake. Most of the time you forgot to call the function.

## Examples

<!--tabs-->

### ❌ Incorrect

```ts
const fn = () => ({ name: 'name' });
const obj = {
...fn,
value: 1,
};
```

```ts
const fn = () => ({ value: 33 });
const otherFn = ({ value: number }) => ({ value: value });
otherFn({ ...fn });
```

### ✅ Correct

```ts
const fn = () => ({ name: 'name' });
const obj = {
...fn(),
value: 1,
};
```

```ts
const fn = () => ({ value: 33 });
const otherFn = ({ value: number }) => ({ value: value });
otherFn({ ...fn() });
```
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'@typescript-eslint/no-namespace': 'error',
'@typescript-eslint/no-non-null-asserted-nullish-coalescing': 'error',
'@typescript-eslint/no-non-null-asserted-optional-chain': 'error',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export = {
'@typescript-eslint/no-empty-function': 'error',
'@typescript-eslint/no-empty-interface': 'error',
'@typescript-eslint/no-inferrable-types': 'error',
'@typescript-eslint/no-misused-spread': 'error',
'@typescript-eslint/non-nullable-type-assertion-style': 'error',
'@typescript-eslint/prefer-for-of': 'error',
'@typescript-eslint/prefer-function-type': 'error',
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import noMagicNumbers from './no-magic-numbers';
import noMeaninglessVoidOperator from './no-meaningless-void-operator';
import noMisusedNew from './no-misused-new';
import noMisusedPromises from './no-misused-promises';
import noMisusedSpread from './no-misused-spread';
import noMixedEnums from './no-mixed-enums';
import noNamespace from './no-namespace';
import noNonNullAssertedNullishCoalescing from './no-non-null-asserted-nullish-coalescing';
Expand Down Expand Up @@ -198,6 +199,7 @@ export default {
'no-meaningless-void-operator': noMeaninglessVoidOperator,
'no-misused-new': noMisusedNew,
'no-misused-promises': noMisusedPromises,
'no-misused-spread': noMisusedSpread,
'no-mixed-enums': noMixedEnums,
'no-namespace': noNamespace,
'no-non-null-asserted-nullish-coalescing': noNonNullAssertedNullishCoalescing,
Expand Down
46 changes: 46 additions & 0 deletions packages/eslint-plugin/src/rules/no-misused-spread.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { type TSESLint, type TSESTree } from '@typescript-eslint/utils';

import { createRule, getParserServices } from '../util';

type MessageIds = 'forbiddenFunctionSpread';

export default createRule<[], MessageIds>({
defaultOptions: [],
name: 'no-misused-spread',
meta: {
docs: {
description:
"Disallow spread operator that shouldn't be spread most of the time",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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",

recommended: 'stylistic',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm
is it stylistic?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be your recommendation?
Should it be strict?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

requiresTypeChecking: true,
},
messages: {
forbiddenFunctionSpread:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?',

},
schema: [],
type: 'suggestion',
Copy link
Contributor

@StyleShit StyleShit Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@StyleShit StyleShit Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
create: (context): TSESLint.RuleListener => {
const listener = (node: TSESTree.SpreadElement): void => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 {
    // ...
  }
};

const services = getParserServices(context);
const checker = services.program.getTypeChecker();

const tsNode = services.esTreeNodeToTSNodeMap.get(node.argument);
const type = checker.getTypeAtLocation(tsNode);
Comment on lines +27 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);


if (
type.getProperties().length === 0 &&
type.getCallSignatures().length > 0
) {
context.report({
node,
messageId: 'forbiddenFunctionSpread',
});
}
};
return {
SpreadElement: listener,
};
},
});
106 changes: 106 additions & 0 deletions packages/eslint-plugin/tests/rules/no-misused-spread.test.ts
Copy link
Contributor

@StyleShit StyleShit Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean in the valid part?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import { RuleTester } from '@typescript-eslint/rule-tester';

import rule from '../../src/rules/no-misused-spread';
import { getFixturesRootDir } from '../RuleTester';

const rootDir = getFixturesRootDir();
const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
parserOptions: {
tsconfigRootDir: rootDir,
project: './tsconfig.json',
},
});

ruleTester.run('no-misused-spread', rule, {
valid: [
`
const a = () => ({ value: 33 });
const b = {
...a(),
name: 'name',
};
`,
`
const a = (x: number) => ({ value: x });
const b = {
...a(42),
name: 'name',
};
`,
`
interface FuncWithProps {
property?: string;
(): number;
}
const funcWithProps: FuncWithProps = () => 1;
funcWithProps.property = 'foo';
const spreadFuncWithProps = { ...funcWithProps };
`,
`
type FuncWithProps = {
property?: string;
(): number;
};
const funcWithProps: FuncWithProps = () => 1;
funcWithProps.property = 'foo';
const spreadFuncWithProps = { ...funcWithProps };
`,
`
const a = { value: 33 };
const b = {
...a,
name: 'name',
};
`,
`
const a = {};
const b = {
...a,
name: 'name',
};
`,
`
const a = () => ({ value: 33 });
const b = ({ value: number }) => ({ value: value });
b({ ...a() });
`,
`
const a = [33];
const b = {
...a,
name: 'name',
};
`,
],
invalid: [
{
code: `
const a = () => ({ value: 33 });
const b = {
...a,
name: 'name',
};
`,
errors: [{ line: 4, column: 3, messageId: 'forbiddenFunctionSpread' }],
},
{
code: `
const a = (x: number) => ({ value: x });
const b = {
...a,
name: 'name',
};
`,
errors: [{ line: 4, column: 3, messageId: 'forbiddenFunctionSpread' }],
},
{
code: `
const a = () => ({ value: 33 });
const b = ({ value: number }) => ({ value: value });
b({ ...a });
`,
errors: [{ line: 4, column: 5, messageId: 'forbiddenFunctionSpread' }],
},
],
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.