-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fixed some variance measurements in type aliases of generic functions #54866
base: main
Are you sure you want to change the base?
Fixed some variance measurements in type aliases of generic functions #54866
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@@ -223,6 +226,10 @@ immutable.ts(391,22): error TS2430: Interface 'Set<T>' incorrectly extends inter | |||
export function Stack<T>(): Stack<T>; | |||
export function Stack<T>(collection: Iterable<T>): Stack<T>; | |||
export interface Stack<T> extends Collection.Indexed<T> { | |||
~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time assessing if this is a good change or not, could use some help with that 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a good change. This used to stack out, using more variance calculations removed the stack out, but I think have been hiding the error with incorrect results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this change looked familiar - I'd built this into the suite of changes in #43887 long ago. I do think this is very much still needed, and since it can stand on its' own, we should probably take it (though I am biased and should get someone else to peek at it). There's a bit more that you should handle, like propagating this "erased" type through indexed access operations, similarly to how wildcardType
already works, and any other type constructor that wildcards propagate through.
@@ -223,6 +226,10 @@ immutable.ts(391,22): error TS2430: Interface 'Set<T>' incorrectly extends inter | |||
export function Stack<T>(): Stack<T>; | |||
export function Stack<T>(collection: Iterable<T>): Stack<T>; | |||
export interface Stack<T> extends Collection.Indexed<T> { | |||
~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a good change. This used to stack out, using more variance calculations removed the stack out, but I think have been hiding the error with incorrect results.
Do u know what kind of tests I should use to verify that I'm moving in the right direction? I guess that for the most part I can heavily borrow from #43887 but since you mentioned that was a broader change... I'm not really able to tell quickly which parts are borrowable and which ones are not 😅 |
An experiment on how the variance measurement could be improved when intersections and unions are involved in situations like this.
related to the issue found here, cc @RyanCavanaugh