Skip to content
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.

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

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

Open
4 tasks done
RedGuy12 opened this issue May 7, 2024 · 6 comments
Open
4 tasks done
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@RedGuy12
Copy link

RedGuy12 commented May 7, 2024

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

My proposal is suitable for this project

  • 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

@RedGuy12 RedGuy12 added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 7, 2024
@kirkwaiblinger
Copy link
Member

Note: a related idea came up in the rule proposal, see #7526 (comment)

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.

@bradzacher
Copy link
Member

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.

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Jun 3, 2024
@Josh-Cena Josh-Cena added wontfix This will not be worked on and removed triage Waiting for maintainers to take a look labels Jun 4, 2024
@Josh-Cena
Copy link
Member

Josh-Cena commented Jun 4, 2024

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.

@kirkwaiblinger
Copy link
Member

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

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for maintainers to take a look and removed wontfix This will not be worked on labels Jun 4, 2024
@JoshuaKGoldberg
Copy link
Member

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

@Josh-Cena
Copy link
Member

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 Josh-Cena added evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important and removed triage Waiting for maintainers to take a look labels Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants