-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
rustdoc: Use own logic to print #[repr(..)]
attributes in JSON output.
#138018
Conversation
98cba84
to
67e9486
Compare
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi Some changes occurred in tests/rustdoc-json |
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.
This looks really good! Thanks for the very thorough tests, it's appreciated.
Could you update the docs on Item::attribute
? After that and rebasing to bump the format version again, this should be good to merge.
src/rustdoc-json-types/lib.rs
Outdated
@@ -30,7 +30,7 @@ pub type FxHashMap<K, V> = HashMap<K, V>; // re-export for use in src/librustdoc | |||
/// This integer is incremented with every breaking change to the API, | |||
/// and is returned along with the JSON blob as [`Crate::format_version`]. | |||
/// Consuming code should assert that this value matches the format version(s) that it supports. | |||
pub const FORMAT_VERSION: u32 = 40; | |||
pub const FORMAT_VERSION: u32 = 41; |
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.
Another PR currently in the queue is making this change: #138109 (comment)
Once that lands, you should rebase ontop of that, and then bump again.
Happy to do it, though I'm currently traveling for another week so it won't
be soon. I'd be thrilled to have this merged ASAP though, so if you'd like
to make the necessary changes and then merge, go for it. I don't care whose
name is on the commit :)
…On Tue, Mar 11, 2025, 4:55 PM Alona Enraght-Moony ***@***.***> wrote:
***@***.**** commented on this pull request.
This looks really good! Thanks for the very thorough tests, it's
appreciated.
Could you update the docs on Item::attribute? After that and rebasing to
bump the format version again, this should be good to merge.
------------------------------
In tests/rustdoc-json/attrs/repr_transparent.rs
<#138018 (comment)>:
> @@ -0,0 +1,35 @@
+#![no_std]
+
+// Rustdoc JSON *only* includes `#[repr(transparent)]`
+// if the transparency is public API:
+// - if a non-1-ZST field exists, it has to be public
+// - otherwise, all fields are 1-ZST and at least one of them is public
NIT: It'd be nice to cite where these rules come from:
https://doc.rust-lang.org/nomicon/other-reprs.html#reprtransparent
------------------------------
In src/rustdoc-json-types/lib.rs
<#138018 (comment)>:
> @@ -30,7 +30,7 @@ pub type FxHashMap<K, V> = HashMap<K, V>; // re-export for use in src/librustdoc
/// This integer is incremented with every breaking change to the API,
/// and is returned along with the JSON blob as [`Crate::format_version`].
/// Consuming code should assert that this value matches the format version(s) that it supports.
-pub const FORMAT_VERSION: u32 = 40;
+pub const FORMAT_VERSION: u32 = 41;
Another PR currently in the queue is making this change: #138109 (comment)
<#138109 (comment)>
Once that lands, you should rebase ontop of that, and then bump again.
—
Reply to this email directly, view it on GitHub
<#138018 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAR5MSQMCSMYZWTJIBFBHC32T5SVVAVCNFSM6AAAAABYKTDHVCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNZWGE4TONJSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
☔ The latest upstream changes (presumably #138548) made this pull request unmergeable. Please resolve the merge conflicts. |
I've done the test changes in only in #138569. I'll look at the content changes in a bit (if you don't get around to it first). Good thing about doing tests before impl change: It tells you more about what's being changed. In this case, we go from always showing |
…laumeGomez rustdoc-json: Add tests for `#[repr(...)]` Works towards rust-lang#137645 and rust-lang#81359 Based on rust-lang#138018, but with only the test changes. CC `@obi1kenobi` r? `@GuillaumeGomez`
…laumeGomez rustdoc-json: Add tests for `#[repr(...)]` Works towards rust-lang#137645 and rust-lang#81359 Based on rust-lang#138018, but with only the test changes. CC ``@obi1kenobi`` r? ``@GuillaumeGomez``
…laumeGomez rustdoc-json: Add tests for `#[repr(...)]` Works towards rust-lang#137645 and rust-lang#81359 Based on rust-lang#138018, but with only the test changes. CC ```@obi1kenobi``` r? ```@GuillaumeGomez```
My arguments for the behavior change are:
I hope you find these arguments convincing :) If not, then in the interest of merging the rest of the PR quickly (important for cargo-semver-checks!), I can separate out the behavior change into its own PR so we can discuss it separately. |
Yeah fair enough that makes sense. Maybe oneday |
FWIW I hope we get an explicit Rebasing this commit and resolving merge conflicts now. Should be updated momentarily. |
743870f
to
724900a
Compare
This comment has been minimized.
This comment has been minimized.
724900a
to
f7c8539
Compare
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.
Code/test changes look good to me. This can be merged after updating the docs, and rebasing on top of #138763 (which is currently in the queue and this will conflict with)
@@ -30,7 +30,7 @@ pub type FxHashMap<K, V> = HashMap<K, V>; // re-export for use in src/librustdoc | |||
/// This integer is incremented with every breaking change to the API, | |||
/// and is returned along with the JSON blob as [`Crate::format_version`]. | |||
/// Consuming code should assert that this value matches the format version(s) that it supports. | |||
pub const FORMAT_VERSION: u32 = 42; | |||
pub const FORMAT_VERSION: u32 = 43; |
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.
The docs for Item::attrs
should be updated to explain what's happened 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.
Done; I also updated the Item::attrs
comment to also explain that the #[deprecated]
attribute is no longer present there (a change we made previously but wasn't mentioned explicitly in the doc).
f7c8539
to
fe20ada
Compare
Updated the docs; waiting for the other PR to merge so I can rebase. Can I |
✌️ @obi1kenobi, you can now approve this pull request! If @aDotInTheVoid told you to " |
☔ The latest upstream changes (presumably #138830) made this pull request unmergeable. Please resolve the merge conflicts. |
fe20ada
to
bafdbca
Compare
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#138018 (rustdoc: Use own logic to print `#[repr(..)]` attributes in JSON output.) - rust-lang#138294 (Mark some std tests as requiring `panic = "unwind"`) - rust-lang#138468 (rustdoc js: add nonnull helper and typecheck src-script.js) - rust-lang#138675 (Add release notes for 1.85.1) - rust-lang#138765 (Fix Thread::set_name on cygwin) - rust-lang#138786 (Move some driver code around) - rust-lang#138793 (target spec check: better error when llvm-floatabi is missing) - rust-lang#138822 (De-Stabilize `file_lock`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#138018 - obi1kenobi:pg/librustdoc_repr_attr, r=aDotInTheVoid rustdoc: Use own logic to print `#[repr(..)]` attributes in JSON output. Switch away from `Debug`-like representation of `#[repr(..)]` attributes, and start using rustdoc's own logic for pretty-printing `#[repr(..)]` in rustdoc JSON. Part of addressing rust-lang#137645 but not a complete solution for it. r? `@aDotInTheVoid`
Switch away from
Debug
-like representation of#[repr(..)]
attributes, and start using rustdoc's own logic for pretty-printing#[repr(..)]
in rustdoc JSON.Part of addressing #137645 but not a complete solution for it.
r? @aDotInTheVoid