Skip to content

Generalize the fastpath for comparisons of unions which are correspondences #41903

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 15, 2020
Merged

Generalize the fastpath for comparisons of unions which are correspondences #41903

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcc6aa8
Generalize the fastpath for comparisons of unions which are correspon…
weswigham Dec 9, 2020
8ee7b38
Add comment
weswigham Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17258,14 +17258,31 @@ namespace ts {
return Ternary.False;
}

function getUndefinedStrippedTargetIfNeeded(source: Type, target: Type) {
// As a builtin type, `undefined` is a very low type ID - making it almsot always first, making this a very fast check to see
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What types have lower type IDs? This seems potentially fragile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the any-likes are manufactured before undefined (yes, this is arbitrary and fragile, but it turns out, also useful), and they can't exist in a union: so undefined will actually always be the first element, if present.

// if we need to strip `undefined` from the target
if (source.flags & TypeFlags.Union && target.flags & TypeFlags.Union &&
!((source as UnionType).types[0].flags & TypeFlags.Undefined) && (target as UnionType).types[0].flags & TypeFlags.Undefined) {
return extractTypesOfKind(target, ~TypeFlags.Undefined);
}
return target;
}

function eachTypeRelatedToType(source: UnionOrIntersectionType, target: Type, reportErrors: boolean, intersectionState: IntersectionState): Ternary {
let result = Ternary.True;
const sourceTypes = source.types;
// We strip `undefined` from the target if the `source` trivially doesn't contain it for our correspondence-checking fastpath
// since `undefined` is frequently added by optionality and would otherwise spoil a potentially useful correspondence
const undefinedStrippedTarget = getUndefinedStrippedTargetIfNeeded(source, target as UnionType);
for (let i = 0; i < sourceTypes.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to access (undefinedStrippedTarget as UnionType).types.length quite a few times. Would it be worthwhile to extract it out of the loop, since it won't change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehh, it's a little awkward because we can only get it after the Union flag check, which is done internally to the loop.

const sourceType = sourceTypes[i];
if (target.flags & TypeFlags.Union && (target as UnionType).types.length === sourceTypes.length) {
if (undefinedStrippedTarget.flags & TypeFlags.Union && sourceTypes.length >= (undefinedStrippedTarget as UnionType).types.length && sourceTypes.length % (undefinedStrippedTarget as UnionType).types.length === 0) {
// many unions are mappings of one another; in such cases, simply comparing members at the same index can shortcut the comparison
const related = isRelatedTo(sourceType, (target as UnionType).types[i], /*reportErrors*/ false, /*headMessage*/ undefined, intersectionState);
// such unions will have identical lengths, and their corresponding elements will match up. Another common scenario is where a large
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this order guaranteed somehow? If not, would we have someway of knowing that it changed, breaking this optimization?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind-of. It's not guaranteed, just very often true. Often enough to be a useful heuristic. Unions are sorted by type IDs, type IDs are assigned on type construction, type are constructed in a top-down/left-to-right/as-they-appear kinda way when normalizing or mapping (mostly as an artifact of our mostly lazy compiler design). A notable thing that can easily break this ordering heuristic is code which pulls one of the type out of the union and makes it in advance, for example:

type A = {a};
type B = {b};
type FooB = "foo" & B;
type Union = ("foo" | "bar") & (A | B);

The presence of the FooB type changes the order in the Union type. Not much to be done when that happens, except hope it doesn't shift around too much. (Since partial correspondences still benefit from this check, just not as much) That's one reason why we still fall back to the exhaustive check if the corresponding element fails to match.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, would we have someway of knowing that it changed, breaking this optimization?

That test that I commented on in the other PR would time out, for one. :V

// union has a union of objects intersected with it. In such cases, if the input was, eg `("a" | "b" | "c") & (string | boolean | {} | {whatever})`,
// the result will have the structure `"a" | "b" | "c" | "a" & {} | "b" & {} | "c" & {} | "a" & {whatever} | "b" & {whatever} | "c" & {whatever}`
// - the resulting union has a length which is a multiple of the original union, and the elements correspond modulo the length of the original union
const related = isRelatedTo(sourceType, (undefinedStrippedTarget as UnionType).types[i % (undefinedStrippedTarget as UnionType).types.length], /*reportErrors*/ false, /*headMessage*/ undefined, intersectionState);
if (related) {
result &= related;
continue;
Expand Down