Skip to content

Too-permissive assignability with complex discriminated unions #34751

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
Harpush opened this issue Oct 26, 2019 · 5 comments
Closed

Too-permissive assignability with complex discriminated unions #34751

Harpush opened this issue Oct 26, 2019 · 5 comments
Assignees
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@Harpush
Copy link

Harpush commented Oct 26, 2019

I was trying to add type safety to some model i have and it seems to sometimes work in a weird way.

Code

export enum FilterType {
  String = 'string',
  Number = 'number',
  Boolean = 'boolean'
}

export interface FilterTypeToLiteralTypeMap {
  [FilterType.String]: string;
  [FilterType.Number]: number;
  [FilterType.Boolean]: boolean;
}

export type AllowedFilterTypeLiteralTypes = FilterTypeToLiteralTypeMap[FilterType];

export type Values<T> = T[keyof T];

export type FilterTypeFromLiteralType<
  T extends AllowedFilterTypeLiteralTypes
> = Exclude<
  Values<
    {
      [key in FilterType]: FilterTypeToLiteralTypeMap[key] extends T
        ? key
        : never;
    }
  >,
  never
>;

export interface Filterable<T extends AllowedFilterTypeLiteralTypes> {
  isFilterable: true;
  filterType: FilterTypeFromLiteralType<T>;
}

export interface NotFilterable {
  isFilterable: false;
}

export type FilterableTrait<T extends AllowedFilterTypeLiteralTypes> =
  | Filterable<T>
  | NotFilterable;

What i did here is created an enum and mapped each entry to a literal type. That way when using the FilterableTrait interface it can be either Filterable or NotFilterable and if filterable the FilterType is decided based on the given type.

Some usages that work

// Errors correctly
export const a: FilterableTrait<string> = {
  isFilterable: true,
  filterType: FilterType.Number
};

// Works correctly
export const b: FilterableTrait<string> = {
  isFilterable: true,
  filterType: FilterType.String
};

export const c: FilterableTrait<string> = {
  isFilterable: false,
  filterType: FilterType.String // Errors correctly
};

// Errors correctly
export const d: FilterableTrait<string> | FilterableTrait<number> = {
  isFilterable: true,
  filterType: FilterType.Boolean
};

// Works correctly
export const e: FilterableTrait<string> | FilterableTrait<number> = {
  isFilterable: true,
  filterType: FilterType.Number
};

The problem starts when i do this

export const f: FilterableTrait<string> | FilterableTrait<number> = {
  isFilterable: false,
  filterType: FilterType.Number // No error? Auto complete doesn't add this as an option but it compiles
};

A few notes here:

  1. If i change Filterable to be filterType: FilterTypeFromLiteralType<string> instead of filterType: FilterTypeFromLiteralType<T> it works... it doesnt work just when i use the generic type.
  2. I know i can use
export const g: FilterableTrait<string | number> = {
  isFilterable: false,
  filterType: FilterType.Number
};

and it will work but my actual use case requires me to use something like

export const h:
  | ({ type: '1' } & FilterableTrait<string>)
  | ({ type: '2' } & FilterableTrait<number>) = {
  type: '2',
  isFilterable: true,
  filterType: FilterType.Number
};

Which means i can't use the above

Expected behavior:
Compilation error

Actual behavior:
Compiles successfully

Playground Link: http://www.typescriptlang.org/play/?ts=3.7-Beta&ssl=31&ssc=3&pln=17&pc=1#

@joshuafairchild1
Copy link

joshuafairchild1 commented Oct 27, 2019

Would something like the following work for your use case?

// Note the I removed the usage of `Exclude` from this type, as it wasn't actually doing anything,
// `never` type is always excluded from union types
export type FilterTypeFromLiteralType<
  T extends AllowedFilterTypeLiteralTypes
> = Values<
  {
    [K in FilterType]: FilterTypeToLiteralTypeMap[K] extends T
    ? K
    : never
  }
>

interface TypedFilter<T extends AllowedFilterTypeLiteralTypes> {
  filterType: FilterTypeFromLiteralType<T>
}

type FilterableTrait<T extends AllowedFilterTypeLiteralTypes, B extends boolean = true> =
  { isFilterable: B } extends infer F
    ? B extends true
      ? (F & TypedFilter<T>)
      : F
    : never

Using this new definition, we get the following (expected) errors when assigning a value to a union of FilterableTraits:

const f: FilterableTrait<string> | FilterableTrait<number> = {
  isFilterable: false, // Error: Type 'false' is not assignable to type 'true'
  filterType: FilterType.Number
}

const f0: FilterableTrait<string, false> | FilterableTrait<number, false> = {
  isFilterable: false,
  filterType: FilterType.Number // Error: 'filterType' does not exist in type '{ isFilterable: false }'
}

// No errors
const f1: FilterableTrait<string, false> | FilterableTrait<number, false> = {
  isFilterable: false
}

This solution does have a large difference with the original implementation, in that whether or not a FilterableTrait should actually have a filterType associated with it is driven by the second generic passed to FilterableTrait.

@Harpush
Copy link
Author

Harpush commented Oct 29, 2019

@joshuafairchild1 Well not really... The idea is when writing i want a "double discriminate union" - which means that given this:

export const h:
  | ({ type: '1' } & FilterableTrait<string>)
  | ({ type: '2' } & FilterableTrait<number>) = {
  type: '2',
  isFilterable: true,
  filterType: FilterType.Number
};

It should be able to discriminate the FilterableTrait based on the type and then discriminate based on isFilterable if i can add filterType or not.
In what you wrote FilterableTrait is no more a discriminate union and i need to be explicit in the types about whether its filterable or not and not through the isFilterable value assignment.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Oct 30, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.8.0 milestone Oct 30, 2019
@sandersn sandersn changed the title Weird behavior with complex discriminated unions Too-permissive assignability with complex discriminated unions Jan 27, 2020
@sandersn
Copy link
Member

I think this example gets the essence of the problem without relying on too many other features. Let me know what you think.

export const ff:
    | { isFilterable: true, filterType: "string" }
    | { isFilterable: true, filterType: "number" }
    | { isFilterable: false } = {
  isFilterable: false,
  filterType: "number" // No error? Auto complete doesn't add this as an option but it compiles
};

This seems similar to other union assignability problems we've had. I'll look around for prior art.

@Harpush
Copy link
Author

Harpush commented Jan 28, 2020

@sandersn Seems like the same problem... One thing to note (maybe not relevant as i dont know how the internal compiler works) is that in my case it uses another generic interface and it seems to mess up the union and intersection relation. Anyway considering what you wrote is the real issue underneath - please make sure generics like in my example will work too.
One more thing is that in my case it is a bit more complicated as i have the boolean as first discriminate which affects the second discriminate and both of them affect another property - whether it can exist and with which type

@sandersn
Copy link
Member

First, notice that, without excess property checking, { isFilterable: false, filterType: "number" } is actually assignable to { isFilterable: false }. So we rely on excess property checking to reject this assignment.

Unfortunately, this doesn't work because both isFilterable and filterType are discriminable properties; true | false and "string" | "number" respectively. We would like the compiler to choose just isFilterable, but the excess property check doesn't check the powerset of properties to find the best discriminant set. Instead it uses a quadratic algorithm I developed with @weswigham and @jack-williams. This algorithm requires a property to be present in all constituents of the target union AND for its type to be assignable. Neither isFilterable nor filterType succeed: isFilterable: false isn't assignable to the first two constituents, and filterType isn't present in the third constituent.

I don't think it's possible to do better without taking exponential time here.

A workaround is to add filterType to the last constituent so that isFilterable succeeds as a discriminant and excess property checking works. This works in your original example with NotFilterable too.

@sandersn sandersn added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Needs Investigation This issue needs a team member to investigate its status. labels Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants