Skip to content

Improve EPC for unions with multiple discriminants #32711

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

Closed
wants to merge 1 commit into from
Closed

Improve EPC for unions with multiple discriminants #32711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 29 additions & 7 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14272,20 +14272,42 @@ namespace ts {
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary): Type | undefined;
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue: Type): Type;
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue?: Type) {
let match: Type | undefined;
// `candidates` maps a `target` constituent to the number
// of discriminant properties the constituent matches.
const candidates: Map<number> = createMap();
let matchedProperties = 0;
for (const [getDiscriminatingType, propertyName] of discriminators) {
let matched = 0;
for (const type of target.types) {
const targetType = getTypeOfPropertyOfType(type, propertyName);
if (targetType && related(getDiscriminatingType(), targetType)) {
if (match) {
if (type === match) continue; // Finding multiple fields which discriminate to the same type is fine
return defaultValue;
}
match = type;
matched = 1;
const strId = "" + type.id;
candidates.set(strId, (candidates.get(strId) || 0) + 1);
}
}
matchedProperties += matched;
}
/*
* `matchID` has type:
* - string when exactly one `target` constituent matches all matched properties.
* - false when more than one `target` constituent matches all matched properties.
* - undefined when no `target` constituent matches all matched properties.
*
* We only proceed in the first case. The second case introduces ambiguity that we choose
Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining what criteria this is filtering/selecting on would be useful. I think it's that it returns the first type which matches all discriminant properties (out of multiple which may match), but some text expressing that would be nice.

Copy link
Collaborator Author

@jack-williams jack-williams Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Will do. If there are multiple valid matches it fails rather than picking the first. I did this to avoid situations where the order of the union influenced whether you got an error, sometimes incorrectly. (I think it was a case where some constituents were technically subtypes of another, but the union was unreduced).

Copy link
Collaborator Author

@jack-williams jack-williams Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@weswigham Done. In the case where there are multiple candidates we could return a new union of just those candidates, but I'm not sure of the error message and perf consequences---if you want me to experiment with that approach let me know.

* not to resolve: selecting an arbitrary type would be wrong, and combining the matches
* would require synthesizing a new union type.
*/
let matchID: string | false | undefined;
candidates.forEach((value, id) => {
if (matchID !== false && value === matchedProperties) {
matchID = matchID === undefined ? id : false;
}
});
if (typeof matchID === "string") {
return firstDefined(target.types, type => "" + type.id === matchID ? type : undefined) || defaultValue;
}
return match || defaultValue;
return defaultValue;
}

/**
Expand Down
78 changes: 78 additions & 0 deletions tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts(30,5): error TS2322: Type '{ type: "number"; value: number; multipleOf: number; format: string; }' is not assignable to type 'Primitive'.
Object literal may only specify known properties, and 'multipleOf' does not exist in type 'Float'.
tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts(41,5): error TS2322: Type '{ p1: "left"; p2: false; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'.
Object literal may only specify known properties, and 'p3' does not exist in type '{ p1: "left"; p2: boolean; }'.
tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts(57,5): error TS2322: Type '{ p1: "right"; p2: false; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'.
Object literal may only specify known properties, and 'p3' does not exist in type '{ p1: "right"; p2: false; p4: string; }'.


==== tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts (3 errors) ====
// Repro from #32657

interface Base<T> {
value: T;
}

interface Int extends Base<number> {
type: "integer";
multipleOf?: number;
}

interface Float extends Base<number> {
type: "number";
}

interface Str extends Base<string> {
type: "string";
format?: string;
}

interface Bool extends Base<boolean> {
type: "boolean";
}

type Primitive = Int | Float | Str | Bool;

const foo: Primitive = {
type: "number",
value: 10,
multipleOf: 5, // excess property
~~~~~~~~~~~~~
!!! error TS2322: Type '{ type: "number"; value: number; multipleOf: number; format: string; }' is not assignable to type 'Primitive'.
!!! error TS2322: Object literal may only specify known properties, and 'multipleOf' does not exist in type 'Float'.
format: "what?"
}


type DisjointDiscriminants = { p1: 'left'; p2: true; p3: number } | { p1: 'right'; p2: false; p4: string } | { p1: 'left'; p2: boolean };

// This has excess error because variant three is the only applicable case.
const a: DisjointDiscriminants = {
p1: 'left',
p2: false,
p3: 42,
~~~~~~
!!! error TS2322: Type '{ p1: "left"; p2: false; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'.
!!! error TS2322: Object literal may only specify known properties, and 'p3' does not exist in type '{ p1: "left"; p2: boolean; }'.
p4: "hello"
};

// This has no excess error because variant one and three are both applicable.
const b: DisjointDiscriminants = {
p1: 'left',
p2: true,
p3: 42,
p4: "hello"
};

// This has excess error because variant two is the only applicable case
const c: DisjointDiscriminants = {
p1: 'right',
p2: false,
p3: 42,
~~~~~~
!!! error TS2322: Type '{ p1: "right"; p2: false; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'.
!!! error TS2322: Object literal may only specify known properties, and 'p3' does not exist in type '{ p1: "right"; p2: false; p4: string; }'.
p4: "hello"
};

91 changes: 91 additions & 0 deletions tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
//// [excessPropertyCheckWithMultipleDiscriminants.ts]
// Repro from #32657

interface Base<T> {
value: T;
}

interface Int extends Base<number> {
type: "integer";
multipleOf?: number;
}

interface Float extends Base<number> {
type: "number";
}

interface Str extends Base<string> {
type: "string";
format?: string;
}

interface Bool extends Base<boolean> {
type: "boolean";
}

type Primitive = Int | Float | Str | Bool;

const foo: Primitive = {
type: "number",
value: 10,
multipleOf: 5, // excess property
format: "what?"
}


type DisjointDiscriminants = { p1: 'left'; p2: true; p3: number } | { p1: 'right'; p2: false; p4: string } | { p1: 'left'; p2: boolean };

// This has excess error because variant three is the only applicable case.
const a: DisjointDiscriminants = {
p1: 'left',
p2: false,
p3: 42,
p4: "hello"
};

// This has no excess error because variant one and three are both applicable.
const b: DisjointDiscriminants = {
p1: 'left',
p2: true,
p3: 42,
p4: "hello"
};

// This has excess error because variant two is the only applicable case
const c: DisjointDiscriminants = {
p1: 'right',
p2: false,
p3: 42,
p4: "hello"
};


//// [excessPropertyCheckWithMultipleDiscriminants.js]
// Repro from #32657
var foo = {
type: "number",
value: 10,
multipleOf: 5,
format: "what?"
};
// This has excess error because variant three is the only applicable case.
var a = {
p1: 'left',
p2: false,
p3: 42,
p4: "hello"
};
// This has no excess error because variant one and three are both applicable.
var b = {
p1: 'left',
p2: true,
p3: 42,
p4: "hello"
};
// This has excess error because variant two is the only applicable case
var c = {
p1: 'right',
p2: false,
p3: 42,
p4: "hello"
};
143 changes: 143 additions & 0 deletions tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
=== tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts ===
// Repro from #32657

interface Base<T> {
>Base : Symbol(Base, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 0, 0))
>T : Symbol(T, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 2, 15))

value: T;
>value : Symbol(Base.value, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 2, 19))
>T : Symbol(T, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 2, 15))
}

interface Int extends Base<number> {
>Int : Symbol(Int, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 4, 1))
>Base : Symbol(Base, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 0, 0))

type: "integer";
>type : Symbol(Int.type, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 6, 36))

multipleOf?: number;
>multipleOf : Symbol(Int.multipleOf, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 7, 20))
}

interface Float extends Base<number> {
>Float : Symbol(Float, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 9, 1))
>Base : Symbol(Base, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 0, 0))

type: "number";
>type : Symbol(Float.type, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 11, 38))
}

interface Str extends Base<string> {
>Str : Symbol(Str, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 13, 1))
>Base : Symbol(Base, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 0, 0))

type: "string";
>type : Symbol(Str.type, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 15, 36))

format?: string;
>format : Symbol(Str.format, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 16, 19))
}

interface Bool extends Base<boolean> {
>Bool : Symbol(Bool, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 18, 1))
>Base : Symbol(Base, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 0, 0))

type: "boolean";
>type : Symbol(Bool.type, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 20, 38))
}

type Primitive = Int | Float | Str | Bool;
>Primitive : Symbol(Primitive, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 22, 1))
>Int : Symbol(Int, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 4, 1))
>Float : Symbol(Float, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 9, 1))
>Str : Symbol(Str, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 13, 1))
>Bool : Symbol(Bool, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 18, 1))

const foo: Primitive = {
>foo : Symbol(foo, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 26, 5))
>Primitive : Symbol(Primitive, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 22, 1))

type: "number",
>type : Symbol(type, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 26, 24))

value: 10,
>value : Symbol(value, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 27, 19))

multipleOf: 5, // excess property
>multipleOf : Symbol(multipleOf, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 28, 14))

format: "what?"
>format : Symbol(format, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 29, 18))
}


type DisjointDiscriminants = { p1: 'left'; p2: true; p3: number } | { p1: 'right'; p2: false; p4: string } | { p1: 'left'; p2: boolean };
>DisjointDiscriminants : Symbol(DisjointDiscriminants, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 31, 1))
>p1 : Symbol(p1, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 30))
>p2 : Symbol(p2, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 42))
>p3 : Symbol(p3, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 52))
>p1 : Symbol(p1, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 69))
>p2 : Symbol(p2, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 82))
>p4 : Symbol(p4, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 93))
>p1 : Symbol(p1, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 110))
>p2 : Symbol(p2, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 34, 122))

// This has excess error because variant three is the only applicable case.
const a: DisjointDiscriminants = {
>a : Symbol(a, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 37, 5))
>DisjointDiscriminants : Symbol(DisjointDiscriminants, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 31, 1))

p1: 'left',
>p1 : Symbol(p1, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 37, 34))

p2: false,
>p2 : Symbol(p2, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 38, 15))

p3: 42,
>p3 : Symbol(p3, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 39, 14))

p4: "hello"
>p4 : Symbol(p4, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 40, 11))

};

// This has no excess error because variant one and three are both applicable.
const b: DisjointDiscriminants = {
>b : Symbol(b, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 45, 5))
>DisjointDiscriminants : Symbol(DisjointDiscriminants, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 31, 1))

p1: 'left',
>p1 : Symbol(p1, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 45, 34))

p2: true,
>p2 : Symbol(p2, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 46, 15))

p3: 42,
>p3 : Symbol(p3, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 47, 13))

p4: "hello"
>p4 : Symbol(p4, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 48, 11))

};

// This has excess error because variant two is the only applicable case
const c: DisjointDiscriminants = {
>c : Symbol(c, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 53, 5))
>DisjointDiscriminants : Symbol(DisjointDiscriminants, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 31, 1))

p1: 'right',
>p1 : Symbol(p1, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 53, 34))

p2: false,
>p2 : Symbol(p2, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 54, 16))

p3: 42,
>p3 : Symbol(p3, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 55, 14))

p4: "hello"
>p4 : Symbol(p4, Decl(excessPropertyCheckWithMultipleDiscriminants.ts, 56, 11))

};

Loading