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

[kirkwaiblinger]

PR Checklist

• Addresses an existing open issue: fixes Rule proposal: always use type (error: unknown) => void with promise.catch #7526
• That issue was marked as accepting prs
• Steps in Contributing were taken

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]

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]

✅ 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... Use your smartphone camera to open QR code link.
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 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

• nx test eslint-plugin
• nx test typescript-estree
• nx test type-utils
• nx test typescript-eslint
• nx test visitor-keys
• nx test utils
• nx test scope-manager
• nx run types:build
• nx run repo-tools:postinstall-script
• nx test repo-tools
• nx test rule-schema-to-typescript-types
• nx test eslint-plugin --coverage=false
• nx test parser
• nx test eslint-plugin-internal
• nx run-many --target=lint --parallel --exclude eslint-plugin
• nx test typescript-estree --coverage=false
• nx test ast-spec
• nx test type-utils --coverage=false
• nx test utils --coverage=false
• nx test scope-manager --coverage=false
• nx test visitor-keys --coverage=false
• nx test typescript-eslint --coverage=false
• nx test parser --coverage=false
• nx test repo-tools --coverage=false
• nx test rule-schema-to-typescript-types --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx test ast-spec --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx run-many --target=typecheck --parallel
• nx run repo-tools:generate-configs
• nx run integration-tests:test

---

Sent with 💌 from NxCloud.

[codecov]

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]

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]

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.

[JoshuaKGoldberg]

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

[bradzacher]

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

[bradzacher]

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

[JoshuaKGoldberg]

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

[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

[kirkwaiblinger]

@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]

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