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

Missing type narrowing based on control flow analysis of logical expressions #50739

Closed
mhofman opened this issue Sep 12, 2022 · 4 comments
Closed
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@mhofman
Copy link

mhofman commented Sep 12, 2022

Bug Report

πŸ”Ž Search Terms

Control-flow analysis logical

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

declare function fail(): never;

declare const a: string | null;
declare const b: string | null;

if (!a) fail();
a.charAt(0); // a is correctly narrowed to `string`

b || fail();
b.charAt(0); // Object is possibly 'null'. ts(2531)

πŸ™ Actual behavior

It seems that the branches of logical expressions do not impact control flow analysis, and thus the logical expression form is not equivalent to the conditional form, resulting in the type of b not being narrowed.

πŸ™‚ Expected behavior

b narrowed to the string type.

@jakebailey
Copy link
Member

jakebailey commented Sep 12, 2022

Based on #32695 I'm pretty sure that this is a design limitation and is working as expected, specifically, this rule:

the call occurs as a top-level expression statement

The fail() expression is not a top-level expression statement, so is not included in CFA.

See also issues like #47634 (or other issues that link back to #32695).

@jakebailey jakebailey added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Sep 12, 2022
@mhofman
Copy link
Author

mhofman commented Sep 12, 2022

This is unfortunate. In our case we have tons of checks of the form assert(condition, errorMessageExpression) that we realized have a significant performance cost because more often than not, errorMessageExpression is complex. We're looking at alternative styles and would really prefer using logical operators for their terseness, but would lose type narrowing.

@JoostK
Copy link
Contributor

JoostK commented Sep 13, 2022

You could consider an approach using optional chaining syntax, where the receiver object is nullified in production contexts:

debug?.assert(...);

TypeScript is doing something similar for trace logging.

Edit: although thinking about it some more, that might still not allow CFA narrowing as the guard is then only conditionally applied.

@mhofman
Copy link
Author

mhofman commented Nov 7, 2022

You could consider an approach using optional chaining syntax, where the receiver object is nullified in production contexts

I fail to see how this is relevant. Our checks are meant to be for production, not for local debugging. The part of the check on the left-hand side of the || operator is the condition being checked/asserted, which can be arbitrary. The fail call needs to be performed conditionally solely based on the result of the check, not some environment configuration.

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

4 participants