From 990c0ffc46ee790c15e084dbc8fc2f9f6444befe Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Sat, 29 Aug 2026 06:53:25 +0000 Subject: [PATCH 1/3] internal: stop `toposort` panicking when a dependency key is repeated --- prqlc/prqlc/src/utils/toposort.rs | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/prqlc/prqlc/src/utils/toposort.rs b/prqlc/prqlc/src/utils/toposort.rs index cc2ca87713d0..08ec9bc9fbe4 100644 --- a/prqlc/prqlc/src/utils/toposort.rs +++ b/prqlc/prqlc/src/utils/toposort.rs @@ -35,20 +35,21 @@ pub fn toposort<'a, Key: Eq + std::hash::Hash + Clone>( visiting: false, done: false, }; + // Sized by `dependencies`, not `index`: a repeated key collapses to a single + // `index` entry, but `dag` still has a node per element of `dependencies`. let mut toposort = Toposort { - nodes: vec![empty; index.len()], - order: Vec::with_capacity(index.len()), + nodes: vec![empty; dependencies.len()], + order: Vec::with_capacity(dependencies.len()), }; if let Some(start) = start.map(|s| index.get(s).unwrap()) { // use only the provided visit start toposort.visit(&dag, *start).ok()?; } else { - // start visits from all nodes - while toposort.order.len() < dependencies.len() { - for start_at in 0..index.len() { - toposort.visit(&dag, start_at).ok()?; - } + // start visits from all nodes; one pass reaches every node, since + // `visit` returns immediately for one that's already done + for start_at in 0..dependencies.len() { + toposort.visit(&dag, start_at).ok()?; } } @@ -142,6 +143,21 @@ mod tests { assert_eq!(order, vec!["b", "a", "c", "d"]); } + /// A repeated key used to panic (or, for other shapes, loop forever), + /// because `nodes` was sized by the deduplicated key count while `dag` was + /// sized by the number of entries. + #[test] + fn repeated_key() { + let dependencies = vec![("c", vec!["a"]), ("a", vec![]), ("a", vec![])]; + + let order = toposort(&dependencies, None).unwrap(); + + // Every entry appears once, and the `a` that `c` resolves to — the + // last one declared — comes before it. + let order = order.into_iter().copied().collect_vec(); + assert_eq!(order, vec!["a", "c", "a"]); + } + #[test] fn with_root() { let dependencies = vec![ From 4f0d4a25e096546872a8783a79ac94e3dbb65d4c Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:04:37 +0000 Subject: [PATCH 2/3] internal: document `toposort`'s contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repeated keys are now permitted, later declarations win for edge resolution, and the output has one entry per input element — none of which was written down outside a test. Also labels the existing `start` panic with an `expect` message instead of a bare `unwrap`; the behaviour is unchanged. --- prqlc/prqlc/src/utils/toposort.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/prqlc/prqlc/src/utils/toposort.rs b/prqlc/prqlc/src/utils/toposort.rs index 08ec9bc9fbe4..504cde117ca3 100644 --- a/prqlc/prqlc/src/utils/toposort.rs +++ b/prqlc/prqlc/src/utils/toposort.rs @@ -13,6 +13,23 @@ struct Node { done: bool, } +/// Sorts `dependencies` so that each entry follows the entries it depends on. +/// +/// Returns one element per element of `dependencies` — not per distinct key — +/// or `None` if the dependencies contain a cycle. +/// +/// - A key may be declared more than once. Edges resolve to the *last* +/// declaration of a key, and every declaration appears in the output, so a +/// repeated key appears in the output more than once. +/// - A dependency on a key that is never declared is ignored. +/// - `start` limits the output to the entries reachable from that key, +/// including the entry for the key itself. +/// +/// # Panics +/// +/// Panics if `start` is `Some` and names a key that isn't declared in +/// `dependencies`. Note `None` is already used for "there's a cycle", so an +/// unknown start key can't currently be reported through the return type. pub fn toposort<'a, Key: Eq + std::hash::Hash + Clone>( dependencies: &'a [(Key, Vec)], start: Option<&'_ Key>, @@ -42,7 +59,11 @@ pub fn toposort<'a, Key: Eq + std::hash::Hash + Clone>( order: Vec::with_capacity(dependencies.len()), }; - if let Some(start) = start.map(|s| index.get(s).unwrap()) { + if let Some(start) = start.map(|s| { + index + .get(s) + .expect("`start` must name a key declared in `dependencies`") + }) { // use only the provided visit start toposort.visit(&dag, *start).ok()?; } else { From 3822758b43c8740d032e514ba137248a9049f5c6 Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:16:42 +0000 Subject: [PATCH 3/3] internal: qualify `toposort`'s cycle-detection claim for `start` With `start` set, `visit` is only entered from that node, so a cycle in an unreachable part of `dependencies` is never seen and the function returns `Some`. The doc comment claimed `None` for any cycle. Adds a test pinning the behaviour. --- prqlc/prqlc/src/utils/toposort.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/prqlc/prqlc/src/utils/toposort.rs b/prqlc/prqlc/src/utils/toposort.rs index 504cde117ca3..e9863c2e6a4f 100644 --- a/prqlc/prqlc/src/utils/toposort.rs +++ b/prqlc/prqlc/src/utils/toposort.rs @@ -16,7 +16,8 @@ struct Node { /// Sorts `dependencies` so that each entry follows the entries it depends on. /// /// Returns one element per element of `dependencies` — not per distinct key — -/// or `None` if the dependencies contain a cycle. +/// or `None` if a cycle is reached. With `start`, only the reachable portion +/// is visited, so a cycle elsewhere in `dependencies` isn't detected. /// /// - A key may be declared more than once. Edges resolve to the *last* /// declaration of a key, and every declaration appears in the output, so a @@ -179,6 +180,19 @@ mod tests { assert_eq!(order, vec!["a", "c", "a"]); } + /// Cycle detection only covers the visited part of the graph, so a cycle + /// that `start` can't reach goes unreported. + #[test] + fn cycle_unreachable_from_root() { + let dependencies = vec![("a", vec![]), ("b", vec!["c"]), ("c", vec!["b"])]; + let root = "a"; + + let order = toposort(&dependencies, Some(&root)).unwrap(); + + let order = order.into_iter().copied().collect_vec(); + assert_eq!(order, vec!["a"]); + } + #[test] fn with_root() { let dependencies = vec![