Add heuristic for extracting irreducible null and undefined types from intersections of unions
#33150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 67ddb65. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 67ddb65. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 67ddb65. You can monitor the build here. It should now contribute to this PR's status checks. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
For the third time today, once again the user baselines are just today's as yet uncaptured/accepted update (just changes in upstream projects), so are clean~ |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 67ddb65. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
sandersn
requested changes
Sep 4, 2019
| @@ -9996,6 +9996,16 @@ namespace ts { | |||
| return true; | |||
| } | |||
|
|
|||
| function extractIrreducible(types: Type[], flag: TypeFlags) { | |||
| if (every(types, t => !!(t.flags & TypeFlags.Union) && some((t as UnionType).types, tt => !!(tt.flags & flag)))) { | |||
There was a problem hiding this comment.
could you add Null and Undefined to TypeFlags.IncludesMask to avoid the second check?
actually, would it be possible to add a special TypeFlags.EveryUnion that gets calculated in addTypesToIntersection and then added to includes? This uses up another TypeFlags, but is faster than trying to return two things. So..maybe not worth it.
I do notice that the two calls to extractIrreducible might iterate typeSet twice whenever it contains all-unions, but doesn't have any null or undefineds.
There was a problem hiding this comment.
could you add Null and Undefined to TypeFlags.IncludesMask to avoid the second check?
Nope, because we're looking for members-of-members that are undefined-or-null. We'd need 3 (or two) brand new flags (null and undefined are already used to track weather the intersection itself contains them). One for "every member is a union", one for "every member contains undefined", and one for "every member contains null". We only have two unused type flags left (all currently assigned type flags have a meaning in intersection construction), sooo.... Yeah.
This already bails early in every common scenario, the worst case where it's not needed is soemthing like a 52 element union where the first 51 contain undefined, but the 52nd contains null, which is irreducible and, notably, will trigger the "excessively large union" check in the else clause once the filtering fails, and cease to do any more work. So the "worst case" is either the case where it needs to occur and all the work needs to be done, or where it's very close to that but not, and will likely be converted to any, anyway.
| @@ -10114,6 +10124,12 @@ namespace ts { | |||
| // reduced we'll never reduce again, so this occurs at most once. | |||
| result = getIntersectionType(typeSet, aliasSymbol, aliasTypeArguments); | |||
| } | |||
| else if (extractIrreducible(typeSet, TypeFlags.Undefined)) { | |||
| result = getUnionType([getIntersectionType(typeSet), undefinedType], UnionReduction.Literal, aliasSymbol, aliasTypeArguments); | |||
There was a problem hiding this comment.
shouldn't we run both, for large intersections of the form x | null | undefined & ...?
There was a problem hiding this comment.
we do - note the recursive getIntersectionType call (which is needed to recalculate includes for the now filtered type set)
|
@sandersn Here they are:Comparison Report - master..33150
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg
approved these changes
Sep 4, 2019
|
Hmm, that perf run definitely looks odd. Seems like everything slowed down by 5-7%. |
… from intersections of unions
weswigham
force-pushed
the
strip-irreducible-types
branch
from
67ddb65
to
b86c86d
Compare
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at b86c86d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..33150
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn
approved these changes
Sep 4, 2019
|
@typescript-bot cherry-pick this into release-3.6 |
typescript-bot
pushed a commit
to typescript-bot/TypeScript
that referenced
this pull request
Sep 6, 2019
Component commits: b86c86d Add heuristic for extracting irreducible `null` and `undefined` types from intersections of unions
|
Hey @weswigham, I've opened #33285 for you. |
weswigham
pushed a commit
that referenced
this pull request
Sep 6, 2019
Component commits: b86c86d Add heuristic for extracting irreducible `null` and `undefined` types from intersections of unions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #33130