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

Calling a function that returns never isn't treated as stopping control flow #32677

Closed
marijnh opened this issue Aug 2, 2019 · 10 comments
Closed
Labels
Duplicate An existing issue was already created

Comments

@marijnh
Copy link

marijnh commented Aug 2, 2019

Code

function raise(msg: string): never { throw new Error(msg) }

let x = 1 as (string | number)

if (typeof x != "number") raise("Stop")

console.log(x * 2)

Expected behavior:

I would expect the control flow analysis to treat the console.log as only being reachable if the if branch wasn't taken, which means that x is a number, so the code is accepted. This does happen when you inline the throw.

Actual behavior:

The type of x isn't narrowed and an error is reported for x * 2.

Playground Link: here

@nattthebear
Copy link

Duplicate of #16546, more or less.

@marijnh
Copy link
Author

marijnh commented Aug 2, 2019

Ah, didn't find that. But that one wasn't ever resolved and closed for housekeeping reasons. The return solution mostly works, but is awkward and adds extra typechecking-related cruft to the output JavaScript, so though I'm using it, but am not too happy with it.

Is there a technical reason for the way this currently works?

@jack-williams
Copy link
Collaborator

Duplicate of #8655.

Is there a technical reason for the way this currently works?

Read here.

@fatcerberus
Copy link

One thing that the above linked comment doesn’t mention is that the control flow graph is constructed before type information is available, so effectively this would require every single function call to be added to it. As you can imagine that would be prohibitively expensive to do.

@jack-williams
Copy link
Collaborator

It sorta does, just not in those words.

The control flow graph used to determine which expressions change the type of other expressions is constructed syntactically.

@fatcerberus
Copy link

That’s misleading though because if the return type is explicitly annotated then it’s part of the AST (and therefore still syntactical). TS just hasn’t constructed the necessary type information internally for that to matter.

@marijnh
Copy link
Author

marijnh commented Aug 2, 2019

I, for one, would be perfectly satisfied with this only working for functions explicitly annotated to return never. Would that still run into the control flow graph performance issue you mentioned?

@dragomirtitian
Copy link
Contributor

@marijnh yes it would still have the same performance issues.

At the time the control flow graph is created the compiler does not know the return type of a function, it just sees a call. So you either include all calls in the graph or no calls.

All you can use to decide if a node is to be included is the syntax.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Aug 2, 2019
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@acutmore
Copy link
Contributor

acutmore commented Nov 18, 2021

EDIT: Apparently this happens as otherwise it would make CFA too expensive performance wise.


Now that we have #32695 - I wasn't expecting a different between the following two snippets:

function throws(): never {
  throw new Error();
}
throws();
console.log('here'); // ⚠ unreachable code detected

However:

function throws(): never {
  throw new Error();
}
let n: string = throws();
console.log(n); // considered reachable

Is there somewhere that details why this is? ( I understand why 'never' is assignable to 'string', more about why the code is considered reachable). EDIT: It is for performance reasons, to reduce the number of function calls that are also CFA nodes

https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABFAFgJzgdwM4AoCUAXImAKYBupaiA3gFCLLpYmmaICiaGaBA3HQC+dOgBtSUEogC8iAOQoqpOQLoB6NVNmoMOfog2JwEOAFtTpMJIvrNOrHnx8DmuNRt0TYbHHEA6UTgAc1wwfDogA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

8 participants