Fixed an issue in boolean comparison narrowing when the reference is an optional chain #56504
Conversation
…an optional chain
typescript-bot
added
the
For Uncommitted Bug
| @@ -27929,10 +27929,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (isMatchingConstructorReference(right)) { | |||
| return narrowTypeByConstructor(type, operator, left, assumeTrue); | |||
| } | |||
| if (isBooleanLiteral(right)) { | |||
| if (isBooleanLiteral(right) && !isAccessExpression(left)) { | |||
There was a problem hiding this comment.
It turns out that the original type can be reassigned based on optionalChainContainsReference. That messed up the else branch because narrowTypeByBooleanComparison was operating on the wrong type.
I've also tried keeping originalType around and pass that to narrowTypeByBooleanComparison. That fixed the reported issue~ related to the wrong type in the else branch but it broke the same type within the if block. This happened because originally the intention was for that reassigned type to be returned through the return at the bottom of this function but I changed it to return originalType through narrowTypeByBooleanComparison.
So all in all, this seems like the simplest fix and reflects the intention the best. The motivation behind the original change was to improve non-access expression narrowing. Access expressions are already handled by discrimination-based narrowing etc.
Even though it worked previously (as far as I know) for non-optional chain access expressions, this helps them avoid some redundant work too.
There was a problem hiding this comment.
This seems fine as a quick fix for now, though I suspect that we could try and do better later.
There was a problem hiding this comment.
Then again, not sure what "better" even means; an expression like (options?.a === false || options.b is not the same as (!options?.a || options.b) so realistically I don't think there's any extra info gained here.
|
@typescript-bot test top200 @typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at b549581. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at b549581. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at b549581. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at b549581. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at b549581. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
| @@ -27929,10 +27929,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (isMatchingConstructorReference(right)) { | |||
| return narrowTypeByConstructor(type, operator, left, assumeTrue); | |||
| } | |||
| if (isBooleanLiteral(right)) { | |||
| if (isBooleanLiteral(right) && !isAccessExpression(left)) { | |||
There was a problem hiding this comment.
This seems fine as a quick fix for now, though I suspect that we could try and do better later.
|
@typescript-bot cherry-pick this to release-5.3 |
|
Heya @jakebailey, I've started to cherry-pick this into |
typescript-bot
added
For Milestone Bug
|
Hey @jakebailey, I was unable to cherry-pick this PR. Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/6961303591 |
|
@typescript-bot cherry-pick this to release-5.3 |
|
Heya @jakebailey, I've started to cherry-pick this into |
|
Hey @jakebailey, I was unable to cherry-pick this PR. Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/6961580628 |
|
@typescript-bot cherry-pick this to release-5.3 |
|
Heya @jakebailey, I've started to cherry-pick this into |
|
Hey @jakebailey, I've created #56512 for you. |
|
Is using |
|
Never mind, I think I understand now. |
If you write an expression like It's possible that this could work with just |
…to release-5.3 (microsoft#56512) Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
fixes #56482
cc @jakebailey