-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Fixed an issue in boolean comparison narrowing when the reference is an optional chain #56504
Fixed an issue in boolean comparison narrowing when the reference is an optional chain #56504
Conversation
…an optional chain
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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