Skip to content
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

consider spread tuples when matching signature #18004

Closed

Conversation

KiaraGrouwstra
Copy link
Contributor

@KiaraGrouwstra KiaraGrouwstra commented Aug 24, 2017

Fixes #4130.

Edit: fixed error on other test. Looks like test results improved there as well.

KiaraGrouwstra added a commit to KiaraGrouwstra/TypeScript that referenced this pull request Aug 24, 2017
Fixes the third part of microsoft#5453, needed to effectively type `bind` (Function.prototype), `curry` and `compose`. Depends on microsoft#17961 type calls, microsoft#17884 type spread and microsoft#18004 tuple spread. The first 2/3 are included, so changes will look more cluttered here though new changes just span 10 lines over 2 functions. Will properly test this when those are ready -- now still broken.
@Igorbek
Copy link
Contributor

Igorbek commented Aug 25, 2017

This is really awesome. I think all these error message changes make them more informative.

@KiaraGrouwstra
Copy link
Contributor Author

Thanks! 😃
Felt like this one worked out without many bumps, yeah.
Wish that were the case for the bigger ones haha.

@microsoft microsoft deleted a comment from msftclas Sep 27, 2017
@MartinMuzatko
Copy link

please merge this!

@briantanner
Copy link

I'm stuck without this!

@eknowles
Copy link

should all those tests have been committed?

@KiaraGrouwstra
Copy link
Contributor Author

@eknowles: yeah, that was intended; if I hadn't committed the test changes, CI would have errored out on unexpected changes. fortunately the changes themselves seem positive.

@sandersn
Copy link
Member

sandersn commented Jan 8, 2018

I think we should actually update resolveCall to understand spread tuple arguments instead of rewriting to a series of element accesses. The compiler doesn't expect rewrites of the AST inside the checker, and there's no infrastructure for doing it right either.

I poked at this last month but found it was non-trivial. I'm going to try again and see if I can get a limited version working at least.

@KiaraGrouwstra
Copy link
Contributor Author

@sandersn: thank you for your take on this, I much appreciate it.
I hadn't really looked into that option, but in retrospect, I think you're right about the AST rewrites...

Although over here I hadn't directly bumped into issues yet, I'd tried this route in my shot at type calls (6606) as well, where the consequences of AST manipulation became all too evident.

I may definitely have been naive in my approach, and I'd be interested to see what a proper approach would end up looking like here. But yeah, I'll take your word for it it wouldn't be trivial...

@sandersn
Copy link
Member

sandersn commented Jan 9, 2018

@tycho01 I put a prototype up at the branch spread-tuples. It's pretty undone: it duplicates code, doesn't handle complex cases [1], and doesn't allow spread tuples to precede non-spread arguments.

[1] Specifically, overloads and functions with type parameters.

@KiaraGrouwstra
Copy link
Contributor Author

@sandersn: thanks, I just checked it too. It does look tough having to handle several cases separately rather than being able to just hook into existing logic.

To throw this out there, had the idea of using synthetic nodes been discussed before? I won't deny it feels hacky, though it also helped simplify a few things.

For reference, pain points I experienced with the approach, particularly in #17961, revolved around error reporting / matching nodes as used in the tooltip messages.

I'd definitely only managed to address part of the issues there, but then again, my knowledge about the compiler is still kinda limited anyway.

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler incorrectly reports parameter/call target signature mismatch when using the spread operator
9 participants