-
Notifications
You must be signed in to change notification settings - Fork 13k
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
rustdoc: Correctly merge import's and its target's docs in one more case #109266
Conversation
r? @notriddle (rustbot has picked a reviewer for you, use r? to override) |
@@ -2373,21 +2374,22 @@ fn clean_maybe_renamed_item<'tcx>( | |||
_ => unreachable!("not yet converted"), | |||
}; | |||
|
|||
let mut extra_attrs = Vec::new(); | |||
let mut import_attrs = Vec::new(); |
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.
I'd nitpick to rename it into imports_attrs
but feel free to ignore it.
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.
If there are multiple imports there, then there should be multiple import_id
s and their parents, so there's still a lot to fix here besides the naming.
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.
Just to be sure we're talking about this same thing:
mod foo {
struct A;
pub use self::A as B;
pub use self::B as C;
}
pub use foo::C as D;
Intermediate imports are B
and C
and they are the ones we're gathering attributes from.
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.
Yes, and their attributes should not be all merged together, but should instead be broken into groups with a separate group with a separate parent id for every use
item in the reexport chain.
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.
Do you have cases where it would be problematic by any chance?
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.
Same as the test in this PR, but with a doc link on an intermediate import.
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.
Ok, I'm gonna send a fix for that too in the next days. Thanks for fixing this bug already.
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.
I'm not sure how many places in rustdoc merge attributes from different items, but all of them should ideally apply the same fix - a separate parent per item.
Apart from my nitpick, looks all good to me, thanks! r=me if you don't want to fix the nitpicking. |
@bors r+ |
rustdoc: Correctly merge import's and its target's docs in one more case Fixes rust-lang#108334. Fixes rust-lang#108378. Fixes rust-lang#108658.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#109170 (Set `CMAKE_SYSTEM_NAME` for Linux targets) - rust-lang#109266 (rustdoc: Correctly merge import's and its target's docs in one more case) - rust-lang#109267 (Add tests for configure.py) - rust-lang#109273 (Make `slice::is_sorted_by` implementation nicer) - rust-lang#109277 (Fix generics_of for impl's RPITIT synthesized associated type) - rust-lang#109307 (Ignore `Inlined` spans when computing caller location.) - rust-lang#109364 (Only expect a GAT const param for `type_of` of GAT const arg) - rust-lang#109365 (Update mdbook) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…rt-intra-doc-ice, r=petrochenkov rustdoc: Fix ICE for intra-doc link on intermediate re-export Fixes rust-lang#109282. This PR is based on rust-lang#109266 as it includes its commit to make this work. `@petrochenkov:` It was exactly as you predicted, adding the `DefId` to the attributes fixed the error for intermediate re-exports as well. Thanks a lot! r? `@petrochenkov`
Adding context to the beta nomination -- @kpreid asked for this to be nominated because they hit the problem on beta (or so it seems, and linked to these logs: https://github.com/kpreid/exhaust/actions/runs/4653637162/jobs/8234695803?pr=15). @petrochenkov, do you know when the regression fixed by this PR was originally introduced into the compiler? That's probably useful information in helping pinpoint whether this should be backported or if there's another fix that needs to be backported to fix @kpreid's problem instead. |
In #94857. |
I am approving the beta backport by looking at the number of issues it fixes. The beta cut is tomorrow IIRC, though (so not a lot of time). @rustbot label +beta-accepted |
…troalbini [stable] Prepare Rust 1.69.0 Last minute backports: * rust-lang#109643 * rust-lang#110135 * rust-lang#109938 * rust-lang#109937 * rust-lang#109266 This PR also bumps the channel to stable, and backports the release notes from master. r? `@ghost` cc `@rust-lang/release`
Workaround for rust-lang/rust#109266 This pins the docs build to MSRV, which should prevent regressions where new releases break the docs build.
Workaround for rust-lang/rust#109266 This pins the docs build to MSRV, which should prevent regressions where new releases break the docs build.
Fixes #108061.
Fixes #108334.
Fixes #108378.
Fixes #108658.