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

Discriminated Unions don't cast with single type #18056

Closed
mindblight opened this issue Aug 25, 2017 · 8 comments
Closed

Discriminated Unions don't cast with single type #18056

mindblight opened this issue Aug 25, 2017 · 8 comments
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@mindblight
Copy link

TypeScript Version: 2.3.4

Code

type Choices = {
  type: "A"
};

function chooser(action: Choices): boolean {
  switch (action.type) {
    case "A":
      return true;
    default:
      const exhaustivenessCheck: never = action;
      return false;
  }
}

Expected behavior:
Code compiles.

Actual behavior:
Receive Error:

test.ts(10,13): error TS2322: Type 'Choices' is not assignable to type 'never'.

Reasoning
For libraries like Redux, there are places where it makes sense to have a switch statement with a single case for Discriminated Unions. (e.g. in reducers with a single action)

@RyanCavanaugh
Copy link
Member

Less amenable to immediate editing, but you can write:

function chooser(action: Choices): boolean {
  const singularCheck: "A" = action.type;
  return true;
}

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Aug 25, 2017
@mindblight
Copy link
Author

mindblight commented Aug 28, 2017

That's slick :). Unfortunately, it doesn't work for the reducer pattern:

export type Actions = {
  type: "A"
};

export function reducer(state: any, action: Actions): any {
  switch (action.type) {
    case "A":
      return {};
    default:
      const exhaustivenessCheck: never = action;
      return state;
  }
}

Redux will pass every action in the system (more than just type: "A") to the reducer, and it's expected to ignore those by returning state. This abuses the type system a bit, but it's a really powerful pattern that works if Actions are a union.

More generally, what is the expected behaviour? It seems unintuitive that it const exhaustivenessCheck: never doesn't work for the singleton case.

@RyanCavanaugh
Copy link
Member

If there's no union type, then the code for narrowing unions doesn't kick in (IOW a type is not equivalent to a union type of one constituent, if that were a possible thing to construct).

I'd be somewhat concerned about how this behavior would behave with strictNullChecks off - it's possible in that world for type to be null/undefined and it would not be correct to make it never in the default block.

@mindblight
Copy link
Author

Responding in reverse
What are your concerns w/ type=null over type="Foo"? Technically neither of them should make it to never, but because of how Redux propagates actions that line of code will eventually be hit at runtime in the union example.

We (my team) thought that was probably what was happening :). I think the confusing part is that, although Discriminated Unions are a different concept, they feel like a natural extension of string literal type guards. So, with the following types:

type A = {
  type: "A"
  foo: number
};
type B = {
  type: "B"
  bar: number
};

I can comfortably do

function reducer(state: any, action: A): any {
  if (action.type === "A") {
    return { bar: action.foo };
  } else {
    return state;
  }
}

And,

function reducer(state: any, action: A | B): any {
  if (action.type === "A") {
    return { bar: action.foo };
  } else if (action.type === "B") {
    return { bar: action.bar };
  } else {
    const exhaustivenessCheck: never = action;
    return state;
  }
}

But not,

function reducer(state: any, action: A): any {
  if (action.type === "A") {
    return { bar: action.foo };
  } else {
    const exhaustivenessCheck: never = action;
    return state;
  }
}

@RyanCavanaugh
Copy link
Member

Regarding null, I just mean that if you wrote something like this (with strictNullChecks off) I think it'd be very confusing to issue an error like this:

type Thing = {
  length: 0;
  color: string;
}

function fn(x: Thing) {
  if (x.length !== 0) {
    // Error, cannot read 'color' of type 'never'
    console.log(`Cannot process thing with color ${x.color} because its length is wrong`);
  }
}

It's Real Bad ™ if people are regularly getting never-typed values which are actually observed at runtime. I think what we actually want is a subtraction type (#4183) for literals so you can say

type Action = { type: "A", payload: number } |
  { type: string - "A", payload: void };

Unrelated, does this work for you?

type Choices = {
  type: "A"
} | { type: never };

function chooser(action: Choices): boolean {
  switch (action.type) {
    case "A":
      return true;
    default:
      const exhaustivenessCheck: never = action.type;
      return false;
  }
}

@mindblight
Copy link
Author

Answering in reverse again
Yep, { type: never } works :). It definitely feels hacky, but it does give us better compile-time errors.

About substitution type; I think we still end up losing the exhaustivenessCheck. It's more correct, but we give up a very useful compile-time check.

@matshch
Copy link

matshch commented Sep 22, 2017

Reducer pattern works if we check never for action.type, not action:

interface MyAction {
  type: 'MY_ACTION';
}

type KnownAction = MyAction;

const reducer: Reducer<MyState> = (state: MyState, action: KnownAction) => {
  switch (action.type) {
    case 'MY_ACTION':
      return {};
    default:
      const exhaustivenessCheck: never = action.type;
  }

  return state || {};
}

@RyanCavanaugh RyanCavanaugh added Too Complex An issue which adding support for may be too complex for the value it adds and removed In Discussion Not yet reached consensus labels Oct 10, 2017
@RyanCavanaugh
Copy link
Member

We don't think the performance hit for trying to detect whether we should synthesize new types after every comparison is worth it relative to the difficulty of the workaround here. Non-union discriminated types (???) ought to be rather rare in practice.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests

3 participants