[2/n] [ls-apis] return walked dependencies instead of using a callback (#10458)

We're going to return more data from this function (the set of omitted
nodes that were seen) in an upcoming PR.

I tried out a few different approaches:

* two callbacks
* a trait
* a hybrid approach where we keep the current callback and return the
additional data as a value

Based on the prototyping I feel like this ends up being the cleanest
approach.

There are no functional changes in this PR.

Best reviewed in conjunction with its dependent PR:

* #10459

Author: sunshowers

dev-tools/ls-apis/src/cargo.rs

@@ -11,6 +11,9 @@ use camino::Utf8Path;
 use camino::Utf8PathBuf;
 use cargo_metadata::{CargoOpt, Package};
 use cargo_metadata::{DependencyKind, PackageId};
+use iddqd::IdOrdItem;
+use iddqd::IdOrdMap;
+use iddqd::id_upcast;
 use std::collections::BTreeSet;
 use std::collections::{BTreeMap, VecDeque};
 
@@ -234,23 +237,16 @@ impl Workspace {
         Ok(path)
     }
 
-    /// Iterate over the required dependencies of package `root`, invoking
-    /// `func` for each one as:
+    /// Walks the required (normal and build) dependencies of package `root`.
     ///
-    /// ```ignore
-    /// func(package: &Package, dep_path: &DepPath)
-    /// ```
-    ///
-    /// where `package` is the package that is (directly or indirectly) a
-    /// dependency of `root` and `dep_path` describes the dependency path from
-    /// `root` to `package`.
-    pub fn walk_required_deps_recursively(
-        &self,
+    /// Returns a [`WalkOutcome`] describing every package reachable from
+    /// `root`, each paired with every dependency path from `root` to it.
+    pub fn walk_required_deps_recursively<'a>(
+        &'a self,
         root: &Package,
-        func: &mut dyn FnMut(&Package, &DepPath),
-    ) -> Result<()> {
-        struct Remaining<'a> {
-            node: &'a cargo_metadata::Node,
+    ) -> Result<WalkOutcome<'a>> {
+        struct Remaining<'n> {
+            node: &'n cargo_metadata::Node,
             path: DepPath,
         }
 
@@ -268,6 +264,7 @@ impl Workspace {
             path: DepPath::for_pkg(root.id.clone()),
         }];
         let mut seen: BTreeSet<PackageId> = BTreeSet::new();
+        let mut found: IdOrdMap<PackageWalkOutcome<'a>> = IdOrdMap::new();
 
         while let Some(Remaining { node: next, path }) = remaining.pop() {
             for d in &next.deps {
@@ -288,7 +285,14 @@ impl Workspace {
                 // package metadata.
                 let dep_pkg = self.packages_by_id.get(did).unwrap();
                 let dep_node = self.nodes_by_id.get(did).unwrap();
-                func(dep_pkg, &path);
+                found
+                    .entry(&dep_pkg.id)
+                    .or_insert_with(|| PackageWalkOutcome {
+                        package: dep_pkg,
+                        dep_paths: Vec::new(),
+                    })
+                    .dep_paths
+                    .push(path.clone());
                 if seen.contains(did) {
                     continue;
                 }
@@ -299,7 +303,7 @@ impl Workspace {
             }
         }
 
-        Ok(())
+        Ok(WalkOutcome { found })
     }
 
     /// Return all package ids for the given `pkgname`
@@ -343,6 +347,38 @@ fn cargo_toml_parent(
     Ok(path)
 }
 
+/// The result of [`Workspace::walk_required_deps_recursively`].
+pub struct WalkOutcome<'a> {
+    /// Every package encountered as a required (normal or build) dependency of
+    /// the walk's `root`, each paired with every dependency path from `root`
+    /// to that package.
+    pub found: IdOrdMap<PackageWalkOutcome<'a>>,
+}
+
+/// A single entry in [`WalkOutcome::found`]: one package and every dependency
+/// path from the walk's `root` to it.
+#[derive(Debug)]
+pub struct PackageWalkOutcome<'a> {
+    /// The package that was found during the walk.
+    pub package: &'a Package,
+
+    /// The list of dependency paths from the walk's `root` to this package.
+    ///
+    /// A package reachable by more than one path appears once per path.
+    pub dep_paths: Vec<DepPath>,
+}
+
+impl<'a> IdOrdItem for PackageWalkOutcome<'a> {
+    type Key<'b>
+        = &'a PackageId
+    where
+        Self: 'b;
+    fn key(&self) -> Self::Key<'_> {
+        &self.package.id
+    }
+    id_upcast!();
+}
+
 /// Describes a "dependency path": a path through the Cargo dependency graph
 /// from one package to another, which describes how one package depends on
 /// another

dev-tools/ls-apis/src/system_apis.rs

@@ -22,7 +22,6 @@ use crate::workspaces::Workspaces;
 use anyhow::Result;
 use anyhow::{Context, anyhow, bail};
 use camino::Utf8PathBuf;
-use cargo_metadata::Package;
 use iddqd::IdOrdItem;
 use iddqd::IdOrdMap;
 use iddqd::id_upcast;
@@ -173,14 +172,21 @@ impl SystemApis {
                 let (workspace, server_pkg) =
                     workspaces.find_package_workspace(dunit_pkg)?;
                 let dep_path = DepPath::for_pkg(server_pkg.id.clone());
-                tracker.found_package(dunit_pkg, dunit_pkg, &dep_path);
+                tracker.found_package(
+                    dunit_pkg,
+                    dunit_pkg,
+                    std::slice::from_ref(&dep_path),
+                );
 
-                workspace.walk_required_deps_recursively(
-                    server_pkg,
-                    &mut |p: &Package, dep_path: &DepPath| {
-                        tracker.found_package(dunit_pkg, &p.name, dep_path);
-                    },
-                )?;
+                let outcome =
+                    workspace.walk_required_deps_recursively(server_pkg)?;
+                for pkg_outcome in &outcome.found {
+                    tracker.found_package(
+                        dunit_pkg,
+                        &pkg_outcome.package.name,
+                        &pkg_outcome.dep_paths,
+                    );
+                }
             }
         }
 
@@ -219,24 +225,22 @@ impl SystemApis {
         for server_pkgname in server_component_units.keys() {
             let (workspace, pkg) =
                 workspaces.find_package_workspace(server_pkgname)?;
-            workspace
-                .walk_required_deps_recursively(
-                    pkg,
-                    &mut |p: &Package, dep_path: &DepPath| {
-                        deps_tracker.found_dependency(
-                            server_pkgname,
-                            &p.name,
-                            dep_path,
-                        );
-                    },
-                )
+            let outcome = workspace
+                .walk_required_deps_recursively(pkg)
                 .with_context(|| {
                     format!(
                         "iterating dependencies of workspace {:?} package {:?}",
                         workspace.name(),
                         server_pkgname
                     )
                 })?;
+            for pkg_outcome in &outcome.found {
+                deps_tracker.found_dependency(
+                    server_pkgname,
+                    &pkg_outcome.package.name,
+                    &pkg_outcome.dep_paths,
+                );
+            }
         }
 
         let (apis_consumed, api_consumers) =
@@ -1414,7 +1418,7 @@ impl<'a> ServerComponentsTracker<'a> {
     }
 
     /// Record that deployment unit package `dunit_pkgname` depends on package
-    /// `pkgname` via dependency chain `dep_path`
+    /// `pkgname` via each of the given dependency chains `dep_paths`
     ///
     /// This only records anything if `pkgname` turns out to be a known API
     /// client package name, in which case this records that the server
@@ -1423,14 +1427,16 @@ impl<'a> ServerComponentsTracker<'a> {
         &mut self,
         dunit_pkgname: &ServerComponentName,
         pkgname: &str,
-        dep_path: &DepPath,
+        dep_paths: &[DepPath],
     ) {
         let Some(apis) = self.known_server_packages.get(pkgname) else {
             return;
         };
 
-        for api in apis {
-            self.found_api_producer(api, dunit_pkgname, dep_path);
+        for dep_path in dep_paths {
+            for api in apis {
+                self.found_api_producer(api, dunit_pkgname, dep_path);
+            }
         }
     }
 
@@ -1488,7 +1494,7 @@ impl<'a> ClientDependenciesTracker<'a> {
     }
 
     /// Record that comopnent `server_pkgname` consumes package `pkgname` via
-    /// dependency chain `dep_path`
+    /// each of the given dependency chains `dep_paths`
     ///
     /// This only records cases where `pkgname` is a known client package for
     /// one of our APIs, in which case it records that this server component
@@ -1497,7 +1503,7 @@ impl<'a> ClientDependenciesTracker<'a> {
         &mut self,
         server_pkgname: &ServerComponentName,
         pkgname: &str,
-        dep_path: &DepPath,
+        dep_paths: &[DepPath],
     ) {
         let Some(api) = self.api_metadata.client_pkgname_lookup(pkgname) else {
             return;
@@ -1506,18 +1512,22 @@ impl<'a> ClientDependenciesTracker<'a> {
         // This is the name of a known client package.  Record it.
         let status = api.restricted_to_consumers.status(server_pkgname);
         let client_pkgname = ClientPackageName::from(pkgname.to_owned());
-        self.api_consumers
-            .entry(client_pkgname.clone())
-            .or_insert_with(IdOrdMap::new)
-            .entry(&server_pkgname)
-            .or_insert_with(|| ApiConsumer::new(server_pkgname.clone(), status))
-            .add_path(dep_path.clone());
-        self.apis_consumed
-            .entry(server_pkgname.clone())
-            .or_insert_with(BTreeMap::new)
-            .entry(client_pkgname)
-            .or_insert_with(Vec::new)
-            .push(dep_path.clone());
+        for dep_path in dep_paths {
+            self.api_consumers
+                .entry(client_pkgname.clone())
+                .or_insert_with(IdOrdMap::new)
+                .entry(&server_pkgname)
+                .or_insert_with(|| {
+                    ApiConsumer::new(server_pkgname.clone(), status.clone())
+                })
+                .add_path(dep_path.clone());
+            self.apis_consumed
+                .entry(server_pkgname.clone())
+                .or_insert_with(BTreeMap::new)
+                .entry(client_pkgname.clone())
+                .or_insert_with(Vec::new)
+                .push(dep_path.clone());
+        }
     }
 }