Skip to content

Rule proposal: always use type (error: unknown) => void with promise.catch #7526

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

Closed
6 tasks done
NotWoods opened this issue Aug 23, 2023 · 9 comments · Fixed by #8383
Closed
6 tasks done

Rule proposal: always use type (error: unknown) => void with promise.catch #7526

NotWoods opened this issue Aug 23, 2023 · 9 comments · Fixed by #8383
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@NotWoods
Copy link
Contributor

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

My proposal is suitable for this project

  • My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
  • My proposal is not a "formatting rule"; meaning it does not just enforce how code is formatted (whitespace, brace placement, etc).
  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

TypeScript has the useUnknownInCatchVariables option to use unknown with catch clauses instead of any. However, promise.catch is stuck with using an any type no matter how you configure this option.

I'd like a rule to enforce using the type unknown with the caught error handler for promises. This could either use type inference or just lint every method named .catch.

Fail Cases

promise.catch((error) => { ... });
promise.catch((error: Error) => { ... });
promise.catch((error: any) => { ... });

Pass Cases

promise.catch((error: unknown) => { ... });

Additional Info

No response

@NotWoods NotWoods added enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Aug 23, 2023
@bradzacher
Copy link
Member

Has this been proposed to TS?
It seems like a logical extension to the compiler option to improve the type of theib types.

Aside from that - honestly I think that a rule like this is better served by improving the lib types themselves - which people can do for their project without the need to patch anything!

For example this project improves the types for many, many, many parts of the lib types:
https://github.com/uhyo/better-typescript-lib

Includes a better type for the catch type:
https://github.com/uhyo/better-typescript-lib/blob/master/lib/lib.es5.d.ts#L635

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Aug 23, 2023
@Josh-Cena
Copy link
Member

Josh-Cena commented Aug 23, 2023

Yes, see microsoft/TypeScript#45602

Fixing it in TS is tricky because compiler flags can't change type defs; fixing it in user land is hard as well because accepting unknown is a strictly narrower type than accepting any, so unless you disable the default lib, the base type will still match all calls. All this makes a lint rule reasonable.

@bradzacher
Copy link
Member

fixing it in user land is hard as well because accepting unknown is a strictly narrower type than accepting any, so unless you disable the default lib, the base type will still match all calls.

But the above package shows its completely possible to do in userland because it replaces the base types using the TS provided workflows to replace them.

@NotWoods
Copy link
Contributor Author

While possible in userland I think there is benefit to having a lint rule (which is relatively easy to adopt) vs replacing the entire DOM lib

@fregante
Copy link
Contributor

fregante commented Oct 2, 2023

I fixed this in my global.d.ts in the projects where it matters. I don’t think it's worth setting up a whole linting rule when the same can be achieved faster in a few lines of code, see microsoft/TypeScript#45602 (comment)

But generally I just disable .catch() altogether:

https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/prefer-await-to-then.md

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed awaiting response Issues waiting for a reply from the OP or another party labels Oct 20, 2023
@JoshuaKGoldberg
Copy link
Member

I'm in favor - this kind of thing is really inconvenient to do in userland.

@rubiesonthesky
Copy link
Contributor

I think this would be nice addition. I don't think that modifying global types is very familiar to most users.

There is similar rule for rxjs https://github.com/cartant/eslint-plugin-rxjs/blob/main/docs/rules/no-implicit-any-catch.md

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Jan 24, 2024

I've been working on a PR for this rule. One question I have for the maintainers is: do we want to use the type checker for this rule?

Without it, I have code passing every test case I can think of where there is a function literal in the .catch callback. For example, the following all flag:

Promise.reject().catch(x => f(x));
Promise.reject().catch(function(x) { return f(x)});
Promise.reject().catch((...x: [string]) => f(x));

However, the following types of cases present some trouble without the type checker:

declare const yoloHandler: (x: number) => void;
Promise.reject().catch(yoloHandler);

function yoloHandler(x: number): void { }
Promise.reject().catch(yoloHandler);

const rejectedPromise = Promise.reject()
const cursedCode =  rejectedPromise.catch.bind(rejectedPromise);
cursedCode(x => 'could flag with appropriate mental gymnastics');

At least the first two examples above would be surgically flaggable with the type checker. Or, alternately, I could just write a rule that requires a function literal, regardless of the type passed in, i.e. flagging on the following, even though it does actually have the correct signature

function handlerThatUsesUnknown(x: unknown): void { }
Promise.reject().catch(handlerThatUsesUnknown);

Technically that's a false positive.... but always using an arrow function is pretty idiomatic anyway, so I, personally, wouldn't feel too bad about if it flagged there and even came with a suggestion to transform it to

Promise.reject().catch((err: unknown) => handlerThatUsesUnknown(err));

Note that the other tradeoff is that without the type checker, we'll be inspecting any .catch method call, regardless of whether its type is actually Promise.prototype.catch or just some random method unfortunately named .catch on any old object.

So we'll get the false positive

const hasCatch = {
    catch(f: (x: string) => string)): void {
         console.log(f('catch'));
    }
}
hasCatch.catch((x: string) => x);

My intuition here is that it's probably reasonable to take the compromise of forcing function literals and avoid the type checker. But I don't have a good sense of the balance between edge-case correctness and type-checker aversion, so, I'd love for a maintainer to chime in! Thanks :)

@bradzacher
Copy link
Member

I would love us to just do a syntactic version of it - but the reality is that we have had cases in the past where people have gotten bitten by us being overly lax with our rules (eg there were cases where we assumed some array iteration method names were always arrays and then some libs broke that).

I think that having types to do that extra layer of validation that the thing is actually a promise is good - and it also lets us ensure that users are being safe with passed-in catch handlers too - two wins seems like it's worth the trade-off.

If someone wanted to fork the rule after-the-fact into a 3rd party plugin without type-info, then they could do that. But we are big enough that we need to be stricter and more correct.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2024
@bradzacher bradzacher added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Apr 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
7 participants