Strip literal freshness when contextually typed by instantiable type that has return mapper constraint inferences #50759
Conversation
This file contains 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
…that has return mapper constraint inferences
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot user test this inline |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 410d2f7. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the diff-based user code test suite on this PR at 410d2f7. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 410d2f7. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 410d2f7. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here are the results of running the user test suite comparing Everything looks good! |
|
@andrewbranch Here they are:
CompilerComparison Report - main..50759
System
Hosts
Scenarios
TSServerComparison Report - main..50759
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
andrewbranch
changed the title
typescript-bot
added
For Uncommitted Bug
typescript-bot
added
For Milestone Bug
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 410d2f7. You can monitor the build here. |
|
Hey @andrewbranch, 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 |
|
@typescript-bot user test inline |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at e12b99d. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at e12b99d. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at e12b99d. You can monitor the build here. |
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
|
@andrewbranch Here they are:
CompilerComparison Report - main..50759
System
Hosts
Scenarios
TSServerComparison Report - main..50759
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hmm, I'm not sure about this one. The thing that strikes me is the convoluted use of generics in declare function asLiteral<T extends string | boolean | number>(...literals: T[]): (val: unknown) => val is T;I'm not sure I think we need to do anything other than recommend this rewrite. |
|
@ahejlsberg what about the other issue this fixes? #50787 type Model = { s: string; b: boolean }
let pick: <Keys extends keyof Model>(properties: readonly Keys[]) => Pick<Model, Keys>
let transform: <T>(obj: T) => T
const result1 = transform(pick(["s"]))
// 4.7: Pick<Model, "s">
// 4.8: Pick<Model, keyof Model> |
|
@andrewbranch The root cause of #50787 is that we consider the apparent type of an unconstrained type parameter to be I'd suggest changing these lines in source = getReducedType(source);
if (!(priority & InferencePriority.NoConstraints && source.flags & (TypeFlags.Intersection | TypeFlags.Instantiable))) {
const apparentSource = getApparentType(source);to this source = getReducedType(source);
if (!(priority & InferencePriority.NoConstraints && source.flags & (TypeFlags.Intersection | TypeFlags.Instantiable)) &&
!(source.flags & TypeFlags.Instantiable && !getBaseConstraintOfType(source))) {
const apparentSource = getApparentType(source);i.e. skip making inferences where there is no constraint, regardless of mode. It fixes the issue with no changes in the test suite. But of course it may affect some of the extended test suites. |
|
It’s not clear though why specifying a very wide constraint like type Model = { s: string; b: boolean }
declare let pick: <Keys extends keyof Model>(properties: readonly Keys[]) => Pick<Model, Keys>
declare let transform2: <T extends {}>(obj: T) => T
const result2 = transform2(pick(["s"]))
// 4.7: Pick<Model, "s">
// 4.8: Pick<Model, keyof Model>
// This PR: Pick<Model, "s">
// Suggested change: Pick<Model, keyof Model>I don’t see a good reason why |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
From the test:
When inferring
TforasLiteral(...), we get the type of each argument (starting with"these") with the contextual typeT[0],T[1], and so on. We use the contextual type to determine whether to widen or strip freshness from literal types, but we try to instantiate it first, using inferences made from the return type. The return type inference turns out to map theTofasLiteralto theTofcreateObjectGuard(which, uh, seems wrong, but doesn’t actually matter here), and since the former is not assignable to the latter, the inference goes to the constraint,(string | boolean | number)[]. So the contextual typeT[0]is instantiated tostring | number(through some special unrelated logic to filter booleans out of union contextual types). That type is not a literal type, so"these"remains fresh, allowing it to widen tostringas the spread argument. The weird thing is, if we had just asked whether"these"was a literal type of the contextual typeT[0]whereT extends (string | boolean | number)[]instead of eagerly instantiating that type, we would have said yes; when an instantiable type is constrained to a primitive, we want to preserve the literalness of arguments it contextually types. So, this PR asks whether a literal type could be a literal of its contextual type, instantiated or uninstantiated.I think a possible alternative would be to discard contextual type instantiations where the return type mapper simply produces an inference to the constraint of the type parameter it’s inferring, indicating that nothing useful came from trying to infer from the return type.
Fixes #50635
Fixes #50787