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

StyleShit · 2024-02-19T20:33:21Z

Closes #748

typescript-eslint · 2024-02-19T20:33:25Z

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 · 2024-02-19T20:33:39Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`8a1a309`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66572cc79506e100089a4a11
😎 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: 92 (🔴 down 6 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 · 2024-02-19T20:34:36Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8a1a309. 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 31 targets

Sent with 💌 from NxCloud.

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

codecov · 2024-02-19T21:11:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.27%. Comparing base (4d25b07) to head (ab20e73).

❗ Current head ab20e73 differs from pull request most recent head 8a1a309

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8509      +/-   ##
==========================================
- Coverage   88.13%   87.27%   -0.87%     
==========================================
  Files         411      252     -159     
  Lines       14291    12363    -1928     
  Branches     4171     3899     -272     
==========================================
- Hits        12596    10790    -1806     
+ Misses       1387     1303      -84     
+ Partials      308      270      -38

Flag	Coverage Δ
unittest	`87.27% <100.00%> (-0.87%)`	⬇️

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 228 files with indirect coverage changes

…sused-spread

StyleShit · 2024-02-20T17:58:17Z

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

JoshuaKGoldberg

Very clean! I like this a lot. Just a few touchup requests. ✨

packages/eslint-plugin/docs/rules/no-misused-spread.md

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

packages/eslint-plugin/docs/rules/no-misused-spread.md

…sused-spread

JoshuaKGoldberg

Progress! 🚀

Sorry, I'm only now realizing the class instance stuff might need an option. I think I've look at this PR too much - we should get someone else from @typescript-eslint/triage-team to look too. 😅

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

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

…sused-spread

auvred · 2024-05-04T18:35:10Z

packages/eslint-plugin/docs/rules/no-misused-spread.md

[tests] After #8497 we render snapshots of ESLint output for each code sample in docs. Could you please merge main and update the snapshots with yarn test docs -u?

Done, thanks!

auvred · 2024-05-04T18:35:12Z

packages/eslint-plugin/docs/rules/no-misused-spread.md

+
+## Examples
+
+<!--tabs-->


[docs] The documentation is now being built with Docusaurus v3: #8209. Could you please update these  comments to <Tabs> MDX tags?

auvred · 2024-05-04T18:35:29Z

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

+function isString(type: ts.Type): boolean {
+  return tsutils
+    .typeParts(type)
+    .some(t => !!isTypeFlagSet(t, ts.TypeFlags.StringLike));
+}


Suggested change

function isString(type: ts.Type): boolean {

return tsutils

.typeParts(type)

.some(t => !!isTypeFlagSet(t, ts.TypeFlags.StringLike));

}

function isString(type: ts.Type): boolean {

return tsutils

.unionTypeParts(type)

.some(unionPart => tsutils.intersectionTypeParts(unionPart)

.some(intersectionPart => !!isTypeFlagSet(t, ts.TypeFlags.StringLike))

);

}

typeParts iterates through the first level of .types property only. Looks like a bug:

declare const aa: (string & { foo: 1 }) | (string & { bar: 1 }) [...aa] // not reported - bug declare const bb: (string & { foo: 1 }) [...bb] // reported - ok

playground

Great catch!

auvred · 2024-05-04T18:35:33Z

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

+function isPromise(program: ts.Program, type: ts.Type): boolean {
+  return tsutils.typeParts(type).some(t => isPromiseLike(program, t));
+}


Suggested change

function isPromise(program: ts.Program, type: ts.Type): boolean {

return tsutils.typeParts(type).some(t => isPromiseLike(program, t));

}

isPromiseLike already contains the type part iteration logic 😄 I think there is no need to do typeParts here!

typescript-eslint/packages/type-utils/src/builtinSymbolLikes.ts

Lines 132 to 141 in f248e68

if (type.isIntersection()) {

return type.types.some(t =>

isBuiltinSymbolLikeRecurser(program, t, predicate),

);

}

if (type.isUnion()) {

return type.types.every(t =>

isBuiltinSymbolLikeRecurser(program, t, predicate),

);

}

seems like it doesn't catch this case:

declare const maybePromise: Promise<number> | { a: number }; const o = { ...maybePromise };

and I think we should catch it, because it means we might spread a promise in runtime, right?

anyway, see this comment:
#8509 (comment)

I'll be glad to get some help here with removing this check completely 😄
Wasn't able to figure out why isClassOrInterface didn't work here

…sused-spread

bradzacher

A few questions, one blocker.
This is looking great overall - good work!

Feel free to turn this rule on for this repo.
It'll be a good test case and we'll want this rule anyways.
Eventually (probably v9) I can see us turning this on as a recommended rule.

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

bradzacher · 2024-05-28T03:10:17Z

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

+      if (isPromise(services.program, type)) {
+        context.report({
+          node,
+          messageId: 'noSpreadInObject',
+          data: {
+            type: 'Promise',
+          },
+        });
+
+        return;
+      }


question - do we want to handle this with this rule?
no-misused-promises also handles this case

It's okay if the answer is yes! Just wanna make sure we consciously say "yes we're okay with both rules reporting this". I'm personally not super bothered by the dual reporting.

honestly? I'm not sure about it
I don't have any strong opinion in this case, and had the same thought about no-misused-promises.

@JoshuaKGoldberg said we should remove it, and that it should be reported by the isClassInstance function (which doesn't work for some reason... should we call this bug a feature and call it a day? 😅)

My 2c:
I'm okay with having such a check here as it's possible to have one rule but not the other.

If we are going to special-case promise in this rule then the error should also be special-cased as well - eg by adding "did you forget to await the promise?"

Sounds good to me
done :)

bradzacher · 2024-05-28T03:19:14Z

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

+      if (isIterable(type, checker) && !isString(type)) {
+        context.report({
+          node,
+          messageId: 'noSpreadInObject',
+          data: {
+            type: 'Iterable',
+          },
+        });
+
+        return;
+      }


question: what about cases like this:

const iterator = { *[Symbol.iterator]() { yield 1; yield 2; yield 3; }, prop: 1, }; ({ ...iterator });

playground

should this be allowed?

spreading iterables is quite weird because it creates a clone of the Symbol.iterator function itself - but ofc it doesn't account for the fact that they're sharing implementation details.

for example

let outerState = 1; const iterator = { *[Symbol.iterator]() { if (outerState > 1) { return; } yield outerState++; }, }; const x = ({ ...iterator }); for (const y of x) { console.log(y) } // -> 1 for (const y of iterator) { console.log(y) } // no logs

I'm not sure what the correct behaviour should be here for the rule.

Well... that's a great question actually

Regarding the first case - I still think the result you get is weird, who would expect to get an object with the first value of the iterator, and a key-value pair of the iterator function?

const iterator = { *[Symbol.iterator]() { yield 1; yield 2; yield 3; }, prop: 1, }; console.log({ ...iterator }); // { prop: 1, Symbol(Symbol.iterator): * Symbol.iterator() }

Regarding the second one - That's a great catch, haven't thought about it, but again, you'd get a (subjectively?) weird result from spreading the iterator in the first place, so... 🤷‍♂️

Should we make it an option? (if we're already there, should we make everything an option?)

I'm just not familiar enough with custom iterables to know for sure what the "idiomatic" usages are.

Are there many cases where you do want to clone an iterables via an object spread?

Having an option to control the case it is probably the best option?

Cc @typescript-eslint/triage-team

Note: It looks like the "array in object spread" case is handled via this "iterator" handling.

If we do decide to remove this handling or we put this handling behind an option -- then we should special-case arrays and tuples so they're always banned, given that that's a concrete, well-defined cases that you'd never ever want, and because it's easy to catch from the type system side of things (there's utils to detect the array type)

whoah, that's a good catch, thank you!
fixed

Here's what I know: you would almost never have a custom iterable that actually spreads and remains iterable. This is because usually [Symbol.iterator]() is defined on the class (such as Set.prototype[@@iterator]), not as an own property of each instance, and it's usually bad practice to make methods enumerable in any case.

I would suggest forbidding all iterables from being spreaded as objects, with an option that allows certain types. There's probably no reason you want to spread maps or sets but there might be reasons to spread other iterables.

bradzacher · 2024-05-28T03:22:20Z

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

+        'Using the spread operator on a function without additional properties can cause unexpected behavior. Did you forget to call the function?',
+
+      noClassInstanceSpreadInObject:
+        'Using the spread operator on class instances without `[Symbol.iterator]` can cause unexpected behavior.',


RC:
I'm not sure I understand this error message.
Symbol.iterator isn't used for object spreads.

class C { prop = 1; *[Symbol.iterator]() { yield 2 } } ({...(new C)}) // -> {prop: 1}

This error probably should mention the loss of the super class / prototype chain?

I... don't know why I wrote that 😂
yup, that's wrong, I don't even check that in the rule...

changed the message

bradzacher · 2024-05-28T03:24:35Z

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

+        properties: {
+          allowClassInstances: {
+            description:
+              'Whether to allow spreading class instances in objects.',
+            type: 'boolean',
+          },
+        },


question:
do we want to add options for each of the cases here just to stave off future issues from people asking to allow specific cases?

I'm personally okay with being lazy and not - just worth calling out so we have the decision logged.

huh, I just proposed the same in this comment 😄

will do

bradzacher · 2024-05-28T03:26:04Z

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

+      noClassDeclarationSpreadInObject:
+        'Using the spread operator on class declarations can cause unexpected behavior. Did you forget to instantiate the class?',


Spreading a class declaration will spread the static properties of the class.
it does carry the same risks as spreading a class instance (losing the prototype chain / the "static this" context).

Should this similarly be covered by an option?
Again I'm okay with the answer being no :)

added an option, and clarified the message

bradzacher · 2024-05-28T03:27:08Z

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

+      if (node.argument.type === AST_NODE_TYPES.ClassExpression) {
+        context.report({
+          node,
+          messageId: 'noClassDeclarationSpreadInObject',
+        });
+
+        return;
+      }


RC:
we should use types for this case to trace the type back to determine if it's a class declaration or not.

not sure that I fully understand what you mean, can you explain please?

EDIT: seems like my code reports wrongly in a real codebase (this repo). I'm assuming it's related to what you're saying?

on a side not: what does "RC" mean? 😅

RC = "request change"

Currently this check is a pure syntactic check. It is specifically only looking for this case: ({...(class C {})})

This is, obviously, fairly limited and very specific. Instead we should be looking at the type of the thing being spread to determine if it is a class declaration or not so we can support cases like

class A {} const B = class {}; const C = Set<string>; ({...A}); ({...B}); ({...C});

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

…sused-spread

bradzacher · 2024-05-29T01:41:40Z

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

+      allowPromises: false,
+      allowFunctions: false,
+      allowMaps: false,


My 2c: I think that we don't want allowPromises or allowMaps.
Like the Array case - these cases are special cases that are always going to be bad.

gotcha, done

…sused-spread

omril1 · 2024-05-29T15:48:31Z

packages/eslint-plugin/docs/rules/no-misused-spread.mdx

+### `allowClassDeclarations`
+
+By default, this rule disallows using the spread operator on class declarations:


I don't like the name of this option, as the class could also be passed as a reference or be generated by a class factory.
I asked ChatGPT for a better name and it suggested "class reference" as an alternative, WDYT?

I personally don't have any strong opinion about either of the names

Maybe let's wait for one of the maintainers to decide?

I would suggest allowClasses. Any concerns?

@Josh-Cena It's better than declaration, but allowClasses can be understood (or confused) as class instances which is another option.

Right, but "class instances" and "classes" are obviously different things and the fact that we already have an "instances" option further mitigates that.

Well, yes, assuming you read about both in proximity, seeing an eslint-disable comment or config option can lose the context

I still don't find "allowClasses allows classes to be spread" as being ambiguous. Sure maybe someone will use the same name for an option that checks instances, but there's no way this is wrong.

Also classes are just sugared classes so I'm not sure if we want to treat them any more specially than functions or anything with a new signature...

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

75812fc

Closes typescript-eslint#748

StyleShit commented Feb 19, 2024

View reviewed changes

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

StyleShit added 3 commits February 19, 2024 22:41

fix docs

f18acb8

add tests

22a50e5

wip

985795a

StyleShit added 3 commits February 20, 2024 16:54

fix functions

3ff215f

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

8e760e0

…sused-spread

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

5f9fae6

…sused-spread

change type name to Iterable

990006f

JoshuaKGoldberg requested changes Feb 23, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 23, 2024

StyleShit added 3 commits February 27, 2024 17:54

wip

49692d2

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

1f76659

…sused-spread

fix lint

c45038f

StyleShit requested a review from JoshuaKGoldberg February 27, 2024 16:39

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 27, 2024

JoshuaKGoldberg requested changes Mar 5, 2024

View reviewed changes

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

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

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

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

JoshuaKGoldberg requested a review from a team March 5, 2024 17:39

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 5, 2024

auvred added the awaiting response Issues waiting for a reply from the OP or another party label Mar 7, 2024

StyleShit added 3 commits March 8, 2024 14:32

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

a12c9ff

…sused-spread

wip

9f07bc1

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

ab20e73

…sused-spread

StyleShit requested a review from JoshuaKGoldberg March 10, 2024 17:34

auvred requested changes May 4, 2024

View reviewed changes

auvred added the awaiting response Issues waiting for a reply from the OP or another party label May 4, 2024

StyleShit added 3 commits May 19, 2024 20:46

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

26434c3

…sused-spread

wip

0921b80

wip

b8d7e47

StyleShit requested a review from auvred May 19, 2024 18:39

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 19, 2024

StyleShit added 2 commits May 19, 2024 22:17

configs

3f9d545

snapshot

b133698

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label May 28, 2024

bradzacher requested changes May 28, 2024

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label May 28, 2024

StyleShit added 5 commits May 28, 2024 17:51

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

c449dba

…sused-spread

add options

bb3031f

better message

99c67a0

add map

ef12015

messages

c3fa340

StyleShit requested a review from bradzacher May 28, 2024 15:53

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 28, 2024

StyleShit added 5 commits May 28, 2024 21:40

array

bc3bcf2

promise + iterable

137c371

class declaration

db72726

class instance

92140f2

lint

8a5e2dc

bradzacher reviewed May 29, 2024

View reviewed changes

StyleShit added 3 commits May 29, 2024 16:12

wip

d2fe874

docs

0b0ac03

Merge remote-tracking branch 'typescript-eslint/main' into feat/no-mi…

8a1a309

…sused-spread

StyleShit requested a review from bradzacher May 29, 2024 13:26

omril1 reviewed May 29, 2024

View reviewed changes

	function isPromise(program: ts.Program, type: ts.Type): boolean {
	return tsutils.typeParts(type).some(t => isPromiseLike(program, t));
	}

	if (type.isIntersection()) {
	return type.types.some(t =>
	isBuiltinSymbolLikeRecurser(program, t, predicate),
	);
	}
	if (type.isUnion()) {
	return type.types.every(t =>
	isBuiltinSymbolLikeRecurser(program, t, predicate),
	);
	}

		noClassDeclarationSpreadInObject:
		'Using the spread operator on class declarations can cause unexpected behavior. Did you forget to instantiate the class?',

		### `allowClassDeclarations`

		By default, this rule disallows using the spread operator on class declarations:


		## Examples

		<!--tabs-->

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

Are you sure you want to change the base?

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

Conversation

StyleShit commented Feb 19, 2024

typescript-eslint bot commented Feb 19, 2024

netlify bot commented Feb 19, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Feb 19, 2024 • edited

☁️ Nx Cloud Report

codecov bot commented Feb 19, 2024 • edited

Codecov Report

StyleShit commented Feb 20, 2024

JoshuaKGoldberg left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradzacher left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit May 28, 2024 • edited

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 May 28, 2024 • edited

Choose a reason for hiding this comment

bradzacher May 28, 2024 • edited

Choose a reason for hiding this comment

bradzacher May 28, 2024 • edited

Choose a reason for hiding this comment

StyleShit May 28, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit May 28, 2024 • edited

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

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

Josh-Cena Jun 1, 2024 • edited

Choose a reason for hiding this comment

netlify bot commented Feb 19, 2024 •

edited

nx-cloud bot commented Feb 19, 2024 •

edited

codecov bot commented Feb 19, 2024 •

edited

bradzacher left a comment •

edited

StyleShit May 28, 2024 •

edited

StyleShit May 28, 2024 •

edited

bradzacher May 28, 2024 •

edited

bradzacher May 28, 2024 •

edited

StyleShit May 28, 2024 •

edited

StyleShit May 28, 2024 •

edited

Josh-Cena Jun 1, 2024 •

edited