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

No excess property check for value with valid discriminated union type #35890

Closed
bela53 opened this issue Dec 28, 2019 · 3 comments
Closed

No excess property check for value with valid discriminated union type #35890

bela53 opened this issue Dec 28, 2019 · 3 comments
Assignees
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@bela53
Copy link

bela53 commented Dec 28, 2019

TypeScript Version: 3.7.2, Nightly

Search Terms: discriminated union, discriminant, excess property check, literal type, singleton type

Code

type S1 = { d1: "foo"; }
type S2 = { d1: "bar"; isSuccess: true; }
type S3 = { d1: "baz"; isSuccess: false; errorMessage: string; }

type State = S1 | S2 | S3;

const testState: State = {
  d1: "foo",
  isSuccess: true, // no error, why no excess property check here?
  errorMessage: "Error!" // no error, why no excess property check here?
}

Expected behavior:
After having set d1: "foo", TS should recognize that object literal testState belongs to S1 and emit an error when trying to add excess properties isSuccess and/or errorMessage. There should be no other discriminant candidate than d1, because only d1 exists in all three states.

Actual behavior:
There is no compile error.

Playground Link:
here

Related Issues:
Stackoverflow question

#32657 (maybe)
It kinda seems, isSuccess: boolean is interpreted as a second discriminant candidate. You can also substitute isSuccess: true/isSuccess: false with isSuccess: 1/isSuccess: 0 and neither get an error. An error is only raised, when isSuccess is changed to a non-literal type like string. But isSuccess shouldn't be treated as discriminant in the first place!

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jan 14, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.9.0 milestone Jan 14, 2020
@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label May 20, 2020
@RyanCavanaugh RyanCavanaugh removed the Rescheduled This issue was previously scheduled to an earlier milestone label Jul 15, 2020
@sandersn sandersn added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Oct 12, 2020
@sandersn
Copy link
Member

sandersn commented Oct 12, 2020

There's no way for Typescript to know that isSuccess shouldn't be a discriminant; it doesn't have a way to mark only one field as discriminant, so it uses any field that has unit types. Given the two discriminants d1 and isSuccess in State, the object literal matches both S1 and S2, so it doesn't give any excess property errors.

@bela53
Copy link
Author

bela53 commented Oct 19, 2020

@sandersn to understand a bit better: I would have assumed, TS only forms valid discriminants for common union properties, that exist on all union constituents. isSuccess for me is not a common union property, as it does not exist on type S1.

At least, this is how I have interpreted the docs:

Common properties of unions are now considered discriminants as long as they contain some singleton type (e.g. a string literal, null, or undefined), and they contain no generics.

Is a common property considered to be a property, that exists on two or more union constituents (not on all)?

@sandersn
Copy link
Member

Excess property checks have different, more heuristic rules than other handling of discriminant properties because the check has to compare the source to a single target of the union. In order to avoid false errors, the heuristics treat any discriminant of a subset as a discriminant of the entire union.

Basically, we only want to give excess property errors that are correct, so the rules unavoidably miss some possible errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants