feat(eslint-plugin): add no-implicit-any-catch rule

[phiresky]

In the next TS version, adding both catch (e: unknown) {} and catch (e: any) {} will be supported.

This PR adds support for enforcing specifying a type for catch clauses. This is basically somethinng that should be caught by the noImplicitAny compiler flag, but probably won't be added to that because of backwards compatibility.

See microsoft/TypeScript#36775 which was fixed here: microsoft/TypeScript#39015

This PR also adds support for the new syntax to typescript-estree.

The following pattern is considered a warning:

try {
  // ...
} catch (e) { // warning: Implicit any in catch clause
  // ...
}

The following patterns are not warnings:

try {
  // ...
} catch (e: unknown) {
  // ...
}

try {
  // ...
} catch (e: any) {
  // ...
}

This PR will throw some "This snippet should be formatted correctly. Use the fixer to format the code" errors since this syntax is not yet supported in the formatter the lint is using.

To make this work, you need to set the TypeScript dependency to nightly (one containing this fix, wait until tomorrow :))

[typescript-eslint]

Thanks for the PR, @phiresky!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[phiresky]

Also, since prettier does not support this syntax yet, the autoformatter currently strips the types from the docs etc on auto-format, which is pretty unhandy.

[codecov]

Codecov Report

Merging #2202 into master will decrease coverage by 0.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #2202      +/-   ##
==========================================
- Coverage   93.11%   93.10%   -0.02%     
==========================================
  Files         283      284       +1     
  Lines        9061     9074      +13     
  Branches     2483     2487       +4     
==========================================
+ Hits         8437     8448      +11     
- Misses        301      302       +1     
- Partials      323      324       +1     

Flag	Coverage Δ
#unittest	93.10% <84.61%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
packages/eslint-plugin/src/configs/all.ts	100.00% <ø> (ø)
...s/eslint-plugin/src/rules/no-implicit-any-catch.ts	84.61% <84.61%> (ø)

[bradzacher]

haven't reviewed the code yet, just eyeballed it quickly.
two quick comments

thanks for your contribution!

[tom-sherman]

Can we have an option on this that enforces unknown as the argument type in this PR? Or should that be discussed in a seperate issue?

I ideally want catch (e) and catch (e: any) to be errors.

[bradzacher]

Can we have an option on this that enforces unknown as the argument type in this PR? Or should that be discussed in a seperate issue?

@tom-sherman, doesn't this PR already cover that?
https://github.com/typescript-eslint/typescript-eslint/pull/2202/files#diff-edf249e9f9f8af86a2cf0ed9bb7c9a3dR42-R48

[phiresky]

It does cover that, but I've defaulted it to false since I think you'd usually want to use the no-explicit-any rule for that since it covers all of those cases.

[tom-sherman]

@bradzacher

doesn't this PR already cover that?

You're right, I missed that part - nice!

[bradzacher]

I made a few updates to fix the CI, but for some reason github isn't picking up the changes.
I committed to your master branch which this PR is tracking - so the changes are there, but unsure why GH isn't adding them to the PR.

Feel free to take a look and merge master into your branch as well.

[WORMSS]

I committed to your master branch which this PR is tracking

You were able to push to a different users master branch?

[phiresky]

I committed to your master branch which this PR is tracking - so the changes are there,

Wait, what? I did not know that was possible and I didn't add change any access permissions. Maybe this is a gihubt bug since github itself didn't even notice the repo change happened, and now the "update from master" button is broken?

Regardless, I've merged upstream master into my branch again so now github has picked up the changes again.

[phiresky]

Never mind, I checked the Allow edits by maintainers checkbox when I created the PR. And github picking it up is probably just because it was down (again) earlier today.

[bradzacher]

You were able to push to a different users master branch?

@WORMSS As phiresky mentioned - GH has a setting on PRs which allows maintainers of the project you're PRing to to commit to the remote branch you want to merge.

This permission is what enables the "update from master" button to work on the UI, but it also lets a maintainer do things like quick edit files in the UI. A keen observer also might note that this ofc means you can checkout the branch locally, make whatever changes, and push the changes.

I usually avoid doing anything beyond updating from master because it:

• removes agency and ownership from the contributor
• is a pain as their local branch will be out of sync with their remote

I usually prefer to rely upon the contributor owning e2e and making the changes that I request, but sometimes I use it in the following cases:

• If I have a minor one-line change (i.e. a documentation change), that's not worth RCing and bothering the contributor with.
• If I want to get something in for a release, I sometimes just make changes myself because I don't know when , and comment with what I did and why.

probably just because it was down (again) earlier today

Yeah, it looks like it was just a problem with github. Seems to be working now.

[bradzacher]

okay as for the failing tests here - I think we should probably look to split out the 4.0.0 upgrade as a separate PR now.

It looks like 4.0.0 has changed how null types are represented in addition to the other changes.

Before, the TS parser just output NullKeyword, but now it outputs LiteralType > NullKeyword, which is a breaking change that we need to "undo".

I'll submit a PR this week to bump the version and fix all of the issues, and then we can rebase this PR to add the rule + support for type annotations on catch clause variables.

[bradzacher]

This now LGTM - thanks for your contribution.

I'm going to wait until the 4.0.0-rc is released before merging.

New rules get automatically added to our all config, so I don't want to force consumers of that to upgrade to a beta TS version.

[cyrilgandon]

What about thrown Promises, should it be included in this rule too?

async function foo() {
}

foo().catch((err) => console.error(err));
            ---- Disallow usage of the implicit `any` type in catch clauses

[bradzacher]

No - that catch is a function call - so it is handled by things like the no-unsafe-* ruleset.
This rule is specifically about enforcing usage of the new TS4.0 synax.