Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Correctly merge import's and its target's docs in one more case #109266

Merged
merged 1 commit into from
Mar 20, 2023
Merged

rustdoc: Correctly merge import's and its target's docs in one more case #109266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/librustdoc/clean/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use std::hash::Hash;
use std::mem;
use thin_vec::ThinVec;

use crate::clean::inline::merge_attrs;
use crate::core::{self, DocContext, ImplTraitParam};
use crate::formats::item_type::ItemType;
use crate::visit_ast::Module as DocModule;
Expand Down Expand Up @@ -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();
Copy link
Member

@GuillaumeGomez GuillaumeGomez Mar 17, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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_ids and their parents, so there's still a lot to fix here besides the naming.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

let mut target_attrs = Vec::new();
if let Some(import_id) = import_id &&
let Some(hir::Node::Item(use_node)) = cx.tcx.hir().find_by_def_id(import_id)
{
let is_inline = inline::load_attrs(cx, import_id.to_def_id()).lists(sym::doc).get_word_attr(sym::inline).is_some();
// Then we get all the various imports' attributes.
get_all_import_attributes(use_node, cx.tcx, item.owner_id.def_id, &mut extra_attrs, is_inline);
add_without_unwanted_attributes(&mut extra_attrs, inline::load_attrs(cx, def_id), is_inline);
get_all_import_attributes(use_node, cx.tcx, item.owner_id.def_id, &mut import_attrs, is_inline);
add_without_unwanted_attributes(&mut target_attrs, inline::load_attrs(cx, def_id), is_inline);
} else {
// We only keep the item's attributes.
extra_attrs.extend_from_slice(inline::load_attrs(cx, def_id));
target_attrs.extend_from_slice(inline::load_attrs(cx, def_id));
}

let attrs = Attributes::from_ast(&extra_attrs);
let cfg = extra_attrs.cfg(cx.tcx, &cx.cache.hidden_cfg);
let import_parent = import_id.map(|import_id| cx.tcx.local_parent(import_id).to_def_id());
let (attrs, cfg) = merge_attrs(cx, import_parent, &target_attrs, Some(&import_attrs));

let mut item =
Item::from_def_id_and_attrs_and_parts(def_id, Some(name), kind, Box::new(attrs), cfg);
Expand Down
16 changes: 16 additions & 0 deletions tests/rustdoc-ui/intra-doc/import-inline-merge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Import for `A` is inlined and doc comments on the import and `A` itself are merged.
// After the merge they still have correct parent scopes to resolve both `[A]` and `[B]`.

// check-pass

#![allow(rustdoc::private_intra_doc_links)]

mod m {
/// [B]
pub struct A {}

pub struct B {}
}

/// [A]
pub use m::A;