typescript-eslint/typescript-eslintPublic

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

chore: enable no-unreachable-loop#9540

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

Merged

JoshuaKGoldberg merged 5 commits intotypescript-eslint:mainfromabrahamguo:no-unreachable-loop

Jul 20, 2024

Merged

chore: enable no-unreachable-loop#9540

JoshuaKGoldberg merged 5 commits intotypescript-eslint:mainfromabrahamguo:no-unreachable-loop

Jul 20, 2024

Conversation

Copy link

Contributor

abrahamguo commentedJul 11, 2024•
edited
Loading

PR Checklist

Addresses an existing open issue: fixes part ofRepo: enable more core ESLint rules internally #9523
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Enableno-unreachable-loop (docs)

no-unreachable-loop

0d0e907

Copy link

Contributor

typescript-eslintbot commentedJul 11, 2024

Thanks for the PR,@abrahamguo!

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 commentedJul 11, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`8bf8fd3`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/669c1774f3b5d80008c88caa
😎 Deploy Preview	https://deploy-preview-9540--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 84 (🔴 down 10 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 commentedJul 11, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit8bf8fd3. 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 💌 fromNxCloud.

Copy link

codecovbot commentedJul 11, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.45%. Comparing base(5d895b7) to head(8bf8fd3).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@##             main    #9540      +/-   ##==========================================- Coverage   88.45%   88.45%   -0.01%==========================================  Files         422      422                Lines       14694    14693       -1       Branches     4299     4299              ==========================================- Hits        12997    12996       -1  Misses       1372     1372                Partials      325      325

Flag	Coverage Δ
unittest	`88.45% <100.00%> (-0.01%)`	⬇️

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

Files	Coverage Δ
packages/eslint-plugin/src/rules/no-mixed-enums.ts	`97.10% <100.00%> (-0.09%)`	⬇️
...ckages/eslint-plugin/src/rules/no-unsafe-return.ts	`96.77% <100.00%> (+0.05%)`	⬆️

add test

47050cb

abrahamguo commented

Jul 11, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts

		return [] as any[];
		}
		`,
		'const foo: (() => void) \| undefined = () => 1;',

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

functionType.getCallSignatures() can return an empty array, but that code path was not being tested. This ensures that the code path is now covered.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

This was brought to light because when I initially refactored the rule to assume thatgetCallSignatures()[0] was always defined, the tests passed, but the rule crashed when linting this repo, therefore showing that a test was missing.

Josh-Cena reviewed

Jul 12, 2024

View reviewed changes

packages/eslint-plugin/src/rules/no-mixed-enums.ts

		:AllowedType.Number;
		}
		}
		const[{ initializer}]=(declarations[0]asts.EnumDeclaration)

Copy link

Member

Josh-CenaJul 12, 2024

Choose a reason for hiding this comment

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

Love this because this exposes a bug (not sure if it's intentional)! Checkingdeclarations[0] is almost always a bug.

Copy link

ContributorAuthor

abrahamguoJul 12, 2024

Choose a reason for hiding this comment

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

Is this truly a bug? Is it possible for a symbol to have multiple declarations, that would be different from each other?

Copy link

Member

Josh-CenaJul 12, 2024

Choose a reason for hiding this comment

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

No; if a symbol returns multiple declarations, that means these sites are merged together and should be treated as one. That's my understanding; I haven't verified it.

kirkwaiblinger mentioned this pull request

Jul 12, 2024

Repo: enable more core ESLint rules internally#9523

Open

kirkwaiblinger previously approved these changes

Jul 13, 2024

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Appreciate you being proactive about adding that test 🙂

Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…

ea884fb

…o-unreachable-loop

abrahamguo dismissedkirkwaiblinger’sstale review viaea884fb

July 16, 2024 19:32

Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…

74bae9e

…o-unreachable-loop

Josh-Cena previously approved these changes

Jul 19, 2024

View reviewed changes

kirkwaiblinger previously approved these changes

Jul 20, 2024

View reviewed changes

JoshuaKGoldberg changed the title~~chore: enable no-unreachable-loop~~fix(eslint-plugin): correct no-unsafe-return call signatures by enabling no-unreachable-loop internally

Jul 20, 2024

JoshuaKGoldberg previously approved these changes

Jul 20, 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.

Achore so good, it got promoted to afix(eslint-plugin). Nice! 💪

Copy link

Member

Josh-Cena commentedJul 20, 2024

@JoshuaKGoldberg I don't think there are behavior differences with this PR? The logic of no-unsafe-return wasn't buggy, I think (well it could be, since only caring about the first signature still seems fishy).

Copy link

Member

JoshuaKGoldberg commentedJul 20, 2024

Oh, gotcha - I misinterpreted. Never mind then. 🙂

JoshuaKGoldberg changed the title~~fix(eslint-plugin): correct no-unsafe-return call signatures by enabling no-unreachable-loop internally~~chore: enable no-unreachable-loop

Jul 20, 2024