8000 feat(eslint-plugin): add rule `use-unknown-in-catch-callback-variables` by kirkwaiblinger · Pull Request #8383 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

feat(eslint-plugin): add rule use-unknown-in-catch-callback-variables #8383

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Mar 17, 2024
Merged

feat(eslint-plugin): add rule use-unknown-in-catch-callback-variables #8383

merged 28 commits into from
Mar 17, 2024

Conversation

kirkwaiblinger
Copy link
Member
@kirkwaiblinger kirkwaiblinger commented Feb 5, 2024 •
edited
Loading

PR Checklist

Overview

This change implements the rule proposal requiring variables in .catch() callbacks to have type unknown. The rule fundamentally works by flagging on the condition

  • the first argument to a .catch() call (on a Thenable) is a function
  • And the first parameter to said function is both present, and not definitely the intrinsic unknown type (according to the type checker, not syntax analysis).

Note that the backing issue mentions the return type, but any return type is legitimate in a .catch callback, so this rule does not check the return type.

Fixes for cases that are analyzable syntactically have been implemented as editor suggestions.

Limitations

  • Because this is flagging based on the types of the handler, not the syntax, it probably cannot be used to force you to provide err: unknown if you have modified the gobal type declarations for .catch().

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @kirkwaiblinger!

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 Netlify
Copy link
netlify bot commented Feb 5, 2024 •
edited
Loading

✅ Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 89bed41
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65f6e128dddc5400084c4616
😎 Deploy Preview https://deploy-preview-8383--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		 1 paths audited
Performance: 99 (🟢 up 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 Nx Cloud
Copy link
nx-cloud bot commented Feb 5, 2024 •
edited
Loading

☁️ Nx Cloud Report

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

@kirkwaiblinger kirkwaiblinger changed the title Add rule use-unknown-in-catch-callback-variables feat(eslint-plugin): add rule use-unknown-in-catch-callback-variables Feb 5, 2024
@codecov Codecov
Copy link
codecov bot commented Feb 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.10%. Comparing base (994812b) to head (89bed41).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8383      +/-   ##
==========================================
+ Coverage   88.02%   88.10%   +0.08%     
==========================================
  Files         401      402       +1     
  Lines       13934    14028      +94     
  Branches     4087     4116      +29     
==========================================
+ Hits        12265    12359      +94     
  Misses       1370     1370              
  Partials      299      299
Flag Coverage Δ
unittest 88.10% <100.00%> (+0.08%) ⬆️

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

Files Coverage Δ
...ges/eslint-plugin/src/rules/no-misused-promises.ts 97.46% <100.00%> (ø)
...ages/eslint-plugin/src/rules/no-unsafe-argument.ts 83.54% <100.00%> (-0.21%) ⬇️
...rc/rules/use-unknown-in-catch-callback-variable.ts 100.00% <100.00%> (ø)
packages/eslint-plugin/src/util/misc.ts 98.63% <100.00%> (+0.12%) ⬆️

JoshuaKGoldberg
JoshuaKGoldberg requested changes Feb 8, 2024
View reviewed changes
Copy link
Member
@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

This looks great, awesome job on all the edge cases & whatnot!

Left a few comments inline, requesting changes. But really solid all-up - no major notes! 🙌

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 8, 2024
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 9, 2024
@kirkwaiblinger kirkwaiblinger deleted the unknown-in-catch-variables-typed branch February 21, 2024 21:06
@kirkwaiblinger kirkwaiblinger restored the unknown-in-catch-variables-typed branch February 21, 2024 21:10
JoshuaKGoldberg
JoshuaKGoldberg requested changes Feb 22, 2024
View reviewed changes
Copy link
Member
@JoshuaKGoldberg JoshuaKGoldberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great to me, nicely done! 👏

Since this rule touches a lot of logical areas, I think I'd want another review from @typescript-eslint/triage-team before merging. Adding 1 approval in case someone has time in the next few weeks.

Classic he-man "I have the power!" scene where he lifts his sword

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Feb 22, 2024
@kirkwaiblinger kirkwaiblinger force-pushed the unknown-in-catch-variables-typed branch from 6dbd99a to ef47613 Compare February 23, 2024 16:33
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Mar 8, 2024
View reviewed changes
Copy link
Member
@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

...I clicked the wrong button last time, sorry. 😄

Screenshot of my review requesting changes

@bradzacher bradzacher changed the title feat(eslint-plugin): add rule use-unknown-in-catch-callback-variables feat(eslint-plugin): add rule use-unknown-in-catch-callback-variables Mar 16, 2024
@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Mar 16, 2024
bradzacher
bradzacher reviewed Mar 16, 2024
View reviewed changes
Copy link
Member
@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

quick eyeball - haven't got time for a full review at this second.

packages/eslint-plugin/src/configs/strict-type-checked.ts
@@ -76,5 +76,6 @@ export = {
'@typescript-eslint/triple-slash-reference': 'error',
'@typescript-eslint/unbound-method': 'error',
'@typescript-eslint/unified-signatures': 'error',
'@typescript-eslint/use-unknown-in-catch-callback-variable': 'error',
Copy link
Member

Choose a reason for hiding this comment

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

just double checking/confirming we want to add this to the strict config before the next major?
i don't think we've ever expressly discussed our strategy for the strict configs.
they're not technically included in our versioning policy for a major release if they're changed - but I wonder if adding a brand new rule in a minor might be a bit disruptive for users?

cc @JoshuaKGoldberg

Copy link
Member

Choose a reason for hiding this comment

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

Yup confirmed, we've done this before (#7957, #8011). Since the strict config is explicitly out of semver, users are opting into this.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 6d68020 into typescript-eslint:main Mar 17, 2024
@fregante
Copy link
Contributor

In some of my projects, I already set the type of the exception to unknown via global.d.ts types. Can this rule detect and accept that? This is basically a lib issue

@kirkwaiblinger
Copy link
Member Author

@fregante

In some of my projects, I already set the type of the exception to unknown via global.d.ts types. Can this rule detect and accept that? This is basically a lib issue

To be honest, I didn't test this, since the purpose of this rule existing (per the issue) is for the people who don't modify the lib, and I don't really remember since I worked on this a while back. I did put a note in the docs that that's not the intended use case, though. https://main--typescript-eslint.netlify.app/rules/use-unknown-in-catch-callback-variable#when-not-to-use-it

Feel free to share what you find if you test it!

@JoshuaKGoldberg
Copy link
Member

Yeah if there's a real use case for overriding global types & this rule, then I'd love to see that. In a new issue please. 🙂

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2024
@kirkwaiblinger kirkwaiblinger deleted the unknown-in-catch-variables-typed branch November 23, 2024 19:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg left review comments

@bradzacher bradzacher bradzacher left review comments

Assignees
No one assigned
Labels
1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rule proposal: always use type (error: unknown) => void with promise.catch
4 participants
@kirkwaiblinger @fregante @JoshuaKGoldberg @bradzacher
0