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

Type inference in switch statements based on class token #17397

Closed
ovangle opened this issue Jul 25, 2017 · 11 comments
Closed

Type inference in switch statements based on class token #17397

ovangle opened this issue Jul 25, 2017 · 11 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@ovangle
Copy link

ovangle commented Jul 25, 2017

TypeScript Version: 2.4.2

Code

Some common interfaces used in code examples below:

interface Type<T> {
  new (...args: any[]): T;
}

interface Action {
  type: string | Type<any>;
}

Typescript infers the correct type to use in a case statement, if the switch is on a readonly property of the object and the value is const. e.g.

export const SET_LOGIN_USER = 'set login user';
export class SetLoginUser implements Action {
  readonly type = SET_LOGIN_USER;
  constructor(public user: number) {}
}

export const SET_LOGIN_ERRORS = 'set login errors';
export class SetLoginErrors implements Action {
  readonly type = SET_LOGIN_ERRORS;
  constructor(public errors: any) {}
}

export type Actions = SetLoginUser | SetLoginErrors;

export function run(action: Actions): void {
  switch (action.type) {
    case SET_LOGIN_USER:
      // No type error is printed here, the type is inferred as `LoginUser`
      console.log(action.user);
      break;
  }
}

Expected behavior:

Since typescript is able to infer the type based on the value of a string constant, I'd expect it to infer the type based on the type token as well,

Actual behaviour:

However, it fails with a TS2339 error. A class/function definition is effectively const, isn't it? So why shouldn't it work in exactly the same way as a explicitly typed const value?

export class SetLoginUser implements Action {
  readonly type = SetLoginUser;
  constructor(public user: number) {}
}

export function run(action: Actions): void {
  switch (action.type) {
    case SetLoginUser:
      // Fails with TS2339: Property 'user' does not exist on type 'Actions'
      action.user;
      break;
  }
}
@ovangle ovangle changed the title Restriction of type in if statement Type restriction in switch statement Jul 25, 2017
@ovangle ovangle changed the title Type restriction in switch statement Type inference in switch statements based on class token Jul 25, 2017
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 25, 2017
@RyanCavanaugh
Copy link
Member

Type discriminants can only be unit types - types for which there is exactly one value. For example, 3 is a unit type because there's only one 3, and we know that for all values x and y of type 3, x === y.

For rather obvious reasons, it's only safe to discriminate on the basis of unit types. For example, this declaration is legal and completely breaks the switch (in terms of substitutability) when a SetLoginUser reference aliases a SetLoginUser2 :

export class SetLoginUser2 extends SetLoginUser {
  readonly type = SetLoginUser2;
  constructor(public user: number) { super(user) }
}

The class constructor function (or any function for that matter) is not a unit type. There may be multiple constructor functions that return the same class (sounds weird but it is true) so we don't consider the switch here to be a valid narrowing.

@ovangle
Copy link
Author

ovangle commented Jul 26, 2017

completely breaks the switch (in terms of substitutability)

But using the string keys violates liskov substutability too doesn't it? So that's not a very good reason.

export class SetLoginUser2 extends SetLoginUser {
   readonly type = "SET_LOGIN_USER_2"
   constructor(public user: not_a_number) {}
}

switch (action.type) {
   case "SET_LOGIN_USER":
      // The fact that `SetLoginUser` is inferred here means
      // `action.type === "SET_LOGIN_USER"` is a provable fact about SetLoginUser 
      // and thus SetLoginUser2
   
      // So can this be a SetLoginUser2?
      let user = action.user;
   }
}

I actually came across this recently when I asserted that I didn't think that this inference could be made (and webstorm 2016.? agreed with me coincidentally) but was shot down by a developer in another issue against another repo when I was shown that webstorm was buggy and that this was the typescript behaviour.

Type discriminants can only be unit types - types for which there is exactly one value

Now I agree that a function is not a unit type, but a module, function pair is a unit type, isn't it? There can only be one 'module_x.foo' object? That if I could express the idea

interface Type<T> {
   <module_x>;
   new (...args): T;
}

Then that type, would be a unit type? But it's impossible to express "I belong to this module" in the type system (nor would I think you want to), so Type<T> is the closest approximation to that idea we can express, and that it's relatively unambiguous what it is supposed to represent.

Angular uses this pattern everywhere in their DI framework and as far as I know it hasn't caused any real ambiguity (sure, there's theoretical ambiguity due to new Function and eval, but there are no practical problems for users).

@ovangle
Copy link
Author

ovangle commented Jul 26, 2017

Sorry, no I'm 100% wrong as usual.

I didn't check whether the first example was actually valid typescript before posting -- but now that I understand that the language prevents overriding a "unit typed" readonly declaration in a subclass your example became a whole lot more straight forward. Yes, working as intended.

You guys think about this stuff far more than I do.

@ovangle
Copy link
Author

ovangle commented Jul 26, 2017

Wait, so if it was possible to declare a final/sealed class in typescript, then this would this be possible?

Obviously I'm not saying "add sealed classes" or anything, just wondering...

@mhegazy
Copy link
Contributor

mhegazy commented Aug 17, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Aug 17, 2017
@ovangle
Copy link
Author

ovangle commented Aug 20, 2017

@mhegazy @RyanCavanaugh

Could you at least answer my question about "final" types? I understand why this issue was closed. I also understand that final types are unlikely to ever appear in typescript because any class can be "implemented" by an arbitrary object, so I'm not going to go open a new issue if you reply "yes, that's right". It's merely for my own edification.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 21, 2017

Final classes are #8306, which is currently tagged as Won't Fix.

@ovangle
Copy link
Author

ovangle commented Aug 23, 2017

@kitsonk

Yeah, that doesn't answer the question I asked about them at all. I'm not interested in seeing them implemented, I just wanted to know that if they were implemented, would it mean you'd be able to treat a "final" class as a unit type.

Or, since apparently you can implement them using a private constructor (according to #8306), does that mean that types with a private constructor can be used as unit types and therefore it's possible to apply the inference I originally suggested to them?

But thanks I guess...

@kitsonk
Copy link
Contributor

kitsonk commented Aug 23, 2017

I just wanted to know that if they were implemented, would it mean you'd be able to treat a "final" class as a unit type

That is a "how long is a piece of string question"... You seem to be asking, if something that is not currently planned to be implemented was implemented how would it work.

@ovangle
Copy link
Author

ovangle commented Aug 23, 2017

Basically why I'm asking is that we use this pattern a lot in our angular code base, and it's never caused us problems (although we enforce a pretty strict "interface, don't subclass" policy).

I've got a pending issue in our issue tracker which says we should change any occurrences of this because typescript team thinks it's stupid, but I wanted to know whether it could be changed in order to make it more "correct". I don't think we'll be making all our constructors private any time soon (lol), but since we treat every class as implicitly final, I just want to know that we're not causing ourselves problems down the road.

The type inference would just be a convenience, we're fine with typecasting in the case statements.

ps. I don't think it's a "how long is a piece of string question", what I'm asking is that if our internal coding style prohibits using the "extends" keyword then can we safely assume that the inference I described above holds (even if we have to manually cast the result), or is the pattern broken for some other reason?

@vladimiry
Copy link

@ovangle did you try to use constructor based switching https://github.com/ngrx/example-app/issues/110#issuecomment-326866631 (switch (action.constructor) {)? Looks like it will allow you to get rid of the readonly type stuff.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants