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

Conflicting definitions for Set constructor causes unexpected default generic in TS 3.5 #31990

Open
evmar opened this issue Jun 19, 2019 · 9 comments

Comments

@evmar
Copy link
Contributor

commented Jun 19, 2019

TypeScript Version: 3.4.5 vs versions after 3.5

Search Terms: set constructor setconstructor unknown

Code

let s = new Set();

Expected behavior:

In TS 3.4: we expect the default chosen param to give us a Set<{}>.
In TS 3.5: we expect a Set<unknown>.
That change was an intended change in TS 3.5.

Or, if Set has a default generic arg, I expect the same default chosen in 3.4 and 3.5.

Actual behavior:

In TS 3.4, this actually was a Set<any>!

It appears there are multiple overloads of the Set constructor.

lib.es2015.collection.d.ts says:

    new <T = any>(values?: ReadonlyArray<T> | null): Set<T>;

while lib.es2105.iterable.d.ts says:

    new <T>(iterable?: Iterable<T> | null): Set<T>;

Note that one has T = any and the other doesn't.

So somehow TS 3.4 vs TS 3.5 decided to change whether to obey the any default, I think?

@evmar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@weswigham I think this might be unintended fallout of your default type parameter constraints change, maybe? #30637

I think the two constructors should at least be consistent, but I'm not sure which is preferred, probably the any-less one?

@weswigham

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

I'm not sure which is preferred, probably the any-less one?

Yeah, I'm not sure why we even have a version of the constructor that defaults to any - the's obviously quite unsafe.

@fatcerberus

This comment has been minimized.

Copy link

commented Jun 20, 2019

Array has that problem too:

let arr1 = new Array(10);  // any[] - bad
let arr2 = new Array<number>(10);  // number[] - good
@weswigham

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@DanielRosenwasser do we want to change our behavior here? I think we last looked at it some time around #25374

@evmar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Perhaps related, I notice in 3.4->3.5, SetConstructor changed from

   new <T>(iterable: Iterable<T>): Set<T>;

to

    new <T>(iterable?: Iterable<T> | null): Set<T>;

So perhaps that is changing which overload is selected when.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Yeah, it's unsafe, but it would be incredibly annoying to break people.

This sort of comes back to the "I want this to be an 'implicit any' for the noImplicitAny scenarios."

@fatcerberus

This comment has been minimized.

Copy link

commented Jun 21, 2019

I guess it can’t really be unknown[] since then it wouldn’t be assignable to anything but it also can’t reasonably be never[] because then you can’t put stuff in it (unless it could be made to work like evolving array types).

@evmar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Just to restate, what I am concerned about here is whether "new Set()" is supposed to give you a Set<any> or a Set<unknown>, given the conflicting definitions in the typings.

I believe the intention was for 3.5 change it to <unknown> and for that to break apps -- that is at least what 3.5 did in practice -- but I think the resolution of this bug is to make the typings express that intent more clearly.

@evmar

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Circling back on this -- I think maybe I failed to communicate the bug here.

In practice, TS3.4 => TS3.5 changed what new Set() gives you, from Set<any> to Set<unknown>. (That is too late to undo, though massively annoying for us to upgrade through.)

But the actual bug here is that I think you didn't change the definition of Set! It still says any in the typings, something else must be going wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.