Fix type variable leaks and cache inconsistencies#61668
Conversation
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two long‐standing issues—type variable leaks and cache inconsistencies—in our type inference system by properly incorporating the base type mapper into cache key computations and ensuring that outer type parameters in contextual types are instantiated before inference. In doing so, it reverts a previous change (#57403) and updates several tests and baseline outputs to reflect the correct, more conservative inference behavior.
- Properly include the base type mapper when computing cache keys
- Instantiate outer type parameters to avoid circular inferences during return type inference
- Update test cases and baselines to match the new behavior
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/cases/compiler/*.ts | New tests and example functions (e.g. withP3) to confirm that generic call inference behaves as expected. |
| tests/baselines/reference/* | Baseline outputs have been updated (instantiation counts, inferred types, and symbols) to reflect the fixes. |
| src/compiler/types.ts | Removed duplicate flag and deprecated interface definitions for SingleSignatureType to prevent leaks. |
| src/compiler/checker.ts | Refactored signature instantiation logic (e.g. removal of unused helper functions, introduction of createOuterReturnMapper) to improve cache consistency and type inference accuracy. |
Comments suppressed due to low confidence (2)
src/compiler/checker.ts:16310
- [nitpick] Changing the parameter type from 'readonly Type[]' to 'Type[]' in getSignatureInstantiation improves mutability control and clarity; ensure that all relevant usages are consistent with this change.
function getSignatureInstantiation(signature: Signature, typeArguments: readonly Type[] | undefined, isJavascript: boolean, inferredTypeParameters?: readonly TypeParameter[]): Signature {
src/compiler/types.ts:6495
- The duplication of the SingleSignatureType flag has been removed and defined only once. Verify that removing the previously stored 'outerTypeParameters' in SingleSignatureType does not affect dependent logic.
SingleSignatureType = 1 << 27, // A single signature type extracted from a potentially broader type
|
@typescript-bot test it |
|
@ahejlsberg Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
There was a problem hiding this comment.
Q: If the issue seems to be that we shouldn't ever get the input type parameter as an inference result, doesn't that imply we should never make it available in inference as an option to match in the first place? Eg, we should always be matching against a clone that we later instantiate to inference results/constraints, as was already done within an implementation signature?
That implies |
Not quite sure I understand your question. When inferring for a generic function call, we do want the type parameters of that call to appear in contextual types for arguments (and we have logic to ensure they're instantiated with fixing or non-fixing mappers as appropriate), but we do not want type parameters of any containing function calls to appear. Hence the |
The fact that we infer type declare function call<A, R>(f: (x: A) => R, arg: A): R
declare function id(x: unknown): unknown;
call(id, 1) // Infers type `1` for `A`Above, we have two inferences for |
There was a problem hiding this comment.
Is there anything else left to do here?
|
Hm, are we worried about those fp-ts breaks? |
I'm not concerned with those. They're tests that check for exact types and, as expected, the types have changed. The new inferred types aren't per se worse, they're just different |
|
I guess, but it's seemingly pretty simple, "does our isSome call filtered preserve the tupleness of this other thing", and now it doesn't. const actual1 = _.filter(O.isSome)([O.some(3), O.some(2), O.some(1)])
U.deepStrictEqual(actual1, O.some([O.some(3), O.some(2), O.some(1)]))(#61668 (review) felt pretty correct to me) |
|
I have code using MobX's Here is the code working in TypeScript 5.8 Here is the same code in TypeScript 5.9 The error is: I've also included in the example using |
|
Please file an issue if you're having trouble, we cant track comments in old PRs |
|
I opened a PR on MobX to fix the issue. It's not clear why this change in TS caused the problem, but my "fix" is there: mobxjs/mobx#4578 |
This PR fixes two issues leading to type variable leaks and cache inconsistencies.
The first issue, reported in #59937, relates to instantiations of single-signature types with inferred type parameters sometimes leaking into other types because of omitted information in cache keys. The fix to this issue is to properly include the base type mapper when computing cache keys.
The second issue, reported in #61667, relates to circular inferences, i.e. situations where inferences reference type parameters for which the inferences are being made. This was reported fixed by #57403, but the situation still persists as demonstrated by #61667.
Our type inference infrastructure makes a core assumption that inferences aren't circular, and while #57403 manages to permit them in certain cases, it is unfortunately not a general solution. For that reason, this PR reverts #57403 and instead puts in place logic that ensures outer type parameters in contextual types are instantiated (possibly just to their constraints) before inferences are made to return types of inner function calls. Specifically, in this code
the result of
wrap(id)has type{ x: <T>(x: T) => T }and is contextually typed by{ x: (a: A) => R }. Previously, we'd infer(x: A) => Aas the type argument forXin the call towrap, propagate that into the{ x: (x: A) => A }result type, and then infer from that result type to{ x: (a: A) => R }, producing the inferenceAforA. Which doesn't make sense (or at least would require a more sophisticated unification algorithm to reason about). With this PR, we ensure that the contextual type for thewrap(id)call is instantiated to be free of references to outer type parameters. This means that the contextual type is{ x: (x: unknown) => unknown }and thus we make low priority inferences ofunknownforAandR. We then subsequently make a better inference of1forA.It would be nice if the example above would manage to propagate the inference of
1forAall the way through toR, but we lack the machinery to do so (because we don't perform full unification). We are equipped to do it for "unwrapped" functions like<T>(x: T) => T, but not for generic functions nested in object types. It's simply a design limitation.Because this PR reverts #57403, it is technically a breaking change. But it wasn't working right in the first place.
Fixes #59937.
Fixes #61667.