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

Update Array.concat type signature to fix #6594 #6629

Merged
merged 1 commit into from
Feb 4, 2016
Merged

Update Array.concat type signature to fix #6594 #6629

merged 1 commit into from
Feb 4, 2016

Conversation

LPGhatguy
Copy link
Contributor

I wasn't sure whether I should commit the LKG changes with the source lib changes; there was another change that hasn't propagated to LKG yet (removal of WeakMap/WeakSet.clear) but I didn't commit it.

This should fix #6594.

@msftclas
Copy link

Hi @LPGhatguy, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@DanielRosenwasser
Copy link
Member

Can you undo the changes to lib?

@LPGhatguy
Copy link
Contributor Author

Yeah; does that mean that the updates to lib are done all at once?

@@ -1011,12 +1011,12 @@ interface Array<T> {
* Combines two or more arrays.
* @param items Additional items to add to the end of array1.
*/
concat<U extends T[]>(...items: U[]): T[];
concat<U extends T[]>(...items: (U | U[])[]): T[];
Copy link
Member

Choose a reason for hiding this comment

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

I think this overload can be removed, as @ahejlsberg mentioned on #6594 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that break any existing use of concat<T> by declaring it as a non-generic method?

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 actually know why this was generic in the first place. Is there a reason T[] couldn't suffice?

Additionally, even if we decide not to drop this overload, this change is incorrect, since [1,2,3].concat([[1]],[[23]]) now works and still returns number[].

Give the following a try in the playground:

interface Array<T> {
    myConcat<U extends T[]>(...items: (U | U[])[]): T[];
    myConcat(...items: (T | T[])[]): T[];
}

var x = [1,2,3].myConcat([[23]])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the generic line fixes that case, which makes me wonder whether the generic worked correctly before either.

I'll ditch it in the PR because I don't know why it would be in there either.

Copy link
Member

Choose a reason for hiding this comment

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

The generic overload should go. It was there for historical reasons (it was a workaround for an issue in the old compiler having to do with instantiations of Array<T> within Array<T> itself).

@LPGhatguy
Copy link
Contributor Author

This new diff no longer has the generic overload; the following works in the playground:

interface Array<T> {
    myConcat(...items: (T | T[])[]): T[];
}

var x = [1,2,3].myConcat(23, [23]);
console.log(x);

While this does not, which is correct:

interface Array<T> {
    myConcat(...items: (T | T[])[]): T[];
}

var x = [1,2,3].myConcat([[23]]);
console.log(x);

@sandersn
Copy link
Member

Sounds good to me. @ahejlsberg, can you confirm that it's in line with what you were thinking?

@ahejlsberg
Copy link
Member

👍

mhegazy added a commit that referenced this pull request Feb 4, 2016
Update Array.concat type signature to fix #6594
@mhegazy mhegazy merged commit 727b9a9 into microsoft:master Feb 4, 2016
@rkirov
Copy link
Contributor

rkirov commented May 20, 2016

An interesting consequence of this change is the interplay with literal type freshness. Previously, the following was valid [{a: ''}].concat([{a: '', b: ''}]), but after the change it is not, because Object literal may only specify known properties. The old types implicitly allow for subtypes because of the type parameter U.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing type error message in concat
7 participants