Skip to content

Refactor rustc_attr_data_structures documentation #142082

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Jun 5, 2025

I was reading through AttributeKind and realized that attributes like InlineAttr didn't appear in it, however, I found them in rustc_codegen_ssa and understood why (guessing).

There's almost no overall documentation for this crate, I've added the organized documentation at the top of lib.rs, and I've grouped the Attributes into two categories: AttributeKind that run all through the compiler, and the ones that are only used in codegen_ssa, such as InlineAttr, OptimizeAttr, InstructionSetAttr.

Also, I've added documentation for AttributeKind that further explains why attributes like InlineAttr don't appear in it, with examples for each variant.

r? @jdonszelmann

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 5, 2025

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

@xizheyin xizheyin force-pushed the rustc_attr_data_structures branch from ff9276d to 1cf957f Compare June 5, 2025 14:16
Comment on lines -148 to -155
/// The word "parsed" could be a little misleading here, because the parser already parses
/// attributes early on. However, the result, an [`ast::Attribute`]
/// is only parsed at a high level, still containing a token stream in many cases. That is
/// because the structure of the contents varies from attribute to attribute.
/// With a parsed attribute I mean that each attribute is processed individually into a
/// final structure, which on-site (the place where the attribute is useful for, think the
/// the place where `must_use` is checked) little to no extra parsing or validating needs to
/// happen.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By adding Note on Attribute Organization, we don't have to explain the meaning of parsed as we did. It would be clearer.

Copy link
Contributor

@jdonszelmann jdonszelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my previous comments, won't approve this PR for the time being despite it having some good changes.

/// [`rustc_attr_parsing`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_attr_parsing/index.html
#[derive(Clone, Debug, HashStable_Generic, Encodable, Decodable, PrintAttribute)]
pub enum AttributeKind {
// tidy-alphabetical-start
/// Allows unstable features in const functions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just approved someone else's PR that added this same documentation, it was briefer, but I'd actually say that's alright. Before we merge this let's wait on that one (#140372) and then I'll decide whether it's better to have this. This is quite a lot of repeated typing for every new attribute, but it's nice to have them documented somewhere too. That place might not be here though. Alas, for that reason I won't quite approve this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some day we might be able to parse these comments and use a derive macro to document them (and use them as templates for diags? Something similar but worse already happens so we'll see)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is some repetitive work here,and it looks heavy.My original intention in writing this document is, many developers can't understand all these macros, so they need to search everywhere to understand their functions when reading this code.

But it's a good idea to use macros to eliminate duplication of work. I'll delete it here first. Comments with examples with macros should be meaningful to many enumerations in rustc C. Can we add it as a separate function?

@@ -1,3 +1,37 @@
//! Data structures for representing parsed attributes in the Rust compiler.
//!
//! This crate is part of a series of crates that handle attribute processing:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this can easily get out-of-date. I'd prefer a reference to attr parsing here and to centralize all docs there. The story is rather similar anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Here is a duplicate of that document.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2025
@xizheyin xizheyin force-pushed the rustc_attr_data_structures branch from 1cf957f to b867d3e Compare June 6, 2025 14:10
Copy link
Contributor Author

@xizheyin xizheyin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also remove the comments in variants of AttributeKind.
@rustbot ready

Comment on lines +153 to +162
/// Some attributes can be applied multiple times to the same item, and they are "collapsed" into a single
/// semantic attribute. For example:
/// ```rust
/// #[repr(C)]
/// #[repr(packed)]
/// struct S { }
/// ```
/// This is equivalent to `#[repr(C, packed)]` and results in a single [`AttributeKind::Repr`] containing
/// both `C` and `packed` annotations. This collapsing happens during parsing and is reflected in the
/// data structures defined in this enum.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add repr example to explain "collapsed"

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants