From b596f7aca68cf4f1006b590760cdbcb94adfe243 Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 19 Mar 2026 18:45:19 +0100 Subject: [PATCH 1/7] docs: improve docs for `Traversal::converge` Signed-off-by: squidfunk --- crates/zrx-graph/src/graph/traversal.rs | 37 +++++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/crates/zrx-graph/src/graph/traversal.rs b/crates/zrx-graph/src/graph/traversal.rs index 6996218..209bca2 100644 --- a/crates/zrx-graph/src/graph/traversal.rs +++ b/crates/zrx-graph/src/graph/traversal.rs @@ -267,12 +267,25 @@ impl Traversal { Ok(()) } - /// Attempts to converge this traversal with another traversal. + /// Attempts to converge with the given traversal. /// - /// This method attempts to combine both traversals into a single traversal, - /// which is possible when they have common descendants. This is useful for - /// deduplication of computations that need to be carried out for traversals - /// that originate from different sources, but converge at some point. + /// This method attempts to merge both traversals into a single traversal, + /// which is possible when they have common descendants, a condition that + /// is always true for directed acyclic graphs with a single component. + /// There are several cases to consider when converging two traversals: + /// + /// - If traversals start from the same set of source nodes, they already + /// converged, so we just restart the traversal at these source nodes. + /// + /// - If traversals start from different source nodes, yet both have common + /// descendants, we merge those at the first layer of common descendants, + /// as those are the initial nodes that must be visited in both of them. + /// Ancestors of the common descendants that have already been visited in + /// either traversal don't need to be revisited, and thus are carried over + /// from both traversals in their current state. + /// + /// - If traversals are disjoint, they can't be converged, so we return the + /// given traversal back to the caller wrapped in [`Error::Disjoint`]. /// /// # Errors /// @@ -327,15 +340,15 @@ impl Traversal { topology: self.topology.clone(), }; - // If there are no common descendants, the traversals are disjoint, and - // we can't converge them, so we return the traversal to the caller + // If there are no common descendants, the traversals are disjoint and + // can't converge, so we return the given traversal back to the caller let mut iter = graph.common_descendants(&initial); let Some(common) = iter.next() else { return Err(Error::Disjoint(other)); }; // Create the combined traversal, and mark all already visited nodes - // that are ancestors of the common descendants as visited as well + // that are ancestors of the common descendants as visited let prior = mem::replace(self, Self::new(&self.topology, initial)); // Compute the visitable nodes for the combined traversal, which is the @@ -347,10 +360,10 @@ impl Traversal { let p = prior.dependencies[node]; let o = other.dependencies[node]; - // If the node has been visited in either traversal, and is not - // part of the common descendants, mark it as visited as well in - // the combined traversal, since we don't need to revisit nodes - // that are ancestors of the common descendants + // If the node has been visited in either traversal, and is not part + // of the first layer of common descendants, mark it as visited in + // the combined traversal, since we don't need to revisit ancestors + // that have already been visited in either traversal if (p == u8::MAX || o == u8::MAX) && !common.contains(&node) { self.complete(node)?; } else { From 41b3e498f458cef89e6a4dbe8c44dda9f00640d7 Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 19 Mar 2026 20:01:57 +0100 Subject: [PATCH 2/7] fix: comparison of `Id` and `Selector` susceptible to hash collisions Signed-off-by: squidfunk --- crates/zrx-id/src/id.rs | 4 +++- crates/zrx-id/src/id/matcher/selector.rs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/zrx-id/src/id.rs b/crates/zrx-id/src/id.rs index 3ff647a..852e30e 100644 --- a/crates/zrx-id/src/id.rs +++ b/crates/zrx-id/src/id.rs @@ -367,7 +367,9 @@ impl PartialEq for Id { /// ``` #[inline] fn eq(&self, other: &Self) -> bool { - self.hash == other.hash + // We first compare the precomputed hashes, which is extremly fast, as + // it saves us the comparison when the identifiers are different + self.hash == other.hash && self.format == other.format } } diff --git a/crates/zrx-id/src/id/matcher/selector.rs b/crates/zrx-id/src/id/matcher/selector.rs index a5aad64..ea14a61 100644 --- a/crates/zrx-id/src/id/matcher/selector.rs +++ b/crates/zrx-id/src/id/matcher/selector.rs @@ -343,7 +343,9 @@ impl PartialEq for Selector { /// ``` #[inline] fn eq(&self, other: &Self) -> bool { - self.hash == other.hash + // We first compare the precomputed hashes, which is extremely fast, as + // it saves us the comparison when the identifiers are different + self.hash == other.hash && self.format == other.format } } From b2870f5a2f96d2b2c374c73abf220d2273feb2c1 Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 19 Mar 2026 21:07:46 +0100 Subject: [PATCH 3/7] performance: remove branching in `PartialEq` impl for `Id` Signed-off-by: squidfunk --- crates/zrx-id/src/id.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/zrx-id/src/id.rs b/crates/zrx-id/src/id.rs index 852e30e..82fc14c 100644 --- a/crates/zrx-id/src/id.rs +++ b/crates/zrx-id/src/id.rs @@ -419,13 +419,7 @@ impl Ord for Id { /// ``` #[inline] fn cmp(&self, other: &Self) -> Ordering { - // Fast path - first, we compare for equality by using the precomputed - // hashes, as it's a simple and extremely fast integer comparison - if self.eq(other) { - Ordering::Equal - } else { - self.format.cmp(&other.format) - } + self.format.cmp(&other.format) } } From dad5ed4c724ef957f57a2d498be8f684c0785dea Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 19 Mar 2026 21:08:54 +0100 Subject: [PATCH 4/7] performance: provide dedicated `PartialEq` impl for `Selector` Signed-off-by: squidfunk --- crates/zrx-id/src/id/matcher/selector.rs | 51 +++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/crates/zrx-id/src/id/matcher/selector.rs b/crates/zrx-id/src/id/matcher/selector.rs index ea14a61..a6dcfb6 100644 --- a/crates/zrx-id/src/id/matcher/selector.rs +++ b/crates/zrx-id/src/id/matcher/selector.rs @@ -27,6 +27,7 @@ use ahash::AHasher; use std::borrow::Cow; +use std::cmp::Ordering; use std::fmt::{self, Debug, Display}; use std::hash::{Hash, Hasher}; use std::str::FromStr; @@ -104,7 +105,7 @@ pub use convert::TryToSelector; /// # Ok(()) /// # } /// ``` -#[derive(Clone, PartialOrd, Ord)] +#[derive(Clone)] pub struct Selector { /// Formatted string. format: Arc>, @@ -353,6 +354,54 @@ impl Eq for Selector {} // ---------------------------------------------------------------------------- +impl PartialOrd for Selector { + /// Orders two selectors. + /// + /// # Examples + /// + /// ``` + /// # use std::error::Error; + /// # fn main() -> Result<(), Box> { + /// use zrx_id::Selector; + /// + /// // Create and compare selectors + /// let a: Selector = "zrs:::::**/*.md:".parse()?; + /// let b: Selector = "zrs:::::**/*.rs:".parse()?; + /// assert!(a < b); + /// # Ok(()) + /// # } + /// ``` + #[inline] + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Selector { + /// Orders two selectors. + /// + /// # Examples + /// + /// ``` + /// # use std::error::Error; + /// # fn main() -> Result<(), Box> { + /// use zrx_id::Selector; + /// + /// // Create and compare selectors + /// let a: Selector = "zrs:::::**/*.md:".parse()?; + /// let b: Selector = "zrs:::::**/*.rs:".parse()?; + /// assert!(a < b); + /// # Ok(()) + /// # } + /// ``` + #[inline] + fn cmp(&self, other: &Self) -> Ordering { + self.format.cmp(&other.format) + } +} + +// ---------------------------------------------------------------------------- + impl Display for Selector { /// Formats the selector for display. fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { From 595a4d7be3be8330ed64bb0e1043d888e390c46a Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 19 Mar 2026 22:06:36 +0100 Subject: [PATCH 5/7] docs: adjust comment Signed-off-by: squidfunk --- crates/zrx-graph/src/graph/traversal.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/zrx-graph/src/graph/traversal.rs b/crates/zrx-graph/src/graph/traversal.rs index 209bca2..fb7bb54 100644 --- a/crates/zrx-graph/src/graph/traversal.rs +++ b/crates/zrx-graph/src/graph/traversal.rs @@ -278,8 +278,8 @@ impl Traversal { /// converged, so we just restart the traversal at these source nodes. /// /// - If traversals start from different source nodes, yet both have common - /// descendants, we merge those at the first layer of common descendants, - /// as those are the initial nodes that must be visited in both of them. + /// descendants, we converge at the first layer of common descendants, as + /// all descendants of them must be revisited in the combined traversal. /// Ancestors of the common descendants that have already been visited in /// either traversal don't need to be revisited, and thus are carried over /// from both traversals in their current state. From dd4e3f9c7e7038ad4a34560e07edda0782fe6c3c Mon Sep 17 00:00:00 2001 From: squidfunk Date: Fri, 20 Mar 2026 11:44:15 +0100 Subject: [PATCH 6/7] feature: add `Traversal::initial` impl to obtain initial nodes Signed-off-by: squidfunk --- crates/zrx-graph/src/graph.rs | 2 +- crates/zrx-graph/src/graph/traversal.rs | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/zrx-graph/src/graph.rs b/crates/zrx-graph/src/graph.rs index 499a9a5..f811633 100644 --- a/crates/zrx-graph/src/graph.rs +++ b/crates/zrx-graph/src/graph.rs @@ -190,7 +190,7 @@ impl Graph { #[allow(clippy::must_use_candidate)] impl Graph { - /// Returns the graph topology. + /// Returns a reference to the graph topology. #[inline] pub fn topology(&self) -> &Topology { &self.topology diff --git a/crates/zrx-graph/src/graph/traversal.rs b/crates/zrx-graph/src/graph/traversal.rs index fb7bb54..2127bc1 100644 --- a/crates/zrx-graph/src/graph/traversal.rs +++ b/crates/zrx-graph/src/graph/traversal.rs @@ -381,12 +381,18 @@ impl Traversal { #[allow(clippy::must_use_candidate)] impl Traversal { - /// Returns the graph topology. + /// Returns a reference to the graph topology. #[inline] pub fn topology(&self) -> &Topology { &self.topology } + /// Returns a reference to the initial nodes. + #[inline] + pub fn initial(&self) -> &[usize] { + &self.initial + } + /// Returns the number of visitable nodes. #[inline] pub fn len(&self) -> usize { From a69615de80b917d8665cc57592fde962ad6f29e5 Mon Sep 17 00:00:00 2001 From: squidfunk Date: Fri, 20 Mar 2026 20:51:39 +0100 Subject: [PATCH 7/7] feature!: replace `Graph::with` with `Graph::adjacent` accessor Signed-off-by: squidfunk --- crates/zrx-graph/src/graph/operator.rs | 2 +- .../graph/operator/{with.rs => adjacent.rs} | 40 ++++++++----------- 2 files changed, 17 insertions(+), 25 deletions(-) rename crates/zrx-graph/src/graph/operator/{with.rs => adjacent.rs} (83%) diff --git a/crates/zrx-graph/src/graph/operator.rs b/crates/zrx-graph/src/graph/operator.rs index c4dfcfc..e4d9bf0 100644 --- a/crates/zrx-graph/src/graph/operator.rs +++ b/crates/zrx-graph/src/graph/operator.rs @@ -25,5 +25,5 @@ //! Graph operators. +mod adjacent; mod map; -mod with; diff --git a/crates/zrx-graph/src/graph/operator/with.rs b/crates/zrx-graph/src/graph/operator/adjacent.rs similarity index 83% rename from crates/zrx-graph/src/graph/operator/with.rs rename to crates/zrx-graph/src/graph/operator/adjacent.rs index 0d35eec..5557d96 100644 --- a/crates/zrx-graph/src/graph/operator/with.rs +++ b/crates/zrx-graph/src/graph/operator/adjacent.rs @@ -23,7 +23,7 @@ // ---------------------------------------------------------------------------- -//! With operator. +//! Adjacent operator. use crate::graph::Graph; @@ -44,7 +44,7 @@ pub struct Adjacent<'a> { // ---------------------------------------------------------------------------- impl Graph { - /// Retrieve a reference to a node's data. + /// Retrieve a reference to a node and its adjacent nodes. /// /// # Examples /// @@ -63,24 +63,20 @@ impl Graph { /// builder.add_edge(a, b)?; /// builder.add_edge(b, c)?; /// - /// // Create graph from builder and retrieve nodes + /// // Create graph from builder and retrieve node /// let graph = builder.build(); - /// for node in &graph { - /// graph.with(node, |name, _| { - /// println!("{name:?}"); - /// }); - /// } + /// + /// // Obtain reference to node and adjacent nodes + /// let (data, adj) = graph.adjacent(a); /// # Ok(()) /// # } /// ``` #[inline] - pub fn with(&self, node: usize, f: F) -> R - where - F: FnOnce(&T, Adjacent) -> R, - { + #[must_use] + pub fn adjacent(&self, node: usize) -> (&'_ T, Adjacent<'_>) { let incoming = self.topology.incoming(); let outgoing = self.topology.outgoing(); - f( + ( &self.data[node], Adjacent { incoming: &incoming[node], @@ -89,7 +85,7 @@ impl Graph { ) } - /// Retrieve a mutable reference to a node's data. + /// Retrieve a mutable reference to a node and its adjacent nodes. /// /// # Examples /// @@ -110,22 +106,18 @@ impl Graph { /// /// // Create graph from builder and retrieve node /// let mut graph = builder.build(); - /// for node in &graph { - /// graph.with_mut(node, |name, _| { - /// println!("{name:?}"); - /// }); - /// } + /// + /// // Obtain mutable reference to node and adjacent nodes + /// let (data, adj) = graph.adjacent_mut(a); /// # Ok(()) /// # } /// ``` #[inline] - pub fn with_mut(&mut self, node: usize, f: F) -> R - where - F: FnOnce(&mut T, Adjacent) -> R, - { + #[must_use] + pub fn adjacent_mut(&mut self, node: usize) -> (&'_ mut T, Adjacent<'_>) { let incoming = self.topology.incoming(); let outgoing = self.topology.outgoing(); - f( + ( &mut self.data[node], Adjacent { incoming: &incoming[node],