-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Allow struct and enum to contain inner attrs #84414
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
It feels a bit weird to do this after also having removed it from things like |
I'm fairly indifferent to the question of inner attributes in general. Personally, I've never felt the need to write an inner attribute instead of an outer attribute (except at the crate root). One of the rationales for #83312 is to reduce the places where inner attributes can occur in an expression, allowing us to use a simple lookahead check to determine if inner attributes are even possible during expression parsing. However, that does not apply here - in general, items can already support inner attributes, so we don't have any logic in place to bail out of token collection early. Also, there are significantly more expressions than items in any normal program, so there are fewer performance gains to be had by avoiding work during the parsing of items. |
With #83312 we get a simple rule for where inner attributes are supported - in all containers for statements (or some subset of statements), this PR makes the rule somewhat random again. Given the number of issues with inner attributes (e.g. #54726 or token collection) I'd personally only allow them when there's no other choice - at the crate root (+ perhaps out-of-line modules) if we were designing things from scratch. |
This comment has been minimized.
This comment has been minimized.
|
Linking some closely related discussion on where and how to support inner attributes: #84879. |
I personally 'parse' #![attr]
use a::b;
type X = i32;
fn x() ->X; Structs, enums, and unions are only a comma-separated list of members/variants, not containers of all kind of items like traits and modules are. So an 'item' like
I suppose that same argument could be made for putting methods inside a struct body too, but that's not something that would syntactically work very well either. |
I don't find "like an item, seen as part of a bunch of items" to be a beneficial intuition for inner attributes. That would heavily imply:
neither of which is realistic. If anything, it seems beneficial to help people out of the mental model of inner attributes as items. |
Dropping I-nominated -- discussed in last week's lang meeting, waiting on @joshtriplett for a summary on this PR of that discussion. |
This observation from https://github.com/rust-lang/lang-team/blob/023fa403807f298d45753077eb6528219ce9aadf/minutes/2021-06-29.md#allow-struct-and-enum-to-contain-inner-attrs-rust84414 is not accurate:
Attribute macro input is required to be syntactically valid Rust (and I would hate for that to change). Otherwise this PR already wouldn't be necessary, as #![...] are tokens too. |
☔ The latest upstream changes (presumably #93594) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We discussed this again in today's @rust-lang/lang meeting. In general, we didn't feel enthusiastic about adding inner attributes in more places than those already supported, particularly where outer attributes are already an option. I appreciate that being able to put a token inside the enum braces is evocative in this case, where variants are being generated by the attribute. However, we'd like to avoid adding more variation here in a direction that we've previously shifted away from, and we find @petrochenkov's comments compelling here. @rfcbot close |
Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. 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. |
@rfcbot reviewed I see the appeal of inner attribute, but indeed I find @petrochenkov's concerns valid, and the use case seems insufficiently compelling right now to add any risk of doing something we might regret. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to close, 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. |
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Parse inner attributes on inline const block According to rust-lang#84414 (comment), inner attributes are intended to be supported *"in all containers for statements (or some subset of statements)"*. This PR adds inner attribute parsing and pretty-printing for inline const blocks (rust-lang#76001), which contain statements just like an unsafe block or a loop body. ```rust let _ = const { #![allow(...)] let x = (); x }; ```
Previously inner attributes have been supported in modules and functions ever since Rust 1.0.0:
and in traits since Rust 1.43.0 (#68728):
This PR extends the Rust parser to additionally handle inner attributes inside of structs, enums, and unions.
Rationale: I am interested in applying this syntax in https://github.com/dtolnay/cxx, for example for pulling enum variant names and discriminant values out of a Clang AST dump. Allowing the attribute to go where the variants go is more evocative than putting it outside of the enum.