Skip to content

Treat array literal contextually typed by homomorphic mapped types as in tuple context #56555

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

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

Andarist
Copy link
Contributor

fixes #55382

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Nov 27, 2023
Comment on lines +11 to +14
declare function test2<T extends readonly unknown[]>(arg: {
[K in keyof T]: T[K];
}): T;
const result2 = test2(["foo", 42]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO this alone is worth this improvement. Today we can infer tuples using mapped types but we need to awkwardly spread them (TS playground):

declare function broken<T extends readonly unknown[]>(arg: {
  [K in keyof T]: T[K];
}): T;
const result1 = broken(["foo", 42]);
//    ^? const result1: (string | number)[]

declare function workaround<T extends readonly unknown[]>(
  arg: [
    ...{
      [K in keyof T]: T[K];
    },
  ],
): T;
const result2 = workaround(["foo", 42]);
//    ^? const result2: [string, number]

That makes the recursive scenarios much more complicated because we can't "just" iterate over the recursive type parameter and have that tuple context assigned to array literals.

IMHO, the mapped type itself strongly indicates that the user wants to have the tuples inferred as they might want to type each element separately.

Choose a reason for hiding this comment

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

It doesn’t infer the tuple type if T is marked const? I thought that was the whole reason behind const type params, because people wanted to infer tuples instead of (T | U | V)[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn’t infer the tuple type if T is marked const?

It does (although not always, see this TS playground, but that is being fixed by #55794

I thought that was the whole reason behind const type params, because people wanted to infer tuples instead of (T | U | V)[]

I don't think that was the whole reason behind it. The reason was that people want to use schema-like arguments for which often strings, numbers, etc have to be kept as literal strings. The fact that tuples are also preserved is just a byproduct of this - since a tuple is a more specific type than an array, just like a string literal is a more specific type than a string type.

Regardless... there are times when constness of the whole argument is not something that I want and preserving the "tuple context" is all that I want. I might want to infer [string] and not let's say ["foo"].

Copy link

@fatcerberus fatcerberus Nov 27, 2023

Choose a reason for hiding this comment

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

I might want to infer [string] and not let's say ["foo"].

Hmm, now that I think about this, there’s no precedent for that in the type system today, is there? When inferring arrays in situ you either write [ "foo", "bar" ] and get string[] or else add an as const and get readonly [ "foo", "bar" ].

So the upshot is that this change would add a way to infer [ string, string ] from an array literal via a generic and people will start writing more constrained identity functions to accomplish that (…and likely complain about having to do so 😜)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can already do this today - you have to use the workaround mentioned above. This PR makes this whole thing simpler while also making it possible (or just way easier) to write recursive types that preserve tuples like this (especially in the "mixed" scenarios when your type is supposed to iterate through both object and array types).

I also think that the workaround above isn't that intuitive. After all an unconstrained generic mapped type when iterating over a tuple preserves its tupleness.I would also conceptually consider all array literal expressions to start as tuples~. It's the widening behavior that usually widens them to arrays unless some other hint that the tuple should be preserved is given. So I just expect that contextual mapped type to be considered as such a hint.

Choose a reason for hiding this comment

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

That’s fair. To be honest it just feels wrong that generic inference seems to have more knobs than what you can control inline. In general, the fewer CIFs I have to write to get type inference to do what I want, the better.

@Andarist Andarist force-pushed the reverse-mapped-type-tuple-context branch from 50099fa to ba2f465 Compare December 8, 2023 11:59
@Andarist
Copy link
Contributor Author

Andarist commented Dec 8, 2023

@jakebailey would u be so kind as to create a playground for this one? :)

@jakebailey
Copy link
Member

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 8, 2023

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at ba2f465. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 8, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at ba2f465. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 8, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at ba2f465. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 8, 2023

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at ba2f465. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 8, 2023

Heya @jakebailey, I've started to run the regular perf test suite on this PR at ba2f465. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 8, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/159029/artifacts?artifactName=tgz&fileId=A40D72A4F999A91CD8F225E0EAFB3733B247455261D8AECDFE7F6350159B71D702&fileName=/typescript-5.4.0-insiders.20231208.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-56555-7".;

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/56555/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

We spoke about this in the design meeting finally and decided this was a good idea (tm) - hopefully it won't cause any unexpected weirdness in the broader community when merged. :)

@weswigham weswigham merged commit 2c71621 into microsoft:main Dec 13, 2023
@Andarist Andarist deleted the reverse-mapped-type-tuple-context branch December 13, 2023 23:17
c0sta pushed a commit to c0sta/TypeScript that referenced this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Recursive reverse mapped types don't work well with mixed object and tuple structures
5 participants