rustdoc-json: Refractor and document Id's by aDotInTheVoid · Pull Request #133981 · rust-lang/rust

aDotInTheVoid · 2024-12-06T21:12:00Z

While working on documenting Id's, I realized alot of the way they were generated was weird and unnecessary. Eg:

The fully uninterned id type was (FullItemId, Option<FullItemId>), meaning it wasn't actually full!
None of the extra fields in Option<FullItemId> would ever be used
imported_item_id was a rustdoc_json_types::Id instead of a simpler DefId.

I believe the new implementation still covers all the same cases, but in a more principled way (and explaining why each piece is needed).

This was written to be reviewed commit-by-commit, but it might be easier to review all at once if you're not interested in tracking how the original code became the final code.

cc @its-the-shrimp

r? @fmease

its-the-shrimp · 2024-12-07T19:01:26Z

src/librustdoc/json/ids.rs

+pub(super) struct FullItemId {
+    /// The "main" id of the item.
+    ///
+    /// In most cases this unequely identifies the item, other fields are just
+    /// used for edge-cases.
+    def_id: DefId,
+
+    /// An extra [`DefId`], which we need for:
+    ///
+    /// 1. Auto-trait impls synthesized by rustdoc.
+    /// 2. Blanket impls synthesized by rustdoc.
+    /// 3. Splitting of reexports of multiple items.
+    ///
+    ///    Eg:
+    ///
+    ///    ```rust
+    ///     mod module {
+    ///         pub struct Foo {} // Exists in type namespace
+    ///         pub fn Foo(){} // Exists in value namespace
+    ///     }
+    ///
+    ///     pub use module::Foo; // Imports both items
+    ///     ```
+    ///
+    ///     In HIR, the `pub use` is just 1 item, but in rustdoc-json it's 2, so
+    ///     we need to disambiguate.
+    extra_id: Option<DefId>,
+
+    /// Needed for `rustc_doc_primitive` modules.
+    ///
+    /// For these, 1 [`DefId`] is used for both the primitive and the fake-module
+    /// that holds it's docs.
+    ///
+    /// N.B. This only matters when documenting the standard library with
+    /// `--document-private-items`. Maybe we should delete that module, and
+    /// remove this.
+    name: Option<Symbol>,
+}


could this struct be an enum? I'd expect the fields extra_id & name to never be Some simultaneously

Also, do we need to store the name? if this only exists to generate 2 rustdoc IDs for what is 1 ID in rustc, this field could be a boolean, say, is_primitive, or just an enum variant

I guess we could do that, and end up with something like clean::ItemId again.

Now I'm almost curious if we could use clean::ItemId in place of json::ids::FullItemId.

Probably not, FullItemId would also need a Primitive variant then, along with the variants of ItemId

I think it's best to leave that to a followup then. The structure's already sufficiently different.

bors · 2024-12-17T22:20:53Z

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

fmease

I'm very sorry that this took so long, this shouldn't happen in the future! The changes look good.

r=me with nitpicks addressed and squashed into ~1 commit (the SingleItemId excursion needn't be in the final history :)).

src/librustdoc/json/ids.rs

fmease · 2025-03-12T17:20:44Z

src/librustdoc/json/ids.rs

+/// One of these coresponds to every:
+/// 1. [`rustdoc_json_types::Item`].
+/// 2. [`rustdoc_json_types::Id`] transitivly (as each `Item` has an `Id`).


I would rephrase it like "Each one corresponds to an:" because "one […] corresp. to every" sounds like a one-to-many relation to me (but I'm L2) while this is is a one-to-one-to-one relation, right?

src/librustdoc/json/ids.rs

fmease · 2025-03-12T17:26:11Z

src/librustdoc/json/ids.rs

+    ///     mod module {
+    ///         pub struct Foo {} // Exists in type namespace
+    ///         pub fn Foo(){} // Exists in value namespace
+    ///     }
+    ///
+    ///     pub use module::Foo; // Imports both items
+    ///     ```
+    ///
+    ///     In HIR, the `pub use` is just 1 item, but in rustdoc-json it's 2, so
+    ///     we need to disambiguate.


These lines are intended by one extra space (this actually shows up literally in the rendered code block)

src/librustdoc/json/ids.rs

fmease · 2025-03-12T17:38:12Z

src/librustdoc/json/ids.rs

+        let (def_id, mut extra_id) = match item_id {
+            clean::ItemId::DefId(did) => (did, None),
+            clean::ItemId::Blanket { for_, impl_id } => (for_, Some(impl_id)),
+            clean::ItemId::Auto { for_, trait_ } => (for_, Some(trait_)),
+        };


What if you wrote it like this:

Suggested change

let (def_id, mut extra_id) = match item_id {

clean::ItemId::DefId(did) => (did, None),

clean::ItemId::Blanket { for_, impl_id } => (for_, Some(impl_id)),

clean::ItemId::Auto { for_, trait_ } => (for_, Some(trait_)),

};

let (def_id, extra_id) = match item_id {

clean::ItemId::DefId(did) => (did, imported_id),

clean::ItemId::Blanket { for_, impl_id } => (for_, Some(impl_id)),

clean::ItemId::Auto { for_, trait_ } => (for_, Some(trait_)),

};

fmease · 2025-03-12T17:38:47Z

src/librustdoc/json/ids.rs

+
+        if let Some(imported_id) = imported_id {
+            assert_eq!(extra_id, None, "On an import item, so won't have extra from impl");
+            extra_id = Some(imported_id);
+        }


...then you could remove this entirely, right?

Suggested change

if let Some(imported_id) = imported_id {

assert_eq!(extra_id, None, "On an import item, so won't have extra from impl");

extra_id = Some(imported_id);

}

I want to work on this in a followup commit, so in this commit I make it self-contained. Contains no code changes, all functions are defined exactly as they were in conversions.rs.

aDotInTheVoid · 2025-03-12T21:40:25Z

@bors r=fmease

bors · 2025-03-12T21:40:27Z

📌 Commit 2e1d3d1 has been approved by fmease

It is now in the queue for this repository.

Alot of the current id handling is weird and unnecessary. e.g: 1. The fully uninterned id type was (FullItemId, Option<FullItemId>) meaning it wasn't actually full! 2. None of the extra fields in Option<FullItemId> would ever be used 3. imported_item_id was a rustdoc_json_types::Id instead of a simpler DefId This commit removes the unnessessary complexity, and documents where the remaining complexity comes from. Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>

aDotInTheVoid · 2025-03-12T22:12:04Z

@bors r=fmease

bors · 2025-03-12T22:12:06Z

📌 Commit a05d6ab has been approved by fmease

It is now in the queue for this repository.

…=fmease rustdoc-json: Refractor and document Id's Closes rust-lang#133780 While working on documenting Id's, I realized alot of the way they were generated was weird and unnecessary. Eg: 1. The fully uninterned id type was `(FullItemId, Option<FullItemId>)`, meaning it wasn't actually full! 2. None of the extra fields in `Option<FullItemId>` would ever be used 3. `imported_item_id` was a `rustdoc_json_types::Id` instead of a simpler `DefId`. I believe the new implementation still covers all the same cases, but in a more principled way (and explaining why each piece is needed). This was written to be reviewed commit-by-commit, but it might be easier to review all at once if you're not interested in tracking how the original code became the final code. cc `@its-the-shrimp` r? `@fmease`

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#126856 (remove deprecated tool `rls`) - rust-lang#133981 (rustdoc-json: Refractor and document Id's) - rust-lang#136842 (Add libstd support for Trusty targets) - rust-lang#137355 (Implement `read_buf` and vectored read/write for SGX stdio) - rust-lang#137457 (fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries) - rust-lang#138162 (Update the standard library to Rust 2024) - rust-lang#138273 (metadata: Ignore sysroot when doing the manual native lib search in rustc) - rust-lang#138346 (naked functions: on windows emit `.endef` without the symbol name) - rust-lang#138370 (Simulate OOM for the `try_oom_error` test) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#126856 (remove deprecated tool `rls`) - rust-lang#133981 (rustdoc-json: Refractor and document Id's) - rust-lang#136842 (Add libstd support for Trusty targets) - rust-lang#137355 (Implement `read_buf` and vectored read/write for SGX stdio) - rust-lang#138162 (Update the standard library to Rust 2024) - rust-lang#138273 (metadata: Ignore sysroot when doing the manual native lib search in rustc) - rust-lang#138346 (naked functions: on windows emit `.endef` without the symbol name) - rust-lang#138370 (Simulate OOM for the `try_oom_error` test) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#133981 - aDotInTheVoid:document-docs-ids, r=fmease rustdoc-json: Refractor and document Id's Closes rust-lang#133780 While working on documenting Id's, I realized alot of the way they were generated was weird and unnecessary. Eg: 1. The fully uninterned id type was `(FullItemId, Option<FullItemId>)`, meaning it wasn't actually full! 2. None of the extra fields in `Option<FullItemId>` would ever be used 3. `imported_item_id` was a `rustdoc_json_types::Id` instead of a simpler `DefId`. I believe the new implementation still covers all the same cases, but in a more principled way (and explaining why each piece is needed). This was written to be reviewed commit-by-commit, but it might be easier to review all at once if you're not interested in tracking how the original code became the final code. cc ``@its-the-shrimp`` r? ``@fmease``

rustbot assigned fmease Dec 6, 2024

rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 6, 2024

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the document-docs-ids branch from 58a9e1d to e62b16f Compare December 6, 2024 21:54

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the document-docs-ids branch from e62b16f to 324a73d Compare December 7, 2024 01:11

its-the-shrimp reviewed Dec 7, 2024

View reviewed changes

rust-cloud-vms bot force-pushed the document-docs-ids branch from 324a73d to 70e22b3 Compare December 17, 2024 23:05

fmease closed this Mar 12, 2025

fmease reopened this Mar 12, 2025

fmease approved these changes Mar 12, 2025

View reviewed changes

fmease 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 Mar 12, 2025

rust-cloud-vms bot force-pushed the document-docs-ids branch 2 times, most recently from 35add17 to 5f510a3 Compare March 12, 2025 21:33

rustdoc-json: Extract Id handling into its own module

0a47792

I want to work on this in a followup commit, so in this commit I make it self-contained. Contains no code changes, all functions are defined exactly as they were in conversions.rs.

rust-cloud-vms bot force-pushed the document-docs-ids branch from 5f510a3 to 2e1d3d1 Compare March 12, 2025 21:39

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2025

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the document-docs-ids branch from 2e1d3d1 to a05d6ab Compare March 12, 2025 22:10

matthiaskrgr mentioned this pull request Mar 13, 2025

Rollup of 9 pull requests #138443

Closed

matthiaskrgr mentioned this pull request Mar 13, 2025

Rollup of 8 pull requests #138448

Merged

bors merged commit b3ab695 into rust-lang:master Mar 13, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 13, 2025

Uh oh!

Conversation

aDotInTheVoid commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

its-the-shrimp Dec 7, 2024

Choose a reason for hiding this comment

Uh oh!

its-the-shrimp Dec 7, 2024

Choose a reason for hiding this comment

Uh oh!

aDotInTheVoid Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

its-the-shrimp Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aDotInTheVoid Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

bors commented Dec 17, 2024

Uh oh!

fmease left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmease Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmease Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmease Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

fmease Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aDotInTheVoid commented Mar 12, 2025

Uh oh!

bors commented Mar 12, 2025

Uh oh!

This comment has been minimized.

aDotInTheVoid commented Mar 12, 2025

Uh oh!

bors commented Mar 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aDotInTheVoid commented Dec 6, 2024 •

edited

Loading

its-the-shrimp Dec 9, 2024 •

edited

Loading

fmease Mar 12, 2025 •

edited

Loading

fmease Mar 12, 2025 •

edited

Loading