Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Let properties having primitive types discriminate object unions #60718
base: main
Are you sure you want to change the base?
Let properties having primitive types discriminate object unions #60718
Changes from 27 commits
7f3d3fb
1bc0a55
48e86a7
dc0866f
8542a99
2b69230
6959a73
a4c22c2
fd99a91
6f6d928
3c9b24b
086bbe4
f46d0dd
8e0cf94
e8741f2
443320a
2e87e81
81b5d4a
6c0d085
5566306
602ca72
ae873d4
21feb28
d401d2e
868d5dc
addeec3
c846958
981e7ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 check can probably be done in
createUnionOrIntersectionProperty
, setting a newCheckFlag.HasPrimitive
, like we do forHasNonUniformType
.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.
@gabritto Yes @Andarist had already planted the idea in my mind, and I had considered this possibility in case there were performance issues. Do you think I should implement it in any case?
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.
Even if it doesn't matter for the perf benchmarks we have, I think yes, to be consistent and keep similar code in a single place.
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.
@gabritto some feedback.
The basic change is quite easy, but there are some problems because
createUnionOrIntersectionProperty
terminates withresult.links.type = isUnion ? getUnionType(propTypes) : getIntersectionType(propTypes)
, and is this resulting type the one on which I'm currently runningsomeType(propType, t => !!(t.flags & TypeFlags.Primitive)))
. It's like it has already been reduced/cleaned up/whatever.Just setting to
1
theCheckFlag.HasPrimitiveType
beforehand like we do for other flags doesn't seem a good idea. I got 8 failing baselines and some changes are pretty weird. I'd suspect the right condition for setting theHasPrimitiveType
flag will be a little convoluted. Or will be convoluted the one insideisDiscriminantProperty
.I pushed the current changes so you can have a look and maybe spot some stupid error I'm doing, or provide some insights for the above conditions.
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.
@gabritto plot twist, I disabled the new
considerNonUniformPrimitivePropDiscriminant
discriminating flag everywhere but in the only place I was sure we need it and things have improved a lot. I like it! The new discriminating power is being activated in more cases with your suggestion, that's why I changed more baselines, but the changes seemed to be correct to me. Please re-check them.I still have to check why the
binder.ts
stopped compiling tho...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.
Pushed a fix that you may dislike, so I'm open to suggestions.
The problem:
The condition
node.body
is checked so its type is refined to justModuleBody
. That's fine!Then you check
!node.body.parent
where:It cannot be non-defined! So the check
!node.body.parent
now narrows the type ofnode.body
fromModuleBody
tonever
.Now, this is happening when building that structure so it's perfectly possible that the parent is not set yet. And I understand why you may not want to reflect this into types. Probably for all other intents and purposes that field will be always set. But TS just see the types, and with more refinement capabilities it now understands that the
!node.body.parent
case should be impossible.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 PR is turning out to be more and more overzealous; I can already see a ton of projects in the top 400 breaking even worse.