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

Unions without discriminant properties do not perform excess property checking #20863

Open
cazgp opened this issue Dec 22, 2017 · 35 comments
Open
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@cazgp
Copy link

cazgp commented Dec 22, 2017

TypeScript Version: 2.7.0-dev.201xxxxx

Code

// An object to hold all the possible options
type AllOptions = {
    startDate: Date
    endDate: Date
    author: number
}

// Any combination of startDate, endDate can be used
type DateOptions =
    | Pick<AllOptions, 'startDate'>
    | Pick<AllOptions, 'endDate'>
    | Pick<AllOptions, 'startDate' | 'endDate'>

type AuthorOptions = Pick<AllOptions, 'author'>

type AllowedOptions = DateOptions | AuthorOptions

const options: AllowedOptions = {
    startDate: new Date(),
    author: 1
}

Expected behavior:

An error that options cannot be coerced into type AllowedOptions because startDate and author cannot appear in the same object.

Actual behavior:

It compiles fine.

@DanielRosenwasser DanielRosenwasser changed the title Intersection of Pick types does not seem to properly work out types Union of Pick types does not perform excess property checking Dec 22, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 22, 2017

Technically speaking, the expression is a subtype of almost all the types of AllowedOptions, but we usually perform excess property checking. However, that excess property checking isn't occurring here.

I thought this was related to #12745 but it seems to still be present.

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Dec 22, 2017
@DanielRosenwasser
Copy link
Member

Though according to responses to #12745 (comment), this might be working as intended.

@DanielRosenwasser DanielRosenwasser added this to the TypeScript 2.8 milestone Dec 22, 2017
@sylvanaar
Copy link

sylvanaar commented Dec 26, 2017

Isn't this due to contextual typing merging all the fields. Why was this behavior chosen?

When used as a contextual type (section 4.23), a union type has those members that are present in any of its constituent types, with types that are unions of the respective members in the constituent types. Specifically, a union type used as a contextual type has the apparent members defined in section 3.11.1, except that a particular member need only be present in one or more constituent types instead of all constituent types.

@sandersn
Copy link
Member

sandersn commented Jan 2, 2018

Contextual typing is not relevant here. Neither is Pick. Here's a smaller repro that avoids contextual typing:

type A = { d: Date, e: Date } | { n: number }
const o = { d: new Date(), n: 1 }
const a: A = o

Excess property checking of unions does not work because all it does is try to find a single member of the union to check for excess properties. It does this based on discriminant properties, but A doesn't have any overlapping properties, so it can't possibly have any discriminant properties. (The same is true of the original AllowedOptions.) So excess property checking is just skipped. I chose this design because it was conservative enough to avoid bogus errors, if I remember correctly.

To get excess property checks for unions without discriminant properties, we could just choose the first union member that has at least one property shared with the source, but that would behave badly with the original example:

const options: AllowedOptions = {
  startDate: new Date(),
  endDate: new Date()
}

Here, we'd choose Pick<AllOptions, 'startDate'> as the first matching union member and then mark endDate as an excess property.

@sylvanaar @DanielRosenwasser Let me know if you have ideas for some other way to get excess property checks for unions without discriminant properties.

@sandersn sandersn changed the title Union of Pick types does not perform excess property checking Unions without discriminant properties do not perform excess property checking Jan 2, 2018
@pelotom
Copy link

pelotom commented Feb 23, 2018

Is this the same issue as #22129? My repro:

export interface A {
  x?: string;
}
export interface B extends A {
  y?: number;
}
export interface C extends A {
  z?: boolean;
}

const passes: B | C = { x: 'hello', z: 42 };

const fails: B | C = { z: 42 };

@jack-williams
Copy link
Collaborator

@sandersn

Let me know if you have ideas for some other way to get excess property checks for unions without discriminant properties.

Is it possible to sort the union type first by sub-typing, then choose the first matching member?

@sandersn
Copy link
Member

@pelotom No, excess property checking only considers property names, not types.

@jack-williams That would probably work. Here are some problems that might arise:

  1. It would be expensive. Admittedly, the current check is expensive, but pays off (at least for the compiler code base) because it discards many members of large unions early on in assignability checking.
  2. Would a sort on subtyping be stable? I think subtyping is only a partial order. The first member of union might be surprising.
  3. The rule is pretty complex, so we wouldn't want people to have to think about it. That means it should work every time, and every time the same way.

(1) is pretty easy to check given a prototype. Figuring out (2) is a precondition for (3). Working through some examples would help here, to show that the results are not surprising to humans.

@jack-williams
Copy link
Collaborator

@sandersn

Yeah I believe the ordering is only partial, so even if the sort were stable, you'd still have to deal with unrelated elements appear somewhere. Bringing in subtyping seems quite a heavyweight approach given that excess property checking only looks at property names--I don't think that my suggestion was that good, on reflection.

I'm wondering whether a solution based on set inclusion of property names would be easier to predict and implement. For instance, use bloom filters to prune union elements that definitely do not contain properties in the initialising object, then use more expensive checks to resolve the remaining cases that might contain all the specified properties. That might involve straight enumeration, or shortcuts like: if a type in the union has no optional properties, then it must have the same bloom filter as the initialiser.

@ghost
Copy link

ghost commented Apr 12, 2018

Another example:

declare function f(options: number | { a: { b: number } }): void;
f({ a: { b: 0, c: 1 } });

@mhegazy
Copy link
Contributor

mhegazy commented Apr 12, 2018

@sandersn the example in #20863 (comment) is excess property checking. When comparing { x: 'hello', z: 42 } to B | C, { x: 'hello', z: 42 } is not assignable to C since z is a number and not a boolean. { x: 'hello', z: 42 } should not be assign bale to B since { x: 'hello', z: 42 } has an unknown property z. the later is what is not happening now.

@jack-williams
Copy link
Collaborator

jack-williams commented Apr 26, 2018

@andy-ms I believe your example is essentially the same problem as #13813 (but with union instead). I don't know how to fix it for the general case, but perhaps a special case could be made for unions with a single object type?

@mrvisser
Copy link

mrvisser commented Feb 27, 2021

Built-in type guards pretend that TS has excess property checking on unions, when really it doesn't.

type Step =
  | { name: string }
  | {
      name: string;
      start: Date;
      data: Record<string, unknown>;
    };

const step: Step = {
  name: 'initializing',
  start: new Date(),
};

if ('start' in step) {
  console.log(step.data.thing);
}

EDIT: I was fooled by comment-compression into thinking the type-guard case was not mentioned. Apologies for the repetition.

@Gruzchick
Copy link

@AnyhowStep @pirix-gh I always use this type to expand out other types, not necessarily better just another option:

export type Compute<A extends any> = {} & { // I usually call it Id
  [P in keyof A]: A[P]
}

Also I tend to use it sparingly, if the union constituents are named, the expanded version is arguably worse IMO.

@dragomirtitian can you explain what in this example gives the intersection with an empty object?

@nebkat
Copy link
Contributor

nebkat commented Dec 11, 2022

Similar behavior here:

export interface Parent {
    type: "foo" | "bar" | "baz";
}

export interface Bar {
    type: "bar";
    bar?: number;
}

export interface Baz {
    type: "baz";
    baz?: number;
}

export type Union = Parent | Bar | Baz;

function test(union: Union) {}

test({
    type: "foo"
});
test({
    type: "foo",
    bar: 1 // ✅ TS(2345): Object literal may only specify known properties, and 'bar' does not exist in type 'Parent'.
});
test({
    type: "bar",
    bar: 1
});
test({
    type: "bar",
    bar: 1,
    baz: 1 // ❌ No error?
});
test({
    type: "bar",
    bar: 1,
    baz: "1" // ❓ TS(2232): Type 'string' is not assignable to type 'number'.
});
test({
    type: "bar",
    bar: 1,
    baz: 1, // ❌ No error?
    unlisted: 1 // ✅ TS(2345): Object literal may only specify known properties, and 'unlisted' does not exist in type 'Union'.
});

TS Playground

Seems as soon as more than 1 element in the union matches, the entire union becomes an intersection?

Motivation is to have properties that can be defined for all types, and some that are only definable for specific types:

const options ColumnOptions {
    type: "varchar"
}

const options: ColumnOptions {
    type: "timestamp",
    onUpdateCurrentTimestamp: true // acceptable because timestamp type
}

const options: ColumnOptions {
    type: "int"
    onUpdateCurrentTimestamp: true // should error but doesnt
}

@htunnicliff
Copy link

htunnicliff commented Oct 17, 2023

I think this might be another example:

type A = {
  onlyA: boolean;
  optionalA?: boolean;
};

type B = {
  onlyB: boolean;
  optionalB?: boolean;
};

type C = {
  onlyC: boolean;
  optionalC?: boolean;
};

type Choices = ["A", A] | ["B", B] | ["C", C];

function fn<T extends Choices>(choice: T) {
  return choice;
}

fn(["A", { onlyA: true, onlyB: true }]); // Should error but doesn't
fn(["B", { onlyB: true, optionalC: true }]); // Should error but doesn't

const a: A = {
  onlyA: true,
  onlyB: true, // Errors as expected
};

const b: B = {
  onlyB: true,
  optionalC: true, // Errors as expected
};

You can also see in this screencapture that TypeScript suggests properties and combinations that shouldn't be allowed:

CleanShot.2023-10-17.at.08.55.27.mp4

@RobertSandiford
Copy link

RobertSandiford commented Aug 28, 2024

@sandersn

Excess property checking of unions does not work because all it does is try to find a single member of the union to check for excess properties. It does this based on discriminant properties, but A doesn't have any overlapping properties, so it can't possibly have any discriminant properties. (The same is true of the original AllowedOptions.) So excess property checking is just skipped. I chose this design because it was conservative enough to avoid bogus errors, if I remember correctly.

@sylvanaar @DanielRosenwasser Let me know if you have ideas for some other way to get excess property checks for unions without discriminant properties.

me @ #42384

I looked at this and figured that when checking compatibility the checker could look for matching union members, then apply the Excess Property Check to each matching member, ruling out any that fail [edit: stopping at the first success], or throw an EPC error if they all fail. But it doesn't seem to.

Is this avoided for performance reasons, or do I not understand something here?

@davidbarratt
Copy link

👋 I'm coming here from #59819 to say that this issue really tripped me up and I was rather surprised that it's been this way since forever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests