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

Allow struct and enum to contain inner attrs #84414

Closed
wants to merge 2 commits into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 21, 2021

Previously inner attributes have been supported in modules and functions ever since Rust 1.0.0:

mod m {
    #![doc = ""]
}

extern "C" {
    #![doc = ""]
}

fn f() {
    #![doc = ""]
}

and in traits since Rust 1.43.0 (#68728):

trait T {
    #![doc = ""]
}

This PR extends the Rust parser to additionally handle inner attributes inside of structs, enums, and unions.

struct S {
    #![doc = ""]
}

enum E {
    #![doc = ""]
}

union U {
    #![doc = ""]
}

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.

#[cxx::bridge]
mod ffi {
    extern "C++" {
        include!("path/to/header.h");
    }

    ...

    enum Enum {
        #![variants_from_header]
    }
}

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2021
@jonas-schievink jonas-schievink added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Apr 21, 2021
@scottmcm
Copy link
Member

It feels a bit weird to do this after also having removed it from things like match in #83312

@Aaron1011
Copy link
Member

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.

@petrochenkov
Copy link
Contributor

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.
In other cases making attributes inner doesn't make much difference in functionality or readability, they would still be only a couple of lines away from their outer equivalents.
So I generally don't support the idea of supporting inner attributes in more places.

@petrochenkov petrochenkov added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2021
@bors

This comment has been minimized.

@dtolnay
Copy link
Member Author

dtolnay commented May 12, 2021

@dtolnay
Copy link
Member Author

dtolnay commented May 12, 2021

Linking some closely related discussion on where and how to support inner attributes: #84879.

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 5, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2021

I personally 'parse' #![attr] mostly like an item just like use ..; and type T = ..; etc. So I'd expect to see these as part of a bunch of items. E.g.

#![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 #![attr] directly followed by the comma-separated list seems very odd to me.

Allowing the attribute to go where the variants go is more evocative than putting it outside of the enum.

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.

@dtolnay
Copy link
Member Author

dtolnay commented Jun 22, 2021

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:

  1. that they are order insensitive:

    pub trait Trait {...}
    
    #![lol]
  2. that they can have attributes:

    /// this doc comment documents the following attribute.
    #![no_std]

neither of which is realistic. If anything, it seems beneficial to help people out of the mental model of inner attributes as items.

@Mark-Simulacrum
Copy link
Member

Dropping I-nominated -- discussed in last week's lang meeting, waiting on @joshtriplett for a summary on this PR of that discussion.

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 24, 2021
@dtolnay
Copy link
Member Author

dtolnay commented Aug 4, 2021

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:

@scottmcm: because it's all in the macro anyway, it could do arbitrary tokens too?

enum Foo { .. from header .. }

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.

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 10, 2021
@bors
Copy link
Contributor

bors commented Feb 2, 2022

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

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 3, 2022
@dtolnay
Copy link
Member Author

dtolnay commented Feb 3, 2022

@pnkfelix pnkfelix added the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 10, 2022
@joshtriplett joshtriplett removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. labels Mar 15, 2022
@joshtriplett
Copy link
Member

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

@rfcbot
Copy link

rfcbot commented Mar 15, 2022

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Mar 15, 2022
@nikomatsakis
Copy link
Contributor

@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.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 22, 2022
@rfcbot
Copy link

rfcbot commented Mar 22, 2022

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 1, 2022
@rfcbot
Copy link

rfcbot commented Apr 1, 2022

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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 1, 2022
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Apr 15, 2022
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
};
```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 15, 2022
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
};
```
RalfJung added a commit to RalfJung/rust that referenced this pull request Apr 15, 2022
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
};
```
RalfJung added a commit to RalfJung/rust that referenced this pull request Apr 15, 2022
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
};
```
RalfJung added a commit to RalfJung/rust that referenced this pull request Apr 15, 2022
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
};
```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 15, 2022
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
};
```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 16, 2022
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
};
```
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 21, 2022
@dtolnay dtolnay deleted the inner branch April 21, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.