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

Union types could be reduced to their 'minimum valid' definitions #16644

Closed
kujon opened this issue Jun 20, 2017 · 14 comments
Closed

Union types could be reduced to their 'minimum valid' definitions #16644

kujon opened this issue Jun 20, 2017 · 14 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@kujon
Copy link
Contributor

kujon commented Jun 20, 2017

TypeScript Version: 2.4.0

Code

// The following works no problem
type ExhaustiveType = Array<number | undefined>;

const test1 = (value: ExhaustiveType) => value.filter(v => v === 2);

/** Cannot invoke an expression whose type lacks a call signature. Type '{ (callbackfn: (this: void, value: number | undefined, index: number, array: (number | undefined)...' has no compatible call signatures. */
type ExcessiveType = number[] | Array<number | undefined>;

const test2 = (value: ExcessiveType) => value.filter(v => v === 2);

Expected behavior:
Both examples compile.

Actual behavior:
The latter example fails with Cannot invoke an expression whose type lacks a call signature. Type '{ (callbackfn: (this: void, value: number | undefined, index: number, array: (number | undefined)...' has no compatible call signatures.

Explanation
ExhaustiveType and ExcessiveType both represent exactly the same types. The latter is more explicit, and while in this particular example it might look silly, w/ more complex types and type aliases, being more explicit allows for more expressivity. IMHO TypeScript could at the time of defining ExcessiveType, 'reduce it down' to the ExhaustiveType. Alternatively (to preserve expressiveness e.g. in the IDE), the compiler could internally treat them as equivalent.

@michaeljota
Copy link

michaeljota commented Jun 20, 2017

I'm sorry, I don't work here or anything, just wanted to test your code, cause it seems to me like Array<number | undefined> !== number[].

So I test it here

However, I can't reproduce that issue you mention. It compiles both types as number[] (witch for me it's an unexpected behavior cause you explicit said that could be undefined).

Moreover, if you set strictNullCheck the error you said are getting threw, so that's a necessary thing to know.

Also, if you use intersection type instead, it works as you expected. Maybe that's what you wanted to do?

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Jun 20, 2017
@RyanCavanaugh
Copy link
Member

The reduction of union types to a constituent that is a common supertype already happens. Perhaps something is missing in your example?

@kujon
Copy link
Contributor Author

kujon commented Jun 20, 2017

@RyanCavanaugh - Typescript is treating both of the types in my example differently (one's blowing up, another one doesn't). So it seems like I've managed to find some edge case maybe?

@jcalz
Copy link
Contributor

jcalz commented Jun 20, 2017

Since Array<number | undefined> is not the same as number[], then isn't this issue an instance of #1805?
(If OP switches to number[] to (number | undefined)[] then the union is reduced and everything works.)

@kujon
Copy link
Contributor Author

kujon commented Jun 20, 2017

@jcalz - sure, but number[] | Array<number | undefined> is the same type as Array<number | undefined>

@jcalz
Copy link
Contributor

jcalz commented Jun 20, 2017

No, not with strictNullChecks it isn't:

var x: number[] | Array<number | undefined>;
var x: Array<number | undefined>; // error: 
// Subsequent variable declarations must have the same type.  
// Variable 'x' must be of type '(number | undefined)[] | number[]', 
// but here has type '(number | undefined)[]'.

EDIT: wait, maybe I see where you're going with this.

Each value of type number[] | (number | undefined)[] should be a value of type (number | undefined)[] and vice versa, right? So the type checker isn't making that inference here.

@jcalz
Copy link
Contributor

jcalz commented Jun 20, 2017

Okay, so you're saying that number[] is a subtype of (number | undefined)[] and that the union should absorb the subtype. I don't know that subtype collapsing is actually happening (I put this in #16386 but it's kind of languishing there). Anyway, yeah, I think I get your point now @kujon.

@RyanCavanaugh

The reduction of union types to a constituent that is a common supertype already happens.

Are you sure of this?

@kujon
Copy link
Contributor Author

kujon commented Jun 20, 2017

@jcalz - precisely this. (number | undefined)[] is a super type of number[] so by definition, super_type union sub_type == super_type

@michaeljota
Copy link

michaeljota commented Jun 20, 2017

This error only happens if you set strictNullCheck, so I don't think this is an error at all, cause it is checking that (number | undefined) !== number because they are not the same if you are checking for null and undefined types.

EDIT: Also, in this environment, (number | undefined) are not a super type of number cause number are strictly treated as such, and can't be undefined nor null.

However, this works as expected if the compiler is not in strict mode.

Example with primitives only

Toggle strictNullCheck to see the error and the union typing changes

@jcalz
Copy link
Contributor

jcalz commented Jun 20, 2017

@michaeljota no, this has nothing to do with undefined. Let's try a different one:

var k = [1, 2, 3];
(k as number[]).filter(x => typeof x === 'string'); // fine
(k as (number | string)[]).filter(x => typeof x === 'string'); // fine
(k as number[] | (number | string)[]).filter(x => typeof x === 'string'); // error!

The typechecker is not absorbing number[] into (number | string)[], even though all values of type number[] are also values of type (number | string)[].

EDIT: however, arrays are mutable... so therefore number[] might not be a subtype of (number | string)[] if you intend to mutate anything. I think ts does some unsound bivariant things with parameters because of this. Blecch.

@kujon
Copy link
Contributor Author

kujon commented Jun 20, 2017

however, arrays are mutable... so therefore number[] might not be a subtype of (number | string)[] if you intend to mutate anything.

If I understand the last message correctly, that would also mean that number[] is not number[] anymore, right?

@jcalz
Copy link
Contributor

jcalz commented Jun 20, 2017

Maybe? Since typescript is perfectly fine assigning a value of type number[] to a variable of type (number|string)[] but not vice versa, then I guess there is a subtype relationship there, even though you can do horrible things like

var a: number[] = [1,2,3];
var b: (number | string)[] = a;
b[0] = 'boom';
console.log(a[0]);

So forget my previous edit, I guess.

@masaeedu
Copy link
Contributor

masaeedu commented Jun 23, 2017

This is somewhat irrelevant to the issue at hand, but technically number[] is not a subtype of (number | undefined)[], because the latter supports inserting undefined members and the former doesn't. Array<T> is not covariant wrt T, although something like ReadOnlyArray<T> should be.

@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
@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
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

6 participants