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

Stabilize `param_attrs` in Rust 1.39.0 #64010

Merged
merged 1 commit into from Sep 21, 2019

Conversation

@c410-f3r
Copy link
Contributor

commented Aug 29, 2019

Stabilization proposal

I propose that we stabilize #![feature(param_attrs)].

Tracking issue: #60406
Version: 1.39 (2019-09-26 => beta, 2019-11-07 => stable).

What is stabilized

It is now possible to add outer attributes like #[cfg(..)] on formal parameters of functions, closures, and function pointer types. For example:

fn len(
    #[cfg(windows)] slice: &[u16],
    #[cfg(not(windows))] slice: &[u8],
) -> usize {
    slice.len()
}

What isn't stabilized

  • Documentation comments like /// Doc on parameters.

  • Code expansion of a user-defined #[proc_macro_attribute] macro used on parameters.

  • Built-in attributes other than cfg, cfg_attr, allow, warn, deny, and forbid. Currently, only the lints unused_variables and unused_mut have effect and may be controlled on parameters.

Motivation

The chief motivations for stabilizing param_attrs include:

  • Finer conditional compilation with #[cfg(..)] and linting control of variables.

  • Richer macro DSLs created by users.

  • External tools and compiler internals can take advantage of the additional information that the parameters provide.

For more examples, see the RFC.

Reference guide

In the grammar of function and function pointer, the grammar of variadic tails (...) and parameters are changed respectively from:

FnParam = { pat:Pat ":" }? ty:Type;
VaradicTail = "...";

into:

FnParam = OuterAttr* { pat:Pat ":" }? ty:Type;
VaradicTail = OuterAttr* "...";

The grammar of a closure parameter is changed from:

ClosureParam = pat:Pat { ":" ty:Type }?;

into:

ClosureParam = OuterAttr* pat:Pat { ":" ty:Type }?;

More generally, where there's a list of formal (value) parameters separated or terminated by , and delimited by ( and ). Each parameter in that list may optionally be prefixed by OuterAttr+.

Note that in all cases, OuterAttr* applies to the whole parameter and not just the pattern. This distinction matters in pretty printing and in turn for macros.

History

  • On 2018-10-15, @Robbepop proposes RFC 2565, "Attributes in formal function parameter position".

  • On 2019-04-30, RFC 2565 is merged and the tracking issue is made.

  • On 2019-06-12, a partial implementation was completed. The implementation was done in #60669 by @c410-f3r and the PR was reviewed by @petrochenkov and @Centril.

  • On 2019-07-29, #61238 was fixed in #61856. The issue fixed was that lint attributes on function args had no effect. The PR was written by @c410-f3r and reviewed by @matthewjasper, @petrochenkov, and @oli-obk.

  • On 2019-08-02, a bug #63210 was filed wherein the attributes on formal parameters would not be passed to macros. The issue was about forgetting to call the relevant method in fn print_arg in the pretty printer. In #63212, written by @Centril on 2019-08-02 and reviewed by @davidtwco, the issue aforementioned was fixed.

  • This PR stabilizes param_attrs.

Tests

Possible future work

  • Custom attributes inside function parameters aren't currently supported but it is something being worked on internally.

  • Since documentation comments are syntactic sugar for #[doc(...)], it is possible to allow literal /// Foo comments on function parameters.

This report is a collaborative work with @Centril.

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

commented Aug 29, 2019

r? @eddyb

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

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@rust-highfive rust-highfive assigned Centril and unassigned eddyb Aug 29, 2019

@Centril Centril added this to the 1.39 milestone Aug 29, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Dear language team and the community at large. The above report gives an overview of the #[attr] param: type feature. I propose that we indeed do stabilize the feature accordingly.

@rfcbot merge

@Centril Centril added the I-nominated label Aug 29, 2019

@c410-f3r c410-f3r force-pushed the c410-f3r:stabilize-attrs-fn branch from fdcb929 to 4a02912 Aug 29, 2019

@rfcbot

This comment has been minimized.

Copy link

commented Aug 29, 2019

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

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@Centril
Copy link
Member

left a comment

r=me on the code changes itself when FCP completes.

@phansch

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Been waiting for this 🎉

Currently, only the lints unused_variables and unused_mut have effect, and may be controlled, on parameters.

How does that work? Was it a deliberate decision somewhere in the code or a coincidental result of the implementation? I'm just asking to find out if there's something we should be aware of with tool lints in Clippy.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Question for @Centril and @c410-f3r --

You mention that procedural macros are not stabilized. Is there a test showing what happens when a procedural macro is applied? I presume that it generates a feature gate? Or is it considered an error?

Similarly, is there a test for lints that are intended to be disallowed?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

(Also, I should add, thanks for all your work on this. ❤️ )

@c410-f3r

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@phansch

According to the RFC, this reduced number of allowed lint was initially proposed to be part of a minimum viable product. Nevertheless, more lints can be added in future releases.

@nikomatsakis

Currently, the compiler shows the the attribute 'some_proc_macro_attribute' is currently unknown to the compiler and may have meaning added to it in the future error. With #63468, it will be changed to expected an inert attribute, found an attribute macro and the message itself behaves as a feature gate.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

@nikomatsakis

You mention that procedural macros are not stabilized. Is there a test showing what happens when a procedural macro is applied? I presume that it generates a feature gate? Or is it considered an error?

https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-builtin-attrs.rs sorta addresses this by using #[test] but I've hardened the tests somewhat in #64031.

Similarly, is there a test for lints that are intended to be disallowed?

The RFC does not specify a specific list of lints that should or should not be allowed but the two lints that are tested and accounted for seemed the most important. We'd need to do a deeper audit of all the lints to see which others could be relevant and that's probably not worth the time at the moment.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

How does that work? Was it a deliberate decision somewhere in the code or a coincidental result of the implementation? I'm just asking to find out if there's something we should be aware of with tool lints in Clippy.

@phansch Reading #61856 is probably most relevant for Clippy development.

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

☔️ The latest upstream changes (presumably #63118) made this pull request unmergeable. Please resolve the merge conflicts.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@rfcbot resolve #64282

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Question for @Centril and @c410-f3r --

You mention that procedural macros are not stabilized. Is there a test showing what happens when a procedural macro is applied? I presume that it generates a feature gate? Or is it considered an error?

Similarly, is there a test for lints that are intended to be disallowed?

Hmm. I wonder if its would be feasible (and worthwhile), in the future, for descriptions in these stabilization reports to actually link to the corresponding tests when there are specific comments like this. (It isn't always something in the diff, e.g. when the test arises from some code that was already in the repository.)

Of course it would make authoring such reports even more arduous. But it seems like it might streamline the review process...

Update: Well, the description does link to the tests, in the list at the end. I was focusing first on the text regarding what was and was not stabilized, and forgot about the list of tests that is at the end of the description. Not really sure how to improve upon what was already done here, other than (maybe) embedding the link directly in the description itself...

@rfcbot

This comment has been minimized.

Copy link

commented Sep 11, 2019

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

@c410-f3r c410-f3r force-pushed the c410-f3r:stabilize-attrs-fn branch from 2dd530c to 299d696 Sep 11, 2019

@Centril Centril removed the I-nominated label Sep 12, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Not really sure how to improve upon what was already done here, other than (maybe) embedding the link directly in the description itself...

@pnkfelix Would it help to put the tests before the history?

@rfcbot

This comment has been minimized.

Copy link

commented Sep 21, 2019

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

@bors r+ p=1

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

📌 Commit 299d696 has been approved by Centril

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

⌛️ Testing commit 299d696 with merge 6c468ee...

bors added a commit that referenced this pull request Sep 21, 2019
Auto merge of #64010 - c410-f3r:stabilize-attrs-fn, r=Centril
Stabilize `param_attrs` in Rust 1.39.0

# Stabilization proposal

I propose that we stabilize `#![feature(param_attrs)]`.

Tracking issue: #60406
Version: 1.39 (2019-09-26 => beta, 2019-11-07 => stable).

## What is stabilized

It is now possible to add outer attributes like `#[cfg(..)]` on formal parameters of functions, closures, and function pointer types. For example:

```rust
fn len(
    #[cfg(windows)] slice: &[u16],
    #[cfg(not(windows))] slice: &[u8],
) -> usize {
    slice.len()
}
```

## What isn't stabilized

* Documentation comments like `/// Doc` on parameters.

* Code expansion of a user-defined `#[proc_macro_attribute]` macro used on parameters.

* Built-in attributes other than `cfg`, `cfg_attr`, `allow`, `warn`, `deny`, and `forbid`. Currently, only the lints `unused_variables` and `unused_mut` have effect and may be controlled on parameters.

## Motivation

The chief motivations for stabilizing `param_attrs` include:

* Finer conditional compilation with `#[cfg(..)]` and linting control of variables.

* Richer macro DSLs created by users.

* External tools and compiler internals can take advantage of the additional information that the parameters provide.

For more examples, see the [RFC][rfc motivation].

## Reference guide

In the grammar of function and function pointer, the grammar of variadic tails (`...`) and parameters are changed respectively from:

```rust
FnParam = { pat:Pat ":" }? ty:Type;
VaradicTail = "...";
```

into:

```rust
FnParam = OuterAttr* { pat:Pat ":" }? ty:Type;
VaradicTail = OuterAttr* "...";
```

The grammar of a closure parameter is changed from:

```rust
ClosureParam = pat:Pat { ":" ty:Type }?;
```

into:

```rust
ClosureParam = OuterAttr* pat:Pat { ":" ty:Type }?;
```

More generally, where there's a list of formal (value) parameters separated or terminated by `,` and delimited by `(` and `)`. Each parameter in that list may optionally be prefixed by `OuterAttr+`.

Note that in all cases, `OuterAttr*` applies to the whole parameter and not just the pattern. This distinction matters in pretty printing and in turn for macros.

## History

* On 2018-10-15, @Robbepop proposes [RFC 2565][rfc], "Attributes in formal function parameter position".

* On 2019-04-30, [RFC 2565][rfc] is merged and the tracking issue is made.

* On 2019-06-12, a partial implementation was completed. The implementation was done in [#60669][60669] by @c410-f3r and the PR was reviewed by @petrochenkov and @Centril.

* On 2019-07-29, [#61238][61238] was fixed in [#61856][61856]. The issue fixed was that lint attributes on function args had no effect. The PR was written by @c410-f3r and reviewed by @matthewjasper, @petrochenkov, and @oli-obk.

* On 2019-08-02, a bug [#63210][63210] was filed wherein the attributes on formal parameters would not be passed to macros. The issue was about forgetting to call the relevant method in `fn print_arg` in the pretty printer. In [#63212][63212], written by @Centril on 2019-08-02 and reviewed by @davidtwco, the issue aforementioned was fixed.

* This PR stabilizes `param_attrs`.

## Tests

* [On Rust 2018, attributes aren't permitted on function parameters without a pattern in trait definitions.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-2018.rs)

* [All attributes that should be allowed. This includes `cfg`, `cfg_attr`, and lints check attributes.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs)

* [Built-in attributes, which should be forbidden, e.g., `#[test]`, are.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-builtin-attrs.rs)

* [`cfg` and `cfg_attr` are properly evaluated.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs)

* [`unused_mut`](https://github.com/rust-lang/rust/blob/46f405ec4d7c6bf16fc2eaafe7541019f1da2996/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs) and [`unused_variables`](https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/lint-unused-variables.rs) are correctly applied to parameter patterns.

* [Pretty printing takes formal parameter attributes into account.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-pretty.rs)

## Possible future work

* Custom attributes inside function parameters aren't currently supported but it is something being worked on internally.

* Since documentation comments are syntactic sugar for `#[doc(...)]`, it is possible to allow literal `/// Foo` comments on function parameters.

[rfc motivation]: https://github.com/rust-lang/rfcs/blob/master/text/2565-formal-function-parameter-attributes.md#motivation
[rfc]: rust-lang/rfcs#2565
[60669]: #60669
[61856]: #61856
[63210]: #63210
[61238]: #61238
[63212]: #63212

This report is a collaborative work with @Centril.
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

💥 Test timed out

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

@bors retry

Centril added a commit to Centril/rust that referenced this pull request Sep 21, 2019
Rollup merge of rust-lang#64010 - c410-f3r:stabilize-attrs-fn, r=Centril
Stabilize `param_attrs` in Rust 1.39.0

# Stabilization proposal

I propose that we stabilize `#![feature(param_attrs)]`.

Tracking issue: rust-lang#60406
Version: 1.39 (2019-09-26 => beta, 2019-11-07 => stable).

## What is stabilized

It is now possible to add outer attributes like `#[cfg(..)]` on formal parameters of functions, closures, and function pointer types. For example:

```rust
fn len(
    #[cfg(windows)] slice: &[u16],
    #[cfg(not(windows))] slice: &[u8],
) -> usize {
    slice.len()
}
```

## What isn't stabilized

* Documentation comments like `/// Doc` on parameters.

* Code expansion of a user-defined `#[proc_macro_attribute]` macro used on parameters.

* Built-in attributes other than `cfg`, `cfg_attr`, `allow`, `warn`, `deny`, and `forbid`. Currently, only the lints `unused_variables` and `unused_mut` have effect and may be controlled on parameters.

## Motivation

The chief motivations for stabilizing `param_attrs` include:

* Finer conditional compilation with `#[cfg(..)]` and linting control of variables.

* Richer macro DSLs created by users.

* External tools and compiler internals can take advantage of the additional information that the parameters provide.

For more examples, see the [RFC][rfc motivation].

## Reference guide

In the grammar of function and function pointer, the grammar of variadic tails (`...`) and parameters are changed respectively from:

```rust
FnParam = { pat:Pat ":" }? ty:Type;
VaradicTail = "...";
```

into:

```rust
FnParam = OuterAttr* { pat:Pat ":" }? ty:Type;
VaradicTail = OuterAttr* "...";
```

The grammar of a closure parameter is changed from:

```rust
ClosureParam = pat:Pat { ":" ty:Type }?;
```

into:

```rust
ClosureParam = OuterAttr* pat:Pat { ":" ty:Type }?;
```

More generally, where there's a list of formal (value) parameters separated or terminated by `,` and delimited by `(` and `)`. Each parameter in that list may optionally be prefixed by `OuterAttr+`.

Note that in all cases, `OuterAttr*` applies to the whole parameter and not just the pattern. This distinction matters in pretty printing and in turn for macros.

## History

* On 2018-10-15, @Robbepop proposes [RFC 2565][rfc], "Attributes in formal function parameter position".

* On 2019-04-30, [RFC 2565][rfc] is merged and the tracking issue is made.

* On 2019-06-12, a partial implementation was completed. The implementation was done in [rust-lang#60669][60669] by @c410-f3r and the PR was reviewed by @petrochenkov and @Centril.

* On 2019-07-29, [rust-lang#61238][61238] was fixed in [rust-lang#61856][61856]. The issue fixed was that lint attributes on function args had no effect. The PR was written by @c410-f3r and reviewed by @matthewjasper, @petrochenkov, and @oli-obk.

* On 2019-08-02, a bug [rust-lang#63210][63210] was filed wherein the attributes on formal parameters would not be passed to macros. The issue was about forgetting to call the relevant method in `fn print_arg` in the pretty printer. In [rust-lang#63212][63212], written by @Centril on 2019-08-02 and reviewed by @davidtwco, the issue aforementioned was fixed.

* This PR stabilizes `param_attrs`.

## Tests

* [On Rust 2018, attributes aren't permitted on function parameters without a pattern in trait definitions.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-2018.rs)

* [All attributes that should be allowed. This includes `cfg`, `cfg_attr`, and lints check attributes.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs)

* [Built-in attributes, which should be forbidden, e.g., `#[test]`, are.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-builtin-attrs.rs)

* [`cfg` and `cfg_attr` are properly evaluated.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs)

* [`unused_mut`](https://github.com/rust-lang/rust/blob/46f405ec4d7c6bf16fc2eaafe7541019f1da2996/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs) and [`unused_variables`](https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/lint-unused-variables.rs) are correctly applied to parameter patterns.

* [Pretty printing takes formal parameter attributes into account.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-pretty.rs)

## Possible future work

* Custom attributes inside function parameters aren't currently supported but it is something being worked on internally.

* Since documentation comments are syntactic sugar for `#[doc(...)]`, it is possible to allow literal `/// Foo` comments on function parameters.

[rfc motivation]: https://github.com/rust-lang/rfcs/blob/master/text/2565-formal-function-parameter-attributes.md#motivation
[rfc]: rust-lang/rfcs#2565
[60669]: rust-lang#60669
[61856]: rust-lang#61856
[63210]: rust-lang#63210
[61238]: rust-lang#61238
[63212]: rust-lang#63212

This report is a collaborative work with @Centril.
bors added a commit that referenced this pull request Sep 21, 2019
Auto merge of #64658 - Centril:rollup-9s3raz6, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #64010 (Stabilize `param_attrs` in Rust 1.39.0)
 - #64136 (Document From trait for LhsExpr in parser)
 - #64342 (factor out pluralisation remains after #64280)
 - #64347 (Add long error explanation for E0312)
 - #64621 (Add Compatibility Notes to RELEASES.md for 1.38.0)
 - #64632 (remove the extra comma after the match arm)
 - #64640 (No home directory on vxWorks)
 - #64641 (Exempt extern "Rust" from improper_ctypes)
 - #64642 (Fix the span used to suggest avoiding for-loop moves)

Failed merges:

r? @ghost

@bors bors merged commit 299d696 into rust-lang:master Sep 21, 2019

4 of 5 checks passed

homu Test timed out
Details
pr Build #20190911.18 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
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.