simpler dep graph

Author: Grant Wuerker

crates/common/src/dependencies/graph.rs

@@ -12,15 +12,11 @@ use crate::InputDb;
 
 type EdgeWeight = (DependencyAlias, DependencyArguments);
 
-#[allow(clippy::too_many_arguments)] // salsa input constructor currently needs all fields
 #[salsa::input]
 #[derive(Debug)]
 pub struct DependencyGraph {
-    local_graph: DiGraph<Url, EdgeWeight>,
-    local_node_map: HashMap<Url, NodeIndex>,
-    remote_graph: DiGraph<Url, EdgeWeight>,
-    remote_node_map: HashMap<Url, NodeIndex>,
-    remote_sets: HashMap<Url, Vec<Url>>,
+    graph: DiGraph<Url, EdgeWeight>,
+    node_map: HashMap<Url, NodeIndex>,
     git_locations: HashMap<Url, RemoteFiles>,
     reverse_git_map: HashMap<RemoteFiles, Url>,
 }
@@ -32,9 +28,6 @@ impl DependencyGraph {
             db,
             DiGraph::new(),
             HashMap::new(),
-            DiGraph::new(),
-            HashMap::new(),
-            HashMap::new(),
             HashMap::new(),
             HashMap::new(),
         )
@@ -54,70 +47,42 @@ impl DependencyGraph {
         }
     }
 
-    pub fn ensure_local_node(&self, db: &mut dyn InputDb, url: &Url) {
-        if self.local_node_map(db).contains_key(url) {
+    pub fn ensure_node(&self, db: &mut dyn InputDb, url: &Url) {
+        if self.node_map(db).contains_key(url) {
             return;
         }
-        let mut graph = self.local_graph(db);
-        let mut node_map = self.local_node_map(db);
+        let mut graph = self.graph(db);
+        let mut node_map = self.node_map(db);
         Self::allocate_node(&mut graph, &mut node_map, url);
-        self.set_local_graph(db).to(graph);
-        self.set_local_node_map(db).to(node_map);
+        self.set_graph(db).to(graph);
+        self.set_node_map(db).to(node_map);
     }
 
-    pub fn contains_local_url(&self, db: &dyn InputDb, url: &Url) -> bool {
-        self.local_node_map(db).contains_key(url)
-    }
-
-    pub fn add_local_dependency(
-        &self,
-        db: &mut dyn InputDb,
-        source: &Url,
-        target: &Url,
-        alias: DependencyAlias,
-        arguments: DependencyArguments,
-    ) {
-        let mut graph = self.local_graph(db);
-        let mut node_map = self.local_node_map(db);
-        let source_idx = Self::allocate_node(&mut graph, &mut node_map, source);
-        let target_idx = Self::allocate_node(&mut graph, &mut node_map, target);
-        graph.add_edge(source_idx, target_idx, (alias, arguments));
-        self.set_local_graph(db).to(graph);
-        self.set_local_node_map(db).to(node_map);
+    pub fn contains_url(&self, db: &dyn InputDb, url: &Url) -> bool {
+        self.node_map(db).contains_key(url)
     }
 
-    pub fn add_remote_dependency(
+    pub fn add_dependency(
         &self,
         db: &mut dyn InputDb,
         source: &Url,
         target: &Url,
         alias: DependencyAlias,
         arguments: DependencyArguments,
     ) {
-        let mut graph = self.remote_graph(db);
-        let mut node_map = self.remote_node_map(db);
+        let mut graph = self.graph(db);
+        let mut node_map = self.node_map(db);
         let source_idx = Self::allocate_node(&mut graph, &mut node_map, source);
         let target_idx = Self::allocate_node(&mut graph, &mut node_map, target);
         graph.add_edge(source_idx, target_idx, (alias, arguments));
-        self.set_remote_graph(db).to(graph);
-        self.set_remote_node_map(db).to(node_map);
-    }
-
-    pub fn ensure_remote_node(&self, db: &mut dyn InputDb, url: &Url) {
-        if self.remote_node_map(db).contains_key(url) {
-            return;
-        }
-        let mut graph = self.remote_graph(db);
-        let mut node_map = self.remote_node_map(db);
-        Self::allocate_node(&mut graph, &mut node_map, url);
-        self.set_remote_graph(db).to(graph);
-        self.set_remote_node_map(db).to(node_map);
+        self.set_graph(db).to(graph);
+        self.set_node_map(db).to(node_map);
     }
 
     pub fn cyclic_subgraph(&self, db: &dyn InputDb) -> DiGraph<Url, EdgeWeight> {
         use petgraph::algo::tarjan_scc;
 
-        let graph = self.local_graph(db);
+        let graph = self.graph(db);
         let sccs = tarjan_scc(&graph);
 
         let mut cyclic_nodes = HashSet::new();
@@ -171,8 +136,8 @@ impl DependencyGraph {
     }
 
     pub fn dependency_urls(&self, db: &dyn InputDb, url: &Url) -> Vec<Url> {
-        let node_map = self.local_node_map(db);
-        let graph = self.local_graph(db);
+        let node_map = self.node_map(db);
+        let graph = self.graph(db);
 
         if let Some(&root) = node_map.get(url) {
             let mut dfs = Dfs::new(&graph, root);
@@ -188,20 +153,6 @@ impl DependencyGraph {
         }
     }
 
-    pub fn add_remote_set(&self, db: &mut dyn InputDb, local_url: &Url, remote_root: Url) {
-        self.ensure_remote_node(db, &remote_root);
-        let mut sets = self.remote_sets(db);
-        sets.entry(local_url.clone()).or_default().push(remote_root);
-        self.set_remote_sets(db).to(sets);
-    }
-
-    pub fn remote_sets_for(&self, db: &dyn InputDb, local_url: &Url) -> Vec<Url> {
-        self.remote_sets(db)
-            .get(local_url)
-            .cloned()
-            .unwrap_or_default()
-    }
-
     pub fn register_remote_checkout(
         &self,
         db: &mut dyn InputDb,

crates/driver/src/handlers/local.rs

@@ -93,20 +93,14 @@ impl<'a> ResolutionHandler<FilesResolver> for LocalIngotHandler<'a> {
                 });
         }
 
-        self.db
-            .dependency_graph()
-            .ensure_local_node(self.db, ingot_url);
+        self.db.dependency_graph().ensure_node(self.db, ingot_url);
 
         let mut pending = Vec::new();
         for dependency in config.dependencies(ingot_url) {
             match dependency.location {
                 DependencyLocation::Local(local) => {
-                    if self
-                        .db
-                        .dependency_graph()
-                        .contains_local_url(self.db, &local.url)
-                    {
-                        self.db.dependency_graph().add_local_dependency(
+                    if self.db.dependency_graph().contains_url(self.db, &local.url) {
+                        self.db.dependency_graph().add_dependency(
                             self.db,
                             ingot_url,
                             &local.url,
@@ -150,7 +144,7 @@ impl<'a> GraphResolutionHandler<Url, DiGraph<Url, (DependencyAlias, DependencyAr
             let from_url = &graph[edge.source()];
             let to_url = &graph[edge.target()];
             let (alias, arguments) = edge.weight();
-            dependency_graph.add_local_dependency(
+            dependency_graph.add_dependency(
                 self.db,
                 from_url,
                 to_url,

crates/driver/src/handlers/remote.rs

@@ -122,10 +122,7 @@ impl<'a> RemoteIngotHandler<'a> {
 }
 
 impl<'a> ResolutionHandler<GitResolver> for RemoteIngotHandler<'a> {
-    type Item = Vec<(
-        GitDescription,
-        (DependencyAlias, DependencyArguments),
-    )>;
+    type Item = Vec<(GitDescription, (DependencyAlias, DependencyArguments))>;
 
     fn handle_resolution(
         &mut self,
@@ -181,10 +178,7 @@ impl<'a> ResolutionHandler<GitResolver> for RemoteIngotHandler<'a> {
 }
 
 impl<'a> ResolutionHandler<FilesResolver> for RemoteIngotHandler<'a> {
-    type Item = Vec<(
-        GitDescription,
-        (DependencyAlias, DependencyArguments),
-    )>;
+    type Item = Vec<(GitDescription, (DependencyAlias, DependencyArguments))>;
 
     fn handle_resolution(&mut self, ingot_url: &Url, resource: FilesResource) -> Self::Item {
         let context = self
@@ -271,6 +265,7 @@ impl<'a>
             if let Some(url) = self.ingot_urls.get(&graph[node_idx]) {
                 let new_idx = converted.add_node(url.clone());
                 node_map.insert(node_idx, new_idx);
+                self.db.dependency_graph().ensure_node(self.db, url);
             }
         }
 
@@ -282,7 +277,7 @@ impl<'a>
                 let from_url = converted[from_idx].clone();
                 let to_url = converted[to_idx].clone();
                 converted.add_edge(from_idx, to_idx, weight.clone());
-                self.db.dependency_graph().add_remote_dependency(
+                self.db.dependency_graph().add_dependency(
                     self.db,
                     &from_url,
                     &to_url,

crates/driver/src/lib.rs

@@ -283,7 +283,6 @@ fn resolve_remote_dependencies(
         (SmolStr, DependencyArguments),
     > = GraphResolverImpl::new(git_resolver);
     let mut handler = RemoteIngotHandler::new(db);
-    let mut resolved_sets = Vec::new();
     let mut diagnostics = Vec::new();
     let multi = MultiProgress::new();
     let spinner_style = ProgressStyle::with_template("{spinner} {msg}")
@@ -303,8 +302,14 @@ fn resolve_remote_dependencies(
         match graph_resolver.graph_resolve(&mut handler, &description) {
             Ok(_graph) => {
                 if let Some(root_url) = handler.ingot_url(&description) {
+                    db.dependency_graph().add_dependency(
+                        db,
+                        &request.parent,
+                        root_url,
+                        request.alias.clone(),
+                        request.arguments.clone(),
+                    );
                     spinner.finish_with_message(format!("✅ Resolved {root_url}"));
-                    resolved_sets.push((request.parent.clone(), root_url.clone()));
                 } else {
                     spinner.finish_and_clear();
                 }
@@ -345,11 +350,6 @@ fn resolve_remote_dependencies(
 
     drop(handler);
 
-    for (parent, remote_root) in resolved_sets {
-        db.dependency_graph()
-            .add_remote_set(db, &parent, remote_root);
-    }
-
     diagnostics
 }
 

crates/fe/src/tree.rs

@@ -606,10 +606,8 @@ impl ResolutionHandler<GitResolver> for TreeRemoteHandler {
         let config_content = match std::fs::read_to_string(&config_path) {
             Ok(content) => content,
             Err(error) => {
-                self.diagnostics.push(format!(
-                    "Remote file error at {}: {error}",
-                    ingot_url
-                ));
+                self.diagnostics
+                    .push(format!("Remote file error at {}: {error}", ingot_url));
                 return vec![];
             }
         };
@@ -634,10 +632,8 @@ impl ResolutionHandler<GitResolver> for TreeRemoteHandler {
             }
         }
 
-        self.metadata.insert(
-            ingot_url.clone(),
-            NodeMetadata::from_config(&config),
-        );
+        self.metadata
+            .insert(ingot_url.clone(), NodeMetadata::from_config(&config));
 
         let mut dependencies = Vec::new();
         for dependency in config.dependencies(&ingot_url) {
@@ -666,10 +662,8 @@ impl ResolutionHandler<GitResolver> for TreeRemoteHandler {
                     }
                 },
                 DependencyLocation::Remote(remote) => {
-                    let mut next_description = GitDescription::new(
-                        remote.source.clone(),
-                        remote.rev.to_string(),
-                    );
+                    let mut next_description =
+                        GitDescription::new(remote.source.clone(), remote.rev.to_string());
                     if let Some(path) = remote.path.clone() {
                         next_description = next_description.with_path(path);
                     }

crates/resolver/src/files.rs

@@ -174,9 +174,7 @@ impl Resolver for FilesResolver {
 
         // Check if the directory exists
         if !ingot_path.exists() || !ingot_path.is_dir() {
-            return Err(FilesResolutionError::DirectoryDoesNotExist(
-                url.clone(),
-            ));
+            return Err(FilesResolutionError::DirectoryDoesNotExist(url.clone()));
         }
 
         // Check for required directories first

crates/resolver/src/git.rs

@@ -151,9 +151,7 @@ impl GitResolver {
         Ok(())
     }
 
-    fn source_for_clone(
-        description: &GitDescription,
-    ) -> Result<String, GitResolutionError> {
+    fn source_for_clone(description: &GitDescription) -> Result<String, GitResolutionError> {
         if description.source.scheme() == "file" {
             let path = description.source.to_file_path().map_err(|_| {
                 GitResolutionError::SourcePathConversion {