Cache inference for recursive mapped types #20622
Conversation
Previously recursive mapped type inferences avoided cycles when looking for inferences. Now, they instead cache as they explore the object structure so that symbol-identical inferences to recursive mapped types don't repeat. This is needed, for example, to allow lodash's use of PartialDeep<T> to be usable with XMLHttpRequest and similarly large object types. Also change to use the existing visited cache instead of a separate stack.
Compilation didn't finish before! Now it does.
|
Improves the fix in #20370. Turns out the test there was too small for real types like |
There was a problem hiding this comment.
Sans the cache key being a bit odd (maybe a comment explaining that the cache can contain lists of symbols and types would be in order), this looks pretty straightforward.
Oh! One more thing: The type baseline has inferences like readonly readyState: { toString: {}; toFixed: {}; toExponential: {}; toPrecision: {}; valueOf: {}; toLocaleString: {}; };. I know you mentioned skipping primitives before, but preventing an inference like this is probably a good reason to actually do it. If I recall, we also don't actually apply mapped types to primitives Partial<number> is just number, so inferring a mapped type to a primitive (even structurally) seems like it's always incorrect. Maybe deserves another issue.
src/compiler/checker.ts
Outdated
| @@ -11485,13 +11484,12 @@ namespace ts { | |||
| // such that direct inferences to T get priority over inferences to Partial<T>, for example. | |||
| const inference = getInferenceInfoForType((<IndexType>constraintType).type); | |||
| if (inference && !inference.isFixed) { | |||
| const key = (source.symbol ? getSymbolId(source.symbol) + "," : "") + getSymbolId(target.symbol); | |||
| if (contains(mappedTypeStack, key)) { | |||
| const key = (source.symbol ? getSymbolId(source.symbol) + "s" : "") + getSymbolId(target.symbol); | |||
There was a problem hiding this comment.
Normally we distinguish different types of cache keys with a leading character, rather than a different separator, see getLiteralType for prior art. Plus, I think something like T123,12 vs S11,14 is going to make more sense in the debugger than 123,12 vs 11s14.
Or rather, it's like that when inferring from |
|
It's easy enough to skip primitives. I'll poke at inferring them too, and include it if it's simple enough. |
|
OK, I infer homomorphic mapped types to primitives. The change is simple but I'm not 100% sure it's correct. @ahejlsberg can you take a look? |
|
I still see |
src/compiler/checker.ts
Outdated
| function inferTypeForHomomorphicMappedType(source: Type, target: MappedType, mappedTypeStack: string[]): Type { | ||
| function inferTypeForHomomorphicMappedType(source: Type, target: MappedType, visited: Map<Type | undefined>): Type { | ||
| if (source.flags & TypeFlags.Primitive) { | ||
| return source; |
There was a problem hiding this comment.
Is this tested anywhere in a more obvious way? e.g.
// @declaration: true
type MyReadonly<T> = { readonly [K in keyof T]: T[K] };
export declare function foo<T>(x: MyReadonly<T>): T;
export let x = foo(100);The original inference cache lookup used Map.get instead of Map.has. With that fix, I had to split the caches back into two so that the original type-only lookup restarted for each call inside inferTypeForHomomorphicMappedTypes.
|
closing in favor of #20711 |
Previously recursive mapped type inferences avoided cycles when looking for inferences. Now, they instead cache as they explore the object structure so that symbol-identical inferences to recursive mapped types don't repeat. This is needed, for example, to allow lodash's use of
PartialDeep<T>to be usable withXMLHttpRequestand similarly large object types.Also change to use the existing visited cache instead of a separate stack.
Edit: Also infer primitives directly to homomorphic mapped types. This improves nested inferences a lot.