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

Why doesn't awaiting a Promise<never> change reachability? #34955

Open
nikeee opened this issue Nov 6, 2019 · 14 comments
Open

Why doesn't awaiting a Promise<never> change reachability? #34955

nikeee opened this issue Nov 6, 2019 · 14 comments
Labels
Bug A bug in TypeScript Domain: Control Flow The issue relates to control flow analysis
Milestone

Comments

@nikeee
Copy link
Contributor

nikeee commented Nov 6, 2019

TypeScript Version: 3.7.2

Search Terms:

  • "Promise"
  • await
  • reachability analysis
  • control flow analysis
  • definite assignment analysis

Code

(async () => {
    let b: string;
    let a1 = await Promise.reject(); // returns Promise<never>
    b = ""; // Not unreachable?
    b.toUpperCase(); // Not unreachable?
})();

Expected behavior:
As a1 is inferred to be never (e.g. behaves like in the non-promised version), I expected the rest of the code to be marked as unreachable aswell:

function returnNever(): never { throw new Error(); }

(async () => {
    let b: string;
    let a0 = returnNever(); // a0 is never
    b = ""; // Unreachable
    b.toUpperCase(); // Unreachable
})();

Actual behavior:
The code after the never-returning promise is marked as reachable.

Related Question on StackOverflow: Has more code: https://stackoverflow.com/questions/58732814
Related Issue: #10973 (although marked as "Working as intended", it was changed later. 3.7.2 behaves like the issue opener expected).

If this is not a bug, what is the background for this behavior?

@nikeee nikeee changed the title Should awaiting a Promise<never> influence reachability? Why doesn't awaiting a Promise<never> influence reachability? Nov 7, 2019
@nikeee nikeee changed the title Why doesn't awaiting a Promise<never> influence reachability? Why doesn't awaiting a Promise<never> change reachability? Nov 7, 2019
@andrewbranch andrewbranch added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Dec 4, 2019
@andrewbranch
Copy link
Member

I think this makes sense, though two things are unclear to me:

  • how complex/expensive this would be to implement
  • if this comes up in real-world scenarios

Have you found real cases where you have a Promise<never>? Would love to see some motivating examples.

@nikeee
Copy link
Contributor Author

nikeee commented Dec 4, 2019

Consider a logErrorAsync function that returns Promise<never>. It awaits something and then throws an error on every path (or goes into an infinite loop etc.). If the function does not return a Promise<never> but a never instead, it behaves as expected.

As of TS 3.7.3:

declare function fetchB(): Promise<string>;

async function logErrorAsync(err: any): Promise<never> {
    await fetch("/foo/...");
    console.log("Error has been submitted to analytics tool");
    throw new Error(err);
}

function logError(err: any): never {
    console.error(err);
    throw new Error(err);
}

(async () => {
    let b: string;
    try {
        b = await fetchB();
    } catch (err) {
        await logErrorAsync(err); // awaiting Promise<never>
    }
    b.toUpperCase(); // "Error: "b" is used before assignment;" would require workarounds
})();

//...
(async () => {
    let b: string;
    try {
        b = await fetchB();
    } catch (err) {
        logError(err); // returns never
    }
    b.toUpperCase(); // No error, as expected
})();

@andrewbranch
Copy link
Member

andrewbranch commented Dec 6, 2019

Yeah, since we actually error on unreachable code, this does seem worthy of the bug label.

@andrewbranch andrewbranch added Bug A bug in TypeScript Domain: Control Flow The issue relates to control flow analysis and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Dec 6, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Sep 11, 2020
@nicolas377
Copy link
Contributor

We're closing in on 2 years of this issue, is this something that the TS team wants to do themselves, or could I take this on as a first PR here?

@andrewbranch
Copy link
Member

Backlog = no immediate plans for us to work on it, but

Lack of "Help Wanted" = not really sure if there are major roadblocks to a solution. I think this would be a difficult first PR, but you are welcome to try.

@RuiNtD
Copy link

RuiNtD commented Dec 21, 2022

Have you found real cases where you have a Promise<never>? Would love to see some motivating examples.

Also this:

https://github.com/FayneAldan/ArtemisWaypointMigrator/blob/a3f38352518ea5de1d9534dabb951bd8e7dda52c/migrate.ts#L15-L23

async function enterToExit(): Promise<never> {
  console.log("Press [Enter] to exit");
  for await (const event of keypress()) {
    if (event.key == "return") {
      break;
    }
  }
  Deno.exit();
}

@bradenhs
Copy link

Here's a real use case for this I ran into today: you want to flush your logs then exit the process e.g.

async function exit(exitCode: number): Promise<never> {
  await Logger.flush();
  process.exit(exitCode);
}

@zefir-git
Copy link

zefir-git commented Mar 4, 2023

I believe the code is unreachable because Promise.reject() throws an error, not because the type is never. How would TypeScript know that the code is unreachable when it does not know that this method can throw an error?

In Java there is the throws keyword which defines what errors a method can throw, e.g

public void errors() throws Exception {
    throw new Exception("Exception!");
}

public void test() {
    this.errors(); // will error because `errors()` can throw `Exception` and you don't handle the error. Your options are to add `throws Exception` to the signature of `test` or handle the exception with try catch
    // unreachable code because possible error not handled
}

I believe this (much-needed IMO) feature has been requested in #13219 and until it's implemented we're stuck with unknown or any in catches and not having any checks for unhandled errors.

@rintaun
Copy link

rintaun commented Jun 16, 2023

@williamd5 this works properly without promises:

function errors(): never {
  throw new Error("Error!")
}

function test() {
  errors()
  console.log('this is unreachable, and typescript knows it')
}

the throws syntax is a separate, unrelated issue about type inference in a catch clause, whereas this issue is a bug regarding reachability calculation by returning never for known, uncaught errors.


I have also run into this problem in the wild, specifically when using zod and a z.never() schema in an async workflow. the specific use case was implementing an ORM that handles PostgreSQL views in addition to standard tables.

the general approach i was taking looked like this...

class Table<TInsert extends z.ZodSchema> {
  // ...
  async insert(data: TInsert): Promise<TInsert extends z.ZodNever : never : TInsert> {
    db.insert(insertSchema.parse(data))
  }
}

@jespertheend
Copy link
Contributor

Have you found real cases where you have a Promise<never>? Would love to see some motivating examples.

I have a TypedMessenger which allows you to send messages to things like Workers or WebSockets.
It works a bit like:

const messenger = new TypedMessenger()
const response = await messenger.send.foo()

and response will have the type of whatever the WebSocket is expected to respond with.

However, if it is known in advance that the WebSocket will never respond when foo() is called, then its type will be Promise<never>.
It would be useful if you could see that awaiting this call would result in unreachable code.

@darrylnoakes
Copy link

Does that leave you with never-to-be-fulfilled promises lying around? Or is it internally constructed so that there are no references to them and they can be garbage collected?

@jespertheend
Copy link
Contributor

I'm implementing the garbage collection using WeakRefs as we speak :)

@jespertheend
Copy link
Contributor

Actually, I just realised what I want to do isn't possible :/ So errors for unreachable code would actually be very convenient here.

@toonvanvr
Copy link

toonvanvr commented Jun 28, 2024

I'm also bumping upon this issue, but was able to work around it using return, which is not a clean solution. My language server was giving a 'could be undefined' type error in the calling scope after wrapping repetitive error reporting in an async call.

Original code

async function parseFile(): Promise<File> {
  try {
    return await something()
  } catch (cause) {
    const errMsg = 'went wrong'
    await dbLog(errMsg, { isError: true })
    console.error(errMsg, { cause })
    throw new Error(errMsg, { cause })
    // unreachable code
  }
}

Async call with bug

async function parseFile(): Promise<File | undefined> {
  try {
    return await something()
  } catch (cause) {
    await loggedCriticalError('went wrong', { cause })
    // incorrectly assumed reachable code
  }
}

Async call with bug workaround

async function parseFile(): Promise<File> {
  try {
    return await something()
  } catch (cause) {
    // returning an awaited Promise<never> will not add void to the return type of parseFile()
    return await loggedCriticalError('went wrong', { cause })
    // unreachable code
  }
}

The need for logging stuff immediately exists because enterprise code is touched by many hands and the error handling in the parent scope might accidentally toss the error message, while fixing that behaviour could introduce its own set of bugs in legacy code that depends on that behaviour. We all know there's no budget for fixing everything. 💸

The inferred return type changed from File to File | undefined and caused a type error right below it, but in other code it could have propagated into the scope of a colleague and they'd be adding a bunch of useless if/elses, adding complexity where it's not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Control Flow The issue relates to control flow analysis
Projects
None yet
Development

No branches or pull requests