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

Adds support for type parameter defaults #13487

Merged
merged 23 commits into from
Feb 15, 2017
Merged

Adds support for type parameter defaults #13487

merged 23 commits into from
Feb 15, 2017

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Jan 14, 2017

This PR adds support for defaults for generic type parameters and replaces #6354 as it is fairly out of date.

Default types for generic type parameters have the following syntax:

TypeParameter :
  BindingIdentifier Constraint? DefaultType?

DefaultType :
  `=` Type

Default types have the following rules:

  • Required type parameters must not follow optional type parameters.
  • When the checker applies defaults for type parameters, they are applied from left to right:
    • If the default for a type parameter refers to a type parameter that occurs earlier in the list, the type argument for that parameter will be used.
    • If the default for a type parameter refers to a type parameter that occurs later in the list, the empty object type {} will be used.
    • NOTE: This helps to avoid the cost of a circularity check for defaults.
  • Default types for a type parameter must satisfy the constraint for the type parameter, if it exists.
  • A class or interface declaration that merges with an existing class or interface declaration may introduce a default for an existing type parameter.
  • A class or interface declaration that merges with an existing class or interface declaration may introduce a new type parameter as long as it specifies a default.
  • When resolving a symbol with the same name as a Type Parameter introduced in a merged declaration, the type parameter will not be considered and resolution will continue on to the next container.
  • When specifying type arguments, you are only required to specify type arguments for the required type parameters. Unspecified type parameters will resolve to their default types.
  • If a default type is not specified and inference cannot choose a candidate, the empty object type {} is inferred. This is the existing behvaior.
  • If a default type is specified and inference cannot chose a candidate, the default type is inferred.

This PR also updates the signature for Promise<T> and PromiseLike<T> to take full advantage of the new support for type parameter defaults.

Fixes #2175
Fixes #10977
Fixes #12725
Fixes #12886
Fixes #12910
Fixes #13008
Fixes #13015
Fixes #13117

@rbuckton
Copy link
Member Author

rbuckton commented Jan 14, 2017

@RyanCavanaugh: this is my take on refreshing the genericDefaults PR. It should resolve the open items you had on #6354 regarding checking defaults against constraints. I also added missing support for typeToString and the declaration emitter. Please let me know if you can suggest any additional tests.

@mhegazy: when we discussed this, I suggested that interface declarations that have defaults must have identical defaults, allowing you to mix declarations with and without defaults:

interface X<A, B> {}
interface X<A, B = number> {}

However, this PR does not reflect that suggestion and is more strict, requiring all declarations to have identical defaults as well. It seems more consistent and we can loosen this restriction at any point if we so choose.

@ahejlsberg: The getResolvedDefault function in checker.ts is somewhat similar to getResolvedBaseConstraint. However, I chose to reuse the existing pushTypeResolution/popTypeResolution functions to perform circularity checks. Is there a specific advantage for getResolvedBaseConstraint to have its own circularity logic? Would you recommend I use that approach for getResolvedDefault, or would it make sense to modify getResolvedBaseConstraint to use the type-resolution functions?

const types = (<UnionOrIntersectionType>type).types;
const defaultTypes = filter(map(types, getResolvedDefaultWorker), x => x !== circularConstraintOrDefaultType);
return type.flags & TypeFlags.Union && defaultTypes.length === types.length ? getUnionType(defaultTypes) :
type.flags & TypeFlags.Intersection && defaultTypes.length ? getIntersectionType(defaultTypes) :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing - so if there were any circular types in a union, you return undefined. But you disregard all circularities in an intersection type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reused this from line 4827, above. It does seem like it should also be performing a comparison against types.length as well, however I am unsure as to whether there was a specific reason for 4827 to ignore circularity in an intersection type.

Copy link
Member Author

@rbuckton rbuckton Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this more consistent in 25cb02e and I've added some additional circularity tests in ca16ba8.

typeArguments = [];
}
const mapper: TypeMapper = t => {
for (let i = 0; i < numTypeParameters; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You technically don't need to run through the initial type arguments that were already supplied, do you? Could you just start at numTypeArguments as you did below?

Copy link
Member Author

@rbuckton rbuckton Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if a type parameter has a default type that is a reference to another type parameter, then we need to map it to the other, possibly earlier, type parameter.

Otherwise you end up with this:

declare function f<T, U = T>();
f<number>() // becomes f<number, T>()

The T is invalid at the call site, so we replace it with the type number for the supplied type argument.

}
const mapper: TypeMapper = t => {
for (let i = 0; i < numTypeParameters; i++) {
if (t === typeParameters[i]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (t !== typeParameters[i]) {
    continue;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have restructured and simplified this in 25cb02e

@rbuckton
Copy link
Member Author

@mhegazy: when we discussed this, I suggested that interface declarations that have defaults must have identical defaults, allowing you to mix declarations with and without defaults...
...this PR does not reflect that suggestion and is more strict, requiring all declarations to have identical defaults as well. It seems more consistent and we can loosen this restriction at any point if we so choose.

The latest commit makes the following changes (also noted above):

  • If any declaration of an interface or class has a default type for a type parameter, all declarations must have an identical default type.
  • A class or interface declaration that merges with an existing class or interface declaration may introduce a default for an existing type parameter.
  • A class or interface declaration that merges with an existing class or interface declaration may introduce a new type parameter as long as it specifies a default.

For example:

interface X { } // no type parameters
interface X<T = never> { } // ok, add a new default type parameter

interface Y<T> { } // one required type parameter
interface Y<T = never> { } // ok, make an existing type parameter optional

interface Z { } // no type parameters
interface Z<T> { } // error, type parameter lists aren't identical

//// uncomment the following to remove the error as the declarations will merge:
// interface Z<T = never> { }

@rbuckton
Copy link
Member Author

@ahejlsberg Per our conversation in the design meeting, I've further modified chooseOverload to avoid additional inference overhead when all type arguments are supplied.

function getInferredType(context: InferenceContext, index: number): Type {
let inferredType = context.inferredTypes[index];
let inferredType = context.inferredTypes[index] || getSuppliedType(context, index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird newline thing here

minTypeArgumentCount = maxTypeArgumentCount;
}
if (typeParameters && typeParameters.length > maxTypeArgumentCount) {
// Trim the type parameters to the maximum length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an example of how this could happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to preserve the current behavior with respect to where we report errors for type parameter lists:

interface X<T> {} // no error on first decl
interface X<T, U> {} // no defaults so we trim to `<T>` and report error for this decl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it works to trim like this. You might have members that reference the trimmed type parameter and those members would never be properly instantiated. In other words, the trimmed type parameters could "leak".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent is that excess type parameters would result in an error, consistent with our behavior today as in the example above. Today we only get the type parameters of the first declaration, so this shouldn't be any different than how we would instantiate X above today.

@@ -2035,6 +2035,18 @@
"category": "Error",
"code": 2704
},
"Required type parameters may not follow optional type parameters": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing trailing period in this and in line 2046

@rbuckton
Copy link
Member Author

rbuckton commented Jan 21, 2017

Following the discussion during the design meeting, I am making the following changes:

  • When specifying type arguments, you are only required to specify type arguments for the required type parameters. Unspecified type parameters can be inferred. Unspecified type parameters will resolve to their default types.

For example:

declare function f<T, U = T>(a: T, b?: U): void;

f(1); // ok. Using inference: T is number, U is number.
f(1, "a"); // ok. Using inference: T is number, U is string.
f<number>(1, "a"); // error. Not using inference: T is number, U is number. 

While this is generally stricter, we can loosen this restriction at a later date if necessary.

@rbuckton
Copy link
Member Author

I have added the following restriction:

  • It is an error to reference a type parameter in a class or interface declaration that is not specified in the declaration, but was introduced in a merged declaration.

This is to provide an error for the following case:

interface A { }
interface B {
  x: A; // error
}
interface B<A = number> {
}

@rbuckton
Copy link
Member Author

I have looked into including #13609 in this change, but it results in significant change in behavior that may be too much to take on if we want to take this for 2.2.

@ahejlsberg Do you have any other feedback for this change?

@rbuckton rbuckton added this to the TypeScript 2.3 milestone Feb 14, 2017
@rbuckton rbuckton merged commit 9be853f into master Feb 15, 2017
@rbuckton rbuckton deleted the genericDefaults branch February 15, 2017 03:32
@mramos-dev
Copy link

@rbuckton You mentioned taking this for 2.2 in an earlier comment but I see that this issue still has the 2.3 milestone attached. Will this be in 2.2?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 15, 2017

@rbuckton You mentioned taking this for 2.2 in an earlier comment but I see that this issue still has the 2.3 milestone attached. Will this be in 2.2?

No. this is scheduled for TS 2.3

@jeffijoe
Copy link

Can't wait for this, writing a library in TS and this will save the consumers from a lot of confusion. Great work!

@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.
Labels
None yet
Projects
None yet
8 participants