Skip to content

Error message improvements for unions with identical discriminants #42598

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

Closed
Closed
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
33 changes: 24 additions & 9 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18988,9 +18988,15 @@ namespace ts {
findMostOverlappyType(source, target);
}

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) {
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue?: undefined, skipPartial?: boolean, allowMultiples?: boolean): Type | undefined;
function discriminateTypeByDiscriminableItems(target: UnionType, discriminators: [() => Type, __String][], related: (source: Type, target: Type) => boolean | Ternary, defaultValue: Type, skipPartial?: boolean, allowMultiples?: boolean): Type;
function discriminateTypeByDiscriminableItems(
target: UnionType,
discriminators: [() => Type, __String][],
related: (source: Type, target: Type) => boolean | Ternary,
defaultValue?: Type,
skipPartial?: boolean,
allowMultiples?: 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)[];
Expand All @@ -19015,13 +19021,16 @@ namespace ts {
if (match === -1) {
return defaultValue;
}

// make sure exactly 1 matches before returning it
let nextMatch = discriminable.indexOf(/*searchElement*/ true, match + 1);
while (nextMatch !== -1) {
if (!isTypeIdenticalTo(target.types[match], target.types[nextMatch])) {
return defaultValue;
if (!allowMultiples) {
let nextMatch = discriminable.indexOf(/*searchElement*/ true, match + 1);
while (nextMatch !== -1) {
if (!isTypeIdenticalTo(target.types[match], target.types[nextMatch])) {
return defaultValue;
}
nextMatch = discriminable.indexOf(/*searchElement*/ true, nextMatch + 1);
}
nextMatch = discriminable.indexOf(/*searchElement*/ true, nextMatch + 1);
}
return target.types[match];
}
Expand Down Expand Up @@ -41037,7 +41046,13 @@ namespace ts {
if (sourceProperties) {
const sourcePropertiesFiltered = findDiscriminantProperties(sourceProperties, target);
if (sourcePropertiesFiltered) {
return discriminateTypeByDiscriminableItems(<UnionType>target, map(sourcePropertiesFiltered, p => ([() => getTypeOfSymbol(p), p.escapedName] as [() => Type, __String])), isRelatedTo, /*defaultValue*/ undefined, skipPartial);
return discriminateTypeByDiscriminableItems(
<UnionType>target,
map(sourcePropertiesFiltered, p => [() => getTypeOfSymbol(p), p.escapedName]),
isRelatedTo,
/*defaultValue*/ undefined,
skipPartial,
/**/ true);
}
}
}
Expand Down
130 changes: 130 additions & 0 deletions tests/baselines/reference/contextualTypeShouldBeLiteral.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
tests/cases/compiler/contextualTypeShouldBeLiteral.ts(40,5): error TS2345: Argument of type '{ type2: "y"; value: "done"; method(): void; }' is not assignable to parameter of type 'X2 | Y2'.
Object literal may only specify known properties, but 'type2' does not exist in type 'X2'. Did you mean to write 'type1'?


==== tests/cases/compiler/contextualTypeShouldBeLiteral.ts (1 errors) ====
interface X {
type: 'x';
value: string;
method(): void;
}

interface Y {
type: 'y';
value: 'none' | 'done';
method(): void;
}

function foo(bar: X | Y) { }

foo({
type: 'y',
value: 'done',
method() {
this;
this.type;
this.value;
}
});

interface X2 {
type1: 'x';
value: string;
method(): void;
}

interface Y2 {
type2: 'y';
value: 'none' | 'done';
method(): void;
}

function foo2(bar: X2 | Y2) { }

foo2({
type2: 'y',
~~~~~~~~~~
!!! error TS2345: Argument of type '{ type2: "y"; value: "done"; method(): void; }' is not assignable to parameter of type 'X2 | Y2'.
!!! error TS2345: Object literal may only specify known properties, but 'type2' does not exist in type 'X2'. Did you mean to write 'type1'?
value: 'done',
method() {
this;
this.value;
}
});
Comment on lines +44 to +54
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new error - I'd argue this is actually a regression.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, isn’t this perfectly assignable to Y2?


interface X3 {
type: 'x';
value: 1 | 2 | 3;
xtra: number;
}

interface Y3 {
type: 'y';
value: 11 | 12 | 13;
ytra: number;
}

let xy: X3 | Y3 = {
type: 'y',
value: 11,
ytra: 12
};

xy;


interface LikeA {
x: 'x';
y: 'y';
value: string;
method(): void;
}

interface LikeB {
x: 'xx';
y: 'yy';
value: number;
method(): void;
}

let xyz: LikeA | LikeB = {
x: 'x',
y: 'y',
value: "foo",
method() {
this;
this.x;
this.y;
this.value;
}
};

xyz;

// Repro from #29168

interface TestObject {
type?: 'object';
items: {
[k: string]: TestGeneric;
};
}

interface TestString {
type: 'string';
}

type TestGeneric = (TestString | TestObject) & { [k: string]: any; };

const test: TestGeneric = {
items: {
hello: { type: 'string' },
world: {
items: {
nested: { type: 'string' }
}
}
}
};

Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts(30,5): erro
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(50,5): error TS2322: Type '{ p1: "left"; p2: true; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'.
Object literal may only specify known properties, and 'p4' does not exist in type '{ p1: "left"; p2: true; p3: number; }'.
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) ====
==== tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts (4 errors) ====
// Repro from #32657

interface Base<T> {
Expand Down Expand Up @@ -63,6 +65,9 @@ tests/cases/compiler/excessPropertyCheckWithMultipleDiscriminants.ts(57,5): erro
p2: true,
p3: 42,
p4: "hello"
~~~~~~~~~~~
!!! error TS2322: Type '{ p1: "left"; p2: true; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'.
!!! error TS2322: Object literal may only specify known properties, and 'p4' does not exist in type '{ p1: "left"; p2: true; p3: number; }'.
Comment on lines +68 to +70
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is definitely an improvement.

};

// This has excess error because variant two is the only applicable case
Expand Down
45 changes: 24 additions & 21 deletions tests/baselines/reference/excessPropertyCheckWithUnions.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@ tests/cases/compiler/excessPropertyCheckWithUnions.ts(11,21): error TS2322: Type
Object literal may only specify known properties, and 'd20' does not exist in type '{ tag: "A"; a1: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(12,1): error TS2322: Type '{ tag: "D"; }' is not assignable to type 'ADT'.
Property 'd20' is missing in type '{ tag: "D"; }' but required in type '{ tag: "D"; d20: 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(29,19): error TS2322: Type '{ tag: "A"; y: number; }' is not assignable to type 'Ambiguous'.
Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(30,28): error TS2322: Type '{ tag: "A"; x: string; y: number; }' is not assignable to type 'Ambiguous'.
Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(33,28): error TS2322: Type '{ tag: "A"; x: string; extra: number; }' is not assignable to type 'Ambiguous'.
Object literal may only specify known properties, and 'extra' does not exist in type 'Ambiguous'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(34,26): error TS2322: Type '{ tag: "A"; y: number; extra: number; }' is not assignable to type 'Ambiguous'.
Object literal may only specify known properties, and 'extra' does not exist in type 'Ambiguous'.
Object literal may only specify known properties, and 'extra' does not exist in type '{ tag: "A"; x: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(34,19): error TS2322: Type '{ tag: "A"; y: number; extra: number; }' is not assignable to type 'Ambiguous'.
Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(39,1): error TS2322: Type '{ tag: "A"; }' is not assignable to type 'Ambiguous'.
Type '{ tag: "A"; }' is not assignable to type '{ tag: "C"; }'.
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: "B"; z: boolean; }'.
Types of property 'tag' are incompatible.
Type '"A"' is not assignable to type '"B"'.
Property 'x' is missing in type '{ tag: "A"; }' but required in type '{ tag: "A"; x: string; }'.
tests/cases/compiler/excessPropertyCheckWithUnions.ts(40,19): error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type 'Ambiguous'.
Object literal may only specify known properties, and 'z' does not exist in type '{ tag: "A"; x: string; }'.
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 All @@ -31,7 +31,7 @@ tests/cases/compiler/excessPropertyCheckWithUnions.ts(113,67): error TS2322: Typ
tests/cases/compiler/excessPropertyCheckWithUnions.ts(114,63): error TS2322: Type 'string' is not assignable to type 'number'.


==== tests/cases/compiler/excessPropertyCheckWithUnions.ts (14 errors) ====
==== tests/cases/compiler/excessPropertyCheckWithUnions.ts (16 errors) ====
type ADT = {
tag: "A",
a1: string
Expand Down Expand Up @@ -71,33 +71,36 @@ tests/cases/compiler/excessPropertyCheckWithUnions.ts(114,63): error TS2322: Typ
// no error for ambiguous tag, even when it could satisfy both constituents at once
amb = { tag: "A", x: "hi" }
amb = { tag: "A", y: 12 }
~~~~~
!!! error TS2322: Type '{ tag: "A"; y: number; }' is not assignable to type 'Ambiguous'.
!!! error TS2322: Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'.
amb = { tag: "A", x: "hi", y: 12 }
~~~~~
!!! error TS2322: Type '{ tag: "A"; x: string; y: number; }' is not assignable to type 'Ambiguous'.
!!! error TS2322: Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'.
Comment on lines 73 to +80
Copy link
Member Author

Choose a reason for hiding this comment

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

These are...ambiguous! Unclear if they should error.

Copy link
Member

@andrewbranch andrewbranch Feb 3, 2021

Choose a reason for hiding this comment

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

Wait, why should they be errors? Particularly the previous error on amb = { tag: "A", y: 12 } seems super wrong. But this one too seems like excess property checking is a little overzealous. For non-discriminated unions of objects, excess property checking only complains about properties non present in any constituent. Here, I would think the same rule would apply after we have filtered out the B and C constituents. So you should be allowed to supply x: string and y: number, but not z: boolean.


// correctly error on excess property 'extra', even when ambiguous
amb = { tag: "A", x: "hi", extra: 12 }
~~~~~~~~~
!!! error TS2322: Type '{ tag: "A"; x: string; extra: number; }' is not assignable to type 'Ambiguous'.
!!! error TS2322: Object literal may only specify known properties, and 'extra' does not exist in type 'Ambiguous'.
!!! error TS2322: Object literal may only specify known properties, and 'extra' does not exist in type '{ tag: "A"; x: string; }'.
amb = { tag: "A", y: 12, extra: 12 }
~~~~~~~~~
~~~~~
Comment on lines -82 to +88
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

!!! error TS2322: Type '{ tag: "A"; y: number; extra: number; }' is not assignable to type 'Ambiguous'.
!!! error TS2322: Object literal may only specify known properties, and 'extra' does not exist in type 'Ambiguous'.
!!! error TS2322: Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'.

// assignability errors still work.
// But note that the error for `z: true` is the fallback one of reporting on
// the last constituent since assignability error reporting can't find a single best discriminant either.
amb = { tag: "A" }
~~~
!!! error TS2322: Type '{ tag: "A"; }' is not assignable to type 'Ambiguous'.
!!! error TS2322: Type '{ tag: "A"; }' is not assignable to type '{ tag: "C"; }'.
!!! error TS2322: Types of property 'tag' are incompatible.
!!! error TS2322: Type '"A"' is not assignable to type '"C"'.
!!! error TS2322: Property 'x' is missing in type '{ tag: "A"; }' but required in type '{ tag: "A"; x: string; }'.
!!! related TS2728 tests/cases/compiler/excessPropertyCheckWithUnions.ts:16:5: 'x' is declared here.
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: "B"; z: boolean; }'.
!!! error TS2322: Types of property 'tag' are incompatible.
!!! error TS2322: Type '"A"' is not assignable to type '"B"'.
!!! error TS2322: Object literal may only specify known properties, and 'z' does not exist in type '{ tag: "A"; x: string; }'.
Comment on lines 100 to +103
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is sort of an improvement - ideally, we'd be able to say that z was in another branch.


type Overlapping =
| { a: 1, b: 1, first: string }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
tests/cases/conformance/expressions/objectLiterals/objectLiteralNormalization.ts(7,14): error TS2322: Type 'number' is not assignable to type 'string | undefined'.
tests/cases/conformance/expressions/objectLiterals/objectLiteralNormalization.ts(8,1): error TS2322: Type '{ b: string; }' is not assignable to type '{ a: number; b?: undefined; c?: undefined; } | { a: number; b: string; c?: undefined; } | { a: number; b: string; c: boolean; }'.
Type '{ b: string; }' is missing the following properties from type '{ a: number; b: string; c: boolean; }': a, c
Property 'a' is missing in type '{ b: string; }' but required in type '{ a: number; b: string; c?: undefined; }'.
tests/cases/conformance/expressions/objectLiterals/objectLiteralNormalization.ts(9,1): error TS2322: Type '{ c: true; }' is not assignable to type '{ a: number; b?: undefined; c?: undefined; } | { a: number; b: string; c?: undefined; } | { a: number; b: string; c: boolean; }'.
Type '{ c: true; }' is missing the following properties from type '{ a: number; b: string; c: boolean; }': a, b
tests/cases/conformance/expressions/objectLiterals/objectLiteralNormalization.ts(17,1): error TS2322: Type '{ a: string; b: number; }' is not assignable to type '{ a: number; b: number; } | { a: string; b?: undefined; } | { a?: undefined; b?: undefined; }'.
Expand All @@ -27,7 +27,8 @@ tests/cases/conformance/expressions/objectLiterals/objectLiteralNormalization.ts
a1 = { b: "y" }; // Error
~~
!!! error TS2322: Type '{ b: string; }' is not assignable to type '{ a: number; b?: undefined; c?: undefined; } | { a: number; b: string; c?: undefined; } | { a: number; b: string; c: boolean; }'.
!!! error TS2322: Type '{ b: string; }' is missing the following properties from type '{ a: number; b: string; c: boolean; }': a, c
!!! error TS2322: Property 'a' is missing in type '{ b: string; }' but required in type '{ a: number; b: string; c?: undefined; }'.
!!! related TS2728 tests/cases/conformance/expressions/objectLiterals/objectLiteralNormalization.ts:2:23: 'a' is declared here.
a1 = { c: true }; // Error
~~
!!! error TS2322: Type '{ c: true; }' is not assignable to type '{ a: number; b?: undefined; c?: undefined; } | { a: number; b: string; c?: undefined; } | { a: number; b: string; c: boolean; }'.
Expand Down