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

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

Closed
aturon opened this Issue May 1, 2017 · 34 comments

Comments

Projects
None yet
@aturon
Copy link
Member

aturon commented May 1, 2017

This is a tracking issue for the RFC "Deprecate anonymous parameters " (rust-lang/rfcs#1685).

Steps:

Unresolved questions:

  • The precise deprecation strategy
@est31

This comment has been minimized.

Copy link
Contributor

est31 commented May 1, 2017

I could work on this. From reading the RFC discussion, I think the "soft" deprecation strategy has been chosen? So on the compiler side, we'll need an allow by default (deprecation) lint for anonymous parameters, right?

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented May 1, 2017

@est31 Awesome! And yes, that's correct. This will be most effective if we add the lint to Clippy as well.

(The open question is around the criteria for moving to a warn-by-default strategy in the future.)

est31 added a commit to est31/rust that referenced this issue May 2, 2017

Removal pass for anonymous parameters
Removes occurences of anonymous parameters from the
rustc codebase, as they are to be deprecated.

See issue rust-lang#41686 and RFC 1685.
@est31

This comment has been minimized.

Copy link
Contributor

est31 commented May 2, 2017

About the clippy lint, I'll leave that to someone else. Given #41692, it should be mostly copy paste, but maybe with official rust claiming the lint name one has to chose another one. No idea.

frewsxcv added a commit to frewsxcv/rust that referenced this issue 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

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

Rollup merge of rust-lang#41693 - est31:anon_params_removal, r=eddyb
Removal pass for anonymous parameters

Removes occurences of anonymous parameters from the
rustc codebase, as they are to be deprecated.

See issue rust-lang#41686 and RFC 1685.

r? @frewsxcv
@matklad

This comment has been minimized.

Copy link
Member

matklad commented May 5, 2017

IJR inspection: intellij-rust/intellij-rust#1211

I'll try to add an automatic fix to rustfmt as well. Not sure it's a great idea, but I want to try :)

bors added a commit that referenced this issue Nov 11, 2017

Auto merge of #45775 - petrochenkov:patnopat, r=nikomatsakis
Accept interpolated patterns in trait method parameters

Permit this, basically
```rust
macro_rules! m {
    ($pat: pat) => {
        trait Tr {
            fn f($pat: u8) {}
        }
    }
}
```
it previously caused a parsing error during expansion because trait methods accept only very restricted set of patterns during parsing due to ambiguities caused by [anonymous parameters](#41686), and this set didn't include interpolated patterns.

Some outdated messages from "no patterns allowed" errors are also removed.

Addresses #35203 (comment)

bors added a commit that referenced this issue Nov 11, 2017

Auto merge of #45775 - petrochenkov:patnopat, r=nikomatsakis
Accept interpolated patterns in trait method parameters

Permit this, basically
```rust
macro_rules! m {
    ($pat: pat) => {
        trait Tr {
            fn f($pat: u8) {}
        }
    }
}
```
it previously caused a parsing error during expansion because trait methods accept only very restricted set of patterns during parsing due to ambiguities caused by [anonymous parameters](#41686), and this set didn't include interpolated patterns.

Some outdated messages from "no patterns allowed" errors are also removed.

Addresses #35203 (comment)
@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 17, 2018

It doesn't appear that this is linting: https://play.rust-lang.org/?gist=b818c6024acb95add60b339e7708d132&version=nightly

Also, we might want to try to finish this for the 2018 epoch, so we should start linting now

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 17, 2018

Oh, duh 🤦‍

It's an allow by default lint.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 17, 2018

Sorry for the multiple rapid posts.

After having read a bit more of the discussion that I can find, I propose a more aggressive deprecation strategy:

  • We make the lint warn-by-default as soon as possible
  • We make anon parameters a hard error at the epoch boundary

cc @matklad @est31 @aturon

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 18, 2018

Should this also apply to fn(Foo) function pointer types?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Feb 18, 2018

@SimonSapin I would say no, because the situation for function pointers is just the opposite: nontrivial patterns are forbidden:

error[E0561]: patterns aren't allowed in function pointer types
 --> main.rs:1:13
  |
1 | type F = fn(&x: &i32);
  |             ^^
 
@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 18, 2018

Also, I think the original RFC doesn't mention fn pointers...

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 19, 2018

@SimonSapin
The problem solved by this RFC is unique to trait methods - they may have or not have body, so they may need or not need the full power of patterns in parameters (and full pattern support is incompatible with anonymous parameters).
Function pointers never have body, so they never need non-trivial patterns, so they can freely use anonymous parameters as well.

@vi

This comment has been minimized.

Copy link
Contributor

vi commented May 27, 2018

Shall it be addressed before Rust 2018?

Maybe there should be a Github label like "rust-2018-checklist"? Or it should be a Github milestone?

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented May 27, 2018

Do the current plan is to make this warn by default starting at the edition and deprecate it in a later edition. (See #48309)

bors added a commit that referenced this issue May 27, 2018

Auto merge of #48309 - mark-i-m:anon_param_lint, r=nikomatsakis
Make anon params lint warn-by-default

This is intended as a followup on anonymous parameters deprecation.

Cross-posting from #41686:

> After having read a bit more of the discussion that I can find, I propose a more aggressive deprecation strategy:
> - We make the lint warn-by-default as soon as possible
> - We make anon parameters a hard error at the epoch boundary

cc @matklad @est31 @aturon
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Aug 10, 2018

In RFC 2522, I argue that to support the cases in that RFC we should expedite the schedule of RFC 1685
such that writing trait Foo { fn bar(MyType) { ... } } becomes a warning in Rust 2015 and a hard error in Rust 2018.

cc @nrc @nikomatsakis

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 21, 2018

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

bors added a commit that referenced this issue Aug 25, 2018

Auto merge of #53272 - mark-i-m:anon_param_error_now, r=nikomatsakis
Warn on anon params in 2015 edition

cc #41686 rust-lang/rfcs#2522
cc  @Centril @nikomatsakis

TODO:
- [x] Make sure the tests pass.
- [x] Make sure there is rustfix-able suggestion. Current plan is to just suggest `_ : Foo`
- [x] Add a rustfix ui test.

EDIT: It seems I already did the last two in #48309

bors added a commit that referenced this issue Aug 25, 2018

Auto merge of #53612 - mark-i-m:anon_param_disallowed_2018, r=petroch…
…enkov

Remove anonymous trait params from 2018 and beyond

cc @Centril @nikomatsakis
cc #41686 rust-lang/rfcs#2522 #53272

This PR removes support for anonymous trait parameters syntactically in rust 2018 and onward.

TODO:
- [x] Add tests

bors added a commit that referenced this issue Aug 28, 2018

Auto merge of #53272 - mark-i-m:anon_param_error_now, r=nikomatsakis
Warn on anon params in 2015 edition

cc #41686 rust-lang/rfcs#2522
cc  @Centril @nikomatsakis

TODO:
- [x] Make sure the tests pass.
- [x] Make sure there is rustfix-able suggestion. Current plan is to just suggest `_ : Foo`
- [x] Add a rustfix ui test.

EDIT: It seems I already did the last two in #48309
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 31, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Aug 31, 2018

The OP mentions some adjusting of documentation. Does anyone know specifically which documentation needs to be adjusted?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Aug 31, 2018

@mark-i-m The forge has a section on updating documentation, but I don't think there's much to do here for deprecations other than to make sure that the book / reference / RBE aren't using the feature.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Aug 31, 2018

@cramertj @mark-i-m We should probably note this in the edition guide as well.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 15, 2018

Triage: We still need to update the documentation.
ping @mark-i-m

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Sep 17, 2018

Sorry for the delay. I have been a bit swamped lately and mostly keeping up from my phone, so not much opportunity to write PRs 😛.

Reference: rust-lang-nursery/reference#420
Edition Guide: rust-lang-nursery/edition-guide#105

I did a brief search through the nomicon, book, RBE sections on traits and didn't see any other anon params, but please let me know if I missed one.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Sep 18, 2018

Docs have all been merged. This is good to go.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 18, 2018

Before we close the tracking issue; we still need to update the reference with notes about the actual edition-breaking change.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Sep 18, 2018

@Centril @steveklabnik I opened rust-lang-nursery/reference#421

Does this look like what you were thinking of?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 18, 2018

@mark-i-m I was thinking of specifying somewhat more formally (BNF, etc.) but this is good in the interim :)

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Sep 18, 2018

@Centril I didn't realize there was a BNF spec of rust anywhere. Could you point me to it so I can update it?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 18, 2018

@mark-i-m I don't think there is a BNF spec of Rust anywhere that covers all of Rust and is up to date;

However, we do specify some parts of the language in BNF; for example: https://doc.rust-lang.org/reference/expressions/literal-expr.html.

I think the most up-to-date specification is in parser-lalr.y but it isn't fully updated either.

We don't need to spec it in BNF right now; but as a long term goal it would be good to do so.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Sep 21, 2018

@Centril Perhaps we can spin that off into its own specific issue?

Currently, I think the last piece of documentation that is mandatory for this issue would be rust-lang-nursery/reference#421. Right?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 21, 2018

@mark-i-m nah; no issue needed; we should define a canonical grammar for Rust at some point, hopefully soon.

Once the reference PR lands we can close this issue.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 22, 2018

The reference PR has been merged, so I am closing this.

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.