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

Some long function return expressions no longer evaluated in 5.5 #58937

Closed
ssalbdivad opened this issue Jun 19, 2024 · 5 comments Β· Fixed by #59170
Closed

Some long function return expressions no longer evaluated in 5.5 #58937

ssalbdivad opened this issue Jun 19, 2024 · 5 comments Β· Fixed by #59170
Assignees
Labels
Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@ssalbdivad
Copy link

ssalbdivad commented Jun 19, 2024

πŸ”Ž Search Terms

function return type inferred isolatedDeclarations regression

πŸ•— Version & Regression Information

This changed occurred as a result of 2b4e7e34e7a40fe0d886aec6f799954420f2ac0c (#58546)

⏯ Playground Link

https://tsplay.dev/WY7ZbN

πŸ’» Code

/** This is the minimal version which seems somewhat innocuous */

export type inferPipe<t, pipe> =
    pipe extends (In: t) => unknown ? (In: t) => ReturnType<pipe> : never

interface Type<t> {
    pipe<fn extends (In: t) => unknown>(fn: fn): Type<inferPipe<t, fn>>
}

declare const t: Type<string>

// Before: Type<(In: string) => number>
// 2b4e7e34e7a40fe0d886aec6f799954420f2ac0c
// After: Type<(In: string) => ReturnType<(s: string) => number>>

const out = t.pipe(s => parseInt(s))


/** In ArkType and in other cases where these expressions are used as part of a deeper type,
 * it can lead to hovers growing exponentially in size as expressions are chained 
 * instead of being evaluted at each step and staying the same.
 * 
 * Here's an example of what occurs with a longer expression
 *  */
 
export type inferPipe2<t, pipe> =
	pipe extends (In: t) => unknown ?
		(In: t) => ReturnType<pipe> extends infer n extends number ? n
		: ReturnType<pipe> extends infer s extends string ? s
		: ReturnType<pipe> extends infer b extends boolean ? b
		: never
	:	never

interface Type2<t> {
	pipe<fn extends (In: t) => unknown>(fn: fn): Type2<inferPipe2<t, fn>>
}

declare const t2: Type2<string>

// Before: Type2<(In: string) => number>
// 2b4e7e34e7a40fe0d886aec6f799954420f2ac0c
// After: Type2<(In: string) => ReturnType<(s: string) => number> extends infer n extends number ? n : ReturnType<(s: string) => number> extends infer s extends string ? s : ReturnType<(s: string) => number> extends infer b extends boolean ? b : never>

const out2 = t2.pipe(s => parseInt(s))

πŸ™ Actual behavior

Trivially evaluated ReturnType not evaluated on hover.

πŸ™‚ Expected behavior

It should be evaluated

Additional information about the issue

The example from the playground can worked around by inferring a variable in the position of unknown to get the return type directly instead of using the ReturnType alias.

This causes a massive regression in the quality of hover displays for many types in ArkType, which I'm still working to determine if can be avoided as-is.

If the team does decide it wants to address this, it would be a huge relief to me to know so I can avoid trying to find workarounds that may involve changing significant parts of the code base to have hovers that were previously a couple dozen characters long be remotely readable after 5.5.

@whzx5byb
Copy link

Workaround: just put the extends infer clause before the arrow function type.

export type inferPipe<t, pipe> =
-    pipe extends (In: t) => unknown ? (In: t) => ReturnType<pipe> : never
+    pipe extends (In: t) => unknown ? ReturnType<pipe> extends infer R ? (In: t) => R : never : never


 export type inferPipe2<t, pipe> =
	pipe extends (In: t) => unknown ?
-		(In: t) => ReturnType<pipe> extends infer n extends number ? n
-		: ReturnType<pipe> extends infer s extends string ? s
-		: ReturnType<pipe> extends infer b extends boolean ? b
+               ReturnType<pipe> extends infer n extends number ? (In: t) => n
+	        : ReturnType<pipe> extends infer s extends string ? (In: t) => s
+		: ReturnType<pipe> extends infer b extends boolean ? (In: t) => b
		: never
	:	never

@ssalbdivad
Copy link
Author

ssalbdivad commented Jun 20, 2024

@whzx5byb From the original post, there is a simpler work around in this case:

The example from the playground can worked around by inferring a variable in the position of unknown to get the return type directly instead of using the ReturnType alias.

It's possible I'll be able to propagate a similar strategy through the multiple layers in which this is occurring in ArkType, but too early to know for sure yet, particularly that it wouldn't interfere with parameter inference at all which can be very sensitive.

Even if I can find a way to work around it, writing things this way is neither natural nor intuitive whereas a more straightforward approach can lead to very bad outcomes.

As I've been working on potential refactors of ArkType to accommodate this, I've frequently seen return expressions like Exclude<number, ArkError | ArkErrors>which trivially evaluate to a simple type (in this case number) but the extra convolution can make them impossible to visually parse as they're composed.

If I understood what @jakebailey mentioned correctly, this is based on the same code that determines what to write to .d.ts files. If that is the case, I'd expect anyone consuming types from a library like this to take a performance hit as well. Even if some of those types are already cached, it doesn't seem feasible to parse and evaluate a long, complex conditional in the same amount of time it would take to parse an evaluated result like number.

@DanielRosenwasser DanielRosenwasser added the Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info label Jun 20, 2024
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.6.0 milestone Jun 20, 2024
@DanielRosenwasser
Copy link
Member

I think @dragomirtitian mentioned he was looking into this one.

@ecyrbe
Copy link

ecyrbe commented Jul 6, 2024

I think this issue is broader than that. As discussed in Linked Effect Issue.
I'm investigating the faulty commit in TS 5.5 and report back ASAP

@jakebailey
Copy link
Member

I spent a little bit of time trying to figure out what was going on here and sent a potential fix (which I'm not 100% certain of). Try using the build at #59170 (comment) and see if that seems to fix things for your cases.

No guarantees, this PR is extremely unscientific.

@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants