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

Deprecate anonymous parameters #1685

Merged
merged 3 commits into from May 1, 2017

Conversation

Projects
@matklad
Copy link
Member

commented Jul 24, 2016

Rendered

Issue: #1351

@WaDelma

This comment has been minimized.

Copy link

commented Jul 26, 2016

Well I have used these quite few times when parameter name wasn't nessary to understand the purpose. Didn't even realise that they cause this much problems.

@nikomatsakis nikomatsakis self-assigned this Jul 28, 2016

bors added a commit to rust-lang/rust that referenced this pull request Aug 4, 2016

Auto merge of #35015 - petrochenkov:forearg, r=nikomatsakis
Properly enforce the "patterns aren't allowed in foreign functions" rule

Cases like `arg @ PATTERN` or `mut arg` were missing.
Apply the same rule to function pointer types.

Closes #35203
[breaking-change], no breakage in sane code is expected though
r? @nikomatsakis

This is somewhat related to rust-lang/rfcs#1685 (cc @matklad).
The goal is to eventually support full pattern syntax where it makes sense (function body may present) and to support *only* the following forms - `TYPE`, `ident: TYPE`, `_: TYPE` - where patterns don't make sense (function body doesn't present), i.e. in foreign functions and function pointer types.
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2016

So I'm torn here. On the one hand, I wish we had removed them long ago, or found another way to resolve the parser ambiguities. I am not happy with the arbitrary limitations!

On the other hand, those could be overcome with more parser lookahead -- maybe a cover grammar too? -- and I worry a lot about this affecting a lot of people. I'm not sure it's really feasible. (I mean, it's only a deprecation, but still.)

@ubsan

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2016

@nikomatsakis parser lookahead is bad :(

Deprecations are easy. We can leave them in for a long time, only taking them out when we're sure that people have stopped using them, or almost all people have stopped using them.

@ticki

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2016

@nikomatsakis parser lookahead is bad :(

But unavoidable.

@matklad

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2016

Is there perhaps any script to download the source code of all crates.io packages locally? It shouldn't be that much data, right? I may try to quantify "a lot of people".

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2016

@matklad

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

Here are the results:

416 of 5560 crates are affected.
1747 occurrences of anonymous parameters in total.

Top occurrences by crate:
149: adapton-0.2.4
101: gfx_core-0.4.0
47: mayda-0.2.1
36: unjson-0.0.4
35: zdd-0.1.0
34: ncollide_geometry-0.1.1
33: glium-0.15.0
31: ncollide_entities-0.5.0
24: delix-0.2.4
22: nalgebra-0.8.2
21: cs-0.23.1
20: oaty-0.1.0
17: stree-0.0.4
13: gfx_phase-0.6.0
12: vulkano-0.2.0
11: aster-0.0.4
11: rui-0.0.2
11: core-nightly-2015.1.7
11: podio-0.1.5
11: gfx_app-0.2.0
11: ntrusive-0.0.4
10: unctor-0.1.2
10: opal-0.1.1
10: simdop-0.1.0
09: cpython-0.0.5
09: nphysics3d-0.4.0
09: psilonz_algebra-0.0.1
09: gfx_device_dx11-0.3.0
09: specs-0.7.0
09: repl-0.7.1

Here is the full report:

https://gist.github.com/matklad/3ab67778a15778717e8b28bb01f7bacf

(warning, 700kb of XML)

EDIT: Note that it does not report anonymous parameters inside macro invocations, macro definitions and macro expansions.

@brson

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

I think it makes sense to deprecate, put it on the breakage wishlist, then revisit in a few years.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2016

I feel mildly uncomfortable with the impact of this deprecation -- particularly since it seems useful to be able to elide parameter names. The main downside of today's setup is patterns, right? So another option would be to basically require more lookahead (or a cover grammar for patterns/types, etc), so that we could support anonymous parameters without some of their downsides, right? I haven't investigated this deeply, but it seems worth considering.

@matklad

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2016

it seems useful to be able to elide parameter names.

I personally think that anonymous parameters are a language feature of negative value, even if we forget about all the problems the RFC mentions.

  1. It adds very little convenience in writing and reading code.
  2. It introduces an alternative syntax for a relatively rare use case (there much more impls than traits).

Almost zero benefit + two syntaxes for the same thing + usual bias of not adding features < 0

@matklad

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

So another option would be to basically require more lookahead (or a cover grammar for patterns/types, etc), so that we could support anonymous parameters without some of their downsides, right?

And yet another option is to do both parser tricks and a deprecation, to allow using full range of patterns today, and to be able to remove old syntax and tricks somewhere in the distant future.

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016

Rollup merge of rust-lang#37378 - petrochenkov:nopat, r=eddyb
Prohibit patterns in trait methods without bodies

They are not properly type checked
```rust
trait Tr {
    fn f(&a: u8); // <- This compiles
}
```
, mostly rejected by the parser already and generally don't make much sense.
This PR is kind of a missing part of rust-lang#35015.

Needs crater run.
cc rust-lang#35078 (comment) rust-lang#35015 rust-lang/rfcs#1685 rust-lang#35203
r? @eddyb

bors added a commit to rust-lang/rust that referenced this pull request Oct 29, 2016

Auto merge of #37378 - petrochenkov:nopat, r=eddyb
Prohibit patterns in trait methods without bodies

They are not properly type checked
```rust
trait Tr {
    fn f(&a: u8); // <- This compiles
}
```
, mostly rejected by the parser already and generally don't make much sense.
This PR is kind of a missing part of #35015.

Given the [statistics from crater](#37378 (comment)), the effect of this PR is mostly equivalent to improving `unused_mut` lint.

cc #35078 (comment) #35015 rust-lang/rfcs#1685 #35203
r? @eddyb

bors added a commit to rust-lang/rust that referenced this pull request Oct 29, 2016

Auto merge of #37378 - petrochenkov:nopat, r=eddyb
Prohibit patterns in trait methods without bodies

They are not properly type checked
```rust
trait Tr {
    fn f(&a: u8); // <- This compiles
}
```
, mostly rejected by the parser already and generally don't make much sense.
This PR is kind of a missing part of #35015.

Given the [statistics from crater](#37378 (comment)), the effect of this PR is mostly equivalent to improving `unused_mut` lint.

cc #35078 (comment) #35015 rust-lang/rfcs#1685 #35203
r? @eddyb
@matklad

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2016

Looks like discussion has died down. FCP maybe?

@mglagla

This comment has been minimized.

Copy link

commented Nov 28, 2016

👍 on this.

Whenever i'm implementing a trait, i usually copy-paste the function signature into my code. Doing this with fmt::Display often makes me stumble a bit because of the anonymous parameter, as this is surprising.
Newcomers to the language also might get confused, since this is syntax that is not explained the book (I think?).

@nrc

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

Looks like discussion has died down. FCP maybe?

I also want to do this, but fear the size of the breakage. Although I agree it is not nice to have alternate syntaxes around (especially when _: Type is so easy to use), I fear that hitting users with a bunch of warnings about deprecation needs more motivation than it being irritating. My feeling is thus that we should not accept this RFC. Put another way, I agree that "anonymous parameters are a language feature of negative value", but that the magnitude of negative value is outweighed by the user inconvenience of deprecating.

@nrc

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

@rfcbot close

There has not been much discussion on this thread, although it has quite a few upvotes. There seems to be a viable alternative for some of the motivation (parser lookahead), and the pain of deprecation (this would affect 7.5% of all crates on crates.io (#1685 (comment))) does not seem to be out-weighed by the benefits.

OTOH, I have to say that I also don't feel strongly about this and wouldn't argue strongly against accepting, if the lang team think it is worth the user pain.

@vi

This comment has been minimized.

Copy link

commented Nov 29, 2016

So is it for Rust 2.0?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

I feel like if somebody just sent a PR implementing a compatibility lint instead on an RFC, this would have higher chances. 😄
Even if this is not going to be removed in the close future, issuing a warning still makes sense, this won't break anyone's code unless they proactively deny warnings, and won't break dependencies.

@vi

This comment has been minimized.

Copy link

commented Nov 29, 2016

Maybe add the lint to the Clippy first?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

@vi

Maybe add the lint to the Clippy first?

Whatever helps to get this out of the language eventually.

@matklad

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

Maybe add the lint to the Clippy first?

There's already an inspection/quick fix in IntelliJ Rust, though it's not enabled yet :)

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

@aturon rightfully argued that:

think there's a basic disconnect here, in terms of the impact of a deprecation. While it's not technically breakage, in that the code continues to compile, the compiler is still asking you to change your code, introducing an irritation and/or churn. When that affects a large number of crates, it needs to be well-motivated and can't be done lightly (IMO).

But the "fix" for a hard deprecation warning is just #![allow(anonymous_parameters)].

As @aturon mentions, that is indeed some irritation and/or churn, but... it looks like pretty minor churn to me.

I think that the following 3 things are very important:

    1. We should inform everybody, including those who don't even read the release notes, that something is deprecated. I don't see a way of doing this with a "soft" opt-in kind of warning. The warning must be "hard". We can start with a soft warning, that is switched to hard in the next release after ironing possible bugs with the feature, to provide a smooth experience though.
    1. There must definitely be something in for those who fix the warnings and we should honestly tell them what that is. We don't do this just for fun, we do it for a reason, so we should tell them why they should fix it, and it must be something "tangible". For example, that bug fixes / performance fixes for the feature won't be prioritized if they complicate the compiler, and that if they opt-in to #![error(anonymous_parameters)] their code will e.g. compile faster.
    1. We should present them their options as clearly as possible, so that they can make the best informed decision about how to proceed:
    • (First, don't overflow them with warnings, show them only the N first warnings.)
    • That they can silence the warning globally by adding #![allow(anonymous_parameters)] to the crate root, passing a flag to the compiler, or adding something to their Cargo.toml file, and reassure them that if they do this their code will still compile forever. We won't break backwards compatibility for this.
    • That they can fix the warning automatically with rustfmt --fix-warning-1234 without needing to reformat any of their code using rustfmt version > y (it is important for those who do not want to format their code with rustfmt to still be able to use it for this).
    • That they can disable the warning on a module per module basis, and fix it incrementally over time.
    • If their code compiles without warnings, we can also tell them that they can add #![error(anonymous_parameters)] to gain something for free.
@est31

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

We can start with a soft warning, that is switched to hard in the next release after ironing possible bugs with the feature, to provide a smooth experience though.

Just noting that some people have started getting rustc from their distro, and while I think its not the best way to obtain rust as rust is evolving at a rapid pace (nor do I want to support anything older than the latest stable in my crates), they only update it in large intervals, e.g. every 6 months, or for LTS distros every 4-5 years. For them it won't be smooth.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

For them it won't be smooth.

By smooth I meant a "bugless" hard warning experience, not time-continuous kind of smooth.

@porky11

This comment has been minimized.

Copy link

commented Mar 1, 2017

I like the ability to define type signatures without names, when the names aren't used anyway.

@aturon

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

Let's see if this works:

@rfcbot fcp cancel

@rfcbot

This comment has been minimized.

Copy link

commented Mar 4, 2017

@aturon proposal cancelled.

@aturon

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

@rfcbot fcp merge

(As per @nikomatsakis's comment)

@rfcbot

This comment has been minimized.

Copy link

commented Mar 4, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

# Motivation
[motivation]: #motivation

Anonymous parameters are a [historic accident]. They cause a number of technical

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Mar 15, 2017

Member

This claim is misrepresenting the text it references.

What Niko wrote there was:

I don't believe it was intended that parameter names can be omitted in trait methods with a body

(emphasis added)

I do not think it was ever an accident that the design allowed anonymous parameters for trait methods without a body.

This comment has been minimized.

Copy link
@matklad

matklad Mar 15, 2017

Author Member

I was referring to another part of the comment, but maybe I've misinterpreted it

I thought we planned to require parameter names in trait definitions for this reason -- but I guess that never happened?

@nikomatsakis what would be the right way to phrase this in the RFC? Or we can just drop this sentence: it does not add anything to the current situation, it just an interesting historical detail :)

This comment has been minimized.

Copy link
@matklad

matklad Mar 15, 2017

Author Member

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 25, 2017

Contributor

Either way. I do remember discussing this at some point but -- as you say -- it never happened.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

@rfcbot reviewed

@rfcbot

This comment has been minimized.

Copy link

commented Apr 19, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

commented Apr 29, 2017

The final comment period is now complete.

@aturon aturon referenced this pull request May 1, 2017

Closed

Tracking issue for RFC#1685: Deprecate anonymous parameters #41686

3 of 3 tasks complete

@aturon aturon merged commit cef568c into rust-lang:master May 1, 2017

@aturon

This comment has been minimized.

Copy link
Member

commented May 1, 2017

This RFC has been merged -- thanks @matklad! Tracking issue is here.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 2, 2017

Rollup merge of rust-lang#41692 - est31:anon_params, r=eddyb
Add a lint to disallow anonymous parameters

Adds a (allow by default) lint to disallow anonymous parameters, like it was decided in RFC 1685 (rust-lang/rfcs#1685).

cc tracking issue rust-lang#41686
@gbutler69

This comment has been minimized.

Copy link

commented Jun 4, 2017

@ubsan - I would disagree with your comment:

Deprecations are easy. We can leave them in for a long time, only taking them out when we're sure that people have stopped using them, or almost all people have stopped using them.

The problem with this is, they NEVER get removed. See the situation with Java. Almost nothing that is deprecated is EVER removed (years later). Over time, it has become a problem for the language and JVM designers because they need to continue to support things that have been deprecated for years even though it keeps them from migrating the language the way they would like to easily.

Deprecations should come with a promise to remove/disable the feature at a pre-defined time in the future. Ideally, deprecation should say which version the feature will be removed/disabled at the time it is deprecated. At that version it should be removed. Otherwise, the notion of deprecation is meaningless.

@WaDelma

This comment has been minimized.

Copy link

commented Jun 4, 2017

As far as I understand the plan is to only remove deprecated items in rust 2.0 as removing it 1.* would be a breaking change, but I could be wrong.

xfix added a commit to xfix/enum-map that referenced this pull request Oct 8, 2017

Name trait function variables
According to depreciation from rust-lang/rfcs#1685

@Centril Centril added the A-syntax label Nov 23, 2018

@matklad matklad deleted the matklad:anon-params branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.