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.

Sign up for GitHub

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

Jump to bottom

[Rust 2018] rustdoc doesn't link "pub use whatever_crate;" #52509

Closed
crepererum opened this issue Jul 18, 2018 · 20 comments
Closed

[Rust 2018] rustdoc doesn't link "pub use whatever_crate;" #52509

crepererum opened this issue Jul 18, 2018 · 20 comments
Assignees
@GuillaumeGomez
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@crepererum
Copy link
Contributor

Rust 2015 (1.27.1)

screenshot from 2018-07-18 22-36-24

Rust 2018 (preview at 1ecf6929d 2018-07-16)

screenshot from 2018-07-18 22-36-49
@crepererum
Copy link
Contributor Author

Please can someone give this a A-rust-2018-preview label?

@kennytm kennytm added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. F-rust_2018_preview `#![feature(rust_2018_preview)]` labels Jul 18, 2018
@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2018
@nikomatsakis
Copy link
Contributor

Nominating for prioritization. cc @rust-lang/docs -- this is a rustdoc bug, is there someone who wants to tackle it?

I added T-compiler so that we take a look at the next compiler meeting otherwise.

@GuillaumeGomez
Copy link
Member

I'll take a look but I assume it's just a non-backported changes for the module system changes...

@nikomatsakis
Copy link
Contributor

cc @GuillaumeGomez -- any news?

@GuillaumeGomez
Copy link
Member

Busy then sick so nothing, sorry about that.

@GuillaumeGomez GuillaumeGomez self-assigned this Oct 17, 2018
@pnkfelix
Copy link
Member

discussed at compiler team meeting. Taking off RC2.

@nikomatsakis
Copy link
Contributor

@GuillaumeGomez ok — if you still have time to tackle, great! Otherwise, let us know and we can find someone to take a peek.

@GuillaumeGomez
Copy link
Member

If I haven't finish this week-end, then please please ask someone else.

@GuillaumeGomez
Copy link
Member

So, here what we got: in here, pub use imported crates are defined as ImportItem(Glob(ImportSource)). Inside it, I found absolutely no track of any possible way to determine if it's how an extern crate is imported or not. Also, here's the current rustdoc output:

screen shot 2018-10-20 at 17 32 06

(Yes, in "Modules".)

@pnkfelix
Copy link
Member

I'll see what I can do to help here.

@pnkfelix pnkfelix self-assigned this Oct 25, 2018
@QuietMisdreavus
Copy link
Member

I think we can modify impl Clean for doctree::Import to check if a "single" import that is importing a module is actually the root of a different crate, then refuse to inline it? That at least makes it appear like an import:

image

(This sample was made by applying #[doc(no_inline)] to the pub use qwop; statement, which provides the same effect.)

Alternately, after that check, we could generate a pub extern crate item instead of an import, which would unify it with the 2015 output. On that note, do we even want to try unifying these outputs? Do we always want docs to look like the latest edition (which would require that we start changing how we render existing pub extern crate statements), or do we render based on the edition passed to rustdoc (which makes docs look different based on the crate being documented)? I'm leaning toward "modify docs to look like newest edition", but i'm going to punt to @rust-lang/docs for that one.

In the immediate term, i guess this issue is now about the eager inlining of external crates as if they were modules inside the current crate, which drives me back to my earlier suggestion.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

T-compiler triage. Tagging P-high. Leaving assigned to self.

@pnkfelix pnkfelix added the P-high High priority label Nov 8, 2018
@QuietMisdreavus QuietMisdreavus mentioned this issue Nov 9, 2018
Merged
@QuietMisdreavus
Copy link
Member

I've opened #55804 to make pub use some_crate turn into a plain re-export in the docs, which should at least help this out.

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 12, 2018
@pietroalbini
Rollup merge of rust-lang#55804 - QuietMisdreavus:eager-crate-inline,…
ed39b93 
… r=pnkfelix

rustdoc: don't inline `pub use some_crate` unless directly asked to

cc rust-lang#52509 (fixes it? i'm not sure about my comment summoning the docs team)

When rustdoc encounters a `pub use` statement for an item from another crate, it will eagerly inline its contents into your crate. This somewhat clashes with the new paths behavior in Rust 2018, in which crates are implicitly linked and re-exported with `pub use` instead of `pub extern crate`. In rust 2015, `pub extern crate` would only create a single line for its re-export in the docs, so i'm making it do the same with `pub use some_crate;`.

The exact new behavior is like this: *If rustdoc sees a `pub use` statement, and the item being imported is the root of another crate, it will only inline it if `#[doc(inline)]` is provided.* I made it only avoid crate roots because otherwise it would stop inlining any module, which may or may not be what people want.
kennytm added a commit to kennytm/rust that referenced this issue Nov 13, 2018
@kennytm
Rollup merge of rust-lang#55804 - QuietMisdreavus:eager-crate-inline,…
d811f84 
… r=pnkfelix

rustdoc: don't inline `pub use some_crate` unless directly asked to

cc rust-lang#52509 (fixes it? i'm not sure about my comment summoning the docs team)

When rustdoc encounters a `pub use` statement for an item from another crate, it will eagerly inline its contents into your crate. This somewhat clashes with the new paths behavior in Rust 2018, in which crates are implicitly linked and re-exported with `pub use` instead of `pub extern crate`. In rust 2015, `pub extern crate` would only create a single line for its re-export in the docs, so i'm making it do the same with `pub use some_crate;`.

The exact new behavior is like this: *If rustdoc sees a `pub use` statement, and the item being imported is the root of another crate, it will only inline it if `#[doc(inline)]` is provided.* I made it only avoid crate roots because otherwise it would stop inlining any module, which may or may not be what people want.
@nikomatsakis
Copy link
Contributor

@QuietMisdreavus would you call this a "blocker" for the final release? Do you think we should backport our fixes?

@QuietMisdreavus
Copy link
Member

@nikomatsakis The bug, as it stands today, is benign and has a small workaround, namely adding #[doc(no_inline)] to the re-export. I'm okay if #55804 is not backported, if we're okay with advertising that fix for one version.

The broader question of "do we want to adapt docs to look like a different edition from what it was compiled for" is something i wanted to get input from @rust-lang/docs on. If we leave this as-is, we default to "no", but i think we can leave the question open for future work, especially with the edition code freeze just next week.

@steveklabnik
Copy link
Member

Do we always want docs to look like the latest edition (which would require that we start changing how we render existing pub extern crate statements), or do we render based on the edition passed to rustdoc (which makes docs look different based on the crate being documented)? I'm leaning toward "modify docs to look like newest edition", but i'm going to punt to @rust-lang/docs for that one.

I feel like the right thing is "based on the edition passed to rustdoc", but I'd be fine with either, to be honest.

@Havvy
Copy link
Contributor

Havvy commented Nov 13, 2018

I said prior on Discord that it'd be nice to have a toggle on the page itself without giving it too much thought. A little more thought, and I realized that since we can cargo doc and get docs of all dependencies as well, we can just render based on the edition of the crate we are ultimately working on. Though how that works for online docs is a bit iffy. It'd be nice to see the stdlib docs in each edition and perhaps likewise with docs.rs?

@nagisa nagisa removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 15, 2018
@nagisa
Copy link
Member

nagisa commented Nov 15, 2018

Removing T-compiler, as it seems that this issue is not critical for Rust2018 and involvement of T-compiler to help getting this fixed ASAP is not necessary.

@alexcrichton
Copy link
Member

@QuietMisdreavus has mentioned on discord that #55804 has fixed this issue enough for the edition, so I'm going to remove it from the milestone and also remove the P-high tag

@alexcrichton alexcrichton removed F-rust_2018_preview `#![feature(rust_2018_preview)]` P-high High priority labels Nov 20, 2018
@alexcrichton alexcrichton removed this from the Rust 2018 Release milestone Nov 20, 2018
@pnkfelix pnkfelix removed their assignment Jan 29, 2019
@jyn514
Copy link
Member

jyn514 commented Apr 8, 2021

This is working on master:
image

@jyn514 jyn514 closed this as completed Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@GuillaumeGomez GuillaumeGomez

Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests