-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Make never rest type top-like #35438
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
Make never rest type top-like #35438
Conversation
28c1b3a
to
73077cc
Compare
73077cc
to
76bc82b
Compare
@typescript-bot test this |
Heya @jack-williams, I've started to run the extended test suite on this PR at 76bc82b. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @jack-williams, I've started to run the parallelized community code test suite on this PR at 76bc82b. You can monitor the build here. It should now contribute to this PR's status checks. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
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.
Makes a lot of sense to me - making never
behave as any
already did here seems to work. Can we also see a test for the nested case, eg
function assignmentWithComplexRest2<T extends any[]>() {
const fn1: (cb: (x: string, ...rest: T) => void) => void = (cb) => {};
const fn2: (cb: (...args: never) => void) => void = fn1;
}
which should error, I believe?
Yep, added! |
Thanks for powering through all these PR's @sandersn and @weswigham! |
Was it known that this makes |
Short answer: no. That function type should not be callable. Longer answer: After looking through this again last night and trying to fix it, I do vaguely remember looking at that behaviour before. I think the code to trigger the error was move involved and touched a couple of the signature checking functions. I guess I never got around to resolving that and when the PR was revived, I had forgotten. I started working on a fix yesterday. Haven't run it through perf and regression test. I do not think it is a big change. Also happy for someone to pick it up. |
Thanks @jack-williams, wasn’t expecting you to pick up a fix, just searching for info, but appreciate it! |
Not an actual fix as the issue is still under discussion. Figured I’d put up and implementation though.
Fixes #33495