-
Notifications
You must be signed in to change notification settings - Fork 13.5k
rustdoc-json: Structured attributes #142936
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
base: master
Are you sure you want to change the base?
rustdoc-json: Structured attributes #142936
Conversation
r? @notriddle rustbot has assigned @notriddle. Use |
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing |
Awesome alona! I'll take a proper look tomorrow <3 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this!
In the near term cargo-semver-checks will have to internally re-stringify the attributes just for code compatibility with prior rustdoc JSON versions. But I look forward to the day when I can rip all that out and enjoy the structured form instead!
☔ The latest upstream changes (presumably #142929) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot has assigned @GuillaumeGomez. Use |
These commits modify Please ensure that if you've changed the output:
|
1d4b795
to
c912543
Compare
Now no longer WIP. When you're happy with it, let me know but don't r+, as I'll need to squash commits. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Looking forward to this!
src/librustdoc/clean/types.rs
Outdated
// NoMangle is special cased, as it's currently the only structured | ||
// attribute that we want to appear in HTML output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a separate concern unrelated to this PR, perhaps HTML output should list both #[no_mangle]
and #[export_name]
in their edition 2024 form i.e. wrapped in unsafe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please open an issue then so it's not forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already track this in #142835 (comment) (jana is assigned)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. It's nice to see less is_json
conditions in the common code. :)
☔ The latest upstream changes (presumably #143074) made this pull request unmergeable. Please resolve the merge conflicts. |
c5a697c
to
df1d953
Compare
This comment has been minimized.
This comment has been minimized.
/// The contents of a `#[repr(...)]` attribute. | ||
/// | ||
/// Used in [`Attribute::Repr`]. | ||
pub struct AttributeRepr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @workingjubilee, how does this look for a structured representation of a #[repr]
attribute?
This comment has been minimized.
This comment has been minimized.
d772cf6
to
244351c
Compare
Rebased. I think this is worth reviewing again, as it's changed it response to changes to rustc since intially opening. @rustbot ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, big fan of this overall. A handful of small typo fix nits.
src/librustdoc/clean/types.rs
Outdated
// NoMangle is special cased, as it appears in HTML output, and we want to show it in source form, not HIR printing. | ||
// It is also used by cargo-semver-checks. | ||
else if let hir::Attribute::Parsed(AttributeKind::NoMangle(..)) = attr { | ||
} else if let hir::Attribute::Parsed(AttributeKind::NoMangle(..)) = attr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of a drive-by, but should we update these to Rust 2024 edition form? #[unsafe(no_mangle)], #[unsafe(export_name = ...)]
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably. We try to use latest edition syntax everywhere. Worth filling an issue if there's not one already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can open a PR and save us all a step or two 👀
/// Things here are explicitly *not* covered by the [`FORMAT_VERSION`] | ||
/// constant, and may change without bumping the format version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since #[doc(hidden)]
is super critical to cargo-semver-checks
I hope we can be mindful of it with respect to this clause. I wouldn't insist on special-casing it here and I know you are planning to tackle it soon — just wanted to raise a possible concern pre-emptively :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @jdonszelmann , who is doing the #[doc]
structure attribute migration according to #131229 (comment).
I think it's doable (and a good idea) to move #[doc(hidden)]
out of Other
when it gets moved to a structured attribute in HIR. That way we do a format version bump and you're fine. (We may even do this earlier if I/someone gets round to it. But right now tests/rustdoc-json has no tests for the attribute #[doc(hidden)]
, so I didn't want to make that change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely fair enough on not making that change here. We should probably have tests for it, though I'm not sure how to make the tests include hidden items 🤔 I'll have a look around and see if I can figure it out, or if you can point me in the right direction, I'll see about opening a PR to add some.
I mostly just don't want this paragraph to be used as justification to break #[doc(hidden)]
without bumping the format version and therefore break c-s-c until we move it out of Other
:)
f31b66e
to
6cc2084
Compare
Implements rust-lang#141358. This has 2 primary benefits: 1. For rustdoc-json consumers, they no longer need to parse strings of attributes, but it's there in a structured and normalized way. 2. For rustc contributors, the output of HIR pretty printing is no longer a versioned thing in the output. People can work on rust-lang#131229 without needing to bump `FORMAT_VERSION`. (Over time, as the attribute refractor continues, I expect we'll add new things to `rustdoc_json_types::Attribute`. But this can be done separately to the rustc changes).
6cc2084
to
fd4d595
Compare
☔ The latest upstream changes (presumably #143645) made this pull request unmergeable. Please resolve the merge conflicts. |
Implements #141358.
This has 2 primary benefits.
FORMAT_VERSION
. CC @jdonszelmann @JonathanBrouwer.(Over time, as the attribute refractor continues, I expect we'll add new things to
rustdoc_json_types::Attribute
. But this can be done separately to the rustc changes).Todo before being mergable:
#[repr]
.Add tests ofI'm gonna punt this to a future PR#[doc(hidden)]
inItem::attrs
(probably in a seperate PR).