Fix dangling ID when pub useing item which is Doc(hidden) or inherits it in rustdoc JSON output

[GuillaumeGomez]

Fixes #117718.

I changed a bit where we actually filter out some reexports to be done directly in the JSON conversion. When the reexport is public but the reexported item is doc hidden, it generates the reexport but with a null ID.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

[GuillaumeGomez]

I rebased and updated the failing test (it was testing something changed in this PR).

[obi1kenobi]

I have a serious concern here, and a request to reconsider the interpretation of how doc(hidden) applies across module boundaries.

Let's look at this example from the doc comment:

#[doc(hidden)]
pub mod foo {
    pub struct Bar;
}

pub use foo::Bar;

To me, this seems like a public re-export of an intended-to-be-public item. In other words, when I see this code, I expect its doc(hidden) semantics to be that it is the module that is hidden, but non-hidden ways to access its contents are public API and "fair game." In my interpretation, if the programmer intended that to not be the case, they could have hidden the re-export (or pub struct Bar itself).

If pub struct Bar is always considered hidden regardless of re-exports (whether themselves hidden or not), I think that has strange implications for what is and is not public API. In that interpretation, the re-export is public API but the item it points to is not public API ... which is just confusing. It seems like it says "I make it part of the public API that this non-public-API type reserves a name in the public API" and nothing else. That's just strange! What public API operations are allowed over that re-export item? It feels to me like the answer is that the item is "neither here nor there" with respect to being public API or not.

It also seems to risk opening a terrible can of worms with further edge cases:

mod defn {
    pub struct Example;
}

#[doc(hidden)]
pub mod hide {
    pub use crate::Example as Example;
}

// Here, `Example` is public API.
pub use defn::Example;

// Replace it with the below code and `Example` is no longer public API.
// Worse, we can't distinguish between these two cases via rustdoc JSON,
// so this interpretation would permanently break cargo-semver-checks.
// pub use hide::Example;

For these reasons, I think a better interpretation of whether an item is doc(hidden) is one based on reachability: if you can reach the item without going through any doc(hidden) items (the item itself + any modules and re-exports), it is not hidden. If all paths require going through at least one hidden item, the item is hidden. This is also how visibility works as well — the pub-in-priv trick wouldn't work otherwise — so I think it makes a lot of sense for doc(hidden) to work that way too.

[GuillaumeGomez]

I followed the same existing logic as for public item in private module:

mod x {
    pub struct Y {}
}

/// a
pub use x::Y as Z;

So I agree with you, but then this would be a change to be done in another PR as it would change how we handle things completely.

[obi1kenobi]

Thanks! Just to double-check since it's important for cargo-semver-checks:

this would be a change to be done in another PR as it would change how we handle things completely.

Am I correct in understanding that you feel the "reachability" interpretation I proposed should be how doc(hidden) works, and the team plans to adjust rustdoc in line with that in future PRs?

I'm asking because I wanted to release a new cargo-semver-checks version next week with the ability to ignore doc(hidden) items and no longer report false-positives when such items change in major breaking ways. However, its implementation of doc(hidden) handling is reachability-based since (as I mentioned in my earlier post), it's the only one that could work within the current rustdoc JSON system and is able to reuse the existing visibility logic. So I wanted to double-check whether those semantics are "deemed correct" so as not to falsely promise users that the new cargo-semver-checks would correctly handle doc(hidden) items. Right now, cargo-semver-checks outputs both private and hidden items so it's not affected by this PR specifically — which is why I'm more worried about the intended semantics than about when those changes are implemented.

[GuillaumeGomez]

I plan to at least open a discussion about how reexported items from private/hidden modules should appear in the JSON output. I'm not too happy with the current situation as it differs between the HTML and JSON outputs. The HTML output inline the items, showing all their information, whereas the JSON output just shows a reexport and we can't get the information of the reexported item.

So I suggest this: please open an issue on this repository and start a discussion on zulip so we can all agree exactly on what we want and once we reach a consensus (between rustdoc team members and cargo-semver-checks), we'll implement this change. Does it sound good to you?

And again: this PR is really about fixing a current bug while not changing the current situation. So it'd be nice to have it merged ahead of time so we can at least have tests to update when we change JSON output.

[obi1kenobi]

Absolutely. I'll open a separate issue and Zulip discussion 👍

Thanks again!

[aDotInTheVoid]

NIT: The definition of inherits_doc_hidden already calls imported_item_is_doc_hidden. That should be de-duplicated, either here or there.

[aDotInTheVoid]

I don't think this is the right behavior. If the underlying item isn't part of the public API than I don't think the import of it should be either.

[GuillaumeGomez]

The import is publicly accessible so I don't see why not?

[aDotInTheVoid]

1. Ideologically: RepeatN has asked to not be part of the public API. We don't show it in the HTML docs.
2. Practicly: Theirs very little use to consumers telling them "You import something here, but you don't get to know what it it". If it's a part of the API, we should tell them the full story. If it isn't then we shouldn't acknowledge the import at all. What purpose does this weird in-between serve? (If theirs a compelling use case for just knowing this, that'd easily change my mind)

[aDotInTheVoid]

As discussed in https://rust-lang.zulipchat.com/#narrow/stream/266220-t-rustdoc/topic/pub.20re-export.20of.20item.20from.20.60doc.28hidden.29.60.20module/near/401627083, this import should be a part of the output, and so the ID should be present

[GuillaumeGomez]

I followed the same logic as for reexport of item inside private module. We discussed that after this PR, we want to inline such items and I think in the meantime, this is the right course of action.

[aDotInTheVoid]

I followed the same logic as for reexport of item inside private module.

When reexporting an item defined in a private module, the import will have the ID of the underlying item. We should do the same thing here.

We discussed that after this PR, we want to inline such items

Please no. Inlining was a massive source of bugs for rustdoc-json, and removing it was a great improvement. I'm strongly opposed to bringing it back.

[GuillaumeGomez]

Having an ID pointing to an item that is not available means that the current bug fixed by this PR will remain.

[bors]

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

[GuillaumeGomez]

I think this PR is not the right way to solve this issue so closing it.