typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.7k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

StyleShit wants to merge57 commits intotypescript-eslint:mainfromStyleShit:feat/no-misused-spread

Closed

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

StyleShit wants to merge57 commits intotypescript-eslint:mainfromStyleShit:feat/no-misused-spread

Conversation

Copy link

Contributor

StyleShit commentedFeb 19, 2024

Closes#748

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

75812fc

Closestypescript-eslint#748

Copy link

Contributor

typescript-eslintbot commentedFeb 19, 2024

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.

Copy link

netlifybot commentedFeb 19, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`07b69b8`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66f2f3bfacea6e00091a83c5
😎 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: 100 (🟢 up 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 yourNetlify site configuration.

Copy link

nx-cloudbot commentedFeb 19, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit07b69b8. 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 2 targets

Sent with 💌 fromNxCloud.

StyleShit commented

Feb 19, 2024

View reviewed changes

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

StyleShit added3 commits

February 19, 2024 22:41

fix docs

f18acb8

add tests

22a50e5

wip

985795a

Copy link

codecovbot commentedFeb 19, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.27%. Comparing base(896c731) to head(ab20e73).
Report is 239 commits behind head on main.

❗Current headab20e73 differs from pull request most recent head07b69b8
Pleaseupload reports for the commit07b69b8 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@##             main    #8509      +/-   ##==========================================- Coverage   88.73%   87.27%   -1.47%==========================================  Files         426      252     -174       Lines       14881    12363    -2518       Branches     4327     3899     -428     ==========================================- Hits        13205    10790    -2415+ Misses       1534     1303     -231- Partials      142      270     +128

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

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

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

... and411 files with indirect coverage changes

StyleShit added3 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

Copy link

ContributorAuthor

StyleShit commentedFeb 20, 2024

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

change type name toIterable

990006f

JoshuaKGoldberg requested changes

Feb 23, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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

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

packages/eslint-plugin/tests/rules/no-misused-spread.test.tsShow resolvedHide resolved

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

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Feb 23, 2024

StyleShit added3 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 fromJoshuaKGoldberg

February 27, 2024 16:39

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Feb 27, 2024

JoshuaKGoldberg requested changes

Mar 5, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

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

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 5, 2024

JoshuaKGoldberg requested a review froma team

March 5, 2024 17:39

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 5, 2024

auvred added the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 7, 2024

StyleShit added3 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 fromJoshuaKGoldberg

March 10, 2024 17:34

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

2d60b29

…sused-spread

StyleShit dismissedbradzacher’sstale review via2d60b29

September 10, 2024 18:41

StyleShit requested a review fromJoshuaKGoldberg

September 10, 2024 18:41

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Sep 10, 2024

StyleShit commented

Sep 10, 2024

View reviewed changes

packages/eslint-plugin/tests/fixtures/tsconfig.json

		@@ -5,7 +5,7 @@
		"module": "commonjs",
		"strict": true,
		"esModuleInterop": true,
		"lib": ["es2015", "es2017", "esnext"],
		"lib": ["es2015", "es2017", "esnext", "DOM"],

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

this fixes theHTMLElement tests

Copy link

Contributor

Choose a reason for hiding this comment

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

This seems like it needs further clarification from#9648

Merge branch 'main' into feat/no-misused-spread

a678af7

JoshuaKGoldberg reviewed

Sep 13, 2024

View reviewed changes

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

Copy link

Member

Choose a reason for hiding this comment

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

Looks like this caught an existing violation in our codebase:

/home/runner/work/typescript-eslint/typescript-eslint/packages/website/src/theme/NotFound/Content/index.tsx  16:17  error  Using the spread operator on a string can cause unexpected behavior. Prefer `String.split('')` instead  @typescript-eslint/no-misused-spread

yarn lint on the repository will need to pass.

(posting this comment here because GitHub won't let me post on an unchanged file)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

haha, seems like we have conflicting rules here 😅

Copy link

Member

Choose a reason for hiding this comment

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

lol. cc@abrahamguo @kirkwaiblinger from#9834 - looks like this just blatantly disagrees withsindresorhus/eslint-plugin-unicorn#1489 ->https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-spread.md.

Maybe we should just not go for strings at all to start? Seems like maybe it's use-case specific?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I don't have any strong opinion for either of the sides, so... 🤷‍♂️

(it's also more of a style thing rather than something that could cause errors, no?)

any objection to removing it@abrahamguo @kirkwaiblinger?

Copy link

Member

Choose a reason for hiding this comment

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

Here are my suggestions.

Never ever usesplit(""). It doesn't do what you think it does for many scripts.
[...str] is fineby principle but may still be surprising and not have the intended behavior, especially for emojis. In fact TC39 now considers strings being iterable as a mistake and all future built-in APIs that accept iterables will reject strings by default. If you really want to spread strings, either disable this rule or useIterator.from, or think about alternatives such asIntl.Segmenter.
An option to allow it is good enough. We can consider changing the default totrue, but my vote is to be strict by default.

Copy link

Member

Choose a reason for hiding this comment

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

+1 from me to everything Josh has indicated. I think this is a really interesting case of the language going one direction (... iterable strings), lint rules following that direction, then the language realizing the direction being wrong. If other lint rules are asking to use... on strings then we should file an issue on them saying they should update.

Copy link

Contributor

abrahamguoOct 8, 2024•
edited
Loading

Choose a reason for hiding this comment

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

@JoshuaKGoldberg Sure. However, if you are choosing betweenstr.split('') vs[...str], don't you agree that[...str] is certainly more correct thanstr.split(''), sincestr.split('') will break surrogate pairs?

and therefore, the premise ofunicorn/prefer-spread is correct, even if[...str] might not be thebest choice?

Copy link

Member

JoshuaKGoldbergOct 8, 2024

Choose a reason for hiding this comment

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

To be honest, I'm coming back from a couple of weeks of intense conference organizing, and don't have the headspace to re-think this deeply right now. I trust@Josh-Cena's opinion on things. 😄

Copy link

Member

Josh-CenaNov 2, 2024

Choose a reason for hiding this comment

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

Bothsplit("") and... are only correct under certain assumptions, and practically, most of the time, the former is not much more wrong than the latter. It's fine for unicorn to recommend spreading since it's nominally safer, and if users accept safety at the level of spreading they are free to configure the allow config, but banning it by default exposes such unsafety.

Copy link

Member

JoshuaKGoldbergDec 26, 2024

Choose a reason for hiding this comment

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

Filedsindresorhus/eslint-plugin-unicorn#2521.

JoshuaKGoldberg reviewed

Sep 13, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-misused-spread.mdxShow resolvedHide resolved

JoshuaKGoldberg requested changes

Sep 13, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Awesome!! I think the only blocker from me is fixing existing violations from this repo.

I took the liberty of merging the latestmain in to fix the unrelated CI failures.

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Sep 13, 2024

Copy link

ContributorAuthor

StyleShit commentedSep 13, 2024•
edited
Loading

Awesome!! I think the only blocker from me is fixing existing violations from this repo.

@JoshuaKGoldberg

Seems like my most important comment here is hidden, so I'm assuming you haven't seen it 😅
#8509 (comment)

The tests will still fail 😞

StyleShit added3 commits

September 24, 2024 19:50

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

283bbb7

…sused-spread

Merge branch 'feat/no-misused-spread' ofhttps://github.com/StyleShit…

804fab4

…/typescript-eslint into feat/no-misused-spread

fix tests

fc0e114

Copy link

ContributorAuthor

StyleShit commentedSep 24, 2024

@JoshuaKGoldberg
Continuingthis thread here, so it'll be easier for you to follow:

builtinSymbolLikes.ts and isSymbolFromDefaultLibrary.ts

Seems like both of them don't follow the references/aliases ofWeakRef. When I try to check forisBuiltinSymbolLike(program, type, 'WeakRefConstructor') on aWeakRef, I getfalse, becauseWeakRef doesn't extendWeakRefConstructor, but references to it:
https://github.com/microsoft/TypeScript/blob/48f0b3cc383bfd6337fc9b5a0afdc1642473a57d/src/lib/es2021.weakref.d.ts#L23

Anyway... Solved with a list of builtins (I don't like it, but I have no other idea at this point 😞)

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

07b69b8

…sused-spread

StyleShit requested a review fromJoshuaKGoldberg

September 24, 2024 17:15

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Sep 24, 2024

Josh-Cena reviewed

Sep 28, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-misused-spread.mdxShow resolvedHide resolved

Josh-Cena reviewed

Sep 28, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-misused-spread.mdxShow resolvedHide resolved

abrahamguo reviewed

Sep 30, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-misused-spread.mdxShow resolvedHide resolved

kirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party label

Oct 9, 2024

Copy link

Member

Josh-Cena commentedNov 2, 2024

Hi@StyleShit do you think you could find some time recently to finish it? Otherwise we could also take it on since there isn't a lot left

Copy link

ContributorAuthor

StyleShit commentedNov 3, 2024

Hi@StyleShit do you think you could find some time recently to finish it? Otherwise we could also take it on since there isn't a lot left

@Josh-Cena

I'm currently on reserve duty so I don't know whether I'll have time in the near future 😓
If you have some time to finish this PR, that would be great!
(honestly, I don't remember how much work is left here)

Thank you! 🙏

JoshuaKGoldberg marked this pull request as draft

November 4, 2024 16:33

JoshuaKGoldberg mentioned this pull request

Nov 10, 2024

feat(eslint-plugin): [require-types-exports] add new rule#8443

Closed

Copy link

Member

JoshuaKGoldberg commentedNov 10, 2024

From#8443 (comment):

Cool! I'd really like to get this one and#8509 in by the end of the year, they're great rules to have that I've wanted for a while. So let's timebox these two to, say, the end of the month. If you don't end up having time by then, no worries, one of us on the team can send in a PR with the same commit history & a co-author attribution.
Good luck + best wishes on reserve duty. ❤️