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

Fix regression: account for trait methods in arg count mismatch error #47844

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jan 29, 2018

Fixed #47706 (#47706 (comment))

Original PR #47747 missed methods on trait definitions.

This edit was done in GitHub. I think I got the signature of the variant right, going by the ICE debug output and the other cases above.

@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 @nikomatsakis (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.

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 29, 2018

cc @estebank

(Complete off-topic: I already made an earlier PR#47083, why did I get the first-time greeting from highfive?)

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 29, 2018

I should probably add a test for this. I can do that about 17 hours from now when I hit the machine where I've got the repo checked out and set up already, or someone else is free to add the test case I posted in the linked issue comment.

@pnkfelix
Copy link
Member

@CAD97 this looks great!

Please squash the commits you have added when you put up the test case.

r=me once the test case is added.

@estebank
Copy link
Contributor

@CAD97 thank you for following up and adding a fix. I would prefer it if it had a test case in the same commit for easier historical tracking. We can wait another 8 hours for it :)

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 29, 2018
@CAD97
Copy link
Contributor Author

CAD97 commented Jan 29, 2018

Test added and squashed!

r? @pnkfelix

@estebank
Copy link
Contributor

[00:04:11] tidy error: /checkout/src/test/ui/issue-47706-trait.rs:15: line longer than 100 chars
[00:04:12] some tidy checks failed

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 29, 2018

Yeah, working on it

@CAD97 CAD97 force-pushed the patch-1 branch 2 times, most recently from a19e701 to 217efbb Compare January 29, 2018 23:33
@CAD97
Copy link
Contributor Author

CAD97 commented Jan 29, 2018

That should fix the tidy issue.

or not >.>

None::<()>.map(Self::f);
}
//~^^ ERROR function is expected to take a single 0-tuple as argument,
//~^^ but it takes 2 distinct arguments [E0593]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove line 16, line 15 will match the beginning and work as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I did something wrong I guess (by putting on the tag?) https://travis-ci.org/rust-lang/rust/builds/334956316

Copy link
Contributor

@estebank estebank Jan 30, 2018

Choose a reason for hiding this comment

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

Yeah, just remove the [E0593] and it should pass.

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 30, 2018

That should be everything.

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 30, 2018

Travis passed.

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 30, 2018

📌 Commit c06c707 has been approved by estebank

kennytm added a commit to kennytm/rust that referenced this pull request Jan 31, 2018
Fix regression: account for trait methods in arg count mismatch error

Fixed rust-lang#47706 (rust-lang#47706 (comment))

Original PR rust-lang#47747 missed methods on trait definitions.

This edit was done in GitHub. I think I got the signature of the variant right, going by the ICE debug output and the other cases above.
@kennytm kennytm 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 31, 2018
bors added a commit that referenced this pull request Jan 31, 2018
Rollup of 16 pull requests

- Successful merges: #47838, #47840, #47844, #47874, #47875, #47876, #47884, #47886, #47889, #47890, #47891, #47795, #47677, #47893, #47895, #47552
- Failed merges:
@bors bors merged commit c06c707 into rust-lang:master Jan 31, 2018
@CAD97 CAD97 deleted the patch-1 branch January 31, 2018 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants