parser: require argument names in trait methods

[matklad]

Rust currently allows omitting parameter names in trati methods, like this.

trait T {
   fn foo(i32) {  }
}

In all other contexts, except for the fn types, parameter names are
mandatory. This makes argument names in trait methods also mandatory.

This is a breaking language change.

I'm not sure that this is a bug, so feel free to close. I haven't run the tests yet ( they are in progress :( ), so build will likelly fail.

Discussion on the users.rust-lang: https://users.rust-lang.org/t/question-why-does-rust-admit-anonymous-parameters-in-traits/3420

[rust-highfive]

r? @nikomatsakis

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

[steveklabnik]

/cc @rust-lang/lang

[nrc]

I believe this is intentional.

[nrc]

(to some extent - there is not much harm in allowing no names for provided methods, and it is intentional that required methods don't require arg names. I guess we could accept a patch that allowed eliding names only for required methods, but I think parsing would be difficult. A patch like this would break too much code, IMO).

[nagisa]

Even if this wasn’t intentional, you really want to check for this kind of things in semantic analysis (i.e. somewhere after parsing is done) rather than syntactic analysis (parsing).

[huonw]

@nagisa I disagree, if we require an argument name/pattern in trait methods, that's firmly a syntactic thing: it's something that a formal grammar would include or not. It would be a semantic thing if we required that argument names were all mentioned in a doc comment attached to the method (hypothetically, and it's definitely a bad idea).

[matklad]

I think it should be closed :)

[nikomatsakis]

Huh. I don't believe it was intended that parameter names can be omitted in trait methods with a body -- I know we allowed it in traits initially (before default methods existed), and I guess it is still accepted just because it is a pain to parse otherwise, since you don't know whether the method has a body until later.

That said, I remember us debating about whether to change this due to parsing ambiguities between patterns and types, in particular when parsing something like fn foo(&T -- at this point we don't know if the &T is a pattern or a type. I thought we planned to require parameter names in trait definitions for this reason -- but I guess that never happened?

[matklad]

Note that current situation causes several inconsistences

//allowed
trait T {
    fn foo(i32) { }
}

//allowed
trait T {
    fn foo(& x: &i32) { }
}

//allowed
trait T {
    fn foo(&& x: &&i32) { }
}

//forbidden
trait T {
    fn foo(&&& x: &&&i32) { }
}

//forbidden
trait Matrix {
    fn at((row, col): (u32, u32)) { }
}

//forbidden
extern {
    fn foo(i32); // this is a declaration, so it should be OK to omit the name
}

But I guess nothing can be done now, due to backwards compatibility :( To me, anonymous parameters look more like an annoying exception, then like a convenient shortcut.

[nikomatsakis]

@matklad we are in the process of formally defining the grammar, and certainly several bugs and inconsistencies have been found in the rust parser that we will be correcting. This however DOES seem like something that is likely to break a lot of code if we change it, we'd have to test -- but the inconsistencies you highlight are worrisome. Of course one way to fix it is to accept a wider range of patterns (and maybe in more places), though this requires more lookahead (and a cover grammar), things we have traditionally tried to avoid. Another option is to keep accepting what we accept now but deprecate it (issue a lint warning).

[matklad]

This indeed breaks the compiler itself :)

I guess deprecating anonymous arguments is the safest thing to do: no code breakage + an ability to chose the appropriate fix later.

[nikomatsakis]

@matklad well, we don't mind updating the compiler :P but I think I agree

[nikomatsakis]

(and regardless, if we did decide to remove anonymous arguments, we'd need a deprecation phase)

[matklad]

Maybe an issue or an RFC should be created?

Language changes discussed in a closed PR can be forgotten easily :)