-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix "Union types are not being narrowed down correctly in 4.3 #44401" #44771
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
Conversation
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.
I originally opened #44101 because it prevented a project of mine from compiling. I can confirm that this PR fixes the issue. I cloned this fork and could compile my project with the local compiler without touching my code in any way. Looking forward to having this merged, thank you so much @caojoshua!
@ahejlsberg 👍/👎? |
src/compiler/checker.ts
Outdated
@@ -18359,9 +18359,9 @@ namespace ts { | |||
if (relation === comparableRelation && target.flags & TypeFlags.Primitive) { | |||
const constraints = sameMap((source as IntersectionType).types, t => t.flags & TypeFlags.Primitive ? t : getBaseConstraintOfType(t) || unknownType); |
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.
I think the correct fix is to change this line to
const constraints = sameMap((source as IntersectionType).types, t => t.flags & TypeFlags.Primitive ? t : getBaseConstraintOfType(t) || unknownType); | |
const constraints = sameMap((source as IntersectionType).types, getBaseConstraintOrType); |
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.
Makes sense. Can confirm is passing the test. Just pushed this change.
source = getIntersectionType(constraints); | ||
if (!(source.flags & TypeFlags.Intersection)) { | ||
return isRelatedTo(source, target, /*reportErrors*/ false); | ||
const newSource = getIntersectionType(constraints); |
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.
These three lines should just be left unchanged. The intent is to overwrite the original intersection with the new and more specific intersection, and we rely on this substitution in the code the follows.
Is there any chance that this will land in 4.4? |
@KnorpelSenf yep, this will be in 4.4 (not in the Beta, which is already out, but any subsequent release) |
Fixes #44401
The regression occurred in this commit