Skip to content

When calculating inferred infer type constraints, replace conditionals… #48648

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

Closed

Conversation

weswigham
Copy link
Member

…within mapped type templates with their constraints.

Fixes #44143

…s within mapped type templates with their constraints
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 12, 2022
// well enough (or at all, from what I've seen - it only filters out parameters which are unreferenced _everywhere_,
// rather than those which are unused within the given node).
//
// Each of those relationship issues, in turn, then cause inference of the `infer` type parameter in question to fail.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment block is informative for whomever comes back to this in the future - I looked at two other ways to solve the problem (the conditional in the template type executing differently than the mapped type around it would imply when instantiated in the above fashion), but both ran against other issues in the checker. If one of the other's root issues get fixed, I think they'd be much more elegant constructions to return here than this conditional-constraint-walking logic (ideally the first of the two, since it much more closely matches the structure traversed).

@@ -9,7 +9,7 @@ type KeysWithoutStringIndex<T> =

// Only "foo" | "bar" as expected, [string] index signature removed
type test = KeysWithoutStringIndex<{ [index: string]: string; foo: string; bar: 'baz' }>
>test : never
>test : "foo" | "bar"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun fact - this test already showed the issue, we just weren't looking for it in this test. (Instead we were looking for a constraint error on a different line.)

@sandersn sandersn requested a review from ahejlsberg April 22, 2022 16:13
@RyanCavanaugh
Copy link
Member

@ahejlsberg 🔔

@ahejlsberg
Copy link
Member

ahejlsberg commented Sep 28, 2022

I'm somewhat puzzled by the logic that attempts to add constraints to infer type parameters in a very narrow scenario (inferring from a mapped type to another mapped type where the infer type parameter occurs in the template type). It was added in #43649 and this PR now makes further modifications to it, but I don't see why any of it is needed. Indeed, if I simply delete the code from #43649, everything works as expected, including the broken test in inferConditionalConstraintMappedMember.ts.

@Andarist
Copy link
Contributor

Andarist commented Feb 21, 2023

@ahejlsberg i stumbled upon this one while looking into something else - removing the code from #43649 indeed fixes the expected type result (as this PR does, from never to 'foo' | 'bar') but removing it brings back the assignability error that was fixed by #43649

    export type RemoveIdxSgn<T> = Pick<T, KeysWithoutStringIndex<T>>
                                          ~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2344: Type 'KeysWithoutStringIndex<T>' does not satisfy the constraint 'keyof T'.
!!! error TS2344:   Type 'unknown' is not assignable to type 'keyof T'.

@Andarist
Copy link
Contributor

Andarist commented Mar 5, 2023

There are currently (at least) 3 issues open related to this: #46020 #44143 #44351

All of them would get fixed if #43649 would get reverted (as in, that PR broke them). "Known keys" would get fixed in all of them if this PR here would get merged but the third issue also suffers from index access error and that is not fixed by this PR here.

Note for other reviewers: the main problem here is that keyof doesn't exactly get the same keys as K within mapped types (K within mapped types can be used to iterate over each key, including index signatures whereas keyof "reduces" the union of possible keys). So when the mapped type from the checkType gets unwrapped away and substituted with the constraint of the template (which is the conditional type referring to K) then we essentially "lose" the information for this case because K gets replaced with keyof T (and that gets reduced from 'knownKey1' | 'knownKey2' | string | number to string | number).

@weswigham weswigham marked this pull request as draft August 13, 2024 22:05
@sandersn
Copy link
Member

sandersn commented Apr 1, 2025

@weswigham This has been open for a couple of years. If this isn't the right solution, should we close it?
Is it possible to fix the error that @Andarist mentions some other way?

Either way, this fix is probably minor enough to wait until Corsa has overtaken Strada.

@sandersn sandersn closed this Apr 30, 2025
@github-project-automation github-project-automation bot moved this from Waiting on reviewers to Done in PR Backlog Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

KnownKeys<T> breaking change in 4.3.1-rc
6 participants