Fixed inferred const rest types in signatures #55034
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
Pretty hyped for this one. I've run into so many issues trying to constrain narrowed array types without breaking inference. Couldn't really tell from the linked issue if this will fix a lot of them, but I have faith 🙏 |
|
After messing with this same code for #53255, #53258 (note a similar @typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at fb32b83. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at fb32b83. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at fb32b83. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at fb32b83. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at fb32b83. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at fb32b83. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, 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 |
When learning about this issue I immediately opened your PR to check out if it's related anyhow to this issue. I think that at the end of the day, it isn't and that this PR shouldn't actually impact anybody negatively. This only makes those rests readonly when the type variable is |
Yeah, I'm pretty sure that I was a little overzealous in propagating |
|
@jakebailey Here they are:Comparison Report - main..55034
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
The bug is marked Working As Intended now, so should we close this PR? |
|
The reported issue had some issues in it (it didnt use a readonly constraint). So im not sure if „working as intended” was applied to only what was oroginally reported or if it was applied to everything that was discussed there. I think that this particular thing that I fixed here still has some merit so i’d appreciate if this would get reviewed |
|
I like where this is going, but I think we also need the changes in #55229. For example, the following is broken by this PR without the additional changes: declare function invoke<const T extends unknown[]>(f: (...args: T) => void, ...args: T): T;
invoke((a: string, b: number) => {}, "hello", 123);I know this isn't a particularly likely break, but the inference failure because of non-readonly constraints is just confounding to most users and hard to justify (even if I've tried). |
ye, this tends to be pretty confusing - would it be worth adding some diagnostics that would prompt users to add the |
I'm hoping there's no need for that with #55229 since inference now adjusts itself based on constraints. |
|
Closing as those commits are now included in #55229 |
fixes #55033