Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

require argument names in trait method signatures? #1351

Closed
pnkfelix opened this issue Nov 2, 2015 · 13 comments
Closed

require argument names in trait method signatures? #1351

pnkfelix opened this issue Nov 2, 2015 · 13 comments
Labels
breaking-change The RFC proposes a breaking change. T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2015

imported from the rust repo, PR rust-lang/rust#29406

Description, as originally written by @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


@nikomatsakis notes in followup discussion on #29406 :

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?

@nagisa
Copy link
Member

nagisa commented Nov 2, 2015

I’m personally more towards forbidding omission of name in all cases, even if it would be a breaking change, because:

  1. It doesn’t affect me and probably majority of users;
  2. It seems to simplify parsing?
  3. Change to fix the breakage is trivial: add _: before affected types.

@matklad
Copy link
Member

matklad commented Nov 2, 2015

Another thing to note is that patterns in trait method arguments are currently forbidden because of the said ambiguity. That is, the following is not a valid syntax at the moment:

trait M {
    fn at((col, row): (u32, u32)) { }
}

@nrc
Copy link
Member

nrc commented Nov 2, 2015

I'm in favour of this change given the simple fix (@nagisa's comment, item 3). As long as we provide a tool to fix this (which should be easy to implement) and a warning cycle or two, then I think the pain is worthwhile. OTOH, the gains seem minor so I understand the resistance to breaking for this (maybe a candidate for when we get to 2.0?).

@nrc nrc added breaking-change The RFC proposes a breaking change. T-lang Relevant to the language team, which will review and decide on the RFC. labels Nov 2, 2015
@seanmonstar
Copy link
Contributor

Seems like a tiny bug (as in, its existence doesn't hurt anyone), so fixing it (and thus causing a breaking change) doesn't seem that warranted, before a major semver change.

@withoutboats
Copy link
Contributor

I often don't declare argument names in trait methods without bodies. I wouldn't mind this going away, but it doesn't seem worth breaking over.

@matklad
Copy link
Member

matklad commented Nov 3, 2015

A deprecation warning with @nagisa suggestion now and a removal in 2.0 seems the safest strategy.

@nikomatsakis
Copy link
Contributor

Ah, perhaps this is the resolution we wound up with. Except that it
seems to be inconsistently enforced.

On Mon, Nov 02, 2015 at 08:15:32AM -0800, Aleksey Kladov wrote:

Another thing to note is that patterns in trait method arguments are currently forbidden because of the said ambiguity. That is, the following is not a valid syntax at the moment:

trait M {
    fn at((col, row): (u32, u32)) { }
}

Reply to this email directly or view it on GitHub:
#1351 (comment)

@bluss
Copy link
Member

bluss commented Nov 9, 2015

I agree with @seanmonstar; breaking changes must have substantive benefits to rust and rust users to be worthwhile.

@munael
Copy link

munael commented Nov 9, 2015

Another thing to note is that patterns in trait method arguments are currently forbidden because of the said ambiguity. That is, the following is not a valid syntax at the moment:

trait M {
    fn at((col, row): (u32, u32)) { }
}

I haven't tested this but it sounds to me like enough benefit for the change (consistency in a feature advertised for functions and usable everywhere else).

@ticki
Copy link
Contributor

ticki commented Dec 9, 2015

Well, nobody is using it without naming the argument anyways. Also, it would make documentation better in some cases (requiring names for parameters). So 👍

matklad referenced this issue in intellij-rust/intellij-rust Jun 14, 2016
@matklad
Copy link
Member

matklad commented Jul 19, 2016

I've written up a pre RFC here: https://internals.rust-lang.org/t/pre-rfc-deprecating-anonymous-parameters/3710/1

@matklad
Copy link
Member

matklad commented Jul 24, 2016

RFC: #1685

@Centril
Copy link
Contributor

Centril commented Oct 7, 2018

This is now required in edition 2018 per notes in rust-lang/rust#41686.

@Centril Centril closed this as completed Oct 7, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
breaking-change The RFC proposes a breaking change. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests