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 for Mapped Union Types Fails #19897

Closed
fongandrew opened this issue Nov 10, 2017 · 6 comments
Closed

Type Inference for Mapped Union Types Fails #19897

fongandrew opened this issue Nov 10, 2017 · 6 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@fongandrew
Copy link

TypeScript Version: 2.6.1 / 2.7.0-dev20171109

Code

type MapToNum<T> = {
  [K in keyof T]?: number|Array<number>;
};

function test<T>(m: MapToNum<T>, k: keyof MapToNum<T>) {
  let y = m[k];
  let z: number[] = [];
  if (y instanceof Array) {
    // Works, but y has type any[], should be number[]
    z = z.concat(y);
  } else if (y) {
    // Error, y has type number|number[], should just be number
    z.push(y);
  }
}

Expected behavior: instanceof Array should narrow number|number[]|undefined to number[] and the else if (y) should narrow further to number.

Actual behavior: Narrowed to any[] and number|number[] respectively.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 10, 2017

m has a generic type, and the compiler can not grantee in the false branch that it is not an Array compatible type, that is not an instance of Array. consider writing the example above as;

    if (typeof y === "number") {
        z.push(y);
    }
    else { 
        z = z.concat(y);
    }

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 10, 2017
@fongandrew
Copy link
Author

I don't think I understand. m has a generic type but y should not. Why isn't y just number|number[]|undefined?

@kpdonn
Copy link
Contributor

kpdonn commented Nov 10, 2017

@fongandrew There are two different things named Array in your code. One is the Array interface at

type MapToNum<T> = {
  [K in keyof T]?: number|Array<number>;
};

and the other is the javascript Array constructor at

  if (y instanceof Array) {

If you are using an IDE and use go to declaration on both of those Array you should notice it takes you to different declarations. The Array interface exists only in typescript and can be implemented implicitly by anything that has the correct methods and index signature. The Array constructor is a real object in javascript and exists at runtime.

Most arrays you use in javascript are instances of the Array constructor but not all. One example I know of where you could have something that implements the Array interface but is not a real instance of Array are observed arrays in MobX.

Due to limitations of native arrays in ES5 observable.array will create a faux-array (array-like object) instead of a real array. In practice, these arrays work just as fine as native arrays and all native methods are supported, including index assignments, up-to and including the length of the array.
Bear in mind however that Array.isArray(observable([])) will yield false.

instanceof Array checks whether a value was created by the Array constructor, not whether it implements the Array interface(because the Array interface doesn't exist at runtime). So if you were using MobX and had a MobX observable array go through your function, y instanceof Array would actually return false despite implementing the Array interface and typescript just caught a real bug by giving you an error.

@kpdonn
Copy link
Contributor

kpdonn commented Nov 10, 2017

@mhegazy Although confusingly everything I said apparently explicitly doesn't work if the things aren't generic so maybe this isn't working as intended?

Compare:

function test1(y: number | number[]) {
  let z: number[] = [];
  if (y instanceof Array) {
    z = z.concat(y);
  } else {
    z.push(y) // no error
  }
}

function test2<T extends number | number[]>(y: T) {
  let z: number[] = [];
  if (y instanceof Array) {
    z = z.concat(y);
  } else {
    z.push(y) // error
  }
}

See #1719 #17344 #18498 for explanations why there's no error in non-generic case. I'm not a fan of typescript doing the narrowing in the else block since the MobX array thing is a common real world problem but it looks like that has already been considered and decided the other way so maybe it should be consistent between the generic and non-generic cases?

@fongandrew
Copy link
Author

fongandrew commented Nov 11, 2017

@kpdonn test2 actually sort of makes sense to me. In that case, you're working with a generic that extends an Array-like type, which signals "this has the shape of an Array but is not necessarily an Array". But in my case, the generic only implicates the key that we access the array under, not the array type itself.

That is, it's this inconsistency that I don't get.

This works.

type Generic<T> = {
  numOrArray: number | number[];
  other: T;
};

function test<T>(g: Generic<T>) {
  let y = g.numOrArray;
  let z: number[] = [];
  if (y instanceof Array) {
    z = z.concat(y);
  } else {
    z.push(y); // Works
  }
}

But this doesn't.

type Generic<T extends string> = {
  [key in T]: number | number[];
};

function test2<T extends string>(g: Generic<T>, key: T) {
  let y = g[key];
  let z: number[] = [];
  if (y instanceof Array) {
    z = z.concat(y);
  } else {
    z.push(y); // Errors
  }
}

@typescript-bot
Copy link
Collaborator

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

@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

4 participants