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

Relax the rule on the compiler side that all declarations have the same constraint #20018

Closed
mhegazy opened this issue Nov 14, 2017 · 8 comments · Fixed by #20883
Closed

Relax the rule on the compiler side that all declarations have the same constraint #20018

mhegazy opened this issue Nov 14, 2017 · 8 comments · Fixed by #20883
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2017

Originally reported in #14840

Consider WeakSet for instance.. today it is defined as interface WeakSet <T> {...}. This is incorrect, as it allows for patterns like const s = new WeakSet<string>(); s.add("foo"); which throws at runtime; the correct definition for WeakSet is interface WeakSet<T extends object> { .. }.

Changing this definition will cause errors in files that redefine WeakSet to add additional members to it.

Error: All declarations of WeakSet must have identical type paramters

The proposal here is to allow type parameter declarations to skip constraints. conflicting constraints should still be reported as errors.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Nov 14, 2017
@RyanCavanaugh
Copy link
Member

Big fan 👋 👋 👋

@Jessidhia
Copy link

Does that mean that, when a constraint is not specified, that it should "inherit" the most specific constraint found elsewhere? Or does that only apply on instantiation?

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 16, 2017

this has nothign to do with instantiations.. it is about declarations..

interface A<T> {}
interface A<T extends number> {}

today this is an error. this issue is to relax the check and treat it as:

interface A<T extends number> {}
interface A<T extends number> {}

Also note that mismatched constraints is still an error:

interface B<T extends number> {}
interface B<T extends string> {}

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Nov 28, 2017
@RyanCavanaugh
Copy link
Member

Easy PR for anyone who wants to take it, or we can pick it up in a bit

@RichiCoder1
Copy link

Where would one look to start tackling this?

@ajafff
Copy link
Contributor

ajafff commented Nov 28, 2017

@RichiCoder1 there's a function areTypeParametersIdentical in src/compiler/checker.ts:22351
That looks like a good place to start.

HerringtonDarkholme added a commit to HerringtonDarkholme/TypeScript that referenced this issue Dec 24, 2017
@mhegazy mhegazy added this to the Community milestone Jan 4, 2018
rbuckton added a commit that referenced this issue Jan 5, 2018
fix #20018, allow skip constraint when merging interfaces
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 5, 2018
@mhegazy mhegazy modified the milestones: Community, TypeScript 2.7 Jan 5, 2018
@mhegazy
Copy link
Contributor Author

mhegazy commented Jan 5, 2018

thanks @HerringtonDarkholme !

@mattmccutchen
Copy link
Contributor

mattmccutchen commented May 4, 2018

I noticed the behavior is order dependent: it's an error if the first declaration skips the constraint. In fact, mhegazy's example doesn't work. I don't think this is intentional, so I filed a new issue: #23909. It is intentional according to the description of #20883, for better or for worse.

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants