Skip to content

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

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 6 commits into from
Nov 26, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ab8bd8
Try to find object/intersection types with the most overlap when fail…
DanielRosenwasser Sep 14, 2018
76e721f
Added tests.
DanielRosenwasser Sep 14, 2018
91af958
Accepted baselines.
DanielRosenwasser Sep 14, 2018
6ae288f
Merge remote-tracking branch 'origin/master' into overlappyTypes
DanielRosenwasser Nov 16, 2018
b7da98b
Use intersection of member names to find largest overlap.
DanielRosenwasser Nov 16, 2018
5a4378e
Accepted baselines.
DanielRosenwasser Nov 16, 2018
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
29 changes: 28 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11472,7 +11472,8 @@ namespace ts {
findMatchingDiscriminantType(source, target) ||
findMatchingTypeReferenceOrTypeAliasReference(source, target) ||
findBestTypeForObjectLiteral(source, target) ||
findBestTypeForInvokable(source, target);
findBestTypeForInvokable(source, target) ||
findMostOverlappyType(source, target);

isRelatedTo(source, bestMatchingType || targetTypes[targetTypes.length - 1], /*reportErrors*/ true);
}
Expand Down Expand Up @@ -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

Copy link
Member

@weswigham weswigham Sep 14, 2018 •
edited
Loading

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.

if (!(source.flags & (TypeFlags.Object | TypeFlags.Intersection))) {
return undefined;
}
const sourceProperties = getPropertiesOfType(source);
let bestType;
let count = -1;
for (const target of unionTarget.types) {
if (!(target.flags & (TypeFlags.Object | TypeFlags.Intersection))) {
continue;
}

let currentCount = 0;
for (const prop of sourceProperties) {
if (getPropertyOfType(target, prop.escapedName)) {
currentCount++;
}
}
if (currentCount >= count) {
count = currentCount;
bestType = target;
}
}
return bestType;
}

// Keep this up-to-date with the same logic within `getApparentTypeOfContextualType`, since they should behave similarly
function findMatchingDiscriminantType(source: Type, target: UnionOrIntersectionType) {
let match: Type | undefined;
Expand Down
24 changes: 14 additions & 10 deletions tests/baselines/reference/contextualTypeWithUnionTypeObjectLiteral.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts(
Type 'string | number' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts(21,5): error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; } | { prop: number; }'.
Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: number; }'.
Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; }'.
Types of property 'prop' are incompatible.
Type 'string | number' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
Type 'string | number' is not assignable to type 'string'.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts(25,5): error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; } | { prop: number; anotherP1: number; }'.
Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: number; anotherP1: number; }'.
Property 'anotherP1' is missing in type '{ prop: string | number; anotherP: string; }'.
Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; }'.
Types of property 'prop' are incompatible.
Type 'string | number' is not assignable to type 'string'.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts(29,5): error TS2322: Type '{ prop: string | number; anotherP: string; anotherP1: number; }' is not assignable to type '{ prop: string; anotherP: string; } | { prop: number; anotherP1: number; }'.
Type '{ prop: string | number; anotherP: string; anotherP1: number; }' is not assignable to type '{ prop: number; anotherP1: number; }'.
Types of property 'prop' are incompatible.
Expand Down Expand Up @@ -63,18 +65,20 @@ tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts(
var objStrOrNum6: { prop: string; anotherP: string; } | { prop: number } = {
~~~~~~~~~~~~
!!! error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; } | { prop: number; }'.
!!! error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: number; }'.
!!! error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; }'.
!!! error TS2322: Types of property 'prop' are incompatible.
!!! error TS2322: Type 'string | number' is not assignable to type 'number'.
!!! error TS2322: Type 'string' is not assignable to type 'number'.
!!! error TS2322: Type 'string | number' is not assignable to type 'string'.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
prop: strOrNumber,
anotherP: str
};
var objStrOrNum7: { prop: string; anotherP: string; } | { prop: number; anotherP1: number } = {
~~~~~~~~~~~~
!!! error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; } | { prop: number; anotherP1: number; }'.
!!! error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: number; anotherP1: number; }'.
!!! error TS2322: Property 'anotherP1' is missing in type '{ prop: string | number; anotherP: string; }'.
!!! error TS2322: Type '{ prop: string | number; anotherP: string; }' is not assignable to type '{ prop: string; anotherP: string; }'.
!!! error TS2322: Types of property 'prop' are incompatible.
!!! error TS2322: Type 'string | number' is not assignable to type 'string'.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
prop: strOrNumber,
anotherP: str
};
Expand Down
56 changes: 56 additions & 0 deletions tests/baselines/reference/errorsOnUnionsOfOverlappingObjects01.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
tests/cases/compiler/errorsOnUnionsOfOverlappingObjects01.ts(18,3): error TS2345: Argument of type '{ a: string; b: string; }' is not assignable to parameter of type 'Foo | Other'.
Type '{ a: string; b: string; }' is not assignable to type 'Foo'.
Types of property 'b' are incompatible.
Type 'string' is not assignable to type 'number'.
tests/cases/compiler/errorsOnUnionsOfOverlappingObjects01.ts(19,3): error TS2345: Argument of type '{ a: string; b: string; }' is not assignable to parameter of type 'Foo | Other'.
Type '{ a: string; b: string; }' is not assignable to type 'Foo'.
Types of property 'b' are incompatible.
Type 'string' is not assignable to type 'number'.
tests/cases/compiler/errorsOnUnionsOfOverlappingObjects01.ts(24,5): error TS2345: Argument of type '{ a: string; b: string; }' is not assignable to parameter of type 'Bar | Other'.
Object literal may only specify known properties, and 'a' does not exist in type 'Bar | Other'.


==== tests/cases/compiler/errorsOnUnionsOfOverlappingObjects01.ts (3 errors) ====
interface Foo {
a: string;
b: number;
};

interface Bar {
b: string;
}

interface Other {
totallyUnrelatedProperty: number;
}

export let x = { a: '', b: '' };

declare function f(x: Foo | Other): any;

f(x);
~
!!! error TS2345: Argument of type '{ a: string; b: string; }' is not assignable to parameter of type 'Foo | Other'.
!!! error TS2345: Type '{ a: string; b: string; }' is not assignable to type 'Foo'.
!!! error TS2345: Types of property 'b' are incompatible.
!!! error TS2345: Type 'string' is not assignable to type 'number'.
f({ a: '', b: '' })
~~~~~~~~~~~~~~~~
!!! error TS2345: Argument of type '{ a: string; b: string; }' is not assignable to parameter of type 'Foo | Other'.
!!! error TS2345: Type '{ a: string; b: string; }' is not assignable to type 'Foo'.
!!! error TS2345: Types of property 'b' are incompatible.
!!! error TS2345: Type 'string' is not assignable to type 'number'.

declare function g(x: Bar | Other): any;

g(x);
g({ a: '', b: '' })
~~~~~
!!! error TS2345: Argument of type '{ a: string; b: string; }' is not assignable to parameter of type 'Bar | Other'.
!!! error TS2345: Object literal may only specify known properties, and 'a' does not exist in type 'Bar | Other'.

declare function h(x: Foo | Bar | Other): any;

h(x);
h({ a: '', b: '' })

43 changes: 43 additions & 0 deletions tests/baselines/reference/errorsOnUnionsOfOverlappingObjects01.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
//// [errorsOnUnionsOfOverlappingObjects01.ts]
interface Foo {
a: string;
b: number;
};

interface Bar {
b: string;
}

interface Other {
totallyUnrelatedProperty: number;
}

export let x = { a: '', b: '' };

declare function f(x: Foo | Other): any;

f(x);
f({ a: '', b: '' })

declare function g(x: Bar | Other): any;

g(x);
g({ a: '', b: '' })

declare function h(x: Foo | Bar | Other): any;

h(x);
h({ a: '', b: '' })


//// [errorsOnUnionsOfOverlappingObjects01.js]
"use strict";
exports.__esModule = true;
;
exports.x = { a: '', b: '' };
f(exports.x);
f({ a: '', b: '' });
g(exports.x);
g({ a: '', b: '' });
h(exports.x);
h({ a: '', b: '' });
77 changes: 77 additions & 0 deletions tests/baselines/reference/errorsOnUnionsOfOverlappingObjects01.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
=== tests/cases/compiler/errorsOnUnionsOfOverlappingObjects01.ts ===
interface Foo {
>Foo : Symbol(Foo, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 0, 0))

a: string;
>a : Symbol(Foo.a, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 0, 15))

b: number;
>b : Symbol(Foo.b, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 1, 14))

};

interface Bar {
>Bar : Symbol(Bar, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 3, 2))

b: string;
>b : Symbol(Bar.b, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 5, 15))
}

interface Other {
>Other : Symbol(Other, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 7, 1))

totallyUnrelatedProperty: number;
>totallyUnrelatedProperty : Symbol(Other.totallyUnrelatedProperty, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 9, 17))
}

export let x = { a: '', b: '' };
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 10))
>a : Symbol(a, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 16))
>b : Symbol(b, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 23))

declare function f(x: Foo | Other): any;
>f : Symbol(f, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 32))
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 15, 19))
>Foo : Symbol(Foo, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 0, 0))
>Other : Symbol(Other, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 7, 1))

f(x);
>f : Symbol(f, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 32))
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 10))

f({ a: '', b: '' })
>f : Symbol(f, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 32))
>a : Symbol(a, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 18, 3))
>b : Symbol(b, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 18, 10))

declare function g(x: Bar | Other): any;
>g : Symbol(g, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 18, 19))
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 20, 19))
>Bar : Symbol(Bar, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 3, 2))
>Other : Symbol(Other, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 7, 1))

g(x);
>g : Symbol(g, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 18, 19))
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 10))

g({ a: '', b: '' })
>g : Symbol(g, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 18, 19))
>a : Symbol(a, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 23, 3))
>b : Symbol(b, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 23, 10))

declare function h(x: Foo | Bar | Other): any;
>h : Symbol(h, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 23, 19))
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 25, 19))
>Foo : Symbol(Foo, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 0, 0))
>Bar : Symbol(Bar, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 3, 2))
>Other : Symbol(Other, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 7, 1))

h(x);
>h : Symbol(h, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 23, 19))
>x : Symbol(x, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 13, 10))

h({ a: '', b: '' })
>h : Symbol(h, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 23, 19))
>a : Symbol(a, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 28, 3))
>b : Symbol(b, Decl(errorsOnUnionsOfOverlappingObjects01.ts, 28, 10))

82 changes: 82 additions & 0 deletions tests/baselines/reference/errorsOnUnionsOfOverlappingObjects01.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
=== tests/cases/compiler/errorsOnUnionsOfOverlappingObjects01.ts ===
interface Foo {
a: string;
>a : string

b: number;
>b : number

};

interface Bar {
b: string;
>b : string
}

interface Other {
totallyUnrelatedProperty: number;
>totallyUnrelatedProperty : number
}

export let x = { a: '', b: '' };
>x : { a: string; b: string; }
>{ a: '', b: '' } : { a: string; b: string; }
>a : string
>'' : ""
>b : string
>'' : ""

declare function f(x: Foo | Other): any;
>f : (x: Foo | Other) => any
>x : Foo | Other

f(x);
>f(x) : any
>f : (x: Foo | Other) => any
>x : { a: string; b: string; }

f({ a: '', b: '' })
>f({ a: '', b: '' }) : any
>f : (x: Foo | Other) => any
>{ a: '', b: '' } : { a: string; b: string; }
>a : string
>'' : ""
>b : string
>'' : ""

declare function g(x: Bar | Other): any;
>g : (x: Bar | Other) => any
>x : Bar | Other

g(x);
>g(x) : any
>g : (x: Bar | Other) => any
>x : { a: string; b: string; }

g({ a: '', b: '' })
>g({ a: '', b: '' }) : any
>g : (x: Bar | Other) => any
>{ a: '', b: '' } : { a: string; b: string; }
>a : string
>'' : ""
>b : string
>'' : ""

declare function h(x: Foo | Bar | Other): any;
>h : (x: Foo | Bar | Other) => any
>x : Foo | Bar | Other

h(x);
>h(x) : any
>h : (x: Foo | Bar | Other) => any
>x : { a: string; b: string; }

h({ a: '', b: '' })
>h({ a: '', b: '' }) : any
>h : (x: Foo | Bar | Other) => any
>{ a: '', b: '' } : { a: string; b: string; }
>a : string
>'' : ""
>b : string
>'' : ""

8 changes: 4 additions & 4 deletions tests/baselines/reference/excessPropertyCheckWithUnions.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ tests/cases/compiler/excessPropertyCheckWithUnions.ts(39,1): error TS2322: Type
Types of property 'tag' are incompatible.
Type '"A"' is not assignable to type '"C"'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(40,1): error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type 'Ambiguous'.
Type '{ tag: "A"; z: true; }' is not assignable to type '{ tag: "C"; }'.
Type '{ tag: "A"; z: true; }' is not assignable to type '{ tag: "B"; z: boolean; }'.
Types of property 'tag' are incompatible.
Type '"A"' is not assignable to type '"C"'.
Type '"A"' is not assignable to type '"B"'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(49,35): error TS2322: Type '{ a: 1; b: 1; first: string; second: string; }' is not assignable to type 'Overlapping'.
Object literal may only specify known properties, and 'second' does not exist in type '{ a: 1; b: 1; first: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(50,35): error TS2322: Type '{ a: 1; b: 1; first: string; third: string; }' is not assignable to type 'Overlapping'.
Expand Down Expand Up @@ -92,9 +92,9 @@ tests/cases/compiler/excessPropertyCheckWithUnions.ts(66,9): error TS2322: Type
amb = { tag: "A", z: true }
~~~
!!! error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type 'Ambiguous'.
!!! error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type '{ tag: "C"; }'.
!!! error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type '{ tag: "B"; z: boolean; }'.
!!! error TS2322: Types of property 'tag' are incompatible.
!!! error TS2322: Type '"A"' is not assignable to type '"C"'.
!!! error TS2322: Type '"A"' is not assignable to type '"B"'.

type Overlapping =
| { a: 1, b: 1, first: string }
Expand Down
Loading