Fix type variable leaks and cache inconsistencies #61668
+1,071
−107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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! |
|
@ahejlsberg Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@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 Everything looks good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.