Improve inference for context sensitive functions within reverse mapped types #54029
Conversation
src/compiler/checker.ts
Outdated
| const sourceValueDeclaration = sourceType.symbol?.valueDeclaration; | ||
| if (sourceValueDeclaration) { |
There was a problem hiding this comment.
I expect that this part is not fully correct. What I'd like to do here is to only get sourceValueDeclaration if it's an anonymous/fresh/smth declaration within within the argument position. I'm not sure how to check for this.
There was a problem hiding this comment.
Use findAncestor to check for argument position membership, use ObjectFlags.FreshLiteral to check for freshness on the type.
There was a problem hiding this comment.
I added .valueDeclaration to reverse mapped properties. This grants me clean access to the node I'm looking for and fixes an issue with array literals used as values of object reverse mapped types (test case). I think that this gives me now exactly what I'm looking for when combined with the freshness check and I don't need to do any findAncestor traversal.
src/compiler/checker.ts
Outdated
| inferTypes([inference], sourceType, templateType); | ||
| const sourceValueDeclaration = sourceType.symbol?.valueDeclaration; | ||
| if (sourceValueDeclaration) { | ||
| const intraExpressionInferenceSites = getInferenceContext(sourceValueDeclaration)?.intraExpressionInferenceSites?.filter(site => isNodeDescendantOf(site.node, sourceValueDeclaration)); |
There was a problem hiding this comment.
The .filter here is unfortunate but my current goal is to make it correct and only after that I can try to make it fast 😉
The goal here is to only try to infer from the intra expression inference sites relevant for this specific reverse mapped symbol. Perhaps the simplest and good enough solution would be to get an appropriate trailing slice of all intraExpressionInferenceSites.
They are "aggregated" throughout the call to checkExpressionWithContextualType so when encountering a first context sensitive function it's a single-element array and when encountering a second one it's a two-element array and so on. inferFromIntraExpressionSites happens while pushing items into intraExpressionInferenceSites so that's why the producer has to always come first before the consumer (TS playground):
declare function f<T>(arg: {
produce: (n: string) => T;
consume: (x: T) => void;
}): void;
f({
produce: (n) => n,
consume: (x) => x.toLowerCase(), // ok, `x` is inferred
});
f({
consume: (x) => x.toLowerCase(), // doesn't work, 'x' is of type 'unknown'.(18046)
produce: (n) => n,
});And thus, by extension... if we'd look for "relevant" nodes from the end we could slice the trailing elements until we meet one that is not relevant. Because intra intra expression inference sites are dependent on source order this should work just fine as it should be guaranteed that by using such a trailing slice we use all current "relevant" nodes and nothing else
There was a problem hiding this comment.
But doesn't this all just imply we should store intra-expression inference sites in a per-property map or something?
There was a problem hiding this comment.
do u mean properties of the reverse mapped type? IIRC we might not know if a property is going to end up as property of such - or I don't know how to check that. So I don't know how to aggregate such a structure since I don't know when it should be created in the first place.
There was a problem hiding this comment.
Since I opened this PR I got a way better understanding of how the whole intra-expression inference works. What is in this PR right now isn't ideal on the high level of things since it comes with a perf penalty cost. I'm trying to figure out how to best store those sites to accommodate the needs of this change and I'm hitting the wall.
Some things to understand about this inference type:
- those sites are continuously ~ gathered in objects/arrays and they are cleared when something "pulls" the information from them
- this is OK because the inference actually infers into all type parameters and not only into the one that got "pulled"
- given the recursive nature of the algorithm sites are gathered in the reverse order - ancestor nodes might only be gathered after descendants because we need to exit the inner work to get the type of an ancestor
- there is quite some repetition in the inference here because if we end up inferring from the ancestor that will also include inferring from its descendants and those descendants might also be in the gathered sites. I was experimenting with some changes to the algorithm to "replace" inner sites with outer ones to skip over some redundant work here. That's only a partial solution though because we can always end up with a sequence like this: push a.b.c > replace with a.b > pull > push a. In a situation like this, we might still infer from the whole
aeven though we already inferred froma.band there is no mechanism to "skip over" this slice of the object. So I didn't end up pursuing this optimization as it seems that it's only a partial one. - In the most common situations though we usually just enter a different part of the node after pulling from those sites so the redundancy problem is likely not that big. Often we just push something, pull from it, clear the sites, push again info unrelated to what we already processed and the cycle continues. So what this PR does right now is that it doesn't clear those sites. This isn't incorrect - it's just redundant since all sites are left behind and each pull infers from all the ones that were gathered up so far.
- Reverse mapped type inference happens at different stages of the overall algorithm, it's a little bit "tacked on" the standard inference thing - it doesn't exactly play by the same rules etc. A special type/symbol is set up as an inference candidate for it and when its members are accessed then we infer from those. This is nice because the whole thing doesn't have to be processed immediately, it happens "on-demand"
- So what happens today on
mainis that those intra-expression inference sites are cleared before the reverse mapped type has a chance to infer from it. - What we can deduce from all of that is that we still need to clear up those sites for the "regular inference" but somehow "keep them around" for the reverse mapped type inference.
- Ideally, we should also have a way of "scoping" those sites within any given object/array property so the reverse mapped type inference could only try to infer from the sites relevant to that slice of an object instead of trying to infer from all of the ones that we were able to gather.
So I'm looking for some way to store/read/clear those sites without hitting any of the mentioned drawbacks and for two different/similar-ush purposes but I can't figure this out so far. cc @jakebailey
src/compiler/checker.ts
Outdated
| pushContextualType(sourceValueDeclaration as any as Expression, templateType, /*isCache*/ false); | ||
| inferFromIntraExpressionSites([inference], intraExpressionInferenceSites); | ||
| popContextualType(); |
There was a problem hiding this comment.
At first this felt quite hacky but now I'm leaning towards saying that it's the easiest and cleanest solution to this problem. The problem that this is solving (together with the getApparentTypeOfContextualType(sourceValueDeclaration.parent.parent) call a few lines back is that it avoids instantiating the property name. Essentially it allows me use the bare templateType without substituting indexed mapped types so inferFromIntraExpressionSites can see a target type like (x: string) => T[K] instead of (x: string) => T["a"] and thus it can infer things for T[K].
I wasn't sure what value I should pass as isCache here though. I'm pretty sure that @ahejlsberg would know this and could advise here
| tests/cases/compiler/mappedTypeContextualTypesApplied.ts(18,10): error TS7023: 'foo' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. | ||
| tests/cases/compiler/mappedTypeContextualTypesApplied.ts(18,15): error TS7006: Parameter 's' implicitly has an 'any' type. | ||
| tests/cases/compiler/mappedTypeContextualTypesApplied.ts(19,10): error TS7023: 'foo' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. | ||
| tests/cases/compiler/mappedTypeContextualTypesApplied.ts(19,15): error TS7006: Parameter 's' implicitly has an 'any' type. |
There was a problem hiding this comment.
Those are not expected. I'm almost certain that all of those would get fixed if this PR would get reviewed and merged: #52095 . This currently fails exactly because the contextual type involves an intersection and getTypeOfPropertyOfContextualType doesn't handle such cases appropriately.
| tests/cases/conformance/types/typeRelationships/typeInference/intraExpressionInferencesReverseMappedTypes.ts(67,21): error TS18046: 'x' is of type 'unknown'. | ||
| tests/cases/conformance/types/typeRelationships/typeInference/intraExpressionInferencesReverseMappedTypes.ts(71,21): error TS18046: 'x' is of type 'unknown'. | ||
| tests/cases/conformance/types/typeRelationships/typeInference/intraExpressionInferencesReverseMappedTypes.ts(80,21): error TS18046: 'x' is of type 'unknown'. | ||
| tests/cases/conformance/types/typeRelationships/typeInference/intraExpressionInferencesReverseMappedTypes.ts(86,21): error TS18046: 'x' is of type 'unknown'. | ||
| tests/cases/conformance/types/typeRelationships/typeInference/intraExpressionInferencesReverseMappedTypes.ts(95,21): error TS18046: 'x' is of type 'unknown'. | ||
| tests/cases/conformance/types/typeRelationships/typeInference/intraExpressionInferencesReverseMappedTypes.ts(101,21): error TS18046: 'x' is of type 'unknown'. |
There was a problem hiding this comment.
All of those relate to the tuple-based tests that I added and those are not expected.
It doesn't work because in tuples both T and T[K] are not reverse mapped symbols/types and thus the inferReverseMappedType isn't called from within checkExpressionWithContextualType (and only within that call intraExpressionInferenceSites and "pushed inference contexts" are available).
With objects createReverseMappedType only creates a ReverseMapped type and doesn't do much else. inferReverseMappedType gets called for properties later from getTypeOfReverseMappedSymbol). However, with tuples it calls inferReverseMappedType eagerly:
https://github.dev/microsoft/TypeScript/blob/eb014a26522dd809ae4d0e85634a62eabda2755a/src/compiler/checker.ts#L24090-L24101
I could try to fix this in this PR but I also feel like this would require changes that could be moved to a separate followup PR. This PR could focus on the base mechanism for this improvement and on object-based cases.
44a9df3 to
557cd99
Compare
…verse-mapped-types # Conflicts: # src/compiler/checker.ts
…for `ReverseMapped` types
…verse mapped types
| export function getDeclarationModifierFlagsFromSymbol(s: Symbol, isWrite = false): ModifierFlags { | ||
| if (s.valueDeclaration) { | ||
| const checkFlags = getCheckFlags(s); | ||
| if (!(checkFlags & CheckFlags.ReverseMapped) && s.valueDeclaration) { |
There was a problem hiding this comment.
Since I added .valueDeclaration to reverse mapped properties, we need to ignore this here - otherwise, we break stripping of readonly modifiers that is required by #12589
There was a problem hiding this comment.
Basically, this change just maintains compatibility with the old behavior as 0 was always returned from here in the implicit case of checkFlags & CheckFlags.ReverseMapped
…verse-mapped-types
…verse-mapped-types
f6bbf41 to
a8e8a24
Compare
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
… inferrable source
…verse-mapped-types
…verse-mapped-types
it's an extension of the algorithm introduced in #48538
closes #53018
fixes #48798