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 2539, "#[cfg_attr] expanding to multiple attributes" #54881

Closed
3 of 4 tasks
Centril opened this issue Oct 7, 2018 · 12 comments · Fixed by #62047
Closed
3 of 4 tasks

Tracking issue for RFC 2539, "#[cfg_attr] expanding to multiple attributes" #54881

Centril opened this issue Oct 7, 2018 · 12 comments · Fixed by #62047
Assignees
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Oct 7, 2018

This is a tracking issue for the RFC "#[cfg_attr] expanding to multiple attributes" (rust-lang/rfcs#2539).

Steps:

Unresolved questions:

None

@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Oct 7, 2018
@Centril
Copy link
Contributor Author

Centril commented Oct 7, 2018

Implementation in #54862

@Centril Centril added B-unstable Blocker: Implemented in the nightly compiler and unstable. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. labels Oct 17, 2018
@Centril
Copy link
Contributor Author

Centril commented Nov 11, 2018

Setting earliest date for stabilization proposal: 2018-11-22 (11 days from now).

@Centril
Copy link
Contributor Author

Centril commented Nov 25, 2018

Stabilization proposal

@rfcbot merge

Originally proposed in RFC rust-lang/rfcs#2539, implemented in #54862, and in nightly since ~ the 12th October, cfg_attr_multi allows users to specify multiple attributes in cfg_attr(condition, foo, bar) where foo and bar are the attributes to expand should condition hold. The benefit of cfg_attr_multi is ergonomics but also readability and maintainability; you avoid having to specify condition several times. This can be particularly beneficial if condition is long and non-trivial.

Version target

The next version is 1.32 which goes into beta the 7th of December; It is quite possible that this will slip into 1.33 however depending on how long the review process takes.

What is stabilized

Users are now permitted to write for example:

#[cfg_attr(all(),)] struct A; // trailing comma permitted as well as zero items.
#[cfg_attr(all(), must_use)] struct A;
#[cfg_attr(all(), must_use,)] struct A; // trailing permitted.
#[cfg_attr(foo, must_use, deprecated)] struct A;
#[cfg_attr(all(), must_use, deprecated,)] struct A; // trailing permitted.

The semantics of these are to expand to for example #[must_use] #[deprecated] on struct A if foo should hold (4th example).

If zero attributes are given, e.g. #[cfg_attr(all(),)] then unused_attributes the lint will be emitted. This behaviour is not witnessed in the current nightly compiler; that is OK since it's a minor issue but it will need to be fixed in the stabilization PR. See https://github.com/rust-lang/rust/blob/master/src/libsyntax/config.rs#L145.

What is not

Users are not permitted to write:

  • More than one trailing comma, e.g. #[cfg_attr(all(), must_use, deprecated,,)]
  • Zero attributes and no comma, e.g. #[cfg_attr(all())]
  • An empty cfg_attr, e.g. #[cfg_attr()]
  • #[foo, bar]; this is proposed in Multiple Attributes in an Attribute Container rfcs#2600.
    This stabilization is forward compatible with that RFC.

Divergences from RFC

None


cc @Havvy can you make the stabilization PR + resolve the minor FIXME in it?
Also cc @petrochenkov who reviewed the implementation.

@rfcbot
Copy link

rfcbot commented Nov 25, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 25, 2018
@Havvy
Copy link
Contributor

Havvy commented Nov 25, 2018

I wouldn't call the implementation of the lint a minor fix. The first time I tried, I bailed when I hit a set of cases I couldn't get something the lint infrastructure wanted.

The lint is also the least value add of the RFC and I propose stabilizing it separately if doing so doesn't break backwards compatibility.

@Centril
Copy link
Contributor Author

Centril commented Nov 25, 2018

@Havvy Warn by default lints can always be added later so if you think it is non-trivial to implement in the stabilization PR, I wouldn't mind deferring that to the future; as you say, it is indeed the least value-add. However, if you do manage to fix it in the stabilization PR, that would be nice (but not required, imo).

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Dec 10, 2018
@rfcbot
Copy link

rfcbot commented Dec 10, 2018

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 10, 2018
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Dec 20, 2018
@rfcbot
Copy link

rfcbot commented Dec 20, 2018

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

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Dec 20, 2018
@Centril
Copy link
Contributor Author

Centril commented Dec 20, 2018

@Havvy Up for writing the stabilization PR?

@Havvy
Copy link
Contributor

Havvy commented Dec 22, 2018

I don't and haven't had motivation pertaining to Rustlang at the moment, sorry. Eventually it'll return.

@Dylan-DPC-zz
Copy link

r? @Dylan-DPC

@Centril Centril assigned Dylan-DPC-zz and unassigned Havvy Dec 23, 2018
bors added a commit that referenced this issue Jan 8, 2019
stabilize cfg_attr_multi

Stabilizes cfg_attr_multi feature

Related to #54881

Will add the lint in a seperate PR

r? @Centril
@Centril
Copy link
Contributor Author

Centril commented Jan 8, 2019

Stabilization has gone through and documentation issue has been filed against the reference; what remains is to fix the lint which @Dylan-DPC will do in a separate PR. After that this issue can be closed.

Centril added a commit to Centril/rust that referenced this issue Jun 22, 2019
…bank

Trigger `unused_attribute` lint on `#[cfg_attr($pred,)]`

Lint on `#[cfg_attr($pred,)]` as decided in rust-lang#54881 (comment).

Closes rust-lang#54881.

r? @estebank
Centril added a commit to Centril/rust that referenced this issue Jun 22, 2019
…bank

Trigger `unused_attribute` lint on `#[cfg_attr($pred,)]`

Lint on `#[cfg_attr($pred,)]` as decided in rust-lang#54881 (comment).

Closes rust-lang#54881.

r? @estebank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants