Improve inference for context sensitive functions within reverse mapped types #54029
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7539,13 +7539,14 @@ export function getCheckFlags(symbol: Symbol): CheckFlags { | |
|
|
||
| /** @internal */ | ||
| export function getDeclarationModifierFlagsFromSymbol(s: Symbol, isWrite = false): ModifierFlags { | ||
| const checkFlags = getCheckFlags(s); | ||
| if (!(checkFlags & CheckFlags.ReverseMapped) && s.valueDeclaration) { | ||
|
There was a problem hiding this comment. Since I added There was a problem hiding this comment. Basically, this change just maintains compatibility with the old behavior as |
||
| const declaration = (isWrite && s.declarations && find(s.declarations, isSetAccessorDeclaration)) | ||
| || (s.flags & SymbolFlags.GetAccessor && find(s.declarations, isGetAccessorDeclaration)) || s.valueDeclaration; | ||
| const flags = getCombinedModifierFlags(declaration); | ||
| return s.parent && s.parent.flags & SymbolFlags.Class ? flags : flags & ~ModifierFlags.AccessibilityModifier; | ||
| } | ||
| if (checkFlags & CheckFlags.Synthetic) { | ||
| // NOTE: potentially unchecked cast to TransientSymbol | ||
| const checkFlags = (s as TransientSymbol).links.checkFlags; | ||
| const accessModifier = checkFlags & CheckFlags.ContainsPrivate ? ModifierFlags.Private : | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| intraExpressionInferencesReverseMappedTypes.ts(67,21): error TS18046: 'x' is of type 'unknown'. | ||
| intraExpressionInferencesReverseMappedTypes.ts(71,21): error TS18046: 'x' is of type 'unknown'. | ||
| intraExpressionInferencesReverseMappedTypes.ts(80,21): error TS18046: 'x' is of type 'unknown'. | ||
| intraExpressionInferencesReverseMappedTypes.ts(86,21): error TS18046: 'x' is of type 'unknown'. | ||
| intraExpressionInferencesReverseMappedTypes.ts(95,21): error TS18046: 'x' is of type 'unknown'. | ||
| intraExpressionInferencesReverseMappedTypes.ts(101,21): error TS18046: 'x' is of type 'unknown'. | ||
|
|
||
|
|
||
| ==== intraExpressionInferencesReverseMappedTypes.ts (6 errors) ==== | ||
| // repro cases based on https://github.com/microsoft/TypeScript/issues/53018 | ||
|
|
||
| declare function f<T>( | ||
| arg: { | ||
| [K in keyof T]: { | ||
| produce: (n: string) => T[K]; | ||
| consume: (x: T[K]) => void; | ||
| }; | ||
| } | ||
| ): T; | ||
|
|
||
| const res1 = f({ | ||
| a: { | ||
| produce: (n) => n, | ||
| consume: (x) => x.toLowerCase(), | ||
| }, | ||
| b: { | ||
| produce: (n) => ({ v: n }), | ||
| consume: (x) => x.v.toLowerCase(), | ||
| }, | ||
| }); | ||
|
|
||
| const res2 = f({ | ||
| a: { | ||
| produce: function () { | ||
| return "hello"; | ||
| }, | ||
| consume: (x) => x.toLowerCase(), | ||
| }, | ||
| b: { | ||
| produce: function () { | ||
| return { v: "hello" }; | ||
| }, | ||
| consume: (x) => x.v.toLowerCase(), | ||
| }, | ||
| }); | ||
|
|
||
| const res3 = f({ | ||
| a: { | ||
| produce() { | ||
| return "hello"; | ||
| }, | ||
| consume: (x) => x.toLowerCase(), | ||
| }, | ||
| b: { | ||
| produce() { | ||
| return { v: "hello" }; | ||
| }, | ||
| consume: (x) => x.v.toLowerCase(), | ||
| }, | ||
| }); | ||
|
|
||
| declare function f2<T extends unknown[]>( | ||
| arg: [ | ||
| ...{ | ||
| [K in keyof T]: { | ||
| produce: (n: string) => T[K]; | ||
| consume: (x: T[K]) => void; | ||
| }; | ||
| } | ||
| ] | ||
| ): T; | ||
|
|
||
| const res4 = f2([ | ||
| { | ||
| produce: (n) => n, | ||
| consume: (x) => x.toLowerCase(), | ||
| ~ | ||
| !!! error TS18046: 'x' is of type 'unknown'. | ||
| }, | ||
| { | ||
| produce: (n) => ({ v: n }), | ||
| consume: (x) => x.v.toLowerCase(), | ||
| ~ | ||
| !!! error TS18046: 'x' is of type 'unknown'. | ||
| }, | ||
| ]); | ||
|
|
||
| const res5 = f2([ | ||
| { | ||
| produce: function () { | ||
| return "hello"; | ||
| }, | ||
| consume: (x) => x.toLowerCase(), | ||
| ~ | ||
| !!! error TS18046: 'x' is of type 'unknown'. | ||
| }, | ||
| { | ||
| produce: function () { | ||
| return { v: "hello" }; | ||
| }, | ||
| consume: (x) => x.v.toLowerCase(), | ||
| ~ | ||
| !!! error TS18046: 'x' is of type 'unknown'. | ||
| }, | ||
| ]); | ||
|
|
||
| const res6 = f2([ | ||
| { | ||
| produce() { | ||
| return "hello"; | ||
| }, | ||
| consume: (x) => x.toLowerCase(), | ||
| ~ | ||
| !!! error TS18046: 'x' is of type 'unknown'. | ||
| }, | ||
| { | ||
| produce() { | ||
| return { v: "hello" }; | ||
| }, | ||
| consume: (x) => x.v.toLowerCase(), | ||
| ~ | ||
| !!! error TS18046: 'x' is of type 'unknown'. | ||
| }, | ||
| ]); | ||
|
|
||
| declare function f3<T>( | ||
| arg: { | ||
| [K in keyof T]: { | ||
| other: number, | ||
| produce: (n: string) => T[K]; | ||
| consume: (x: T[K]) => void; | ||
| }; | ||
| } | ||
| ): T; | ||
|
|
||
| const res7 = f3({ | ||
| a: { | ||
| other: 42, | ||
| produce: (n) => n, | ||
| consume: (x) => x.toLowerCase(), | ||
| }, | ||
| b: { | ||
| other: 100, | ||
| produce: (n) => ({ v: n }), | ||
| consume: (x) => x.v.toLowerCase(), | ||
| }, | ||
| }); | ||
|
|
||
| declare function f4<T>( | ||
| arg: { | ||
| [K in keyof T]: [ | ||
| (n: string) => T[K], | ||
| (x: T[K]) => void | ||
| ]; | ||
| } | ||
| ): T; | ||
|
|
||
| const res8 = f4({ | ||
| a: [ | ||
| (n) => n, | ||
| (x) => x.toLowerCase(), | ||
| ], | ||
| b: [ | ||
| (n) => ({ v: n }), | ||
| (x) => x.v.toLowerCase(), | ||
| ], | ||
| }); | ||
|
|
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
.filterhere 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
checkExpressionWithContextualTypeso 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.inferFromIntraExpressionSiteshappens while pushing items intointraExpressionInferenceSitesso that's why the producer has to always come first before the consumer (TS playground):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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.mainis that those intra-expression inference sites are cleared before the reverse mapped type has a chance to infer from it.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