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

Polymorphic this in static methods is not resolving correctly #58492

Closed
gund opened this issue May 10, 2024 · 10 comments
Closed

Polymorphic this in static methods is not resolving correctly #58492

gund opened this issue May 10, 2024 · 10 comments
Labels
Duplicate An existing issue was already created

Comments

@gund
Copy link

gund commented May 10, 2024

πŸ”Ž Search Terms

polymorphic this in static methods

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgMJgPZQDwBUB8yA3gFDLnIAOUGmYAnpRAFzK4DcJAviQ08gEkQMaOiy5GEPIQC8bZBAAekEABMAzmkw5QIqMgBKhAPyHkrEBABu0TiRIB6B20nqSoSLEQoACjQC2wOoQqBgg6mBQAK4I2sRkFADuwGAAFgYQ6hgANjZQ6tIAFACUrH4YgcEA6inpmTl5BbgANMhpQficPO7g0PBIyOWVUgTxFMjJaRlZudBN+CVlAUEQNVP1s-l4rWI47er4ndz2Hn3eg8vVtdMNc9tsu8hyu9hDK9KHYxTUFSusQnpdhImHhdkdxlANjZClY4Nkoiw2KVkFYMMBVJwIRAAFYQWKFSFwLIgVhREAAaxAGESIGRqPRXXsCGyRM0qCiEQqb2C0gUyggak03JGhFI4wQHMw-hKxGQPG6TmQAFFFHB-JRshASOzOf5hQA6SZ1GaNEr6n7DfUS3UldjIRXYAC0juQAFUAHJKgAaPiVqFwSoAIhdfsFQuFIjE4i7-JLkAAjFB8CAYGBoSVcy5ayyJdO64WFGUyQggKLZbLFQ3XKFzM0WlZWjPS4p2h3O5BVLDkzRhZCgCJwEADfwQNIYDQkIA

πŸ’» Code

interface Ctor<T> {
    prototype: T;
}
type InferCtorType<T> = T extends Ctor<infer R> ? R : never;


// Types
interface PromiseConstructor {
    withResolvers<T>(): PromiseWithResolvers<T, this>;
}

interface Promise<T> {
    withResolvers<T>(): PromiseWithResolvers<T, Ctor<this>>;
}

interface PromiseWithResolvers<T, TCtor = Ctor<Promise<T>>> {
    promise: InferCtorType<TCtor>;
    resolve(value: T): void;
    reject(reason: unknown): void;
}

class CustomPromise<T> extends Promise<T> {
    custom() { }
}

// Example
CustomPromise.withResolvers().promise.custom(); // <-- UNEXPECTED PromiseConstructor - must be typeof CustomPromise
new CustomPromise(() => null).withResolvers().promise.custom(); // <-- Works on instance methods

πŸ™ Actual behavior

Polymorphic "this" in static methods is not resolved to actual type but instead resolves to definition time type.

πŸ™‚ Expected behavior

Polymorphic "this" in static methods must resolve to proper subclass depending on the invocation.

Additional information about the issue

There was a similar bug reported #5863 but it is not talking specifically about the "this" type keyword which has the bug.

@jcalz
Copy link
Contributor

jcalz commented May 10, 2024

Are you sure this has anything to do with static methods? Or is it just declaration merging? Right now you're mucking with existing types. If you rewrite this to be standalone like

interface Ctor<T> {
    prototype: T;
}
type InferCtorType<T> = T extends Ctor<infer R> ? R : never;


interface MyPromiseConstructor {
    withResolvers<T>(): PromiseWithResolvers<T, this>;
    new <T>(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): MyPromise<T>;
}

interface MyPromise<T> {
    withResolvers<T>(): PromiseWithResolvers<T, Ctor<this>>;
}
declare var MyPromise: MyPromiseConstructor;

interface PromiseWithResolvers<T, TCtor = Ctor<MyPromise<T>>> {
    promise: InferCtorType<TCtor>;
    resolve(value: T): void;
    reject(reason: unknown): void;
}

class CustomPromise<T> extends MyPromise<T> {
    custom() { }
}

CustomPromise.withResolvers().promise.custom();
new CustomPromise(() => null).withResolvers().promise.custom();

Playground link
then there's no problem. What am I missing?

@gund
Copy link
Author

gund commented May 10, 2024

The whole point of having a polymorphic this in static methods is to properly type the return value which should change to the correct sub-type depending on the call, just like the instance-level this behaves and this is what my example tries to show:

Promise.withResolvers<void>(); // Must return resolver with Promise<void>
MyCustomPromise.withResolvers<void>(); // Also must return resolver with MyCustomPromise<void> and not Promise<void> assuming that MyCustomPromise extends Promise of course

It does not matter if it's trying to extend existing API - it does not work in custom APIs as well.
In your example you removed polymorphic call as you defined custom base type for custom promise but this is not the point - we need to be able to make sure that polymorphic static methods can properly return the type of a subtype they are being called on.

If you do not like the example I provided, which is btw a real case example of the type that I tried to fix in the latest TS which does not respect polymorphic behavior of the Promise.withResolvers() API and the spec clearly states that it is polymorphic and it will in fact return a subtype if it's called on one, here is a fully isolated example which involves only custom types and has exactly the same bug.

UPDATE: Also your example is broken and actual return type of the static method is any hence you do not see any errors:
image

This error happens due to the way custom type is constructed and inference of the constructor was not able to get the instance level type, once we fix the constructor we see the same bug as in my examples above:
image
Here is your fixed example.

UPDATE 2: The bug is essentially in the resolution of this type in the static method which is resolving to the same type it has been originally used but we need it to be resolved to the type it's being called on:
image

@jcalz
Copy link
Contributor

jcalz commented May 10, 2024

I'm not a TS team member so you don't have to necessarily do anything for my sake, but it definitely helps to have a standalone example that doesn't worry about merging. Real world examples are great for showing use cases, self-contained ones are better for debugging. Anyway, thanks for the updated example.

I'm still not sure if they're going to consider this a bug or not. But, for what it's worth: the workaround from #5863 might work here, too, where you use a this parameter instead of a this type:

interface MyPromiseConstructor {
    withResolvers<T, Th extends MyPromiseConstructor>(this: Th): PromiseWithResolvers<T, Th>;
}

Playground link

@gund
Copy link
Author

gund commented May 10, 2024

A workaround only masks the bug really and does not solve the problem.
Also in some cases where you need to have required generic (in this example Promise.withResolvers does require one) this "workaround" generic will have to be always specified by the end user and even if you will specify a default value for that generic it will have the same bug by defaulting to the original type and not a polymorphic subtype where the call was made.

I cannot see how this is not a bug really, TS is simply breaking down in "static-level" polymorphism scenario and behaves inconsistently with it's "instance-level" polymorphism which works properly.

What can we do to convince TS members that this is in fact a bug so that it gets fixed?

@RyanCavanaugh
Copy link
Member

You can do this, you just need a site-specific this to work around the lack of #5863

interface PromiseConstructor {
    withResolvers<T, U>(this: U): PromiseWithResolvers<T, U>;
}

This works as desired

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jun 18, 2024
@gund
Copy link
Author

gund commented Jun 18, 2024

@RyanCavanaugh I know that you can "workaround" this issue by adding this with generic but as I described in the comment above, it forces users to specify both the type T as well as this generic type, so the whole typing is pretty much useless, because users can simply specify full return type if they have to specify the full type of this anyway inside generics:

CustomPromise.withResolvers<string, CustomPromise<string>(); // Users are forced to write full type in generic as a "workaround"

CustomPromise.withResolvers() as CustomPromise<string>; // So generics in such API are useless as simple typecast is simpler

CustomPromise.withResolvers<string>(); // And this is the goal - return type must be CustomPromise<string> if this issue would get resolved

The problem with this static case is that this behavior properly works in non-static use-case and you do not need this kind of workarounds there, hence I see this as a bug because it does not behave in the same way in static context.

Don't you think that this type should behave similarly in both static and instance level scenarios without the need for such workarounds?

@RyanCavanaugh
Copy link
Member

This isn't a crash, it isn't unsound, it isn't something that can't be accomplished via some means -- it's a feature request. I want TypeScript to have all the good features it can, but it doesn't yet.

@gund
Copy link
Author

gund commented Jun 18, 2024

It is not critical yes as you said it does not crash compiler, but I would say it is unsound, or maybe more precisely - it's inconsistent with it's instance level behavior. Also I would not call this a feature request, as it does not require anything new to be added but rather fix existing behavior to align static level with instance level behavior imho.
It feels like the fix for this should be trivial assuming that instance level this already has this logic, but unfortunately I'm not familiar with TS codebase to make a PR right now.
Just out of curiosity - would you accept a PR for this fix if one were made?

@RyanCavanaugh
Copy link
Member

If it turns out #5863 can be implemented straightforwardly with little or no associated costs, it seems likely we'd accept that PR

@gund
Copy link
Author

gund commented Jun 18, 2024

Great! Maybe I will find time at some point in the future to try to figure it out.
It seems like this issue is the same as #5863 so I will close this one then.
Thanks for clarifications I appreciate it!

EDIT: Btw if you could point out which file(s) to look into to fix this issue, that would greatly help me (or somebody else) to potentially come up with a fix PR 😊

@gund gund closed this as completed Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants