microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
99.06k stars 12.29k forks source link

Capture type of Promises that never reject #45869

Closed thw0rted closed 2 years ago

thw0rted commented 2 years ago

Suggestion

🔍 Search Terms

promise rejection

✅ Viability Checklist

My suggestion meets these guidelines:

⭐ Suggestion

Today, I started using the no-misused-promises rule in typescirpt-eslint. I think everyone agrees it's a good idea to always consider potential Promise rejections. It got me thinking, though -- I have a number of places in my code where I return new Promise(resolve => someHandler.resolveFoo = resolve);, creating a Promise that cannot possibly reject.

Turning on the linked rule generated dozens of linter flags in my code, and I started digging through to check if some un-handled async function calls could possibly reject, updating JSDoc comments to say "rejects when..." or "never rejects". I realized that it would be really handy if there were a type-level mechanism to capture this distinction.

My first instinct was a simple alias: type PromiseNeverRejects<T> = Omit<Promise<T>, "catch">;. (Not perfect, but it would be a start.) I felt very clever for about 5 seconds until I tried to write export async function getFoo(): PromiseNeverRejects<Foo> { ... }, which fails with an error that async functions must return "the global Promise".

So: my suggestion is some type-level way of asserting that a given Promise must only resolve, never reject, and to do so in a way that can be returned from an async function. This should have no effect on runtime behavior, but for example would allow neverRejectsPromise.catch(...) to fail type-checking (since it's unnecessary).

Syntax could be a named type like my alias, a generic argument on the existing Promise, or it could be limited to async-function returns and defined using a JSDoc @-tag. Whatever it is, neverRejectsPromise instanceof Promise should be true (no runtime effect).

📃 Motivating Example

By itself, this metadata would be of limited value, but it would make features like this one in typescript-eslint possible, and could really empower this proposed throws-clause feature in the context of async functions.

💻 Use Cases

async function pokeAfter(ms: number): Promise<void> {
  return new Promise(resolve => {
    setTimeout(() => { foo.poke(); resolve(); }, number);
  };
}

// just want to make sure the foo gets poked at some point, don't need to wait for it
pokeAfter(10);

// wait to peek after poke
pokeAfter(10).then(() => foo.peek());

Static analysis tools can't currently tell whether it's safe to call the async function without a catch (or two-argument then), because the function's return type doesn't know whether the Promise can reject.

IllusionMH commented 2 years ago

I have a number of places in my code where I return new Promise(resolve => someHandler.resolveFoo = resolve);, creating a Promise that cannot possibly reject.

But it can and will - just run in console as is

(function() {
    return new Promise(resolve => someHandler.resolveFoo = resolve);
})();

TS would not provide runtime guarantees that there won't be ReferenceError or other runtime errors somewhere in code especially if data doesn't correspond to types.

thw0rted commented 2 years ago

Your "just run in console" counterexample doesn't makes sense, because this is a type-level (not runtime-level) feature. If someHandler were possibly undefined, the typechecker would flag the code already. Of course TS can only make guarantees based on what you tell it -- if I write declare function foo(): number but foo returns a string at runtime, there's not much anybody can do.

The point of the suggestion is to give TS the vocabulary to let me better describe the shape of the returned Promise.

MartinJohns commented 2 years ago

How could TypeScript statically identify whether the promise will reject or not? Except in a very few cases this would either impossible, computationally too expensive or simply both. Given your example TypeScript would somehow need to be able to make sure that foo.poke() can't throw an error.

thw0rted commented 2 years ago

Maybe this was unclear, the suggestion wasn't that TS tell me whether the promise could reject, it was that I can assert that it can't. It's similar to the suggestion about a throws decoration that I linked in the original post, except instead of function foo(): void throws string, it would be async function(): Promise<void> rejects never. (I don't actually suggest a new keyword, it's just analogous to that proposal.)

ETA: in the Use Case above, it might be async function pokeAfter(ms: number): PromiseNeverRejects<void>, though obviously you'd want a pithier type name.

thw0rted commented 2 years ago

This is probably related to https://github.com/microsoft/TypeScript/issues/7588 and several issues linked from there. I guess the question is, if there are so many edge cases where Promises can reject with a value you didn't expect that it's not worth implementing Promise<ResolveType, RejectType> (which I think was the resolution of that issue), then could the special case of Promise<void, never> be worth doing?

IllusionMH commented 2 years ago

I think #39680 is latest related open issue and probably never throwing promise can be can be Promise<T, never> but from for catch it still should be catch(reason: TReason | Error): Promise<...>, doubt that catch should be removed completely from the interface.

My point was that you of course can say that promise will never reject or have specific rejection type, but there is always runtime that can throw in any moment. Even if your promise constructor doesn't throw now it can do that in a month when someHandler.resolveFoo will become setter that throws in some conditions.

declare const p: PromiseNeverRejects;

p.catch // not needed

p.then(log) // should be changed to regular Promise because there is no indication if log can throw

So never rejecting promise should be confirmed/asserted on each of these steps.

thw0rted commented 2 years ago

If I change the example to return Promise.resolve(true); can we get past trying prove that all Promises might reject due to a change in dependent code? The type system isn't perfect and sometimes we have to describe things in ways that might break if the code changes, we just have to do the best we can.

I agree that by default, calls p.then(cb) should infer that cb returns a Promise, not a PromiseNeverRejects, but if cb is explicitly declared as (): PromiseNeverRejects, then static analysis should still be able to tell that p.then(cb).catch() is not needed, or even flag it as an error.

(I realized another benefit: this would help avoid a conflict between linter and test coverage. You don't want the linter telling you "you must add a catch" and the coverage report saying "why didn't you test this catch callback?".)

andrewbranch commented 2 years ago

This feels very analogous to the request of “Just let us annotate catch clause variables with err: Error”—both are requests to allow implicit assertions about unanalyzable behavior. I do think what you’re asking for is totally unanalyzable, beyond Promise.resolve calls, which are not terribly useful in isolation. I think the primary reason why both this suggestion and err: Error on catch clause variables are unappealing is that it’s really not obvious that there is an assertion. Experienced TS developers are attuned to look out for unsoundness when they see as type assertions; this proposal makes the unsoundness very easy to overlook. And for that footgun, what’s the upside? Silencing an overfussy lint rule? It feels like an eslint-ignore comment saying Don't worry, never rejects is already a perfect solution.

andrewbranch commented 2 years ago

By itself, this metadata would be of limited value, but it would make features like this one in typescript-eslint possible, and could really empower this proposed throws-clause feature in the context of async functions.

This means the feature would be of limited value for the foreseeable future, so it’s not a good candidate for TypeScript today. In a future where we are able to track accurately the types of errors thrown and caught, it could be worth reconsidering, but I think that future is extremely unlikely given the way JavaScript works.

thw0rted commented 2 years ago

I think I went into this knowing it was an assertion about "unanalyzable behavior", but you make a really good point about making it obvious (especially to inexperienced developers) that that's what's happening. I can totally see somebody pasting code from SO to "make the linter shut up" without understanding why it "works". That's a pretty strong argument against. I understand why you declined the suggestion.

OTOH, since I filed this suggestion, I've spent a fair bit of time addressing those linter flags, and did in fact catch at least a half-dozen cases where a not-unlikely Promise rejection was totally unhandled -- I'm glad I turned the rules on! I also had to ignore (with void lookup("foo")) the exact same function in a hundred places. That function probably would have made a better example than the ones I actually used. It looks very approximately like

async function lookup(key: string): Promise<string> {
  try {
    return maybeGetValueEventually(key);
  } catch {
    return "fallback";
  }
}

I can't think of a reason why that Promise would reject. A very clever static analyzer might even validate that it can't reject. It would have saved me about an hour yesterday, if it had been possible to make that analysis / assertion once, rather than checking every callsite to see if it needs a void or a .catch(). I get that the root cause was my "overfussy" linter rule, but as I pointed out, that rule caught a bunch of unhandled error conditions that the typechecker alone could not -- and I still wish I could get those benefits without all the legwork of analyzing every async call by hand.

jamesarosen commented 1 year ago

And for that footgun, what’s the upside? Silencing an overfussy lint rule? It feels like an eslint-ignore comment saying Don't worry, never rejects is already a perfect solution.

Centralization. If a very core component of my code has a Promise<number, never>, then the knowledge of the never-ness is centralized. If that component is used 100 times, I need 100 eslint-ignore comments for it.

andrewbranch commented 1 year ago

Centralized solution: disable the eslint rule 😄

aovchinn commented 6 months ago

one case that led me here is Promise.allSettled with a promise that always resolves, would be nice to somehow mark this async function as the one for which error is already handled

const myF = async () => {
    try {
        // something async
    } catch {
        console.log("oops, handling it");
        return "good fallback";
    }
};