Rollup merge of #107766 - GuillaumeGomez:fix-json-reexports-of-different-items-with-same-name, r=aDotInTheVoid

Fix json reexports of different items with same name

Fixes  #107677.

I renamed `from_item_id*` functions into `id_from_item` instead because it makes more sense now. I also simplified the logic around it a bit so that the `ids` function will now directly pass `&clean::Item` to `id_from_item` and the ID will be consistently generated (it caused an issue when I updated the ID for imports).

So now, the big change of this PR: I changed how imports' ID is generated: it now includes the target item's ID at the end of the ID. It's to prevent two reexported items with the same name (but different types).

r? `@aDotInTheVoid`

Author: GuillaumeGomez

src/librustdoc/json/conversions.rs

@@ -38,7 +38,7 @@ impl JsonRenderer<'_> {
                     Some(UrlFragment::UserWritten(_)) | None => *page_id,
                 };
 
-                (link.clone(), from_item_id(id.into(), self.tcx))
+                (link.clone(), id_from_item_default(id.into(), self.tcx))
             })
             .collect();
         let docs = item.attrs.collapsed_doc_value();
@@ -50,7 +50,8 @@ impl JsonRenderer<'_> {
             .collect();
         let span = item.span(self.tcx);
         let visibility = item.visibility(self.tcx);
-        let clean::Item { name, attrs: _, kind: _, item_id, cfg: _, .. } = item;
+        let clean::Item { name, item_id, .. } = item;
+        let id = id_from_item(&item, self.tcx);
         let inner = match *item.kind {
             clean::KeywordItem => return None,
             clean::StrippedItem(ref inner) => {
@@ -69,7 +70,7 @@ impl JsonRenderer<'_> {
             _ => from_clean_item(item, self.tcx),
         };
         Some(Item {
-            id: from_item_id_with_name(item_id, self.tcx, name),
+            id,
             crate_id: item_id.krate().as_u32(),
             name: name.map(|sym| sym.to_string()),
             span: span.and_then(|span| self.convert_span(span)),
@@ -107,7 +108,7 @@ impl JsonRenderer<'_> {
             Some(ty::Visibility::Public) => Visibility::Public,
             Some(ty::Visibility::Restricted(did)) if did.is_crate_root() => Visibility::Crate,
             Some(ty::Visibility::Restricted(did)) => Visibility::Restricted {
-                parent: from_item_id(did.into(), self.tcx),
+                parent: id_from_item_default(did.into(), self.tcx),
                 path: self.tcx.def_path(did).to_string_no_crate_verbose(),
             },
         }
@@ -204,21 +205,42 @@ impl FromWithTcx<clean::TypeBindingKind> for TypeBindingKind {
     }
 }
 
-/// It generates an ID as follows:
-///
-/// `CRATE_ID:ITEM_ID[:NAME_ID]` (if there is no name, NAME_ID is not generated).
-pub(crate) fn from_item_id(item_id: ItemId, tcx: TyCtxt<'_>) -> Id {
-    from_item_id_with_name(item_id, tcx, None)
+#[inline]
+pub(crate) fn id_from_item_default(item_id: ItemId, tcx: TyCtxt<'_>) -> Id {
+    id_from_item_inner(item_id, tcx, None, None)
 }
 
-// FIXME: this function (and appending the name at the end of the ID) should be removed when
-// reexports are not inlined anymore for json format. It should be done in #93518.
-pub(crate) fn from_item_id_with_name(item_id: ItemId, tcx: TyCtxt<'_>, name: Option<Symbol>) -> Id {
-    struct DisplayDefId<'a>(DefId, TyCtxt<'a>, Option<Symbol>);
+/// It generates an ID as follows:
+///
+/// `CRATE_ID:ITEM_ID[:NAME_ID][-EXTRA]`:
+///   * If there is no `name`, `NAME_ID` is not generated.
+///   * If there is no `extra`, `EXTRA` is not generated.
+///
+/// * `name` is the item's name if available (it's not for impl blocks for example).
+/// * `extra` is used for reexports: it contains the ID of the reexported item. It is used to allow
+///   to have items with the same name but different types to both appear in the generated JSON.
+pub(crate) fn id_from_item_inner(
+    item_id: ItemId,
+    tcx: TyCtxt<'_>,
+    name: Option<Symbol>,
+    extra: Option<&Id>,
+) -> Id {
+    struct DisplayDefId<'a, 'b>(DefId, TyCtxt<'a>, Option<&'b Id>, Option<Symbol>);
 
-    impl<'a> fmt::Display for DisplayDefId<'a> {
+    impl<'a, 'b> fmt::Display for DisplayDefId<'a, 'b> {
         fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-            let DisplayDefId(def_id, tcx, name) = self;
+            let DisplayDefId(def_id, tcx, extra, name) = self;
+            // We need this workaround because primitive types' DefId actually refers to
+            // their parent module, which isn't present in the output JSON items. So
+            // instead, we directly get the primitive symbol and convert it to u32 to
+            // generate the ID.
+            let s;
+            let extra = if let Some(e) = extra {
+                s = format!("-{}", e.0);
+                &s
+            } else {
+                ""
+            };
             let name = match name {
                 Some(name) => format!(":{}", name.as_u32()),
                 None => {
@@ -240,18 +262,33 @@ pub(crate) fn from_item_id_with_name(item_id: ItemId, tcx: TyCtxt<'_>, name: Opt
                     }
                 }
             };
-            write!(f, "{}:{}{}", self.0.krate.as_u32(), u32::from(self.0.index), name)
+            write!(f, "{}:{}{name}{extra}", def_id.krate.as_u32(), u32::from(def_id.index))
         }
     }
 
     match item_id {
-        ItemId::DefId(did) => Id(format!("{}", DisplayDefId(did, tcx, name))),
-        ItemId::Blanket { for_, impl_id } => {
-            Id(format!("b:{}-{}", DisplayDefId(impl_id, tcx, None), DisplayDefId(for_, tcx, name)))
-        }
-        ItemId::Auto { for_, trait_ } => {
-            Id(format!("a:{}-{}", DisplayDefId(trait_, tcx, None), DisplayDefId(for_, tcx, name)))
+        ItemId::DefId(did) => Id(format!("{}", DisplayDefId(did, tcx, extra, name))),
+        ItemId::Blanket { for_, impl_id } => Id(format!(
+            "b:{}-{}",
+            DisplayDefId(impl_id, tcx, None, None),
+            DisplayDefId(for_, tcx, extra, name)
+        )),
+        ItemId::Auto { for_, trait_ } => Id(format!(
+            "a:{}-{}",
+            DisplayDefId(trait_, tcx, None, None),
+            DisplayDefId(for_, tcx, extra, name)
+        )),
+    }
+}
+
+pub(crate) fn id_from_item(item: &clean::Item, tcx: TyCtxt<'_>) -> Id {
+    match *item.kind {
+        clean::ItemKind::ImportItem(ref import) => {
+            let extra =
+                import.source.did.map(ItemId::from).map(|i| id_from_item_inner(i, tcx, None, None));
+            id_from_item_inner(item.item_id, tcx, item.name, extra.as_ref())
         }
+        _ => id_from_item_inner(item.item_id, tcx, item.name, None),
     }
 }
 
@@ -525,7 +562,7 @@ impl FromWithTcx<clean::Path> for Path {
     fn from_tcx(path: clean::Path, tcx: TyCtxt<'_>) -> Path {
         Path {
             name: path.whole_name(),
-            id: from_item_id(path.def_id().into(), tcx),
+            id: id_from_item_default(path.def_id().into(), tcx),
             args: path.segments.last().map(|args| Box::new(args.clone().args.into_tcx(tcx))),
         }
     }
@@ -702,7 +739,7 @@ impl FromWithTcx<clean::Import> for Import {
         Import {
             source: import.source.path.whole_name(),
             name,
-            id: import.source.did.map(ItemId::from).map(|i| from_item_id(i, tcx)),
+            id: import.source.did.map(ItemId::from).map(|i| id_from_item_default(i, tcx)),
             glob,
         }
     }
@@ -791,7 +828,7 @@ fn ids(items: impl IntoIterator<Item = clean::Item>, tcx: TyCtxt<'_>) -> Vec<Id>
     items
         .into_iter()
         .filter(|x| !x.is_stripped() && !x.is_keyword())
-        .map(|i| from_item_id_with_name(i.item_id, tcx, i.name))
+        .map(|i| id_from_item(&i, tcx))
         .collect()
 }
 
@@ -801,12 +838,10 @@ fn ids_keeping_stripped(
 ) -> Vec<Option<Id>> {
     items
         .into_iter()
-        .map(|i| {
-            if !i.is_stripped() && !i.is_keyword() {
-                Some(from_item_id_with_name(i.item_id, tcx, i.name))
-            } else {
-                None
-            }
-        })
+        .map(
+            |i| {
+                if !i.is_stripped() && !i.is_keyword() { Some(id_from_item(&i, tcx)) } else { None }
+            },
+        )
         .collect()
 }

src/librustdoc/json/mod.rs

@@ -28,7 +28,7 @@ use crate::docfs::PathError;
 use crate::error::Error;
 use crate::formats::cache::Cache;
 use crate::formats::FormatRenderer;
-use crate::json::conversions::{from_item_id, from_item_id_with_name, IntoWithTcx};
+use crate::json::conversions::{id_from_item, id_from_item_default, IntoWithTcx};
 use crate::{clean, try_err};
 
 #[derive(Clone)]
@@ -58,7 +58,7 @@ impl<'tcx> JsonRenderer<'tcx> {
                     .map(|i| {
                         let item = &i.impl_item;
                         self.item(item.clone()).unwrap();
-                        from_item_id_with_name(item.item_id, self.tcx, item.name)
+                        id_from_item(&item, self.tcx)
                     })
                     .collect()
             })
@@ -89,7 +89,7 @@ impl<'tcx> JsonRenderer<'tcx> {
 
                         if item.item_id.is_local() || is_primitive_impl {
                             self.item(item.clone()).unwrap();
-                            Some(from_item_id_with_name(item.item_id, self.tcx, item.name))
+                            Some(id_from_item(&item, self.tcx))
                         } else {
                             None
                         }
@@ -150,7 +150,6 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
         // Flatten items that recursively store other items
         item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap());
 
-        let name = item.name;
         let item_id = item.item_id;
         if let Some(mut new_item) = self.convert_item(item) {
             let can_be_ignored = match new_item.inner {
@@ -193,10 +192,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
                 | types::ItemEnum::Macro(_)
                 | types::ItemEnum::ProcMacro(_) => false,
             };
-            let removed = self
-                .index
-                .borrow_mut()
-                .insert(from_item_id_with_name(item_id, self.tcx, name), new_item.clone());
+            let removed = self.index.borrow_mut().insert(new_item.id.clone(), new_item.clone());
 
             // FIXME(adotinthevoid): Currently, the index is duplicated. This is a sanity check
             // to make sure the items are unique. The main place this happens is when an item, is
@@ -207,6 +203,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
                 if !can_be_ignored {
                     assert_eq!(old_item, new_item);
                 }
+                trace!("replaced {:?}\nwith {:?}", old_item, new_item);
             }
         }
 
@@ -246,7 +243,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
                 .chain(&self.cache.external_paths)
                 .map(|(&k, &(ref path, kind))| {
                     (
-                        from_item_id(k.into(), self.tcx),
+                        id_from_item_default(k.into(), self.tcx),
                         types::ItemSummary {
                             crate_id: k.krate.as_u32(),
                             path: path.iter().map(|s| s.to_string()).collect(),

tests/rustdoc-json/reexport/same_name_different_types.rs

@@ -0,0 +1,25 @@
+// Regression test for <https://github.com/rust-lang/rust/issues/107677>.
+
+#![feature(no_core)]
+#![no_core]
+
+pub mod nested {
+    // @set foo_struct = "$.index[*][?(@.docs == 'Foo the struct')].id"
+
+    /// Foo the struct
+    pub struct Foo {}
+
+    // @set foo_fn = "$.index[*][?(@.docs == 'Foo the function')].id"
+
+    #[allow(non_snake_case)]
+    /// Foo the function
+    pub fn Foo() {}
+}
+
+// @ismany "$.index[*][?(@.inner.name == 'Foo' && @.kind == 'import')].inner.id" $foo_fn $foo_struct
+// @ismany "$.index[*][?(@.inner.name == 'Bar' && @.kind == 'import')].inner.id" $foo_fn $foo_struct
+
+// @count "$.index[*][?(@.inner.name == 'Foo' && @.kind == 'import')]" 2
+pub use nested::Foo;
+// @count "$.index[*][?(@.inner.name == 'Bar' && @.kind == 'import')]" 2
+pub use Foo as Bar;