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 #55972: Erroneous self arguments on bare functions emit subpar compilation error #56002

Merged
merged 9 commits into from
Nov 22, 2018

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Nov 16, 2018

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2018
@lcnr lcnr changed the title [WIP] fix #55972: Erroneous self arguments on bare functions emit subpar compilation error fix #55972: Erroneous self arguments on bare functions emit subpar compilation error Nov 16, 2018
@lcnr
Copy link
Contributor Author

lcnr commented Nov 16, 2018

should work, still need to improve the Error message

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Could you rebase against latest master to remove the merge commit?

One nitpick, but the code changes look good.

Can you also add a test for the other case you mentioned in the ticket?

src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/test/ui/bare-function-self.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor Author

lcnr commented Nov 16, 2018

In my opinion it is not worth to handle invalid uses of self differently. To implement this I would probably change parse_arg_general(&mut self, require_name: bool) to parse_arg_general(&mut self, require_name: bool, is_bare_fn: bool), which is somewhat ugly.

@rust-highfive

This comment has been minimized.

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 19, 2018

📌 Commit 5bfdcc1 has been approved by estebank

@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 Nov 19, 2018
impl Foo {
fn c(foo: u32, self) {}
//~^ ERROR unexpected `self` argument in function
//~| NOTE `self` is only valid as the first argument of a trait function
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a function in an inherent impl, it would be nice if the error could emphasize here that the problem is that it must be the first argument.

Copy link
Contributor Author

@lcnr lcnr Nov 20, 2018

Choose a reason for hiding this comment

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

In my opinion it is not worth to handle invalid uses of self differently. To implement this I would probably change parse_arg_general(&mut self, require_name: bool) to parse_arg_general(&mut self, require_name: bool, is_bare_fn: bool), which is somewhat ugly.

I did think about simple ways to check if we currently parse a associated function but did not find one.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a big deal, but still would be nice.

But I am not knowledgeable in the relevant APIs either.^^

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 21, 2018

📌 Commit 88d6094 has been approved by estebank

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 22, 2018
fix rust-lang#55972: Erroneous self arguments on bare functions emit subpar compilation error

rust-lang#55972

r? @estebank
bors added a commit that referenced this pull request Nov 22, 2018
Rollup of 11 pull requests

Successful merges:

 - #55367 (lint if a private item has doctests)
 - #55485 (Return &T / &mut T in ManuallyDrop Deref(Mut) impl)
 - #55784 (Clarifying documentation for collections::hash_map::Entry::or_insert)
 - #55961 (Fix VecDeque pretty-printer)
 - #55980 (Suggest on closure args count mismatching with pipe span)
 - #56002 (fix #55972: Erroneous self arguments on bare functions emit subpar compilation error)
 - #56063 (Update any.rs documentation using keyword dyn)
 - #56067 (Add SGX target to rustc)
 - #56078 (Fix error message for `-C panic=xxx`.)
 - #56106 (Remove some incorrect doc comments)
 - #56126 (core/benches/num: Add `from_str/from_str_radix()` benchmarks)

Failed merges:

r? @ghost
@bors bors merged commit 88d6094 into rust-lang:master Nov 22, 2018
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.

6 participants