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

[rustdoc-json] JSON no longer inlines #93518

Closed

Conversation

CraftSpider
Copy link
Contributor

This removes all inlining from Rustdoc-JSON. Instead, JSON preserves private modules containing public items as 'stripped' modules, indicating that they exist solely to prevent orphaning of contained items. Format version has been bumped and tests updated, as this is a significant change over previous behavior around re-exports. However, the new system fixes multiple ICEs and is generally cleaner.

Fixes #83057
Fixes #83720

This removes all inlining from Rustdoc-JSON. Instead, JSON preserves private modules containing public items as 'stripped' modules, indicating that they exist solely to prevent orphaning of contained items. Format version has been bumped and tests updated, as this is a significant change over previous behavior around re-exports. However, the new system fixes multiple ICEs and is generally cleaner.
@rust-highfive
Copy link
Collaborator

rustdoc-json-types is a public (although nightly-only) API. Consider changing src/librustdoc/json/conversions.rs instead; otherwise, make sure you update format_version.

cc @CraftSpider,@aDotInTheVoid

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 31, 2022
@rust-highfive
Copy link
Collaborator

r? @jsha

(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 Jan 31, 2022
@CraftSpider CraftSpider added the A-rustdoc-json Area: Rustdoc JSON backend label Jan 31, 2022
@CraftSpider
Copy link
Contributor Author

May want to hold off on this until either #93522 or #93524 is fixed

@GuillaumeGomez
Copy link
Member

Waiting for them then. Please ping us when it's ready for review.

@Urgau
Copy link
Member

Urgau commented Feb 8, 2022

I just experimented with this PR and found out that the inlining is still done for intra-doc-links.

@Urgau
Copy link
Member

Urgau commented Feb 9, 2022

Inlining is also still done with double (maybe more, haven't tested) nested re-exports:

mod depth1 {
    mod depth2 {
        pub const C: usize = 7;
    }

    pub use self::depth2::C;
}

pub use depth1::C; // Direct link to `pub const C` instead of `pub use self::depth2::C`

@CraftSpider CraftSpider added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2022
@@ -192,6 +192,9 @@ pub enum ItemKind {
#[serde(tag = "kind", content = "inner", rename_all = "snake_case")]
pub enum ItemEnum {
Module(Module),
StrippedModule {
Copy link
Member

@Enselic Enselic Feb 13, 2022

Choose a reason for hiding this comment

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

For symmetry with most other enum variants, most notably Module(Module), this should be StrippedModule(StrippedModule).

Another option that I find even more attractive since it removes the need for more types and enum variants is to simply add is_stripped to struct Module, so it becomes:

pub struct Module {
    pub is_crate: bool,
    pub is_stripped: bool,
    pub items: Vec<Id>,
}

That would be even more symmetric with existing code, since there is no CrateModule (which would be an alternative to having the current is_crate field in Module).

@bors
Copy link
Contributor

bors commented Mar 4, 2022

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

@GuillaumeGomez
Copy link
Member

I took over this one in #99287.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 16, 2022
…export, r=notriddle

[rustdoc-json] JSON no longer inlines

Fixes rust-lang#98007.
Fixes rust-lang#96161.
Fixes rust-lang#83057.
Fixes rust-lang#83720.

I took over rust-lang#93518 and applied the comments and added more tests.

There was one thing missing (which is in the second commit): if a non-exported item was used in a public API but not reexported, it was still missing.

cc `@CraftSpider` `@Urgau` `@Enselic`

r? `@notriddle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
8 participants