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

Erroneous self arguments on bare functions emit subpar compilation error #55972

Closed
nox opened this issue Nov 15, 2018 · 5 comments · Fixed by #56155
Closed

Erroneous self arguments on bare functions emit subpar compilation error #55972

nox opened this issue Nov 15, 2018 · 5 comments · Fixed by #56155
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@nox
Copy link
Contributor

nox commented Nov 15, 2018

When refactoring methods as bare functions and forgetting to remove the self argument, the compiler fails to parse the new function entirely instead of emitting a better error.

error: expected one of `::` or `:`, found `,`
    --> components/canvas_traits/webgl.rs:1000:10
     |
1000 |     &self,
     |          ^ expected one of `::` or `:` here
@csmoe csmoe added A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. labels Nov 15, 2018
@estebank
Copy link
Contributor

estebank commented Nov 15, 2018

fn parse_fn_args(&mut self, named_args: bool, allow_variadic: bool)

should have a call on the first argument to

fn parse_self_arg(&mut self) -> PResult<'a, Option<Arg>> {

which if the result is Ok(Some(_)) a custom diagnostic should be raised pointing at the argument.

You can look at

fn parse_fn_decl_with_self<F>(&mut self, parse_arg_fn: F) -> PResult<'a, P<FnDecl>>

to see how trait fns parse arguments.

You can see how a diagnostic is created here

fn expected_ident_found(&self) -> DiagnosticBuilder<'a> {
let mut err = self.struct_span_err(self.span,
&format!("expected identifier, found {}",
self.this_token_descr()));
if let Some(token_descr) = self.token_descr() {
err.span_label(self.span, format!("expected identifier, found {}", token_descr));
} else {
err.span_label(self.span, "expected identifier");
if self.token == token::Comma && self.look_ahead(1, |t| t.is_ident()) {
err.span_suggestion_with_applicability(
self.span,
"remove this comma",
String::new(),
Applicability::MachineApplicable,
);
}
}
err
}

and after that all that is needed for that error to be outputted is to call err.emit().


https://forge.rust-lang.org has information on the compiler itself, you'll want to at the very least look at https://forge.rust-lang.org/x-py.html.

@estebank estebank added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Nov 15, 2018
@lcnr
Copy link
Contributor

lcnr commented Nov 16, 2018

Hello, I would like to work on this

@lcnr
Copy link
Contributor

lcnr commented Nov 16, 2018

As this is also causing the same error message, I think that it might actually be better to check for self in every argument position:

struct Foo {}

impl Foo {
    fn foo(a: u32, &mut self) {
        
    }
}

fn bar(a: u32, self) {}

playground

@estebank
Copy link
Contributor

@Axary

As this is also causing the same error message

That is a good point. It's probably a good idea to have different error message for the (so far) three cases. On Foo::foo, you should complain that self should appear first, if there are many that there should be one, in bar you should complain that self is only valid in trait fns.

@lcnr
Copy link
Contributor

lcnr commented Nov 16, 2018

Ok 👍 will do

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue 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 issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants