Skip to content

Conversation

lolbinarycat
Copy link
Contributor

@lolbinarycat lolbinarycat commented Aug 26, 2025

fixes #138251

cc @notriddle

current known issues:

  1. currently, the code does ensure that in the case of duplicate results, it is the trait item that is shown (eg. currently for -> Option, it shows std::char::Lowercase::last as its representative of Iterator::last)

@rustbot rustbot added A-rustdoc-search Area: Rustdoc's search feature S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Aug 26, 2025
@rust-log-analyzer

This comment has been minimized.

@lolbinarycat lolbinarycat force-pushed the rustdoc-search-trait-parent branch from 738dcc9 to 8444219 Compare August 26, 2025 19:35
@lolbinarycat
Copy link
Contributor Author

sometimes traitParent is not set properly for cross-crate implementations (eg. alloc::str::Bytes::last is not getting linked to Iterator::last).

this has been fixed (and removed from the PR description), i just needed to also be checking Cache::external_paths when resolving trait_parent_idx.

@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[TIMING:end] tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu } -- 0.000
fmt check
Diff in /checkout/src/librustdoc/formats/cache.rs:150:
 
     fn parent_stack_last_impl_and_trait_id(&self) -> (Option<DefId>, Option<DefId>) {
         if let Some(ParentStackItem::Impl { item_id, trait_, .. }) = self.parent_stack.last() {
-            
             (item_id.as_def_id(), trait_.as_ref().map(|tr| tr.def_id()))
         } else {
             (None, None)
Diff in /checkout/src/librustdoc/formats/cache.rs:582:
         clean::ItemKind::ImportItem(import) => import.source.did.unwrap_or(item_def_id),
         _ => item_def_id,
     };
-    let (impl_id, trait_parent) =
-        cache.parent_stack_last_impl_and_trait_id();
+    let (impl_id, trait_parent) = cache.parent_stack_last_impl_and_trait_id();
     let search_type = get_function_type_for_search(
         item,
         tcx,
Diff in /checkout/src/librustdoc/formats/cache.rs:619:
 fn handle_orphan_impl_child(cache: &mut Cache, item: &clean::Item, parent_did: DefId) {
     let impl_generics = clean_impl_generics(cache.parent_stack.last());
     let (impl_id, trait_parent) = cache.parent_stack_last_impl_and_trait_id();
-    let orphan_item =
-        OrphanImplItem { parent: parent_did, trait_parent, item: item.clone(), impl_generics, impl_id };
+    let orphan_item = OrphanImplItem {
+        parent: parent_did,
+        trait_parent,
+        item: item.clone(),
+        impl_generics,
+        impl_id,
+    };
     cache.orphan_impl_items.push(orphan_item);
 }
 
Diff in /checkout/src/librustdoc/html/render/search_index.rs:520:
                          ty,
                          module_path,
                          exact_module_path,
-                        parent,
-                        trait_parent,
+                         parent,
+                         trait_parent,
                          deprecated,
                          associated_item_disambiguator,
                      }| EntryData {
Diff in /checkout/src/librustdoc/html/render/search_index.rs:1175:
 
     // Attach all orphan items to the type's definition if the type
     // has since been learned.
-    for &OrphanImplItem { impl_id, parent, trait_parent, ref item, ref impl_generics } in &cache.orphan_impl_items
+    for &OrphanImplItem { impl_id, parent, trait_parent, ref item, ref impl_generics } in
+        &cache.orphan_impl_items
     {
         if let Some((fqp, _)) = cache.paths.get(&parent) {
             let desc = short_markdown_summary(&item.doc_value(), &item.link_names(cache));
Diff in /checkout/src/librustdoc/html/render/search_index.rs:1304:
         .iter_mut()
         .map(|item| {
             let mut defid_to_rowid = |defid, check_external: bool| {
-                cache.paths.get(&defid).or_else(|| check_external.then(|| cache.external_paths.get(&defid)).flatten()).map(|&(ref fqp, ty)| {
-                    let pathid = serialized_index.names.len();
-                    match serialized_index.crate_paths_index.entry((ty, fqp.clone())) {
-                        Entry::Occupied(entry) => *entry.get(),
-                        Entry::Vacant(entry) => {
-                            entry.insert(pathid);
-                            let (name, path) = fqp.split_last().unwrap();
-                            serialized_index.push_path(
-                                name.as_str().to_string(),
-                                PathData {
-                                    ty,
-                                    module_path: path.to_vec(),
-                                    exact_module_path: if let Some(exact_path) =
-                                        cache.exact_paths.get(&defid)
-                                        && let Some((name2, exact_path)) = exact_path.split_last()
-                                        && name == name2
-                                    {
-                                        Some(exact_path.to_vec())
-                                    } else {
-                                        None
+                cache
+                    .paths
+                    .get(&defid)
+                    .or_else(|| check_external.then(|| cache.external_paths.get(&defid)).flatten())
+                    .map(|&(ref fqp, ty)| {
+                        let pathid = serialized_index.names.len();
+                        match serialized_index.crate_paths_index.entry((ty, fqp.clone())) {
+                            Entry::Occupied(entry) => *entry.get(),
+                            Entry::Vacant(entry) => {
+                                entry.insert(pathid);
+                                let (name, path) = fqp.split_last().unwrap();
+                                serialized_index.push_path(
+                                    name.as_str().to_string(),
+                                    PathData {
+                                        ty,
+                                        module_path: path.to_vec(),
+                                        exact_module_path: if let Some(exact_path) =
+                                            cache.exact_paths.get(&defid)
+                                            && let Some((name2, exact_path)) =
+                                                exact_path.split_last()
+                                            && name == name2
+                                        {
+                                            Some(exact_path.to_vec())
+                                        } else {
+                                            None
+                                        },
                                     },
-                                },
-                            );
-                            usize::try_from(pathid).unwrap()
+                                );
+                                usize::try_from(pathid).unwrap()
+                            }
                         }
-                    }
-                })};
+                    })
+            };
             item.parent_idx = item.parent.and_then(|p| defid_to_rowid(p, false));
             item.trait_parent_idx = item.trait_parent.and_then(|p| defid_to_rowid(p, true));
-            debug_assert!(!item.trait_parent.is_some() || item.trait_parent_idx.is_some(), "unable to resolve trait parent for {item:?}");
+            debug_assert!(
+                !item.trait_parent.is_some() || item.trait_parent_idx.is_some(),
+                "unable to resolve trait parent for {item:?}"
+            );
 
             if let Some(defid) = item.defid
                 && item.parent_idx.is_none()
fmt: checked 6340 files
Bootstrap failed while executing `test --stage 0 src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Build completed unsuccessfully in 0:00:46
  local time: Tue Aug 26 19:40:54 UTC 2025
  network time: Tue, 26 Aug 2025 19:40:54 GMT

@lolbinarycat
Copy link
Contributor Author

Right, I just remembered why my original design had its own deduplication pass, instead of leveraging the existing deduplication pass:

if the user searches for char::from, we should show them both ascii::Char::from and char::from. its only when a search matches the actual trait item (in this case, From::from) where we should perform this deduplication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-search Area: Rustdoc's search feature S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blanket impls cause bloated search results in rustdoc
3 participants