Rollup merge of rust-lang#113785 - GuillaumeGomez:tests/rustdoc/issue…

…-105735-fix, r=notriddle,aDotInTheVoid Fix invalid display of inlined re-export when both local and foreign items are inlined Fixes rust-lang#105735. The bug is actually quite interesting: at the `clean` pass, local inlined items have their `use` item removed, however foreign items don't have their `use` item removed because it's in the `clean` pass that we handle them. So when a `use` inlines both a local and a foreign item, it will work as expected for the foreign one, but not for the local as its `use` should not be around anymore. To prevent this, I created a new `inlined_foreigns` field into the `Module` struct to allow to remove the `use` item early on for foreign items as well. Then we iterate it in the `clean` pass directly. r? `@notriddle`
GuillaumeGomez · Jul 19, 2023 · 11a1a06 · 11a1a06
2 parents b7bd83a afec6d2
commit 11a1a06
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 17 deletions.
diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs
@@ -90,6 90,19 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext<
  }
  v
  }));
  items.extend(doc.inlined_foreigns.iter().flat_map(|((_, renamed), (res, local_import_id))| {
  let Some(def_id) = res.opt_def_id() else { return Vec::new() };
  let name = renamed.unwrap_or_else(|| cx.tcx.item_name(def_id));
  let import = cx.tcx.hir().expect_item(*local_import_id);
  match import.kind {
  hir::ItemKind::Use(path, kind) => {
  let hir::UsePath { segments, span, .. } = *path;
  let path = hir::Path { segments, res: *res, span };
  clean_use_statement_inner(import, name, &path, kind, cx, &mut Default::default())
  }
  _ => unreachable!(),
  }
  }));
  items.extend(doc.items.values().flat_map(|(item, renamed, _)| {
  // Now we actually lower the imports, skipping everything else.
  if let hir::ItemKind::Use(path, hir::UseKind::Glob) = item.kind {
@@ -2652,9 2665,6 @@ fn clean_use_statement<'tcx>(
  let mut items = Vec::new();
  let hir::UsePath { segments, ref res, span } = *path;
  for &res in res {
- if let Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) = res {
- continue;
- }
  let path = hir::Path { segments, res, span };
  items.append(&mut clean_use_statement_inner(import, name, &path, kind, cx, inlined_names));
  }
@@ -2669,6 2679,9 @@ fn clean_use_statement_inner<'tcx>(
  cx: &mut DocContext<'tcx>,
  inlined_names: &mut FxHashSet<(ItemType, Symbol)>,
 ) -> Vec<Item> {
  if let Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) = path.res {
  return Vec::new();
  }
  // We need this comparison because some imports (for std types for example)
  // are "inserted" as well but directly by the compiler and they should not be
  // taken into account.

diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs
@@ -35,6 35,8 @@ pub(crate) struct Module<'hir> {
  (LocalDefId, Option<Symbol>),
  (&'hir hir::Item<'hir>, Option<Symbol>, Option<LocalDefId>),
  >,
  /// Same as for `items`.
  pub(crate) inlined_foreigns: FxIndexMap<(DefId, Option<Symbol>), (Res, LocalDefId)>,
  pub(crate) foreigns: Vec<(&'hir hir::ForeignItem<'hir>, Option<Symbol>)>,
 }
 
@@ -54,6 56,7 @@ impl Module<'_> {
  import_id,
  mods: Vec::new(),
  items: FxIndexMap::default(),
  inlined_foreigns: FxIndexMap::default(),
  foreigns: Vec::new(),
  }
  }
@@ -272,21 275,30 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
  return false;
  }
 
- // For cross-crate impl inlining we need to know whether items are
- // reachable in documentation -- a previously unreachable item can be
- // made reachable by cross-crate inlining which we're checking here.
- // (this is done here because we need to know this upfront).
- if !ori_res_did.is_local() && !is_no_inline {
- crate::visit_lib::lib_embargo_visit_item(self.cx, ori_res_did);
- return false;
- }
-
  let is_hidden = !document_hidden && tcx.is_doc_hidden(ori_res_did);
  let Some(res_did) = ori_res_did.as_local() else {
- return false;
  // For cross-crate impl inlining we need to know whether items are
  // reachable in documentation -- a previously unreachable item can be
  // made reachable by cross-crate inlining which we're checking here.
  // (this is done here because we need to know this upfront).
  crate::visit_lib::lib_embargo_visit_item(self.cx, ori_res_did);
  if is_hidden {
  return false;
  }
  // We store inlined foreign items otherwise, it'd mean that the `use` item would be kept
  // around. It's not a problem unless this `use` imports both a local AND a foreign item.
  // If a local item is inlined, its `use` is not supposed to still be around in `clean`,
  // which would make appear the `use` in the generated documentation like the local item
  // was not inlined even though it actually was.
  self.modules
  .last_mut()
  .unwrap()
  .inlined_foreigns
  .insert((ori_res_did, renamed), (res, def_id));
  return true;
  };
 
  let is_private = !self.cx.cache.effective_visibilities.is_directly_public(tcx, ori_res_did);
- let is_hidden = !document_hidden && tcx.is_doc_hidden(ori_res_did);
  let item = tcx.hir().get_by_def_id(res_did);
 
  if !please_inline {
@@ -314,7 326,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
  return false;
  }
 
- let inlined = match tcx.hir().get_by_def_id(res_did) {
  let inlined = match item {
  // Bang macros are handled a bit on their because of how they are handled by the
  // compiler. If they have `#[doc(hidden)]` and the re-export doesn't have
  // `#[doc(inline)]`, then we don't inline it.
@@ -346,7 358,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
  };
  self.view_item_stack.remove(&res_did);
  if inlined {
- self.cx.cache.inlined_items.insert(res_did.to_def_id());
  self.cx.cache.inlined_items.insert(ori_res_did);
  }
  inlined
  }
@@ -483,7 495,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
  continue;
  }
  }
-
  self.add_to_current_mod(item, renamed, import_id);
  }
  }

diff --git a/tests/rustdoc/issue-105735-overlapping-reexport-2.rs b/tests/rustdoc/issue-105735-overlapping-reexport-2.rs
@@ -0,0 1,25 @@
 // Regression test to ensure that both `AtomicU8` items are displayed but not the re-export.
 
 #![crate_name = "foo"]
 #![no_std]
 
 // @has 'foo/index.html'
 // @has - '//*[@class="item-name"]/a[@class="type"]' 'AtomicU8'
 // @has - '//*[@class="item-name"]/a[@class="constant"]' 'AtomicU8'
 // We also ensure we don't have another item displayed.
 // @count - '//*[@id="main-content"]/*[@class="small-section-header"]' 2
 // @has - '//*[@id="main-content"]/*[@class="small-section-header"]' 'Type Definitions'
 // @has - '//*[@id="main-content"]/*[@class="small-section-header"]' 'Constants'
 
 mod other {
  pub type AtomicU8 = ();
 }
 
 mod thing {
  pub use crate::other::AtomicU8;
 
  #[allow(non_upper_case_globals)]
  pub const AtomicU8: () = ();
 }
 
 pub use crate::thing::AtomicU8;
diff --git a/tests/rustdoc/issue-105735-overlapping-reexport.rs b/tests/rustdoc/issue-105735-overlapping-reexport.rs
@@ -0,0 1,21 @@
 // Regression test to ensure that both `AtomicU8` items are displayed but not the re-export.
 
 #![crate_name = "foo"]
 #![no_std]
 
 // @has 'foo/index.html'
 // @has - '//*[@class="item-name"]/a[@class="struct"]' 'AtomicU8'
 // @has - '//*[@class="item-name"]/a[@class="constant"]' 'AtomicU8'
 // We also ensure we don't have another item displayed.
 // @count - '//*[@id="main-content"]/*[@class="small-section-header"]' 2
 // @has - '//*[@id="main-content"]/*[@class="small-section-header"]' 'Structs'
 // @has - '//*[@id="main-content"]/*[@class="small-section-header"]' 'Constants'
 
 mod thing {
  pub use core::sync::atomic::AtomicU8;
 
  #[allow(non_upper_case_globals)]
  pub const AtomicU8: () = ();
 }
 
 pub use crate::thing::AtomicU8;