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

Regression in discriminated unions when a common property may be boolean #32657

Closed
janruo opened this issue Aug 1, 2019 · 3 comments · Fixed by #32755
Closed

Regression in discriminated unions when a common property may be boolean #32657

janruo opened this issue Aug 1, 2019 · 3 comments · Fixed by #32755
Labels
Milestone

Comments

@janruo
Copy link

janruo commented Aug 1, 2019

TypeScript Version:
3.6.0-dev.20190801

Search Terms:
discriminated boolean, tagged boolean

Code

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,
    format: "what?"
}

Expected behavior:
The code should not compile, because properties multipleOf and format don't exist in type Float.

Actual behavior:
The code compiles without errors. Typescript version 3.1.6 is the last version which behaves as expected and prints the error message.

Removing the Bool interface from the union, or changing the type parameter on the Bool interface to any other type causes expected behavior.

Not using inheritance and defining the value property in each interface separately does not change the behavior.

Playground Link:
LINK

Related Issues: -

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Aug 1, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 1, 2019
@fatcerberus
Copy link

Looks like it's not specific to boolean but rather some kind of interaction with literal types (note: boolean is exactly true | false). Changing boolean to e.g. 1 | 2 or "foo" | "bar" also silences the excess property error, but number | string works correctly.

@jack-williams
Copy link
Collaborator

jack-williams commented Aug 5, 2019

This was probably introduced by #27695.

Excess property checking has special logic for discriminated unions to give better results when properties are part of the overall union, but not part of the variant you are using.

The issue here is that both type and value are flagged a discriminant properties---the latter because of the Bool variant. If EPC detects two discriminant properties in the source object that discriminate to different types then the logic I described above is skipped. The result is that you fail to get an error because multipleOf is not an excess property of the union Primitive.

I believe the correct fix would be to relax the condition in discriminateTypeByDiscriminableItems such that you are not required to discriminate to identical (identical here being pointer equality) types. A proposed fix would allow multiple matches if, after taking the intersection of matches across all discriminant properties, there was a single and therefore unambiguous match.

The property type would discriminate to Float, and the value property would discriminate to both Int and Float. As Float appears in all matches, and is the only type that appears in all matches, it would be the discriminated variant.

@jack-williams
Copy link
Collaborator

Fix up at #32711.

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