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

nested generic function inference causes unbound type parameter leak #55467

Closed
tjjfvi opened this issue Aug 22, 2023 · 5 comments Β· Fixed by #57403
Closed

nested generic function inference causes unbound type parameter leak #55467

tjjfvi opened this issue Aug 22, 2023 · 5 comments Β· Fixed by #57403
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@tjjfvi
Copy link
Contributor

tjjfvi commented Aug 22, 2023

πŸ”Ž Search Terms

unbound generic, unbound type parameter

πŸ•— Version & Regression Information

This is the behavior in every version I tried, and I reviewed the FAQ.

⏯ Playground Link

https://www.typescriptlang.org/play?jsx=0#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwHcYoAHAHgA0A+ACgA8AueCgSiYG95Hn4BfAKFCRYCFOmx54YKBAhkAgvBB0MIVMADO8KKgCeAbQC6AGngAVWt07caAOnuwA5hqbyW8ALxVzfU-dtOLvBuTGb8-GB4GhjwECBQANaeUjIQNESkNGQW9KHuXlwspgCMLPwA9OXw1fAAegD8-EA

πŸ’» Code

declare function wrap<X>(x: X): { x: X }
declare function call<A extends any[], T>(x: { x: (...args: A) => T }, ...args: A): T

const leak = call(wrap(<T>(x: T) => x), 1)
//    ^? - const leak: A[0]

πŸ™ Actual behavior

The type of leak involves an unbound type parameter.

πŸ™‚ Expected behavior

The type of leak should be number.

Additional information about the issue

No response

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Aug 22, 2023
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 5.5.0 milestone Aug 22, 2023
@fatcerberus
Copy link

This is like the third or fourth bug I’ve seen recently involving an unbound type parameter escaping from a generic. I wonder if they all have the same root cause…

@rotu
Copy link

rotu commented Jan 26, 2024

This is like the third or fourth bug I’ve seen recently involving an unbound type parameter escaping from a generic. I wonder if they all have the same root cause…

I also found type parameters can leak from how TypeScript treats for-of syntax. #38388 (comment)

@Andarist
Copy link
Contributor

Andarist commented Feb 12, 2024

This is an interesting one :)

  1. the contextualSignature for the <T,>(x: T) => x) here is (...args: A): T
  2. it gets instantiated through instantiateSignatureInContextOf as (x: A[0]): A[0]
  3. wrap manages to infer that A[0] for its T (which is fine)
  4. but then call (the outer call where A originates) manages to collect both covariant ([number]) and contravariant inferences ([x: A[0]]). Both of them have the same priority
  5. but now... preferCovariantType gets computed as false and that's how the leak starts here. A contravariant inference that is derived from itself gets picked here
  6. IIRC preferCovariantType gets computed as false because that covariant inference is not a subtype of that contravariant inference

Perhaps using a different priority when inferring from a rest parameter would do the trick here? Even much simpler cases could benefit from this because this one looks like a completely legit call that should succeed:

declare function fn<A extends readonly unknown[]>(arg: (...args: A) => void, ...args: A): A

const inferred = fn((a: number) => {}, 1, 2) // Expected 2 arguments, but got 3.(2554)
//    ^? const inferred: [a: number]

That said, even when I forced the covariant inference in the case from this thread, I still got a leak through the call's second type parameter (T). That is still inferred as A[0] and it doesn't get instantiated anywhere (that's what got leaked in the first place).

That can be fixed by instantiating the return type of the resolved signature:

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 072ed10aa7..12f8872cc4 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -15477,7 +15477,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
             if (!pushTypeResolution(signature, TypeSystemPropertyName.ResolvedReturnType)) {
                 return errorType;
             }
-            let type = signature.target ? instantiateType(getReturnTypeOfSignature(signature.target), signature.mapper) :
+            let type = signature.target ? instantiateType(instantiateType(getReturnTypeOfSignature(signature.target), signature.mapper), signature.mapper) :
                 signature.compositeSignatures ? instantiateType(getUnionOrIntersectionType(map(signature.compositeSignatures, getReturnTypeOfSignature), signature.compositeKind, UnionReduction.Subtype), signature.mapper) :
                 getReturnTypeFromAnnotation(signature.declaration!) ||
                 (nodeIsMissing((signature.declaration as FunctionLikeDeclaration).body) ? anyType : getReturnTypeFromBody(signature.declaration as FunctionLikeDeclaration));

But even then the type arguments lists are still displayed as uninstantiated so even more work has to be done around this to fix it:

const leak = call(wrap(<T,>(x: T) => x), 1)
//    ^? const leak: number
//           ^? function call<[number], A[0]>(x: { x: (args_0: number) => number; }, args_0: number): number
//                ^? function wrap<<A[0]>(x: A[0]) => A[0]>(x: <A[0]>(x: A[0]) => A[0]): { x: <A[0]>(x: A[0]) => A[0]; }

That said... this is probably not the only source of such leaks related to nested inferences. So a more general approach would still be needed to avoid them in other situations. This infers something that depends on A for A - perhaps instantiating the inferred type with a mapper that maps A to its constraint would be the simplest solution.

@weswigham
Copy link
Member

@Andarist is right, there's a lot going wrong here to make this leak happen, though the particulars of how a specific inference is chosen don't matter too much.

  1. We should outright discard inferences of the form [T[0]] for T - all that means is "T's first element is T's first element" which is a tautological inference.
  2. Even with that in place, thanks to the inference from the unbound type parameter we carry through, we have a type parameter inference result that references a previously inferred type parameter. That's fine for defaults - we just instantiate at time of inference, but not currently for actual inference results. This is pretty trivially fixable - just move the backreference mapper instantiation we do for type parameter defaults down a few lines so we always do it.
  3. But even once we start doing that, we reveal another problem - the instantiation makes the call return type resolve as expected, but now the argument type is still something referencing A[0] instead of number - so signature argument assignability fails - easy enough to fix, just instantiate the expression type with the signature mapper (since the signature's type parameters can appear in the expression thanks to unbound type parameter inference nowadays).
  4. And then once you start making that instantiation, you notice it does nothing because of a bug in couldContainTypeVariables (or rather, in type construction that affects it) - the isolated signature type has no symbol after being instantiated to (x: A[0]) => A[0], and so is considered as "not possibly containing type parameters", which is patently false. This is because getOrCreateTypeFromSignature currently makes its types without a symbol, which is a bug. Easy to fix on the surface, and fixing it probably fixes a handful of other leaks elsewhere.
  5. Unfortunately, fixing that actually turns out to be pretty complicated - adding a symbol and a declaration to the type is easy enough, but making it so anonymous type instantiation machinery can find the right type parameters to instantiate - that's another story entirely! Since we're trying to instantiate a signature type we constructed from basically nothing (an arrow expression and some context), the list of what outer type parameters it can refer to is also constructed, and needs to be the context signature's type argument list (as any of them may be referenced by the expression via inference). Believe it or not, we do not have machinery to allow this yet!

I'm working on something that hits up all those points at #57403, but it'll take a bit to find a place for the fix to land that doesn't break anything, particularly with regards to instantiation changes.

@Andarist
Copy link
Contributor

@weswigham funny enough i was also looking at #43961 over the weekend and discovered exactly what u mention here from the point 3 to 5, I had to pause the investigation before I managed to work around the point 5 πŸ˜… it’s worth adding a test case based on that extra issue into ur PR unless it turns out to add another layer of complexity to this that should be fixed later

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.

7 participants