Skip to content

Commit

Permalink
Improved type argument inference with union types
Browse files Browse the repository at this point in the history
  • Loading branch information
@ahejlsberg
ahejlsberg committed Oct 8, 2014
1 parent fd5b808 commit 3a17b02
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 1 deletion.
35 changes: 34 additions & 1 deletion src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3720,6 +3720,7 @@ module ts {
for (var i = 0; i < typeParameters.length; i++) inferences.push([]);
return {
typeParameters: typeParameters,
inferenceCount: 0,
inferences: inferences,
inferredTypes: new Array(typeParameters.length),
};
Expand Down Expand Up @@ -3757,6 +3758,7 @@ module ts {
var typeParameters = context.typeParameters;
for (var i = 0; i < typeParameters.length; i++) {
if (target === typeParameters[i]) {
context.inferenceCount++;
var inferences = context.inferences[i];
if (!contains(inferences, source)) inferences.push(source);
break;
Expand All @@ -3771,6 +3773,35 @@ module ts {
inferFromTypes(sourceTypes[i], targetTypes[i]);
}
}
else if (target.flags & TypeFlags.Union) {
// Target is a union type

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Oct 11, 2014

Contributor

I would remove this comment
var targetTypes = (<UnionType>target).types;
var startCount = context.inferenceCount;
var typeParameterCount = 0;
var typeParameter: TypeParameter;
// First infer to each type in union that isn't a type parameter
for (var i = 0; i < targetTypes.length; i++) {
var t = targetTypes[i];
if (t.flags & TypeFlags.TypeParameter && contains(context.typeParameters, t)) {
typeParameter = <TypeParameter>t;
typeParameterCount++;
}
else {
inferFromTypes(source, t);

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Oct 11, 2014

Contributor

I thought we were going to infer to the parts that are common among all the constituents, not to each constituent individually.

Take this example:

interface Foo<T> {
    x: T;
}
function f<T, U>(x: T, y: U, z: Foo<T | U>): U { return null }
f("", 0, { x: "" }); // returns number

This would work just fine, because by the time we relate "" to T | U, we have two naked type parameters, and no extra inferences are made from this third argument. But this doesn't work out if we change it to:

interface Foo<T> {
    x: T;
}
function f<T, U>(x: T, y: U, z: Foo<T> | Foo<U>): U { return null }
f("", 0, { x: "" }); // returns number

Now the third parameter is a union of generic type references, and we will infer to each one individually. U will have two disjoint candidates (string and number) and we will get an error here.

This comment has been minimized.

Sign in to view

Copy link
@ahejlsberg

ahejlsberg Oct 11, 2014

Author Member

The common scenario is that a union type is a set of disjoint types. If they weren't disjoint it wouldn't be possible to tell them apart at run-time, so it wouldn't reflect a real world situation. Because the types are disjoint they inherently share little or nothing, so inferring to the common parts wouldn't yield meaningful results. Instead, the better thing to do is to infer each of them such that we get a full set of inferences from the one that matches (and presumably nothing from the others because they're disjoint). For example:

declare function f<T>(x: T | { (): T } | T[]): T;
var s = f("hello");
var s = f(() => "hello");
var s = f(["hello", "world"]);

The function and array types have no common members so inferring to the common parts would yield nothing. Instead, by leveraging the fact that they're disjoint, we can make good inferences.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Oct 11, 2014

Contributor

That makes sense when the constituents have no members in common. But in my example of Foo<T> | Foo<U>, we do end up with a member in common (namely x) of type T | U. And when we infer to both Foo<T> and Foo<U>, we end up inferring a type to both T and U, poisoning whichever one was "the wrong one".

Now that I wrote that, I am looking at your example again and am convinced by it. There doesn't seem anything to do to make your example work, other than what we're doing. In my example, I think we would tell users that if they want to infer to a union type where different type parameters are mentioned in different constituents, try to condense this to a form where the type parameters themselves are unioned. That will then give us the clue not to infer to either of them.
}
}
// If no inferences were produced above and union contains a single naked type parameter, infer to that type parameter
if (context.inferenceCount === startCount && typeParameterCount === 1) {
inferFromTypes(source, typeParameter);
}

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Oct 11, 2014

Contributor

I like this idea.
}
else if (source.flags & TypeFlags.Union) {
// Source is a union type, infer from each consituent type
var sourceTypes = (<UnionType>source).types;
for (var i = 0; i < sourceTypes.length; i++) {
inferFromTypes(sourceTypes[i], target);
}
}
else if (source.flags & TypeFlags.ObjectType && (target.flags & (TypeFlags.Reference | TypeFlags.Tuple) ||
(target.flags & TypeFlags.Anonymous) && target.symbol && target.symbol.flags & (SymbolFlags.Method | SymbolFlags.TypeLiteral))) {
// If source is an object type, and target is a type reference, a tuple type, the type of a method, or a type literal, infer from members
Expand Down Expand Up @@ -5169,7 +5200,9 @@ module ts {

// Try to return the best common type if we have any return expressions.
if (types.length > 0) {
var commonType = getCommonSupertype(types);
// When return statements are contextually typed we allow the return type to be a union type. Otherwise we require the
// return expressions to have a best common supertype.
var commonType = getContextualSignature(func) ? getUnionType(types) : getCommonSupertype(types);
if (!commonType) {
error(func, Diagnostics.No_best_common_type_exists_among_return_expressions);

Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,7 @@ module ts {

export interface InferenceContext {
typeParameters: TypeParameter[];
inferenceCount: number;

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Oct 11, 2014

Contributor

Add a comment explaining why this is different from the sum of inferences[i].length for all i

This comment has been minimized.

Sign in to view

Copy link
@ahejlsberg

ahejlsberg Oct 11, 2014

Author Member

It isn't, but it is easier to access than having to sum up all the lengths each time.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Oct 11, 2014

Contributor

It is different. In the case where you are inferring a type for T that you already have, you do not push the inference again:

if (!contains(inferences, source)) inferences.push(source);

But you do increment the counter. I think that is the right thing to do, I'm just saying it's not equivalent.
inferences: Type[][];
inferredTypes: Type[];
}
Expand Down

0 comments on commit 3a17b02

Please sign in to comment.