Skip to content
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

Elaborate on types in unions with the most overlap in properties #27087

Merged
merged 6 commits into from
Nov 26, 2018

Conversation

DanielRosenwasser
Copy link
Member

When relating a type S to a union type T, if S is an object type or intersection type, then we will find the last object or union type T' in T which shares the most properties with S.

Fixes #26450

@DanielRosenwasser DanielRosenwasser changed the title Overlappy types Elaborate on types in unions with the most overlap in properties Sep 14, 2018
@@ -11512,6 +11513,32 @@ namespace ts {
}
}

function findMostOverlappyType(source: Type, unionTarget: UnionOrIntersectionType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OverlappyType is a new favorite term.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another candidate was getMostShallowSatisfiedType but that sounded slightly judgemental.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a general intuition for why the last most overlappy type is better than the first more overlappy type? For example, do people tend to write union types in a manner where the rightmost types tend to be better than the leftmost?

Copy link
Member Author

Choose a reason for hiding this comment

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

So these types are always sorted by their ID, and those are always based on declaration order.

For named types (e.g. interface declarations) that's not usually that useful, but sometimes there's an intent when the user writes out anonymous types. For example, if the user wrote type literals such as { someProp: number } | { anotherProp: string }, my own experience has shown me that a lot of the time the first one actually is thought of as the more common case. That's part of why I went with the first type in #26895 (though I think keeping it simple was the primary motivator).

When I tried this and only took the first most-overlappy type, I think I found a decent number of hit-or-miss changes. I didn't see significant improvements in the baselines and given that the change is literally switching between a > and a >=, I'd rather keep the baselines cleaner in that point. I think the high order bit is that we just look for overlappy types in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

That all makes sense. this was more curiosity and if there was anything you guys had gleaned from patterns/practices that might weigh in here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

So these types are always sorted by their ID, and those are always based on declaration order.

Oh right... otherwise it would be a pita to figure out equality. normalization totally makes sense here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, in our experience it ended up as more of a lavash or a naan, but I wouldn't be surprised if not sorting the IDs made type comparisons a pita.

Copy link
Contributor

Choose a reason for hiding this comment

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

Barbari or Sangak 4ever.

Copy link
Member

Choose a reason for hiding this comment

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

Please help @JsonFreeman

Copy link
Contributor

Choose a reason for hiding this comment

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

I live in NYC now so I have to vote for bagels

@@ -11512,6 +11513,32 @@ namespace ts {
}
}

function findMostOverlappyType(source: Type, unionTarget: UnionOrIntersectionType) {
Copy link
Member

@weswigham weswigham Sep 14, 2018

Choose a reason for hiding this comment

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

So, I think this is a slightly cleaner implementation that should also handle generics better:

function findMostOverlappyType(source: Type, unionTarget: UnionOrIntersectionType) {
    let bestMatch = unionTarget.types[0];
    let matchingCount = 0;
    for (const target of unionTarget.types) {
        const overlap = getIntersectionType([getIndexType(source), getIndexType(target)]);
        if (overlap.kind & TypeFlags.Index) {
            // perfect overlap of keys
            bestMatch = target;
            matchingCount = Infinity;
        }
        else if (overlap.flags & TypeFlags.Union) {
            // Some subset overlap
            const len = length((overlap as UnionType).types);
            if (len >= matchingCount) {
                bestMatch = target;
                matchingCount = len;
            }
        }
        else if (matchingCount === 0) {
            bestMatch = target;
        }
    }
    return bestMatch;
}

Given something like type MappedTypeUnion<T> = {[x in keyof T]: x} | {[x in "a"]: "b"}, against, eg, {[x in keyof T]: [x]}, this should prefer the first type instead of the latter.

tests/cases/conformance/jsx/file.tsx(49,6): error TS2322: Type '{ children: Element[]; a: number; b: string; }' is not assignable to type 'Prop'.
Types of property 'children' are incompatible.
Type 'Element[]' is not assignable to type 'string | Element'.
Type 'Element[]' is missing the following properties from type 'Element': type, props
Type 'Element[]' is not assignable to type 'string'.
Copy link
Member Author

Choose a reason for hiding this comment

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

Regression :(

Copy link
Member

@weswigham weswigham Nov 17, 2018

Choose a reason for hiding this comment

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

It's probably better this way, for now, until the element -> argument case has a special handler & diagnostic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants