Skip to content

WeakSet<T> should be WeakSet<T extends object> #14840

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

Closed
ghost opened this issue Mar 24, 2017 · 13 comments
Closed

WeakSet<T> should be WeakSet<T extends object> #14840

ghost opened this issue Mar 24, 2017 · 13 comments
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue

Comments

@ghost
Copy link

ghost commented Mar 24, 2017

TypeScript Version: nightly (2.3.0-dev.20170324)

Code

const s = new WeakSet<string>();
s.add("foo");

Expected behavior:

An error, since this will fail at runtime.

Actual behavior:

No error.

@falsandtru
Copy link
Contributor

falsandtru commented Apr 25, 2017

I did it, but @mhegazy reverted that fix in #15278. His reverting doesn't comply with the spec of ecma. We should comply with the spec.

@mhegazy says object constraints of WeakSet causes errors in @types/lodash, but WeakMap is not. Why? lodash has correct constraints for WeakMap like https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/lodash/index.d.ts#L12898. So WeakMap already have object constraints. WeakSet also should be so.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 26, 2017

We reverted the change to avoid breaking @types/lodash. we should reply it, but first we need either 1. remove these definitions from @types/lodash alltogahter, or 2. relax the rule on the compiler side that all declarations have the same constraint.

The definition in lodash is at the end of the file, not visible on github view:

// Backward compatibility with --target es5
declare global {
    interface Set<T> { }
    interface Map<K, V> { }
    interface WeakSet<T> { }  // this breaks if lib.es6.d.ts has WeakSet<T extends object>
    interface WeakMap<K extends object, V> { }
}

@mhegazy mhegazy added the Bug A bug in TypeScript label Apr 26, 2017
@falsandtru
Copy link
Contributor

falsandtru commented Nov 6, 2017

Now that problem seems to have gone away.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 6, 2017

so what happened to lodash use of this API?

@falsandtru
Copy link
Contributor

A last fix is DefinitelyTyped/DefinitelyTyped#21502 and #19756. We need to accept some breaking changes, or never fix this bug.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2017

As i mentioned earlier, we need to first:

relax the rule on the compiler side that all declarations have the same constraint.

@falsandtru
Copy link
Contributor

Is there a proposal issue of that?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2017

nope.

@falsandtru
Copy link
Contributor

Can you make it to resolve this issue?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2017

feel free to file a new issue for it.

@falsandtru
Copy link
Contributor

falsandtru commented Nov 14, 2017

Why you don't do it?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2017

here you go #20018

@falsandtru
Copy link
Contributor

Thanks.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 10, 2018
@mhegazy mhegazy added this to the TypeScript 2.7 milestone Jan 10, 2018
@mhegazy mhegazy added the Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript label Jan 10, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants