-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Allow narrowing for any left-hand in operand #45152
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
1 similar comment
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at b4f2f8c. You can monitor the build here. Update: The results are in! |
Heya @andrewbranch, I've started to run the parallelized community code test suite on this PR at b4f2f8c. You can monitor the build here. |
Heya @andrewbranch, I've started to run the extended test suite on this PR at b4f2f8c. You can monitor the build here. |
@typescript-bot user test this inline |
Heya @andrewbranch, I've started to run the inline community code test suite on this PR at b4f2f8c. You can monitor the build here. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - main..45152
System
Hosts
Scenarios
Developer Information: |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@andrewbranch Here they are:Comparison Report - main..refs/pull/45152/merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with the suggested change.
A while back we put in place a restriction to ensure that assertion functions had to be fully annotated so that they couldn't trigger further CFA computation when trying to resolve them (#33622). It's been a while, so @ahejlsberg do you have an example of when this could happen? Just want to make sure there's nothing we're missing here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it some more, I think this is probably fine.
Extends a more restrictive fix already in place for #42639.
Previously, narrowing of an
in
expression happens if the expression satisfy two constraints:isNarrowableInOperands
is a syntactic constraint that allows narrowing of an expression if the rightin
operand is a narrowing expression, and the leftin
operand is a string literal (e.g.if ('prop' in c) { ... }
).narrowBinaryExpression
only callsnarrowByInKeyword
if the leftin
operand's type is a string literal type.An already merged PR (#44893) relaxed the binder constraint to allow narrowing in cases the left
in
operand is an identifier, to address scenarios such as:This PR further relaxes the constraint in the binder for narrowing
in
expressions, by no longer placing any syntactic constraint on the leftin
operand expression. This means that anin
expression is considered narrowable by the binder if the right expression is narrowing:Concerns:
From what I can tell, a possible concern would be performance, since this PR makes it so the checker attempts to narrow more types of
in
expressions, but the impact may be negligible.A caveat here (brought up by @andrewbranch) regarding perf tests is that, since the checker currently doesn't narrow those kinds of expressions, real world code is unlikely to have those kinds of expressions.