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

Improved union/intersection type inference #5738

Merged
merged 4 commits into from Nov 24, 2015

Conversation

Projects
None yet
8 participants
@ahejlsberg
Member

ahejlsberg commented Nov 20, 2015

This PR improves type inference involving source and target sides that are both union or intersection types. When inferring from a type S to a type T, if S and T are both union types or both intersection types, we first reduce S and T by removing constituents that are matched by a constituent in the other type. For example, when inferring from string | string[] to string | T, we reduce the types to string[] and T, thus inferring string[] for T.

An example:

type Maybe<T> = T | void;

function isDefined<T>(x: Maybe<T>): x is T {
    return x !== undefined && x !== null;
}

function isUndefined<T>(x: Maybe<T>): x is void {
    return x === undefined || x === null;
}

function getOrElse<T>(x: Maybe<T>, defaultValue: T): T {
    return isDefined(x) ? x : defaultValue;
}

function test1(x: Maybe<string>) {
    let x1 = getOrElse(x, "Undefined");         // string
    let x2 = isDefined(x) ? x : "Undefined";    // string
    let x3 = isUndefined(x) ? "Undefined" : x;  // string
}

function test2(x: Maybe<number>) {
    let x1 = getOrElse(x, -1);         // number
    let x2 = isDefined(x) ? x : -1;    // number
    let x3 = isUndefined(x) ? -1 : x;  // number
}

Fixes #2264.
Fixes #4212.
Fixes #5417.
Fixes #5456.

@@ -6244,6 +6254,41 @@ namespace ts {
}
}
function typeIdenticalToSomeType(source: Type, target: UnionOrIntersectionType): boolean {

This comment has been minimized.

@sandersn

sandersn Nov 20, 2015

Member

How about

return forEach(target.types, t => isTypeIdenticalTo(source, t) || undefined);
@sandersn

sandersn Nov 20, 2015

Member

How about

return forEach(target.types, t => isTypeIdenticalTo(source, t) || undefined);

This comment has been minimized.

@ahejlsberg

ahejlsberg Nov 24, 2015

Member

Definitely an option, but not quite as efficient. I will keep what's there.

@ahejlsberg

ahejlsberg Nov 24, 2015

Member

Definitely an option, but not quite as efficient. I will keep what's there.

sourceTypes = sourceTypes.slice(0);
modified = true;
}
sourceTypes.splice(sourceIndex, 1);

This comment has been minimized.

@sandersn

sandersn Nov 20, 2015

Member

I don't think I understand the mutability discipline for the checker. It looks like splice modifies source.types. Is this OK? How do we know that nobody else refers to source? (even though inferFromTypes no longer does after the call to reduceUnionOrIntersection.)

@sandersn

sandersn Nov 20, 2015

Member

I don't think I understand the mutability discipline for the checker. It looks like splice modifies source.types. Is this OK? How do we know that nobody else refers to source? (even though inferFromTypes no longer does after the call to reduceUnionOrIntersection.)

This comment has been minimized.

@sandersn

sandersn Nov 20, 2015

Member

Never mind, I just saw the call to .slice(0) above.

@sandersn

sandersn Nov 20, 2015

Member

Never mind, I just saw the call to .slice(0) above.

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Nov 23, 2015

Member

Yeah, while it seems obvious now, it'd be worthwhile to add a quick comment so that people taking a cursory glance don't think they've found a bug.

@DanielRosenwasser

DanielRosenwasser Nov 23, 2015

Member

Yeah, while it seems obvious now, it'd be worthwhile to add a quick comment so that people taking a cursory glance don't think they've found a bug.

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Nov 20, 2015

Member

👍

Member

sandersn commented Nov 20, 2015

👍

}
}
if (modified) {
return source.flags & TypeFlags.Union ? getUnionType(sourceTypes, /*noSubtypeReduction*/ true) : getIntersectionType(sourceTypes);

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Nov 23, 2015

Member

Can you break this into multiple lines?

@DanielRosenwasser

DanielRosenwasser Nov 23, 2015

Member

Can you break this into multiple lines?

This comment has been minimized.

@ffMathy

ffMathy Dec 7, 2015

Use wordwrapping man.

@ffMathy

ffMathy Dec 7, 2015

Use wordwrapping man.

ahejlsberg added a commit that referenced this pull request Nov 24, 2015

@ahejlsberg ahejlsberg merged commit ea0cc79 into master Nov 24, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@RyanCavanaugh RyanCavanaugh deleted the unionIntersectionTypeInference branch Nov 24, 2015

@mariusschulz

This comment has been minimized.

Show comment
Hide comment
@mariusschulz

mariusschulz Nov 29, 2015

Contributor

@ahejlsberg You've got a little typo in your code example: isUndefined<T> should use or rather than and, shouldn't it? Just trying to avoid confusion here.

function isUndefined<T>(x: Maybe<T>): x is void {
    return x === undefined || x === null;
}
Contributor

mariusschulz commented Nov 29, 2015

@ahejlsberg You've got a little typo in your code example: isUndefined<T> should use or rather than and, shouldn't it? Just trying to avoid confusion here.

function isUndefined<T>(x: Maybe<T>): x is void {
    return x === undefined || x === null;
}
@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Nov 29, 2015

Member

@mariusschulz Yes indeed, thanks for catching that.

Member

ahejlsberg commented Nov 29, 2015

@mariusschulz Yes indeed, thanks for catching that.

@ENikS

This comment has been minimized.

Show comment
Hide comment
@ENikS

ENikS Dec 7, 2015

Will this address issue mentioned in #3215 ?

ENikS commented Dec 7, 2015

Will this address issue mentioned in #3215 ?

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Dec 8, 2015

Member

@ENikS No, the two are unrelated.

Member

ahejlsberg commented Dec 8, 2015

@ENikS No, the two are unrelated.

@masaeedu

This comment has been minimized.

Show comment
Hide comment
@masaeedu

masaeedu Feb 9, 2016

Contributor

@ahejlsberg Would inferring T more accurately in the following scenario be worthwhile:

interface A { a: any; }

function test<T>(arg: A & T): T { ... }

let result = test({a: "", b: 10});

The type of result is inferred as {}, although I would expect it to be inferred as {b: number}. Admittedly I don't have a very clear use case for this at the moment.

Contributor

masaeedu commented Feb 9, 2016

@ahejlsberg Would inferring T more accurately in the following scenario be worthwhile:

interface A { a: any; }

function test<T>(arg: A & T): T { ... }

let result = test({a: "", b: 10});

The type of result is inferred as {}, although I would expect it to be inferred as {b: number}. Admittedly I don't have a very clear use case for this at the moment.

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Feb 9, 2016

Member

@masaeedu We could potentially explore that, but it could get complex and would require type inference to manufacture new types, neither of which I'm crazy about. Specifically, in your example, we'd have to manufacture a new type { b: number } and infer that for T. I think it is desirable to have the property that type inference never infers types that don't already occur in your code.

Member

ahejlsberg commented Feb 9, 2016

@masaeedu We could potentially explore that, but it could get complex and would require type inference to manufacture new types, neither of which I'm crazy about. Specifically, in your example, we'd have to manufacture a new type { b: number } and infer that for T. I think it is desirable to have the property that type inference never infers types that don't already occur in your code.

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.