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 (@typescript-bot) perf test this |
|
Heya Andrew Branch (@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 Andrew Branch (@andrewbranch), I've started to run the parallelized community code test suite on this PR at b4f2f8c. You can monitor the build here. |
|
Heya Andrew Branch (@andrewbranch), I've started to run the extended test suite on this PR at b4f2f8c. You can monitor the build here. |
|
TypeScript Bot (@typescript-bot) user test this inline |
|
Heya Andrew Branch (@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! |
|
Andrew Branch (@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. |
|
Andrew Branch (@andrewbranch) Here they are:Comparison Report - main..refs/pull/45152/merge |
Anders Hejlsberg (ahejlsberg)
left a comment
There was a problem hiding this comment.
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 Anders Hejlsberg (@ahejlsberg) do you have an example of when this could happen? Just want to make sure there's nothing we're missing here. |
Daniel Rosenwasser (DanielRosenwasser)
left a comment
There was a problem hiding this comment.
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
inexpression happens if the expression satisfy two constraints:isNarrowableInOperandsis a syntactic constraint that allows narrowing of an expression if the rightinoperand is a narrowing expression, and the leftinoperand is a string literal (e.g.if ('prop' in c) { ... }).narrowBinaryExpressiononly callsnarrowByInKeywordif the leftinoperand's type is a string literal type.An already merged PR (#44893) relaxed the binder constraint to allow narrowing in cases the left
inoperand is an identifier, to address scenarios such as:This PR further relaxes the constraint in the binder for narrowing
inexpressions, by no longer placing any syntactic constraint on the leftinoperand expression. This means that aninexpression 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
inexpressions, but the impact may be negligible.A caveat here (brought up by Andrew Branch (@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.