-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
base: master
Are you sure you want to change the base?
Fix weird rustdoc output when single and glob reexport conflict on a name #143590
Conversation
r? @notriddle rustbot has assigned @notriddle. Use |
This comment has been minimized.
This comment has been minimized.
b3f629e
to
1b29e03
Compare
Ah bad luck. A function I modified was used in a more up-to-date version. ^^' Anyway, rebased and fixed the CI failure. |
There was a problem hiding this 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?
Attributes on glob reexports are always ignored. The comment I added was misleading so I fixed it.
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. |
1b29e03
to
380ccc6
Compare
Applied suggestions, added test and clarified comment. |
This comment has been minimized.
This comment has been minimized.
380ccc6
to
91f38d5
Compare
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. |
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 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. |
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
Good idea. |
91f38d5
to
80170ae
Compare
Extended test as suggested. |
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. |
80170ae
to
086b13d
Compare
Ah I see. Added a check for short docs on module page. |
@bors r=lolbinary rollup |
… r=lolbinary Fix weird rustdoc output when single and glob reexport conflict on a name Fixes rust-lang#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`
Rollup of 9 pull requests Successful merges: - #143446 (use `--dynamic-list` for exporting executable symbols) - #143590 (Fix weird rustdoc output when single and glob reexport conflict on a name) - #143599 (emit `.att_syntax` when global/naked asm use that option) - #143615 (Fix handling of no_std targets in `doc::Std` step) - #143632 (fix: correct parameter names in LLVMRustBuildMinNum and LLVMRustBuildMaxNum FFI declarations) - #143640 (Constify `Fn*` traits) - #143651 (Win: Use exceptions with empty data for SEH panic exception copies instead of a new panic) - #143660 (Disable docs for `compiler-builtins` and `sysroot`) - #143665 ([rustdoc-json] Add tests for `#[doc(hidden)]` handling of items.) r? `@ghost` `@rustbot` modify labels: rollup
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