Skip to content

Detection of unreachable code after call to function returning never is broken for functions exported via intermediate variable #60368

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
ronjouch opened this issue Oct 29, 2024 · 10 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@ronjouch
Copy link

ronjouch commented Oct 29, 2024

🔎 Summary

Detection of unreachable after call to function returning never generally works.

However, it appears broken when the never-returning function is exported via an intermediate variable.

Here’s my best minimal demo:

Image

(sorry, cannot do a Playground link, as my problem is inherently multi-file, and Playground only supports one file)

🕗 Version & Regression Information

  • I noticed the bug on latest TS 5.6.3

⏯ Playground Link

Cannot share a Playground link, as Playground is single-file only, and my issue is about exports / multi-file

💻 Code

app.mjs

import {libConst} from './lib-export-const.mjs'
const {fnNeverWithConstExport} = libConst;
import {fnNeverWithEsmExport} from './lib-export-proper.mjs'

/** @returns {never} */
function fnSameModule() {
  process.exit(0);
}

function withSameModule() {
  fnSameModule();
  console.log('GOOD: properly seen as unreachable');
}

function withExport() {
  fnNeverWithEsmExport();
  console.log('GOOD: properly seen as unreachable');
}

function withConst() {
  fnNeverWithConstExport();
  console.log('BAD: incorrectly missed as unreachable');
}

// console.logs nothing (runtime behavior all good)
withSameModule();
withExport();
withConst();

lib-export-proper.mjs

/** @returns {never} */
export function fnNeverWithEsmExport() {
  process.exit(0);
}

lib-export-const.mjs

/** @returns {never} */
function fnNeverWithConstExport() {
  process.exit(0);
}

export const libConst = {
  fnNeverWithConstExport,
};

🙁 Actual behavior

TS fails to understand the never-ness of the never-returning function exported via an intermediate variable.

🙂 Expected behavior

I expect TS to understand that code following a function returning never will not run.

This is useful e.g. for early exits useful to narrow a string | undefined into a string.

@ronjouch ronjouch changed the title Detection of unreachable after call to function returning never is broken for functions exported via intermediate variable Detection of unreachable code after call to function returning never is broken for functions exported via intermediate variable Oct 29, 2024
@MartinJohns
Copy link
Contributor

This is working as intended, see #32695:

A function call is analyzed as an assertion call or never-returning call when

  • the call occurs as a top-level expression statement, and
  • the call specifies a single identifier or a dotted sequence of identifiers for the function name, and
  • each identifier in the function name references an entity with an explicit type, and
  • the function name resolves to a function type with an asserts return type or an explicit never return type annotation.

@ronjouch
Copy link
Author

ronjouch commented Oct 29, 2024

@MartinJohns nice find 👏, thanks! Indeed, this condition isn't respected in my case:

  • the call occurs as a top-level expression statement

In this case, from the perspective of an end-user, it feels like “room for improvement”: it’s surprising that this feature sometimes works, but sometimes doesn’t.

→ With that, I’ll leave the issue open, up to TS triagers to close as Won’t Fix, or label/prioritize.

Oh, one last thing to Martin or a passerby or someone maybe fixing this: why the hell am I doing exports via an intermediate const? Because I need to mock them for unit tests, but ESM exports are immutable and thus cannot be stubbed/mocked. Said differently, I’m applying recipe 1) Store and export functions as an object of this article on mocking in an ESM world. I wish I could use top-level export function, but I cannot, and option 1) Object of this article is the best thing I found: 2) Class doesn't fit the context, 3) DI would be quite the refactor and isn’t the project style, 4) 3rd-party tooling is immature.

@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 29, 2024

Your issue is this:

each identifier in the function name references an entity with an explicit type, and

Both fnNeverWithConstExport and libConst don't have explicit types.

nice find 👏

Really no big deal, this issue has come up so often already. Yours is just one of many.

@ronjouch
Copy link
Author

ronjouch commented Oct 29, 2024

Your issue is this:

each identifier in the function name references an entity with an explicit type, and

Both fnNeverWithConstExport and libConst don't have explicit types.

@MartinJohns Iiiiiii... don’t understand.

  • I declared fnNeverWithConstExport as explicitly returning never, and in function withConst, fnNeverWithConstExport, my editor type-resolves it as const fnNeverWithConstExport: () => never
  • I declared libConst as export const libConst = { fnNeverWithConstExport }; , okay the container object is implicitly typed...
    • ... but if I follow your suggestion to explicitly type it: export const /** @type {{ fnNeverWithConstExport: () => never }} */ libConst = { fnNeverWithConstExport }; , then the problem persists

What am I missing?

@nmain
Copy link

nmain commented Oct 30, 2024

const {fnNeverWithConstExport} = libConst;

@ronjouch The identifier that's actually called is the one that needs to be typed before the inference pass runs, eg: https://www.typescriptlang.org/play/?#code/CYUwxgNghgTiAEA3W8IEsBGBhA9gOwGcAXALngG8AoeG+AMzwDkREQYB1NIgC10KICiADwAOOGKXgAKAJTwAvAD54eFm0oBfSpTD5i9Jmo5deewaPGTZC5atYwFqTH2IA6Bs3uceL82IkA3NoeRt6m-ML+RLJBuoQ4ECCuEDgA5lIA5ADuOFkZMkFAA

@ronjouch
Copy link
Author

ronjouch commented Oct 30, 2024

@ronjouch The identifier that's actually called is the one that needs to be typed before the inference pass runs, eg: ts playground

@nmain sorry, still puzzled:

  1. Your TS playground example indeed shows Unreachable code detected for the console.log, but so does this one that accesses libConst.fnNeverWithConstExport without the explicitly-typed fn.
  2. Coming back to my multi-file example, adding explicit typing in the app.mjs file (where I'd expect unreachable code detection to happen), it still doesn't work. See // NEW ANNOTATION comment below:
import {libConst} from './lib-export-const.mjs'
const /** @type {{ fnNeverWithConstExport: () => never }} */ {fnNeverWithConstExport} = libConst; // NEW ANNOTATION
import {fnNeverWithEsmExport} from './lib-export-proper.mjs'

/** @returns {never} */
function fnSameModule() {
  process.exit(0);
}

function withSameModule() {
  fnSameModule();
  console.log('GOOD: properly seen as unreachable');
}

function withExport() {
  fnNeverWithEsmExport();
  console.log('GOOD: properly seen as unreachable');
}

function withConst() {
  fnNeverWithConstExport();
  console.log('BAD: incorrectly missed as unreachable');
}

// console.logs nothing (runtime behavior all good)
withSameModule();
withExport();
withConst();

@nmain
Copy link

nmain commented Oct 30, 2024

@ronjouch

so does this one that accesses libConst.fnNeverWithConstExport without the explicitly-typed fn

Slightly different rule there; the details of #32695 explains it entirely; the "dotted sequence of identifiers" part is relevant.

Coming back to my multi-file example

Let's stick to the playground; there's nothing going on here that can't be represented there.

adding explicit typing in the app.mjs file

I believe that gets into #29526 (or at least, a JSDoc variant of it). Typing destructrings is weird, and because the whole object was typed, there's actually still an inference step to pull out the property. It works fine without destructuring: https://www.typescriptlang.org/play/?filetype=js#code/PQKhAIAECcFMBcCu0B2BncBvFsButoBfcEYAKADNEUBjeASwHsVwKUBlAQwFtYBZRgBNEAG1gAKAJRYy4cPAAW0RgHdwKUSIDcZQmTI1maeOBH0ARgGEjJgLxZWKAHJ4CAdXqLr6eAFEAHgAOjNDwAFyOXLwCwmLghDpkoBCQ8ACegbBYUuC2AHzqrkQk5IY+ji740B5eNgHBobmmFt7GAHRsle6eCq1+QSHwiVS0DMzgKj1R-EKiEtKYspE8M7HzOnJlaIxibSKMAObiAOQA4gDy5wAiEYHKmdAiaeBosLAsnBjUcJw0CpzmMTHSQ6PT6EZ0JgsSa1Hw5RZyTpFGq9OoDUJSDbgLY7WB7Q4nABCAEEbuB6LQQnA6E9wNx6GhXoJwJ9wN9YL9-oDYMDQWQgA

@ronjouch
Copy link
Author

I believe that gets into #29526 (or at least, a JSDoc variant of it). Typing destructuring is weird, and because the whole object was typed, there's actually still an inference step to pull out the property. It works fine without destructuring.

@nmain indeed, now this makes sense and I'm a convinced capybara 👍👍👍. Thank you.


To a potential TS dev looking into this: from a user perspective, this remains unpleasant to use: I wouldn't want to re-type all my imports where they are used! Hope it gets addressed someday.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Oct 31, 2024
@RyanCavanaugh
Copy link
Member

We're definitely aware that it would be nice if this worked, but the perf cost of inserting a control flow graph node at every function call was too high.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Design Limitation" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

5 participants