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.

Sign up for GitHub

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

Jump to bottom

Allow to dispatch fn traits depending on number of parameters #55986

Merged
merged 7 commits into from
Jan 4, 2019
Merged

Allow to dispatch fn traits depending on number of parameters #55986

merged 7 commits into from
Jan 4, 2019

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Nov 15, 2018 •
edited
Loading

Hello,

By following @eddyb's advise on issue #45510, I managed to have the snippets of code in #45510 and #18952 passing without breaking older diagnostics.

EDIT: the codegen tests breakage I experienced is due to the poor quality of my laptop.

If any kind reviewer has any advice, you are very welcome.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2018
@eddyb
Copy link
Member

eddyb commented Nov 15, 2018

r? @nikomatsakis

@bors

This comment has been minimized.

Sign in to view
nikomatsakis
nikomatsakis requested changes Nov 20, 2018
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Looks good! Can you add the comment and explain that one case?

src/librustc_typeck/check/callee.rs Show resolved Hide resolved
@cjgillot cjgillot changed the title WIP: Allow to dispatch fn traits depending on number of parameters Allow to dispatch fn traits depending on number of parameters Nov 25, 2018
nikomatsakis
nikomatsakis requested changes Nov 26, 2018
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

can we tweak the tests a bit to show that we are actually calling the fn as expected etc?

src/test/ui/issue-18952.rs Outdated Show resolved Hide resolved
src/test/ui/issue-45510.rs Outdated Show resolved Hide resolved
@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@cjgillot
test: Add test for issues 45510 and 18952.
4e77c3a 
Those tests are directly taken from the comments on the bug reports.
@cjgillot
rustfmt: librust_typeck/check/callee.rs
6de9c13
@cjgillot
rustc_typeck: Implement resolution advised in issue 45510.
1827d52 
When resolving Fn traits, the compiler failed to take into account
overloaded implementations.  To resolve this, we inform the trait dispatch
code that the arguments will becoming as a tuple of correct arity.
@cjgillot
Move tests to ui.
f518cf9
@cjgillot
Fix failing diagnostic test.
2f42a18
@cjgillot
Add comment.
2530ce9
@cjgillot
Change return types and check return values in tests.
91c155b 
This allows to verify that we handle
different types as return types,
and that we call the expected functions.
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 2, 2019

📌 Commit 91c155b has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 2, 2019
@bors
Copy link
Contributor

bors commented Jan 3, 2019

⌛ Testing commit 91c155b with merge cbacd348a2f5b0ec3efc8813245b6efb6f497883...

@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 3, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@kennytm

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jan 3, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#55986 - cjgillot:issue-45510, r=nikomatsakis
677eef5 
Allow to dispatch fn traits depending on number of parameters

Hello,

By following @eddyb's advise on issue rust-lang#45510, I managed to have the snippets of code in rust-lang#45510 and rust-lang#18952 passing without breaking older diagnostics.

EDIT: the codegen tests breakage I experienced is due to the poor quality of my laptop.

If any kind reviewer has any advice, you are very welcome.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jan 3, 2019
Closed
@bors
Copy link
Contributor

bors commented Jan 4, 2019

⌛ Testing commit 91c155b with merge a602f13...

bors added a commit that referenced this pull request Jan 4, 2019
@bors
Auto merge of #55986 - cjgillot:issue-45510, r=nikomatsakis
a602f13 
Allow to dispatch fn traits depending on number of parameters

Hello,

By following @eddyb's advise on issue #45510, I managed to have the snippets of code in #45510 and #18952 passing without breaking older diagnostics.

EDIT: the codegen tests breakage I experienced is due to the poor quality of my laptop.

If any kind reviewer has any advice, you are very welcome.
@bors
Copy link
Contributor

bors commented Jan 4, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a602f13 to master...

@bors bors merged commit 91c155b into rust-lang:master Jan 4, 2019
@bors bors mentioned this pull request Jan 4, 2019
Closed
@ZhangHanDong ZhangHanDong mentioned this pull request Jan 10, 2019
Open
@nagisa nagisa mentioned this pull request Feb 4, 2019
Open
@kennytm kennytm mentioned this pull request Feb 22, 2019
Closed
@Deewiant Deewiant mentioned this pull request Apr 12, 2019
Closed
@jplatte jplatte mentioned this pull request Jun 6, 2019
Closed
@cjgillot cjgillot deleted the issue-45510 branch November 18, 2019 08:06
@philberty philberty mentioned this pull request Oct 13, 2022
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cjgillot @rust-highfive @eddyb @bors @nikomatsakis @kennytm @cramertj