Skip to content

Disallow partial matches for discriminant properties when generating error messages #37589

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 10 additions & 6 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16882,20 +16882,24 @@ namespace ts {
}

function getBestMatchingType(source: Type, target: UnionOrIntersectionType, isRelatedTo = compareTypesAssignable) {
return findMatchingDiscriminantType(source, target, isRelatedTo) ||
return findMatchingDiscriminantType(source, target, isRelatedTo, /*skipPartial*/ true) ||
findMatchingTypeReferenceOrTypeAliasReference(source, target) ||
findBestTypeForObjectLiteral(source, target) ||
findBestTypeForInvokable(source, target) ||
findMostOverlappyType(source, target);
}

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) {
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue?: undefined, skipPartial?: boolean): Type | undefined;
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue: Type, skipPartial?: boolean): Type;
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue?: Type, skipPartial?: boolean) {
// undefined=unknown, true=discriminated, false=not discriminated
// The state of each type progresses from left to right. Discriminated types stop at 'true'.
const discriminable = target.types.map(_ => undefined) as (boolean | undefined)[];
for (const [getDiscriminatingType, propertyName] of discriminators) {
const targetProp = getUnionOrIntersectionProperty(target, propertyName);
if (skipPartial && targetProp && getCheckFlags(targetProp) & CheckFlags.ReadPartial) {
continue;
}
let i = 0;
for (const type of target.types) {
const targetType = getTypeOfPropertyOfType(type, propertyName);
Expand Down Expand Up @@ -37852,13 +37856,13 @@ namespace ts {
}

// Keep this up-to-date with the same logic within `getApparentTypeOfContextualType`, since they should behave similarly
function findMatchingDiscriminantType(source: Type, target: Type, isRelatedTo: (source: Type, target: Type) => Ternary) {
function findMatchingDiscriminantType(source: Type, target: Type, isRelatedTo: (source: Type, target: Type) => Ternary, skipPartial?: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

When do you not want to skipPartial discriminants?

Copy link
Member Author

@weswigham weswigham Mar 27, 2020

Choose a reason for hiding this comment

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

So we report excess property errors on something like

type Group = {a: 1, b: 1, field: string, data: string}; | {a: 2, field: string} | {b: 2, data: string};
const x: Group = {a: 1, b: 1, field: "ok", data: "ok", excess: "not ok"};

a and b are both partial discriminants, but taken together, can discriminate to the first member. We have a test expecting this.

if (target.flags & TypeFlags.Union && source.flags & (TypeFlags.Intersection | TypeFlags.Object)) {
const sourceProperties = getPropertiesOfType(source);
if (sourceProperties) {
const sourcePropertiesFiltered = findDiscriminantProperties(sourceProperties, target);
if (sourcePropertiesFiltered) {
return discriminateTypeByDiscriminableItems(<UnionType>target, map(sourcePropertiesFiltered, p => ([() => getTypeOfSymbol(p), p.escapedName] as [() => Type, __String])), isRelatedTo);
return discriminateTypeByDiscriminableItems(<UnionType>target, map(sourcePropertiesFiltered, p => ([() => getTypeOfSymbol(p), p.escapedName] as [() => Type, __String])), isRelatedTo, /*defaultValue*/ undefined, skipPartial);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
tests/cases/compiler/unionErrorMessageOnMatchingDiscriminant.ts(21,5): error TS2322: Type 'null' is not assignable to type '{ a: string; }'.


==== tests/cases/compiler/unionErrorMessageOnMatchingDiscriminant.ts (1 errors) ====
type A = {
type: 'a',
data: { a: string }
};

type B = {
type: 'b',
data: null
};

type C = {
type: 'c',
payload: string
};

type Union = A | B | C;

// error
const foo: Union = {
type: 'a',
data: null
~~~~
!!! error TS2322: Type 'null' is not assignable to type '{ a: string; }'.
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//// [unionErrorMessageOnMatchingDiscriminant.ts]
type A = {
type: 'a',
data: { a: string }
};

type B = {
type: 'b',
data: null
};

type C = {
type: 'c',
payload: string
};

type Union = A | B | C;

// error
const foo: Union = {
type: 'a',
data: null
};

//// [unionErrorMessageOnMatchingDiscriminant.js]
"use strict";
// error
var foo = {
type: 'a',
data: null
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
=== tests/cases/compiler/unionErrorMessageOnMatchingDiscriminant.ts ===
type A = {
>A : Symbol(A, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 0, 0))

type: 'a',
>type : Symbol(type, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 0, 10))

data: { a: string }
>data : Symbol(data, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 1, 14))
>a : Symbol(a, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 2, 11))

};

type B = {
>B : Symbol(B, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 3, 2))

type: 'b',
>type : Symbol(type, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 5, 10))

data: null
>data : Symbol(data, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 6, 14))

};

type C = {
>C : Symbol(C, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 8, 2))

type: 'c',
>type : Symbol(type, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 10, 10))

payload: string
>payload : Symbol(payload, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 11, 14))

};

type Union = A | B | C;
>Union : Symbol(Union, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 13, 2))
>A : Symbol(A, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 0, 0))
>B : Symbol(B, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 3, 2))
>C : Symbol(C, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 8, 2))

// error
const foo: Union = {
>foo : Symbol(foo, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 18, 5))
>Union : Symbol(Union, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 13, 2))

type: 'a',
>type : Symbol(type, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 18, 20))

data: null
>data : Symbol(data, Decl(unionErrorMessageOnMatchingDiscriminant.ts, 19, 14))

};
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
=== tests/cases/compiler/unionErrorMessageOnMatchingDiscriminant.ts ===
type A = {
>A : A

type: 'a',
>type : "a"

data: { a: string }
>data : { a: string; }
>a : string

};

type B = {
>B : B

type: 'b',
>type : "b"

data: null
>data : null
>null : null

};

type C = {
>C : C

type: 'c',
>type : "c"

payload: string
>payload : string

};

type Union = A | B | C;
>Union : Union

// error
const foo: Union = {
>foo : Union
>{ type: 'a', data: null} : { type: "a"; data: null; }

type: 'a',
>type : "a"
>'a' : "a"

data: null
>data : null
>null : null

};
23 changes: 23 additions & 0 deletions tests/cases/compiler/unionErrorMessageOnMatchingDiscriminant.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// @strict: true
type A = {
type: 'a',
data: { a: string }
};

type B = {
type: 'b',
data: null
};

type C = {
type: 'c',
payload: string
};

type Union = A | B | C;

// error
const foo: Union = {
type: 'a',
data: null
};