-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Properly account for intersections in getKeyPropertyName
#63117
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
base: main
Are you sure you want to change the base?
Properly account for intersections in getKeyPropertyName
#63117
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| if ( | ||
| types.length < 10 || getObjectFlags(unionType) & ObjectFlags.PrimitiveUnion || | ||
| countWhere(types, t => !!(t.flags & (TypeFlags.Object | TypeFlags.InstantiableNonPrimitive))) < 10 | ||
| countWhere(types, t => !!(t.flags & (TypeFlags.Object | TypeFlags.Intersection | TypeFlags.InstantiableNonPrimitive))) < 10 |
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 condition matches now the existing flags check in mapTypesByKeyProperty (that function is already called only from within the containing function here - getKeyPropertyName)
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
It's kinda a bummer the benchmark numbers don't conclusively show this is an improvement. I'd argue this is like it should have always been unless there is some specific undocumented reason why it shouldn't be. To my best understanding though FWIW, with the Strict subtype cache: 0
Subtype cache: 0
Identity cache: 0
-Assignability cache: 58
+Assignability cache: 8
Type Count: 227 -> 228
Instantiation count: 0
Symbol count: 32,634 |
In the code below,
getMatchingUnionConstituentForType(and thustypeRelatedToSomeType) can't benefit from the key property optimization. So in certain situations (like this one) the source has to be related to each constituent of the target union (until it finds a match) - but it should be able to find the matching constituent using a fast path.This issue affects
test1. I've includedtest2for comparison purposes - the optimization kicks in there.TS playground: