Rollup merge of #108349 - GuillaumeGomez:fix-duplicated-imports2, r=notriddle

rustdoc: Prevent duplicated imports

Fixes #108163.

Interestingly enough, the AST is providing us an import for each corresponding item, even though the `Res` links to multiple ones each time, which leaded to the same import being duplicated.

So in this PR, I decided to prevent the add of the import before the clean pass. However, I originally took a different path by instead filtering after cleaning the path. You can see it [here](https://github.com/rust-lang/rust/compare/master...GuillaumeGomez:rust:fix-duplicated-imports?expand=1). Only the second commit differs.

I think this approach is better though, but at least we can compare both if we want.

The first commit adds the check for duplicated items in the rustdoc-json output as asked in #108163.

cc `@aDotInTheVoid`
r? `@notriddle`

src/librustdoc/clean/mod.rs

@@ -77,7 +77,7 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext<
     // This covers the case where somebody does an import which should pull in an item,
     // but there's already an item with the same namespace and same name. Rust gives
     // priority to the not-imported one, so we should, too.
-    items.extend(doc.items.iter().flat_map(|(item, renamed, import_id)| {
+    items.extend(doc.items.values().flat_map(|(item, renamed, import_id)| {
         // First, lower everything other than imports.
         if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
             return Vec::new();
@@ -90,7 +90,7 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext<
         }
         v
     }));
-    items.extend(doc.items.iter().flat_map(|(item, renamed, _)| {
+    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 {
             let name = renamed.unwrap_or_else(|| cx.tcx.hir().name(item.hir_id()));

src/librustdoc/visit_ast.rs

@@ -1,7 +1,7 @@
 //! The Rust AST Visitor. Extracts useful information and massages it into a form
 //! usable for `clean`.
 
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId, LocalDefIdSet};
@@ -26,8 +26,12 @@ pub(crate) struct Module<'hir> {
     pub(crate) where_inner: Span,
     pub(crate) mods: Vec<Module<'hir>>,
     pub(crate) def_id: LocalDefId,
-    // (item, renamed, import_id)
-    pub(crate) items: Vec<(&'hir hir::Item<'hir>, Option<Symbol>, Option<LocalDefId>)>,
+    /// The key is the item `ItemId` and the value is: (item, renamed, import_id).
+    /// We use `FxIndexMap` to keep the insert order.
+    pub(crate) items: FxIndexMap<
+        (LocalDefId, Option<Symbol>),
+        (&'hir hir::Item<'hir>, Option<Symbol>, Option<LocalDefId>),
+    >,
     pub(crate) foreigns: Vec<(&'hir hir::ForeignItem<'hir>, Option<Symbol>)>,
 }
 
@@ -38,7 +42,7 @@ impl Module<'_> {
             def_id,
             where_inner,
             mods: Vec::new(),
-            items: Vec::new(),
+            items: FxIndexMap::default(),
             foreigns: Vec::new(),
         }
     }
@@ -136,7 +140,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 inserted.insert(def_id)
             {
                     let item = self.cx.tcx.hir().expect_item(local_def_id);
-                    top_level_module.items.push((item, None, None));
+                    top_level_module.items.insert((local_def_id, Some(item.ident.name)), (item, None, None));
             }
         }
 
@@ -294,7 +298,11 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         renamed: Option<Symbol>,
         parent_id: Option<LocalDefId>,
     ) {
-        self.modules.last_mut().unwrap().items.push((item, renamed, parent_id))
+        self.modules
+            .last_mut()
+            .unwrap()
+            .items
+            .insert((item.owner_id.def_id, renamed), (item, renamed, parent_id));
     }
 
     fn visit_item_inner(

src/tools/jsondoclint/src/validator.rs

@@ -71,6 +71,19 @@ impl<'a> Validator<'a> {
         }
     }
 
+    fn check_items(&mut self, id: &Id, items: &[Id]) {
+        let mut visited_ids = HashSet::with_capacity(items.len());
+
+        for item in items {
+            if !visited_ids.insert(item) {
+                self.fail(
+                    id,
+                    ErrorKind::Custom(format!("Duplicated entry in `items` field: `{item:?}`")),
+                );
+            }
+        }
+    }
+
     fn check_item(&mut self, id: &'a Id) {
         if let Some(item) = &self.krate.index.get(id) {
             item.links.values().for_each(|id| self.add_any_id(id));
@@ -83,9 +96,9 @@ impl<'a> Validator<'a> {
                 ItemEnum::Enum(x) => self.check_enum(x),
                 ItemEnum::Variant(x) => self.check_variant(x, id),
                 ItemEnum::Function(x) => self.check_function(x),
-                ItemEnum::Trait(x) => self.check_trait(x),
+                ItemEnum::Trait(x) => self.check_trait(x, id),
                 ItemEnum::TraitAlias(x) => self.check_trait_alias(x),
-                ItemEnum::Impl(x) => self.check_impl(x),
+                ItemEnum::Impl(x) => self.check_impl(x, id),
                 ItemEnum::Typedef(x) => self.check_typedef(x),
                 ItemEnum::OpaqueTy(x) => self.check_opaque_ty(x),
                 ItemEnum::Constant(x) => self.check_constant(x),
@@ -94,7 +107,7 @@ impl<'a> Validator<'a> {
                 ItemEnum::Macro(x) => self.check_macro(x),
                 ItemEnum::ProcMacro(x) => self.check_proc_macro(x),
                 ItemEnum::Primitive(x) => self.check_primitive_type(x),
-                ItemEnum::Module(x) => self.check_module(x),
+                ItemEnum::Module(x) => self.check_module(x, id),
                 // FIXME: Why don't these have their own structs?
                 ItemEnum::ExternCrate { .. } => {}
                 ItemEnum::AssocConst { type_, default: _ } => self.check_type(type_),
@@ -112,7 +125,8 @@ impl<'a> Validator<'a> {
     }
 
     // Core checkers
-    fn check_module(&mut self, module: &'a Module) {
+    fn check_module(&mut self, module: &'a Module, id: &Id) {
+        self.check_items(id, &module.items);
         module.items.iter().for_each(|i| self.add_mod_item_id(i));
     }
 
@@ -181,7 +195,8 @@ impl<'a> Validator<'a> {
         self.check_fn_decl(&x.decl);
     }
 
-    fn check_trait(&mut self, x: &'a Trait) {
+    fn check_trait(&mut self, x: &'a Trait, id: &Id) {
+        self.check_items(id, &x.items);
         self.check_generics(&x.generics);
         x.items.iter().for_each(|i| self.add_trait_item_id(i));
         x.bounds.iter().for_each(|i| self.check_generic_bound(i));
@@ -193,7 +208,8 @@ impl<'a> Validator<'a> {
         x.params.iter().for_each(|i| self.check_generic_bound(i));
     }
 
-    fn check_impl(&mut self, x: &'a Impl) {
+    fn check_impl(&mut self, x: &'a Impl, id: &Id) {
+        self.check_items(id, &x.items);
         self.check_generics(&x.generics);
         if let Some(path) = &x.trait_ {
             self.check_path(path, PathKind::Trait);

tests/rustdoc/reexports-of-same-name.rs

@@ -0,0 +1,26 @@
+// This test ensures that there are 4 imports as expected:
+// * 2 for `Foo`
+// * 2 for `Bar`
+
+#![crate_name = "foo"]
+
+// @has 'foo/index.html'
+
+pub mod nested {
+    /// Foo the struct
+    pub struct Foo {}
+
+    #[allow(non_snake_case)]
+    /// Foo the function
+    pub fn Foo() {}
+}
+
+// @count - '//*[@id="main-content"]//code' 'pub use nested::Foo;' 2
+// @has - '//*[@id="reexport.Foo"]//a[@href="nested/struct.Foo.html"]' 'Foo'
+// @has - '//*[@id="reexport.Foo-1"]//a[@href="nested/fn.Foo.html"]' 'Foo'
+pub use nested::Foo;
+
+// @count - '//*[@id="main-content"]//code' 'pub use Foo as Bar;' 2
+// @has - '//*[@id="reexport.Bar"]//a[@href="nested/struct.Foo.html"]' 'Foo'
+// @has - '//*[@id="reexport.Bar-1"]//a[@href="nested/fn.Foo.html"]' 'Foo'
+pub use Foo as Bar;