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

Inference is broken on intersection type utilizing polymorphic this #53970

Closed
0x706b opened this issue Apr 22, 2023 · 13 comments Β· Fixed by #54112
Closed

Inference is broken on intersection type utilizing polymorphic this #53970

0x706b opened this issue Apr 22, 2023 · 13 comments Β· Fixed by #54112
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@0x706b
Copy link

0x706b commented Apr 22, 2023

Bug Report

πŸ”Ž Search Terms

inference, intersection, polymorphic this, deferred index access

πŸ•— Version & Regression Information

  • This changed between versions 5.1.0-dev.20230313 and 5.1.0-dev.20230315

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

export interface TypeLambda {
  readonly A: unknown
}

export interface TypeClass<F extends TypeLambda> {
  readonly _F: F
}

export type Apply<F extends TypeLambda, A> = F extends { readonly type: unknown }
  ? (F & { readonly A: A })['type']
  : { readonly F: F, readonly A: A }

export interface T<A> {
  value: A
}

export interface TTypeLambda extends TypeLambda {
  readonly type: T<this["A"]>
}

export declare const map: <F extends TypeLambda>(F: TypeClass<F>) => <A, B>(a: Apply<F, A>, f: (a: A) => B) => Apply<F, B>

declare const typeClass: TypeClass<TTypeLambda>

declare const a: T<number>

map(typeClass)
// TS 5.0.4:              <A, B>(a: T<A>, f: (a: A) => B) => T<B>
// TS 5.1.0-dev.20230422: <A, B>(a: (TTypeLambda & { readonly A: A; })["type"], f: (a: A) => B) => (TTypeLambda & { readonly A: B; })["type"]

map(typeClass)(a, (_) => _)
                // ^ TS 5.0.4 infers number
                //   TS 5.1.0-dev.20230422 infers unknown

πŸ™ Actual behavior

Each version including and after 5.1.0-dev.20230315 defers the index access in the Apply type and doesn't allow inference to continue.

πŸ™‚ Expected behavior

Inference should continue to work in this case.

After a bit of research, I think that #53098 is the PR that introduced this change.

@0x706b 0x706b changed the title Inference is broken on intersection type utilizing polymorphic this due to deferred index access resolution Inference is broken on intersection type utilizing polymorphic this Apr 22, 2023
@mikearnaldi
Copy link

This currently breaks the encoding used in @effect/* for HKTs and probably a few other libraries that rely on the same pattern (hotscript, storybook, come to mind)

@ecyrbe
Copy link

ecyrbe commented Apr 24, 2023

Same here, zodios is using this pattern to enable policy based interfaces.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Apr 25, 2023
@ethanresnick
Copy link
Contributor

Seems like those packages should be added to the RWC test suite maybe, or snippets of them added to the main compiler tests

@jakebailey
Copy link
Member

jakebailey commented May 2, 2023

Feel free to send a PR: https://github.com/microsoft/TypeScript-error-deltas

See userTests.

Of course, any PR would include a test case for this.

@ecyrbe
Copy link

ecyrbe commented May 2, 2023

@ahejlsberg if you find another way to better support HKT, it would also be fine from my perspective.
The need is to be able to pass an utility type to another utility type on type level and still have type constraint available.

@ahejlsberg
Copy link
Member

I can confirm that the change in behavior is caused by #53098. We definitely want the deferred resolution implemented by that PR. However, it means that types like (TTypeLambda & { readonly A: A; })["type"] aren't resolved until a non-generic type is substituted for A through instantiation and, unfortunately, it also means that no inferences are made for A in the map(typeClass) invocation.

I think we can implement a targeted fix where, when inferring to types like (TTypeLambda & { readonly A: A; })["type"], we force the resolution that previously happened and then infer to the result of that. This means the type of map(typeClass) will still change as in the example above, but inference will infer number in the final example.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels May 3, 2023
@ahejlsberg ahejlsberg added this to the TypeScript 5.1.1 milestone May 3, 2023
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label May 3, 2023
@mikearnaldi
Copy link

I can confirm that the change in behavior is caused by #53098. We definitely want the deferred resolution implemented by that PR. However, it means that types like (TTypeLambda & { readonly A: A; })["type"] aren't resolved until a non-generic type is substituted for A through instantiation and, unfortunately, it also means that no inferences are made for A in the map(typeClass) invocation.

I think we can implement a targeted fix where, when inferring to types like (TTypeLambda & { readonly A: A; })["type"], we force the resolution that previously happened and then infer to the result of that. This means the type of map(typeClass) will still change as in the example above, but inference will infer number in the final example.

This is very problematic we will have to make a backward incompatible change with serious degradation to developer experience, I am honestly in doubt about what feature of TS to trust, this behaviour was even confirmed as intended explicitly by a TS core member in prior issues. This way we don't have any way of safely develop libraries without risk that in 2 releases the TS team decides to change semantic behaviour.

@ecyrbe
Copy link

ecyrbe commented May 4, 2023

I don't think we need to be this dramatic.
I see this as an opportunnity to clarify TS team stance on HKT.
Community started evolving libraries to take advantage of some HKT features.
As a TS team you have a playground feedback on how community took advantage of it.
Based on this feedback, you Can now either :

  • officially support a new HKT syntax to address current use cases
  • officially announce HKT to not be supported in either way going forward

Clarification is key for us to know if we can leverage some ts features for our use cases.

Again, thank you @ahejlsberg and all ts team for your wonderfull work

@mikearnaldi
Copy link

I don't think we need to be this dramatic. I see this as an opportunnity to clarify TS team stance on HKT. Community started evolving libraries to take advantage of some HKT features. As a TS team you have a playground feedback on how community took advantage of it. Based on this feedback, you Can now either :

  • officially support a new HKT syntax to address current use cases
  • officially announce HKT to not be supported in either way going forward

Clarification is key for us to know if we can leverage some ts features for our use cases.

Again, thank you @ahejlsberg and all ts team for your wonderfull work

I mean that was valid behaviour since TS3, I appreciate the work a lot and TS is a fantastic language but unless we can have some sort of guarantee things won't break (unless by mistake) I don't see how to go forward, there are many edge behaviours that are relied upon from the community and this is totally normal for a language without a spec but it only works if we preserve the "strange behaviour" priorly introduced. I can list a number of features that may break with implications to many libraries like for example behaviour of overload selection, many libraries rely on the current ordering and selection algorithm, what if with the ongoing effort to improve inference we end up breaking? I'd always assumed if we ended up there the decision would be not to accept the changes (maybe at the price of perfection) to maximise usability. If instead the policy is "yeah but the behaviour should have really been Y" then I am afraid I can't be sure of most of the TS behaviour, and I am saying this while generally appreciating the efforts and the language, but as as library developer I need to know the boundaries

@ahejlsberg
Copy link
Member

I understand that esthetically T<A> looks nicer than (TTypeLambda & { readonly A: A; })["type"], though the latter really is a more precise representation of what was requested in Apply. Are there other issues that are caused by this change and not fixed by #54112?

@mikearnaldi
Copy link

We use:

export const f = map(typeClass)

and let it derive the type of f, with this change the typing of f will be impractical to use and understand from the reader's perspective, the problem seems to be related to potential reduction to never that this change aims to preserve, would there be a way of avoiding it in a class of cases where never is known to be impossible? I guess that would require being able to express "TypeLambda is an object without the field A" ? or maybe a way to force it to reduce the type?

In a general sense from a user perspective it isn't necessarily the case that something more precise is always better

@mikearnaldi
Copy link

mikearnaldi commented May 4, 2023

Actually I may have found a way:

export interface TypeClass<F> {
  readonly _F: F
}

export declare namespace TypeLambda {
  type _A<T> = "A" extends keyof T ? T["A"] : never
}

interface TypeLambda { type: unknown }

export type Apply<F, A> = F extends TypeLambda
  ? (F & {A: A})["type"]
  : { readonly F: F, readonly A: A }

export interface T<A> {
  value: A
}

export declare const map: <F>(F: TypeClass<F>) => <A, B>(a: Apply<F, A>, f: (a: A) => B) => Apply<F, B>

interface TTypeLambda {
  type: T<TypeLambda._A<this>>
}

declare const typeClass: TypeClass<TTypeLambda>

declare const a: T<number>

// const x: <A, B>(a: T<A>, f: (a: A) => B) => T<B>
const x = map(typeClass)

map(typeClass)(a, (_) => _)

Not sure if this if considered a bug with the new behaviour or this can be used (usage)

@ahejlsberg
Copy link
Member

It turns out there's a simple way to fix this issue and preserve our old behavior. See latest commits in #54112.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants