typescript-eslint icon indicating copy to clipboard operation
typescript-eslint copied to clipboard

Enhancement: [use-unknown-in-catch-callback-variable] Option to only check inline functions

Open cobaltt7 opened this issue 9 months ago • 6 comments

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • [X] I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/use-unknown-in-catch-callback-variable/

Description

I propose an option to only check on inline functions, i.e. .catch((foo) => bar) and not .catch(baz). If baz accepts any, it's either from an external library or will be flagged by --noImplicitAny/no-explicit-any.

Use case: I have a few instances of .catch(console.error) in my codebase to log errors without throwing. console.error is typed as (...data: any[]): void, and as such this rule triggers an error.

Fail

Promise.resolve().catch((error) => error) // Preserve old behavior

Pass

Promise.resolve().catch((error: unknown) => error) // Preserve old behavior
Promise.resolve().catch(console.log) // Would previously error
Promise.resolve().catch(foo) // Would previously error

function foo(bar: any) { }

Additional Info

No response

cobaltt7 avatar May 07 '24 00:05 cobaltt7

Note: a related idea came up in the rule proposal, see https://github.com/typescript-eslint/typescript-eslint/issues/7526#issuecomment-1907664555

I personally don't have a strong opinion. Several points in favor and several points against spring to mind. We'll see what others say.

kirkwaiblinger avatar May 07 '24 02:05 kirkwaiblinger

yeah I'm personally -1 on this given that the usecase of specifically passing an "external" function that accepts any is rare. This is also trivially worked-around from the user side (eg switching to an explicit arrow .catch((arg: unknown) => console.error(arg))).

Now-a-days I think most 3rd-party things are going to accept unknown instead of any, and within your own codebase you can always switch the fn to unknown.

it's either from an external library or will be flagged by --noImplicitAny/no-explicit-any

Assuming that you have that compiler option turned on or that you have that rule turned on. It's not uncommon to not use the latter -- a lot of people don't love how restrictive it is.

bradzacher avatar May 07 '24 06:05 bradzacher

FWIW the only pain I had with adopting this rule is when I'm passing error-tracking utilities.

redisClient.connect().catch(winston.error);
redisClient.connect().catch(console.error);
redisClient.connect().catch(Sentry.captureException);

NONE OF THESE work.

So, I'm a big fan. If you are not implementing this function there's not much you can do anyway.

Josh-Cena avatar Jun 04 '24 17:06 Josh-Cena

Having thought more thoroughly about this, I am +1 to the request as well. Passed in handlers will never have implicitly any parameters, which is the real problem with supplying a function expression as a handler; the parameter doesn't require an annotation because it's deduced as any

kirkwaiblinger avatar Jun 04 '24 18:06 kirkwaiblinger

I don't feel strongly enough about this to want to push back against multiple 👍s. I'll defer to the rest of the team's majority. 🙂

JoshuaKGoldberg avatar Jun 04 '24 18:06 JoshuaKGoldberg

Kirk and I both hold the thought that this is fine:

function handler(x: any) {
  // ...
}

Promise.reject().catch(handler);

It's still any but it's explicit. If one turns off no-explicit-any, I don't see why we should report this one but not any other blatantly unsafe things going on.

Josh-Cena avatar Jun 04 '24 18:06 Josh-Cena

3 months later and this request has received 2 community reactions. With the team split and low community support, I think we can take this as signal to reject this request.

There are documented workarounds:

  • wrapping the calls with explicit .catch((arg: unknown) => cb(arg))
  • patching the types for the 3rd party lib locally to use unknown

And this is not a super common usecase given the low volume of community engagement. As per our workflow guidelines - we are taking this as signal that this check is not important to the broader community and are thus rejecting the proposal. Thanks for filing!

bradzacher avatar Aug 25 '24 13:08 bradzacher