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

phiresky · 2020-06-11T00:05:37Z

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 · 2020-06-11T00:05:45Z

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 · 2020-06-11T00:08:32Z

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 · 2020-06-11T00:14:59Z

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!

packages/eslint-plugin/src/rules/no-implicit-any-catch.ts

packages/typescript-estree/src/convert.ts

packages/eslint-plugin/docs/rules/no-implicit-any-catch.md

See microsoft/TypeScript#36775

Co-authored-by: Glen <glen.84@gmail.com>

tom-sherman · 2020-07-02T12:56:37Z

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 · 2020-07-02T18:27:56Z

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 · 2020-07-02T18:29:51Z

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 · 2020-07-03T11:18:38Z

@bradzacher

doesn't this PR already cover that?

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

packages/eslint-plugin/docs/rules/no-implicit-any-catch.md

…s to doc to work around prettier force formatting

bradzacher · 2020-07-13T06:39:56Z

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 · 2020-07-13T08:14:51Z

I committed to your master branch which this PR is tracking

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

phiresky · 2020-07-13T12:53:37Z

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 · 2020-07-13T12:58:08Z

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 · 2020-07-13T16:43:17Z

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 · 2020-07-13T16:58:57Z

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.

# Conflicts: # packages/typescript-estree/src/convert.ts # packages/typescript-estree/tests/ast-alignment/fixtures-to-test.ts # packages/typescript-estree/tests/lib/__snapshots__/semantic-diagnostics-enabled.test.ts.snap # packages/typescript-estree/tests/lib/__snapshots__/typescript.ts.snap

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 · 2020-08-21T08:24:39Z

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 · 2020-08-21T08:29:58Z

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.

phiresky mentioned this pull request Jun 11, 2020

Allow unknown type annotation on catch clause variable microsoft/TypeScript#36775

Closed

5 tasks

bradzacher requested changes Jun 11, 2020

View reviewed changes

packages/eslint-plugin/src/rules/no-implicit-any-catch.ts Outdated Show resolved Hide resolved

packages/typescript-estree/src/convert.ts Outdated Show resolved Hide resolved

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Jun 11, 2020

glen-84 reviewed Jun 16, 2020

View reviewed changes

packages/eslint-plugin/docs/rules/no-implicit-any-catch.md Outdated Show resolved Hide resolved

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 19, 2020

phiresky and others added 6 commits June 22, 2020 10:28

feat(typescript-estree): allow catch clause to have a type

d61ed9d

See microsoft/TypeScript#36775

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

81fbc88

fix(eslint-plugin): [no-implicit-any-catch] disable recommended for now

a19c245

fix(typescript-estree): add tests for catch clause type syntax

076e8ef

Update packages/eslint-plugin/docs/rules/no-implicit-any-catch.md

4fbde9b

Co-authored-by: Glen <glen.84@gmail.com>

fix(typescript-estree): ignore catch clause type fixture from babel

4ba7d61

phiresky force-pushed the master branch from 1c4389d to 4ba7d61 Compare June 22, 2020 08:33

fix(typescript-estree): add updated snapshots for catch clause type

cb5be10

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 22, 2020

phiresky mentioned this pull request Jun 30, 2020

TypeScript 4.0 Syntax Support #2260

Closed

WORMSS suggested changes Jul 3, 2020

View reviewed changes

packages/eslint-plugin/docs/rules/no-implicit-any-catch.md Outdated Show resolved Hide resolved

packages/eslint-plugin/docs/rules/no-implicit-any-catch.md Outdated Show resolved Hide resolved

phiresky added 3 commits July 3, 2020 18:23

fix(typescript-estree): no-implicit-any-cache: add commented out type…

75533af

…s to doc to work around prettier force formatting

chore: upgrade typescript for catch clause type

3aa4c07

Merge remote-tracking branch 'upstream/master'

ce0a8a3

tsujp mentioned this pull request Jul 8, 2020

not catching no-implicit-any sheerun/prettier-standard#105

Open

felixfbecker mentioned this pull request Jul 10, 2020

Rule to enforce parameter type declaration of unknown for catchError() cartant/eslint-plugin-rxjs#49

Closed

update to 4.0.0-beta and fix errors

d89ccd3

Merge remote-tracking branch 'upstream/master'

27af1e5

bradzacher added 2 commits July 13, 2020 09:25

fix yarn lockfile not freezing

22282b1

fix type error

61dcd4e

update snaps

acc2bde

bradzacher mentioned this pull request Jul 19, 2020

feat(typescript-estree): support type annotations on catch clauses #2306

Merged

bradzacher added 3 commits July 18, 2020 18:55

fix recommended config

eb14de8

update readme 

3f724c4

bradzacher approved these changes Jul 20, 2020

View reviewed changes

bradzacher added the DO NOT MERGE PRs which should not be merged yet label Jul 20, 2020

bradzacher added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Aug 7, 2020

bradzacher removed the DO NOT MERGE PRs which should not be merged yet label Aug 21, 2020

bradzacher changed the title ~~feat(eslint-plugin): add no-implicit-any-catch rule~~ feat(eslint-plugin): add no-implicit-any-catch rule Aug 21, 2020

bradzacher merged commit fde89d4 into typescript-eslint:master Aug 21, 2020

joachimvh mentioned this pull request Sep 16, 2020

Can't update @typescript-eslint CommunitySolidServer/CommunitySolidServer#4

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

phiresky commented Jun 11, 2020 •

edited

Loading

typescript-eslint bot commented Jun 11, 2020

phiresky commented Jun 11, 2020

codecov bot commented Jun 11, 2020 •

edited

Loading

bradzacher left a comment

tom-sherman commented Jul 2, 2020 •

edited

Loading

bradzacher commented Jul 2, 2020 •

edited

Loading

phiresky commented Jul 2, 2020

tom-sherman commented Jul 3, 2020

bradzacher commented Jul 13, 2020

WORMSS commented Jul 13, 2020

phiresky commented Jul 13, 2020 •

edited

Loading

phiresky commented Jul 13, 2020

bradzacher commented Jul 13, 2020

bradzacher commented Jul 13, 2020

bradzacher left a comment

cyrilgandon commented Aug 21, 2020

bradzacher commented Aug 21, 2020

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

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

Conversation

phiresky commented Jun 11, 2020 • edited Loading

typescript-eslint bot commented Jun 11, 2020

phiresky commented Jun 11, 2020

codecov bot commented Jun 11, 2020 • edited Loading

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

tom-sherman commented Jul 2, 2020 • edited Loading

bradzacher commented Jul 2, 2020 • edited Loading

phiresky commented Jul 2, 2020

tom-sherman commented Jul 3, 2020

bradzacher commented Jul 13, 2020

WORMSS commented Jul 13, 2020

phiresky commented Jul 13, 2020 • edited Loading

phiresky commented Jul 13, 2020

bradzacher commented Jul 13, 2020

bradzacher commented Jul 13, 2020

bradzacher left a comment

Choose a reason for hiding this comment

cyrilgandon commented Aug 21, 2020

bradzacher commented Aug 21, 2020

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

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

phiresky commented Jun 11, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading

tom-sherman commented Jul 2, 2020 •

edited

Loading

bradzacher commented Jul 2, 2020 •

edited

Loading

phiresky commented Jul 13, 2020 •

edited

Loading