Skip to content

Fix weird rustdoc output when single and glob reexport conflict on a name #143590

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 7, 2025

Fixes #143107.

The problem was that the second reexport would overwrite the first, leading to having unexpected results. To fix it, I now group items by their original DefId and their name and keep tracks of all imports for this item (should very rarely be more than one though, and even less often more than 2).

cc @lolbinarycat

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2025

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added 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. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jul 7, 2025
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Ah bad luck. A function I modified was used in a more up-to-date version. ^^'

Anyway, rebased and fixed the CI failure.

Copy link
Contributor

@lolbinarycat lolbinarycat left a comment

Choose a reason for hiding this comment

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

some things could use clarification. i don't entirely understand if things are getting merged or overwritten. the unit test comment is confusing: all glob attrs are ignored, or just those on shadowed items? this should be clarified.

the test could also include coverage for the case where an item is renamed to itself. also what happens if an item is renamed then renamed back to the original name in a chain?

@@ -152,8 +152,12 @@ pub(crate) fn try_inline(
};

cx.inlined.insert(did.into());
let import_def_ids: &[LocalDefId] = match import_def_id {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't Option::as_slice be used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, much better, thanks!

/// ```
///
/// So in this case, we don't want to have two items but just one with attributes from both
/// imports to be merged.
Copy link
Contributor

Choose a reason for hiding this comment

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

is merging the attributes correct? i thought that per the reference, only items not shadowed by non-glob imports would actually be shadowed. i guess that doesn't exactly say what happens when you shadow a glob import with an identical item...

not that i see putting docs on glob imports to be a common usecase, i think much more common is docs on the regular import and no docs on the glob import.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, the comment is wrong: we never merge attributes from glob reexports.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's also tested in the added test (that glob reexports attributes are ignored). ^^'

@GuillaumeGomez
Copy link
Member Author

some things could use clarification. i don't entirely understand if things are getting merged or overwritten. the unit test comment is confusing: all glob attrs are ignored, or just those on shadowed items? this should be clarified.

Attributes on glob reexports are always ignored. The comment I added was misleading so I fixed it.

the test could also include coverage for the case where an item is renamed to itself. also what happens if an item is renamed then renamed back to the original name in a chain?

What matters is the duo name+DefId. So if the name is different, then rustdoc considers it's a different item. And if the item as renamed as itself, then nothing happens either. But I'm gonna add a test for this second case just in case.

@GuillaumeGomez
Copy link
Member Author

Applied suggestions, added test and clarified comment.

@rust-log-analyzer

This comment has been minimized.

@lolbinarycat
Copy link
Contributor

What matters is the duo name+DefId.

non-renamed items have name field of None, and there's special logic to make this work with things renamed to themself, so I want there to be a test that makes sure any refactoring of this logic doesn't mess up this edge case.

@lolbinarycat
Copy link
Contributor

Looks like you rebased and responded to review in the the same force push, which was a bit confusing when I clicked on the "Compare" button and saw a bunch of unrelated changes.

Anyways, it looks mostly correct, although if nothing is getting merged in the unit test, maybe it shouldn't have merge in the name? Or maybe it should just also exercise the case where attributes are actually merge.. is there a case where attributes are actually merged?

Also, I think it would be nice to also test what shows on the module docs, since apparently that can get out of sync with the actual item's docs, as shown in the original issue.

@GuillaumeGomez
Copy link
Member Author

Anyways, it looks mostly correct, although if nothing is getting merged in the unit test, maybe it shouldn't have merge in the name? Or maybe it should just also exercise the case where attributes are actually merge.. is there a case where attributes are actually merged?

The two reexports are getting merged. They are merged in case of reexport chain:

mod foo {
    mod bar {
        /// C
        pub struct A;
    }
    /// B
    pub use self::bar::A;
}

/// A
pub use self::foo::A;

The A struct doc will have A, B and C.

Also, I think it would be nice to also test what shows on the module docs, since apparently that can get out of sync with the actual item's docs, as shown in the original issue.

Good idea.

@GuillaumeGomez
Copy link
Member Author

Extended test as suggested.

@lolbinarycat
Copy link
Contributor

Maybe I should clarify: I think the actual contents of the summary/description lines on the module docs need to be tested, since that's what got out of sync in the first hand, not the amount of items.

We don't want to somehow get an inversion of this bug, where the item docs are fine, but the module docs are getting messed up by the glob.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rustdoc] adding docs on a reexport that shadows a glob reexport does not work properly
5 participants