Unify behaviours for re-exports of #[doc(hidden)]

[GuillaumeGomez]

Fixes #109449.
Fixes #53417.

I also took the comments from zulip about #[doc(no_inline)] and glob re-exports more globally.

So now, for non-glob re-exports, #[doc(hidden)] items are treated the same as private items: they are inlined.

For glob re-exports, #[doc(hidden)] items are not inlined (contrary to private items).

If there #[doc(no_inline)] on a glob re-export, the glob re-export definition will displayed and the referenced items won't be inlined.

cc @petrochenkov
r? @notriddle

[GuillaumeGomez]

Fixed tidy error.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[GuillaumeGomez]

Two traits that are exported and didn't appear into the documentation before just did with this change. They were referencing to another item which doesn't appear into documentation so I added #[doc(hidden)] on both re-exports so they don't appear in documentation like it's currently the case.

[notriddle]

1. This should go through FCP.
2. This should include updates to the rustdoc book, documenting what the behavior here is.

[GuillaumeGomez]

I'll merge this PR with #109456 then.

[GuillaumeGomez]

Added the documentation so let's start the FCP.

@rfcbot fcp merge

[rfcbot]

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

• @GuillaumeGomez
• @Manishearth
• @Nemo157
• @aDotInTheVoid
• @camelid
• @jsha
• @notriddle

Concerns:

• backcompat (Unify behaviours for re-exports of #[doc(hidden)] #109697 (comment))
• document-globs resolved by Unify behaviours for re-exports of #[doc(hidden)] #109697 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[GuillaumeGomez]

Fixed tidy error in the rustdoc book.

[notriddle]

@rfcbot concern document-globs

For glob re-exports, #[doc(hidden)] items are not inlined (contrary to private items).

I don't see this mentioned in the book (which claims that #[doc(hidden)] and privacy act the same in rustdoc).

Assuming I understand this correctly, this means glob imports and explicit imports act differently in the face of doc(hidden), as described in this sample:

pub mod outer {
  mod inner {
    #[doc(hidden)] struct FooBar;
    struct FooBaz;
  }
  /// FooBar will be inlined here, because it's explicitly named.
  pub use inner::FooBar;
  /// FooBaz, of course, will also be inlined here.
  pub use inner::FooBaz;
}
/// FooBar *WON'T* be inlined here, because it's #[doc(hidden)]
/// However, FooBaz will be inlined here, because it's just private.
pub use outer::inner::*;

I get the reasoning behind it, but it's really weird, and needs to be documented.

[bors]

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

[notriddle]

@rfcbot resolve document-globs

[bors]

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

[GuillaumeGomez]

Fixed merge conflict.

[philipc]

The situation in gimli that gives multiple items with the same name is this:

mod m {
    pub struct Foo;
    pub type Bar = Foo;
    #[doc(hidden)]
    pub const Bar: Foo = Foo;
}

pub use m::Bar;

Just one thing though: use statement can have documentation actually. :)

It doesn't in my tests. This is what we would ideally have in gimli:

/// Little endian byte order.
pub struct LittleEndian;

/// The native endianity for the target platform.
#[cfg(target_endian = "little")]
pub use LittleEndian as NativeEndian;

but when I test that, the documentation on the use statement is ignored.

[GuillaumeGomez]

Ah right, I forgot about const/type alias case.

As for the documentation for re-exports, it's only used when the re-exported item is inlined (whether by using #[doc(inline)] or if the item is private or has #[doc(hidden)]).

[obi1kenobi]

Maybe re-exporting items with the same name in the same module should be forbidden by the compiler directly?

This is also a concern from other angles, like semver. I don't think it can be forbidden directly due to backcompat and other reasons, but I've proposed that rustc lint it at the very least:
#107563
#111336

Having the same names multiple times in the same namespace can have surprising downstream impacts, like causing adding an import a to be major breaking change. I wrote up some of these cases on my blog: https://predr.ag/blog/breaking-semver-in-rust-by-adding-private-type-or-import/

[GuillaumeGomez]

A new edition is coming up next year so we could turn it into an error.

[Manishearth]

If we want to edition it we need to add a way of explicitly saying "no, I meant to reexport it publicly here" since the migration path needs to be designed. Just lint allows do not cut it for edition migrations.

I recommend designing that mechanism, making it a lint, and having an edition migration for it that makes it hidden by default. We can then make it a hard error in the next edition, and eventually remove the error and switch the behavior if we want.

[notriddle]

Here's a few other things that seems kinda weird, that probably shouldn't work this way:

#![crate_name = "foo"]

mod alpha {
    #[doc(hidden)]
    pub struct Alpha;
}

mod beta {
    pub use crate::alpha::*;
}

// This is what it currently does.
// Is it what we want?
// @has foo/struct.Alpha.html
pub use beta::Alpha;

This seems like a bug. The reason is that it implies that this should happen, which is definitely not what diesel wants:

#![crate_name = "foo"]

mod alpha {
    pub struct Alpha;
    pub type Beta = Alpha;
    #[doc(hidden)]
    #[allow(nonstandard_style)]
    pub const Beta: Beta = Alpha;
}

mod beta {
    pub use crate::alpha::*;
}

// This is definitely correct.
// @has foo/struct.Alpha.html
// This is also correct.
// @has foo/type.Beta.html
// This probably isn't what we want. But it's the same thing as the first example!
// @has foo/constant.Beta.html
pub use beta::{Alpha, Beta};

[GuillaumeGomez]

It seems like more discussions are required. I think we could go by this simple rule: if any item has #[doc(hidden)], it shouldn't be visible without the --document-hidden-items option. That leads to this question:

mod foo {
    /// a
    pub struct Bar;
}

mod bar {
    /// b
    #[doc(hidden)]
    pub use crate::foo::Bar;
}

pub use crate::bar::Bar;

Since the first re-export is doc hidden, should only this re-export be hidden (and therefore its associated documentation "b" too) or should the re-export and the re-exported item be both hidden? I'd tend to only remove the re-export and its documentation and still make Bar inlined at the top as long as Bar itself isn't doc hidden.

[obi1kenobi]

We have to be careful with inlining Bar at the top, since re-exports do not always behave exactly as if inlined. For example:

mod foo {
    /// a
    pub struct Bar;
}

mod bar {
    /// b
    #[doc(hidden)]
    pub use crate::foo::Bar;
}

// shadow the re-export below
// but only in the values namespace
const Bar: usize = 0;

pub use crate::bar::Bar;

The rustdoc generated for your example code and the one from mine must be different, or else we'll have the same problem as #111338 and https://predr.ag/blog/breaking-semver-in-rust-by-adding-private-type-or-import/

Treating the top-level pub use crate::bar::Bar; as if it said pub use crate::foo::Bar; is also dangerous: just move const Bar inside mod bar and again we have a shadowing problem where the "as if" case isn't equivalent.

I hate jumping in to point out concerns without having a constructive suggestion on how to do better, but I genuinely don't see a good way out of this right now.

[GuillaumeGomez]

JSON output handles things differently compared to the HTML output. And in the end, pub use crate::bar::Bar; is the same as pub use crate::foo::Bar; for the compiler. The whole point here is to have a defined way of handling re-exports of #[doc(hidden)] elements. So having users' input is very welcome.

[notriddle]

It seems like more discussions are required. I think we could go by this simple rule: if any item has #[doc(hidden)], it shouldn't be visible without the --document-hidden-items option.

Yeah. Making doc(hidden) act like private, as elegant as it seemed, doesn't seem to be what people want. And it's too much of a back-compat hazard.

I propose closing this PR and opening a new one to document the new decision (and fix any weird inconsistencies): doc(hidden) will be inherited when an item is re-exported, whether through named use or glob use.

[GuillaumeGomez]

👍 Closing then and thanks everyone for the feedback!

[obi1kenobi]

in the end, pub use crate::bar::Bar; is the same as pub use crate::foo::Bar; for the compiler.

I'm not familiar with the HTML output, and I just realized my earlier code example is slightly broken. Here's an attempt at clarifying what I meant with respect to the JSON output (checked in the playground this time).

In the code below, we cannot replace pub use crate::bar::Bar; with pub use crate::foo:Bar; to avoid exposing a #[doc(hidden)] intermediate re-export. Doing that replacement would change the semantics of this code, which in turn might prevent cargo-semver-checks from catching a breaking change due to the glob.

Playground

mod upstream {
    mod foo {
        /// a
        pub struct Bar;
    }
    
    mod bar {
        /// b
        #[doc(hidden)]
        pub use super::foo::*;
        
        // shadow the re-export below
        // but only in the values namespace
        const Bar: usize = 0;
    }
    
    // This line causes compilation to fail:
    // pub use bar::Bar;
    //
    // But replacing it with this seemingly-equivalent line
    // makes the problem go away -- so it isn't actually equivalent:
    pub use foo::Bar;
}

// imagine this is a downstream crate
mod downstream {
    fn make_bar() -> crate::upstream::Bar {
        crate::upstream::Bar
    }
}

[GuillaumeGomez]

I should have precised but this behaviour enforcement/clarification is only for the HTML output. I'll precise it when I'll open the new PR so it's more clear. The JSON output is handling re-exports differently since its format allows it.

[obi1kenobi]

Ah yes that makes sense now!