-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Default reverse mapped type inference to its constraint #56300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Andarist
wants to merge
3
commits into
microsoft:main
Choose a base branch
from
Andarist:reverse-mapped-type-default-to-constraint
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
154 changes: 154 additions & 0 deletions
154
tests/baselines/reference/reverseMappedDefaultInferenceToConstraint.symbols
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,154 @@ | ||
| //// [tests/cases/compiler/reverseMappedDefaultInferenceToConstraint.ts] //// | ||
|
|
||
| === reverseMappedDefaultInferenceToConstraint.ts === | ||
| // https://github.com/microsoft/TypeScript/issues/56241 | ||
|
|
||
| interface ParameterizedObject { | ||
| >ParameterizedObject : Symbol(ParameterizedObject, Decl(reverseMappedDefaultInferenceToConstraint.ts, 0, 0)) | ||
|
|
||
| type: string; | ||
| >type : Symbol(ParameterizedObject.type, Decl(reverseMappedDefaultInferenceToConstraint.ts, 2, 31)) | ||
|
|
||
| params?: Record<string, unknown>; | ||
| >params : Symbol(ParameterizedObject.params, Decl(reverseMappedDefaultInferenceToConstraint.ts, 3, 15)) | ||
| >Record : Symbol(Record, Decl(lib.es5.d.ts, --, --)) | ||
| } | ||
|
|
||
| declare function setup< | ||
| >setup : Symbol(setup, Decl(reverseMappedDefaultInferenceToConstraint.ts, 5, 1)) | ||
|
|
||
| TContext, | ||
| >TContext : Symbol(TContext, Decl(reverseMappedDefaultInferenceToConstraint.ts, 7, 23)) | ||
|
|
||
| TGuards extends Record<string, ParameterizedObject["params"] | undefined>, | ||
| >TGuards : Symbol(TGuards, Decl(reverseMappedDefaultInferenceToConstraint.ts, 8, 11)) | ||
| >Record : Symbol(Record, Decl(lib.es5.d.ts, --, --)) | ||
| >ParameterizedObject : Symbol(ParameterizedObject, Decl(reverseMappedDefaultInferenceToConstraint.ts, 0, 0)) | ||
|
|
||
| >(_: { | ||
| >_ : Symbol(_, Decl(reverseMappedDefaultInferenceToConstraint.ts, 10, 2)) | ||
|
|
||
| types: { | ||
| >types : Symbol(types, Decl(reverseMappedDefaultInferenceToConstraint.ts, 10, 6)) | ||
|
|
||
| context: TContext; | ||
| >context : Symbol(context, Decl(reverseMappedDefaultInferenceToConstraint.ts, 11, 10)) | ||
| >TContext : Symbol(TContext, Decl(reverseMappedDefaultInferenceToConstraint.ts, 7, 23)) | ||
|
|
||
| }; | ||
| guards: { | ||
| >guards : Symbol(guards, Decl(reverseMappedDefaultInferenceToConstraint.ts, 13, 4)) | ||
|
|
||
| [K in keyof TGuards]: (context: TContext, params: TGuards[K]) => void; | ||
| >K : Symbol(K, Decl(reverseMappedDefaultInferenceToConstraint.ts, 15, 5)) | ||
| >TGuards : Symbol(TGuards, Decl(reverseMappedDefaultInferenceToConstraint.ts, 8, 11)) | ||
| >context : Symbol(context, Decl(reverseMappedDefaultInferenceToConstraint.ts, 15, 27)) | ||
| >TContext : Symbol(TContext, Decl(reverseMappedDefaultInferenceToConstraint.ts, 7, 23)) | ||
| >params : Symbol(params, Decl(reverseMappedDefaultInferenceToConstraint.ts, 15, 45)) | ||
| >TGuards : Symbol(TGuards, Decl(reverseMappedDefaultInferenceToConstraint.ts, 8, 11)) | ||
| >K : Symbol(K, Decl(reverseMappedDefaultInferenceToConstraint.ts, 15, 5)) | ||
|
|
||
| }; | ||
| }): TGuards; | ||
| >TGuards : Symbol(TGuards, Decl(reverseMappedDefaultInferenceToConstraint.ts, 8, 11)) | ||
|
|
||
| const result = setup({ | ||
| >result : Symbol(result, Decl(reverseMappedDefaultInferenceToConstraint.ts, 19, 5)) | ||
| >setup : Symbol(setup, Decl(reverseMappedDefaultInferenceToConstraint.ts, 5, 1)) | ||
|
|
||
| types: { | ||
| >types : Symbol(types, Decl(reverseMappedDefaultInferenceToConstraint.ts, 19, 22)) | ||
|
|
||
| context: { | ||
| >context : Symbol(context, Decl(reverseMappedDefaultInferenceToConstraint.ts, 20, 10)) | ||
|
|
||
| count: 100, | ||
| >count : Symbol(count, Decl(reverseMappedDefaultInferenceToConstraint.ts, 21, 14)) | ||
|
|
||
| }, | ||
| }, | ||
| guards: { | ||
| >guards : Symbol(guards, Decl(reverseMappedDefaultInferenceToConstraint.ts, 24, 4)) | ||
|
|
||
| checkFoo: (_, { foo }: { foo: string }) => foo === "foo", | ||
| >checkFoo : Symbol(checkFoo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 25, 11)) | ||
| >_ : Symbol(_, Decl(reverseMappedDefaultInferenceToConstraint.ts, 26, 15)) | ||
| >foo : Symbol(foo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 26, 19)) | ||
| >foo : Symbol(foo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 26, 28)) | ||
| >foo : Symbol(foo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 26, 19)) | ||
|
|
||
| alwaysTrue: (_) => true, | ||
| >alwaysTrue : Symbol(alwaysTrue, Decl(reverseMappedDefaultInferenceToConstraint.ts, 26, 61)) | ||
| >_ : Symbol(_, Decl(reverseMappedDefaultInferenceToConstraint.ts, 27, 17)) | ||
|
|
||
| }, | ||
| }); | ||
|
|
||
| declare function foo< | ||
| >foo : Symbol(foo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 29, 3)) | ||
|
|
||
| T extends Record<PropertyKey, U>, | ||
| >T : Symbol(T, Decl(reverseMappedDefaultInferenceToConstraint.ts, 31, 21)) | ||
| >Record : Symbol(Record, Decl(lib.es5.d.ts, --, --)) | ||
| >PropertyKey : Symbol(PropertyKey, Decl(lib.es5.d.ts, --, --)) | ||
| >U : Symbol(U, Decl(reverseMappedDefaultInferenceToConstraint.ts, 32, 35)) | ||
|
|
||
| U extends number | boolean, | ||
| >U : Symbol(U, Decl(reverseMappedDefaultInferenceToConstraint.ts, 32, 35)) | ||
|
|
||
| >( | ||
| a: { | ||
| >a : Symbol(a, Decl(reverseMappedDefaultInferenceToConstraint.ts, 34, 2)) | ||
|
|
||
| [K in keyof T]: (arg: T[K]) => void; | ||
| >K : Symbol(K, Decl(reverseMappedDefaultInferenceToConstraint.ts, 36, 5)) | ||
| >T : Symbol(T, Decl(reverseMappedDefaultInferenceToConstraint.ts, 31, 21)) | ||
| >arg : Symbol(arg, Decl(reverseMappedDefaultInferenceToConstraint.ts, 36, 21)) | ||
| >T : Symbol(T, Decl(reverseMappedDefaultInferenceToConstraint.ts, 31, 21)) | ||
| >K : Symbol(K, Decl(reverseMappedDefaultInferenceToConstraint.ts, 36, 5)) | ||
|
|
||
| }, | ||
| b: U, | ||
| >b : Symbol(b, Decl(reverseMappedDefaultInferenceToConstraint.ts, 37, 4)) | ||
| >U : Symbol(U, Decl(reverseMappedDefaultInferenceToConstraint.ts, 32, 35)) | ||
|
|
||
| ): T; | ||
| >T : Symbol(T, Decl(reverseMappedDefaultInferenceToConstraint.ts, 31, 21)) | ||
|
|
||
| declare const num: number; | ||
| >num : Symbol(num, Decl(reverseMappedDefaultInferenceToConstraint.ts, 41, 13)) | ||
|
|
||
| const result1 = foo( | ||
| >result1 : Symbol(result1, Decl(reverseMappedDefaultInferenceToConstraint.ts, 43, 5)) | ||
| >foo : Symbol(foo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 29, 3)) | ||
| { | ||
| a: (arg) => {}, | ||
| >a : Symbol(a, Decl(reverseMappedDefaultInferenceToConstraint.ts, 44, 3)) | ||
| >arg : Symbol(arg, Decl(reverseMappedDefaultInferenceToConstraint.ts, 45, 8)) | ||
|
|
||
| b: () => {}, | ||
| >b : Symbol(b, Decl(reverseMappedDefaultInferenceToConstraint.ts, 45, 19)) | ||
|
|
||
| }, | ||
| num, | ||
| >num : Symbol(num, Decl(reverseMappedDefaultInferenceToConstraint.ts, 41, 13)) | ||
|
|
||
| ); | ||
|
|
||
| const result2 = foo( | ||
| >result2 : Symbol(result2, Decl(reverseMappedDefaultInferenceToConstraint.ts, 51, 5)) | ||
| >foo : Symbol(foo, Decl(reverseMappedDefaultInferenceToConstraint.ts, 29, 3)) | ||
| { | ||
| a: (arg: 100) => {}, | ||
| >a : Symbol(a, Decl(reverseMappedDefaultInferenceToConstraint.ts, 52, 3)) | ||
| >arg : Symbol(arg, Decl(reverseMappedDefaultInferenceToConstraint.ts, 53, 8)) | ||
|
|
||
| b: () => {}, | ||
| >b : Symbol(b, Decl(reverseMappedDefaultInferenceToConstraint.ts, 53, 24)) | ||
|
|
||
| }, | ||
| num, | ||
| >num : Symbol(num, Decl(reverseMappedDefaultInferenceToConstraint.ts, 41, 13)) | ||
|
|
||
| ); | ||
|
|
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.
Hm. A mutation like this is odd, especially if the
context.compareTypescall causes reentrancy on theinferredType(different places in the comparison could witness different mappers with different results). Instead, having a non-fixing-mapper clone of the type seems prudent.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.
Ah yes, this mutation is just the best I was able to figure out so far when it comes to exposing this mapper to
inferReverseMappedType. I meant to comment on that here but you beat me to it and looked here first :PI could maintain this in some stack or something but OTOH the mutation felt simpler because it just makes it directly accessible at the call site that needs this. It looks odd, especially here, but it's because reverse mapped types just don't participate in the same code path as everything else - all of them are kinda going through their own inference passes, even though they somewhat originate in this "parent" pass (they are not linked to it though).
I was also thinking that perhaps instead of assigning a mapper I could just assign the
instantiatedConstraint. In away, the instantiation that I do now ininferReverseMappedTypeis quite similar to potentialgetIndexedAccessType(instantiatedConstraint, propertyNameType). Would that alleviate the reentrancy concern at all?I'm afraid that I don't understand what should be cloned here and where the clone should be used. Could you elaborate on this?
One extra thing that I was thinking about.
getInferredTypecan return cached but non-fixed results. Does it mean that by doing this thing here we might accidentally~ "fix" the properties of a non-fixed result? Or is it just not a concern because the reverse mapped object with those fixed properties doesn't surviveinferredTypeclears so it would be recreated and its properties would get "fixed" again (but now with the newnonFixingMapper/instantiatedConstraint). I didn't yet have time to investigate how this behaves in practice.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.
Have a copy of the reverse mapped type that stores the
nonFixingMapperinstead of the normal mapper (and probably have that reverse mapped type mutually reference the normal version of the type). (And include that variant-ness in the ID of the reverse mapped type, so they're recognized as separate types in the caches)Yes but no. Reverse mapped types are made in inference, so they can be pretty short-lived, but they're identified structurally - all reverse mapped types with the same source, target, and constraint types are the same reverse mapped types, even when they occur at differing places in inference. So even if it's initially short lived, it can be pulled back out later by another attempt to construct one over the same types. That's actually another issue with this as it currently stands - including the mapper in the type (...by including it in the properties via this honestly roundabout way - passing it into
inferTypeForHomomorphicMappedType/createReverseMappedTypeshould clarify things) should affect the type id, but getting a good structural id out of a mapper isn't simple (since function mappers exist aplenty). This means in this PR right now, reverse mapped types mangle one another - only the first inference context within which a reverse mapped type of a certain structure is used gets cached, which is not correct. I think that since reverse mapped types are made within an inference context, them keeping a reference to said context is probably reasonable, but we'll definitely need to check that potentially duplicating reverse mapped types like that isn't terrible for perf on real projects (since inference contexts (and mappers) are rather throwaway).