From 4a819c6838af7395914049982ed44e3f87891ee6 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 25 Aug 2025 16:18:36 +0100 Subject: [PATCH 01/52] Add NameLinkingPolicy::filter_private - minimal tests, TODO need more --- hugr-core/src/hugr/linking.rs | 96 ++++++++++++++++++++++++++++++----- 1 file changed, 82 insertions(+), 14 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 8adb49fa5b..30ae064ed3 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1,6 +1,9 @@ //! Directives and errors relating to linking Hugrs. -use std::{collections::HashMap, fmt::Display}; +use std::{ + collections::{HashMap, VecDeque}, + fmt::Display, +}; use itertools::{Either, Itertools}; @@ -249,7 +252,6 @@ impl NodeLinkingDirective { #[derive(Clone, Debug, PartialEq, Eq)] pub struct NameLinkingPolicy { // TODO: consider pub-funcs-to-add? (With others, optionally filtered by callgraph, made private) - // copy_private_funcs: bool, // TODO: allow filtering private funcs to only those reachable in callgraph /// How to handle cases where the same (public) name is present in both /// inserted and target Hugr but with different signatures. sig_conflict: SignatureConflictHandling, @@ -257,6 +259,9 @@ pub struct NameLinkingPolicy { /// with the same name and signature. // TODO consider Set of names where to prefer new? Or optional map from name? multi_impls: MultipleImplHandling, + /// Whether to filter private functions down to only those required (reached from public) + /// false means just to copy all private functions. + filter_private: bool, // TODO Renames to apply to public functions in the inserted Hugr. These take effect // before [error_on_conflicting_sig] or [take_existing_and_new_impls]. // rename_map: HashMap @@ -343,6 +348,7 @@ impl NameLinkingPolicy { Self { multi_impls, sig_conflict: SignatureConflictHandling::ErrorDontInsert, + filter_private: false, } } @@ -353,6 +359,7 @@ impl NameLinkingPolicy { Self { multi_impls: MultipleImplHandling::UseBoth, sig_conflict: SignatureConflictHandling::UseBoth, + filter_private: false, } } @@ -377,6 +384,60 @@ impl NameLinkingPolicy { &self, target: &T, source: &S, + ) -> Result, NameLinkingError> { + let pub_funcs = self.to_node_linking_public(target, source)?; + if self.filter_private { + let mut res = HashMap::new(); + let mut queue = VecDeque::from_iter(pub_funcs.keys().copied()); + while let Some(n) = queue.pop_front() { + if res.contains_key(&n) { + continue; + } + let nld = match link_sig(source, n).unwrap() { + LinkSig::Private => NodeLinkingDirective::add(), + LinkSig::Public { .. } => pub_funcs[&n].clone(), + }; + + if matches!(nld, NodeLinkingDirective::Add { .. }) { + // Would be great to use a CallGraph here but that is in hugr-passes + // and we cannot have a cyclic dependency between crates !! + // Also call-graph does not deal with (potentially-module-level) Constants. + for d in source.descendants(n) { + if matches!( + source.get_optype(d), + OpType::LoadConstant(_) | OpType::LoadFunction(_) | OpType::Call(_) + ) { + if let Some(static_) = source.static_source(d) { + if source.get_parent(static_) == Some(source.module_root()) { + queue.push_back(static_) + } + } + } + } + } + + res.insert(n, nld); + } + Ok(res) + } else { + let mut res = pub_funcs; + for n in source.children(source.module_root()) { + if matches!(link_sig(source, n), Some(LinkSig::Private)) { + let prev = res.insert(n, NodeLinkingDirective::add()); + debug_assert_eq!(prev, None); + } + } + Ok(res) + } + } + + /// Builds an explicit map of [NodeLinkingDirective]s that implements this policy for a given + /// source (inserted) and target (inserted-into) Hugr, for [Visibility::Public] functions only. + #[allow(clippy::type_complexity)] + fn to_node_linking_public( + &self, + target: &T, + source: &S, ) -> Result, NameLinkingError> { let existing = gather_existing(target); let mut res = NodeLinkingDirectives::new(); @@ -384,11 +445,11 @@ impl NameLinkingPolicy { let NameLinkingPolicy { sig_conflict, multi_impls, + filter_private: _, } = self; for n in source.children(source.module_root()) { let dirv = match link_sig(source, n) { - None => continue, - Some(LinkSig::Private) => NodeLinkingDirective::add(), + None | Some(LinkSig::Private) => continue, Some(LinkSig::Public { name, is_defn, sig }) => { if let Some((ex_ns, ex_sig)) = existing.get(name) { match *sig_conflict { @@ -942,6 +1003,7 @@ mod test { let pol = NameLinkingPolicy { sig_conflict, multi_impls, + filter_private: false, }; let mut target2 = target.clone(); @@ -1014,11 +1076,15 @@ mod test { } #[rstest] - #[case(MultipleImplHandling::UseNew, vec![11])] - #[case(MultipleImplHandling::UseExisting, vec![5])] - #[case(MultipleImplHandling::UseBoth, vec![5, 11])] - #[case(MultipleImplHandling::ErrorDontInsert, vec![])] - fn impl_conflict(#[case] multi_impls: MultipleImplHandling, #[case] expected: Vec) { + #[case(MultipleImplHandling::UseNew, vec![11], vec![5, 11])] // Existing constant is not removed + #[case(MultipleImplHandling::UseExisting, vec![5], vec![5])] + #[case(MultipleImplHandling::UseBoth, vec![5, 11], vec![5,11])] + #[case(MultipleImplHandling::ErrorDontInsert, vec![], vec![])] + fn impl_conflict( + #[case] multi_impls: MultipleImplHandling, + #[case] expect_used: Vec, + #[case] expect_exist: Vec, + ) { fn build_hugr(cst: u64) -> Hugr { let mut mb = ModuleBuilder::new(); let cst = mb.add_constant(Value::from(ConstUsize::new(cst))); @@ -1033,8 +1099,11 @@ mod test { let mut host = backup.clone(); let inserted = build_hugr(11); - let mut pol = NameLinkingPolicy::keep_both_invalid(); - pol.on_multiple_impls(multi_impls); + let pol = NameLinkingPolicy { + sig_conflict: SignatureConflictHandling::ErrorDontInsert, + multi_impls, + filter_private: true, + }; let res = host.link_module(inserted, &pol); if multi_impls == MultipleImplHandling::ErrorDontInsert { assert!(matches!(res, Err(NameLinkingError::MultipleImpls(n, _, _)) if n == "foo")); @@ -1063,14 +1132,13 @@ mod test { .unwrap() }) .collect_vec(); - assert_eq!(func_consts, expected); - // At the moment we copy all the constants regardless of whether they are used: + assert_eq!(func_consts, expect_used); let all_consts: Vec<_> = host .children(host.module_root()) .filter_map(|ch| host.get_optype(ch).as_const()) .map(|c| c.get_custom_value::().unwrap().value()) .sorted() .collect(); - assert_eq!(all_consts, [5, 11]); + assert_eq!(all_consts, expect_exist); } } From 7bfd2ded4c465ee46aa7200f1814081f0684eecf Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 25 Aug 2025 17:29:33 +0100 Subject: [PATCH 02/52] Factor out find_reachable with callback --- hugr-core/src/hugr/linking.rs | 74 ++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 32 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 30ae064ed3..2e86252797 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -387,38 +387,9 @@ impl NameLinkingPolicy { ) -> Result, NameLinkingError> { let pub_funcs = self.to_node_linking_public(target, source)?; if self.filter_private { - let mut res = HashMap::new(); - let mut queue = VecDeque::from_iter(pub_funcs.keys().copied()); - while let Some(n) = queue.pop_front() { - if res.contains_key(&n) { - continue; - } - let nld = match link_sig(source, n).unwrap() { - LinkSig::Private => NodeLinkingDirective::add(), - LinkSig::Public { .. } => pub_funcs[&n].clone(), - }; - - if matches!(nld, NodeLinkingDirective::Add { .. }) { - // Would be great to use a CallGraph here but that is in hugr-passes - // and we cannot have a cyclic dependency between crates !! - // Also call-graph does not deal with (potentially-module-level) Constants. - for d in source.descendants(n) { - if matches!( - source.get_optype(d), - OpType::LoadConstant(_) | OpType::LoadFunction(_) | OpType::Call(_) - ) { - if let Some(static_) = source.static_source(d) { - if source.get_parent(static_) == Some(source.module_root()) { - queue.push_back(static_) - } - } - } - } - } - - res.insert(n, nld); - } - Ok(res) + find_reachable(source, pub_funcs.keys().copied(), |n| { + Ok(pub_funcs[&n].clone()) + }) } else { let mut res = pub_funcs; for n in source.children(source.module_root()) { @@ -511,6 +482,45 @@ fn directive( }) } +fn find_reachable( + h: &H, + starts: impl IntoIterator, + pub_callback: impl Fn(H::Node) -> Result, E>, +) -> Result, E> { + let mut queue = VecDeque::from_iter(starts); + let mut res = HashMap::new(); + while let Some(n) = queue.pop_front() { + if res.contains_key(&n) { + continue; + } + let nld = match link_sig(h, n).unwrap() { + LinkSig::Private => NodeLinkingDirective::add(), + LinkSig::Public { .. } => pub_callback(n)?, + }; + + if matches!(nld, NodeLinkingDirective::Add { .. }) { + // Would be great to use a CallGraph here but that is in hugr-passes + // and we cannot have a cyclic dependency between crates !! + // Also call-graph does not deal with (potentially-module-level) Constants. + for d in h.descendants(n) { + if matches!( + h.get_optype(d), + OpType::LoadConstant(_) | OpType::LoadFunction(_) | OpType::Call(_) + ) { + if let Some(static_) = h.static_source(d) { + if h.get_parent(static_) == Some(h.module_root()) { + queue.push_back(static_) + } + } + } + } + } + + res.insert(n, nld); + } + Ok(res) +} + type PubFuncs<'a, N> = (Either)>, &'a PolyFuncType); enum LinkSig<'a> { From 4b65e70810a846212800793dbc41a8c488e1cf8a Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 25 Aug 2025 17:40:55 +0100 Subject: [PATCH 03/52] Add insert_link_hugr - TODO errors, tests --- hugr-core/src/hugr/linking.rs | 91 +++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 2e86252797..90c1e57a07 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -2,6 +2,7 @@ use std::{ collections::{HashMap, VecDeque}, + convert::Infallible, fmt::Display, }; @@ -169,6 +170,88 @@ pub trait HugrLinking: HugrMut { .add_view_link_nodes(None, other, per_node) .expect("NodeLinkingPolicy was constructed to avoid any error")) } + + /// Inserts the entrypoint-subtree of another Hugr into this one, + /// including copying any private functions it calls and using linking + /// to resolve any public functions. + /// + /// `parent` is the parent node under which to insert the entrypoint. + /// `allow_new_pub` specifies whether new [Visibility::Public] [FuncDefn]s may be copied from `other` + /// + /// # Errors + /// + /// If other's entrypoint is its module-root (see [Self::link_hugr]) + /// + /// If other's entrypoint calls (perhaps transitively) the function containing said entrypoint + /// (an exception is made if the called+containing function is public and there is already + /// an equivalent in `self` - to which the call is redirected). + /// + /// If other's entrypoint calls a public function in `other` which does not yet exist in `self` + /// and `allow_new_pub` is false + fn insert_link_hugr( + &mut self, + parent: Self::Node, + other: Hugr, + // ALAN TODO: we should distinguish between "new names" (which we are more likely to want to avoid) + // and "new code" (i.e. new FuncDefns replacing existing FuncDecls) - of course we could convert + // these to call the existing *decls* + allow_new_pub: bool, + ) -> Result, String> { + let entrypoint_func = { + let mut n = other.entrypoint(); + loop { + let p = other.get_parent(n).ok_or("Entrypoint is module root")?; + if p == other.module_root() { + break n; + }; + n = p; + } + }; + //if entrypoint_func == other.entrypoint() { // Do we need to check this? Ok if parent is self.module_root() ?? + // return Err(format!("Entrypoint is a top-level function")) + //} + let pub_funcs = NameLinkingPolicy { + sig_conflict: SignatureConflictHandling::UseBoth, // We may not *need* both, so don't error + multi_impls: MultipleImplHandling::UseExisting, + filter_private: true, // not used by to_node_linking_public + } + .to_node_linking_public(self, &other) + .map_err(|e| e.to_string())?; + + let per_node = if allow_new_pub { + let Ok(dirvs) = find_reachable(&other, [other.entrypoint()], |n| { + Ok::<_, Infallible>(pub_funcs[&n].clone()) + }); + dirvs + } else { + let dirvs = find_reachable(&other, [other.entrypoint()], |n| match &pub_funcs[&n] { + NodeLinkingDirective::Add { .. } => Err(n), + ue @ NodeLinkingDirective::UseExisting(_) => Ok(ue.clone()), + }) + .map_err(|n| { + format!("Node {n} is a (new-to-self) public function callable from the entrypoint") + })?; + debug_assert!(dirvs.iter().all(|(k, v)| { + !matches!(link_sig(&other, *k), Some(LinkSig::Public { .. })) + || matches!(v, NodeLinkingDirective::UseExisting(_)) + })); + dirvs + }; + + if entrypoint_func != other.entrypoint() + && matches!( + per_node.get(&entrypoint_func), + Some(NodeLinkingDirective::Add { .. }) + ) + { + return Err(format!( + "Would need to add function {entrypoint_func} which contains the entrypoint, avoiding double-copy" + )); + } + Ok(self + .add_hugr_link_nodes(Some(parent), other, per_node) + .expect("NodeLinkingPolicy was constructed to avoid any error")) + } } impl HugrLinking for T {} @@ -493,9 +576,11 @@ fn find_reachable( if res.contains_key(&n) { continue; } - let nld = match link_sig(h, n).unwrap() { - LinkSig::Private => NodeLinkingDirective::add(), - LinkSig::Public { .. } => pub_callback(n)?, + debug_assert!(h.get_parent(n) == Some(h.module_root()) || n == h.entrypoint()); + let nld = match link_sig(h, n) { + // This handles the case where `starts` includes the entrypoint. + None | Some(LinkSig::Private) => NodeLinkingDirective::add(), + Some(LinkSig::Public { .. }) => pub_callback(n)?, }; if matches!(nld, NodeLinkingDirective::Add { .. }) { From b1c223886d111277086262e42e5d58d12191c1f9 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 25 Aug 2025 21:53:00 +0100 Subject: [PATCH 04/52] Rewrite with more options --- hugr-core/src/hugr/linking.rs | 116 +++++++++++++++++++++------------- 1 file changed, 71 insertions(+), 45 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 90c1e57a07..02c59b12e0 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -2,7 +2,6 @@ use std::{ collections::{HashMap, VecDeque}, - convert::Infallible, fmt::Display, }; @@ -12,7 +11,7 @@ use crate::{ Hugr, HugrView, Node, core::HugrNode, hugr::{HugrMut, hugrmut::InsertedForest, internal::HugrMutInternals}, - ops::OpType, + ops::{FuncDecl, OpType}, types::PolyFuncType, }; @@ -191,12 +190,18 @@ pub trait HugrLinking: HugrMut { fn insert_link_hugr( &mut self, parent: Self::Node, - other: Hugr, - // ALAN TODO: we should distinguish between "new names" (which we are more likely to want to avoid) - // and "new code" (i.e. new FuncDefns replacing existing FuncDecls) - of course we could convert - // these to call the existing *decls* - allow_new_pub: bool, + mut other: Hugr, + allow_new_names: Option, + // allow_new = false, SigConflict::Error = error on new name, or same name w/ different sig + // allow_new = false, SigConflict::UseBoth = error on new name, allow same name w/different sig - WEIRD + // allow_new = true, SigConflict::Error = allow new name, but error if different sig + // allow_new = true, SigConflict::UseBoth = allow new name, even if different sig + allow_new_impls: Option, ) -> Result, String> { + // Could extend NameLinkingPolicy with option on both? + // allow_new_names = None -> only adds new impls of existing names - prob. needs reachability + // allow_new_impls = None -> converts all incoming impls to decls + // but, makes no sense for both to be None, as does nothing... let entrypoint_func = { let mut n = other.entrypoint(); loop { @@ -210,43 +215,64 @@ pub trait HugrLinking: HugrMut { //if entrypoint_func == other.entrypoint() { // Do we need to check this? Ok if parent is self.module_root() ?? // return Err(format!("Entrypoint is a top-level function")) //} - let pub_funcs = NameLinkingPolicy { - sig_conflict: SignatureConflictHandling::UseBoth, // We may not *need* both, so don't error - multi_impls: MultipleImplHandling::UseExisting, - filter_private: true, // not used by to_node_linking_public - } - .to_node_linking_public(self, &other) - .map_err(|e| e.to_string())?; - - let per_node = if allow_new_pub { - let Ok(dirvs) = find_reachable(&other, [other.entrypoint()], |n| { - Ok::<_, Infallible>(pub_funcs[&n].clone()) - }); - dirvs - } else { - let dirvs = find_reachable(&other, [other.entrypoint()], |n| match &pub_funcs[&n] { - NodeLinkingDirective::Add { .. } => Err(n), - ue @ NodeLinkingDirective::UseExisting(_) => Ok(ue.clone()), - }) - .map_err(|n| { - format!("Node {n} is a (new-to-self) public function callable from the entrypoint") - })?; - debug_assert!(dirvs.iter().all(|(k, v)| { - !matches!(link_sig(&other, *k), Some(LinkSig::Public { .. })) - || matches!(v, NodeLinkingDirective::UseExisting(_)) - })); - dirvs - }; - - if entrypoint_func != other.entrypoint() - && matches!( - per_node.get(&entrypoint_func), - Some(NodeLinkingDirective::Add { .. }) - ) - { - return Err(format!( - "Would need to add function {entrypoint_func} which contains the entrypoint, avoiding double-copy" - )); + let existing = gather_existing(self); + let mut defns_to_make_decls = Vec::new(); + let per_node = find_reachable(&other, [other.entrypoint()], |n| { + if n == other.entrypoint() { + Ok(NodeLinkingDirective::add()) + } else if n == entrypoint_func { + // ALAN TODO If there's a matching function in `self` then this is OK + Err(format!( + "Adding function {entrypoint_func} would double-copy the entrypoint-subtree" + )) + } else { + Ok(match link_sig(&other, n).unwrap() { + LinkSig::Private => NodeLinkingDirective::add(), + LinkSig::Public { name, is_defn, sig } => match existing.get(name) { + Some((defn_or_decl, ex_sig)) => { + if sig == *ex_sig { + match allow_new_impls.as_ref() { + None => NodeLinkingDirective::UseExisting( + *defn_or_decl.as_ref().left_or_else(|(n, _)| n), + ), + Some(multi_impls) => { + directive(name, n, is_defn, defn_or_decl, multi_impls) + .map_err(|e| e.to_string())? + } + } + } else { + match allow_new_names { + None | Some(SignatureConflictHandling::ErrorDontInsert) => { + return Err(format!( + "Conflicting signature for function {name} callable from entrypoint" + )); + } + Some(SignatureConflictHandling::UseBoth) => { + NodeLinkingDirective::add() + } + } + } + } + None => { + if allow_new_names.is_none() { + return Err(format!("New name {name}")); + } + if is_defn && allow_new_impls.is_none() { + defns_to_make_decls.push(n) + } + NodeLinkingDirective::add() + } + }, + }) + } + })?; + for n in defns_to_make_decls { + while let Some(c) = other.first_child(n) { + other.remove_subtree(c) + } + let o = other.optype_mut(n); + let OpType::FuncDefn(fd) = o else { panic!() }; + *o = FuncDecl::new(fd.func_name().clone(), fd.signature().clone()).into(); } Ok(self .add_hugr_link_nodes(Some(parent), other, per_node) @@ -568,7 +594,7 @@ fn directive( fn find_reachable( h: &H, starts: impl IntoIterator, - pub_callback: impl Fn(H::Node) -> Result, E>, + mut pub_callback: impl FnMut(H::Node) -> Result, E>, ) -> Result, E> { let mut queue = VecDeque::from_iter(starts); let mut res = HashMap::new(); From bac071308d92c573069a1a584bc695930187c57c Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 25 Aug 2025 21:59:24 +0100 Subject: [PATCH 05/52] Refactor following previous (remove to_node_linking_public), docs --- hugr-core/src/hugr/linking.rs | 84 +++++++++++++++++------------------ 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 02c59b12e0..98fdaad462 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -175,33 +175,52 @@ pub trait HugrLinking: HugrMut { /// to resolve any public functions. /// /// `parent` is the parent node under which to insert the entrypoint. - /// `allow_new_pub` specifies whether new [Visibility::Public] [FuncDefn]s may be copied from `other` + /// + /// If `allow_new_names` is `None`, then any FuncDefn/FuncDecl in `other` and reachable + /// from the entrypoint, must have a matching name and signature with one in `self`; + /// if `Some(s)` then `s` details how to handle the case where the name matches but the + /// signature does not. + // Note the use of an Option here, rather than a bool and a separate SignatureConflictHandling, + // rules out raiseing an error on an entirely new name but *accepting* a new+conflicting + // signature for an existing name; the latter is an invalid Hugr and would have to result in + // one of the functions being renamed *OR MADE PRIVATE*, so the latter is a possible case. + // TODO: consider extending(+renaming?) SignatureConflictHandling with new variants + // (and eliding the option here): + // * RejectNewNames (i.e. the none here; more aggressive than any existing enum variant) + // * MakeAllNewNamesPrivate + // * MakeNewConflictsPrivate (i.e. when name exists but with different sig) + /// + /// If `allow_new_impls` is `None`, then any required [`FuncDefn`] in `other` is treated + /// as a [`FuncDecl`] (either being linked with a function existing in `self`, or added if + /// `allow_new_names` is `Some`). + /// If `allow_new_impls` is Some, then that details how such a `FuncDefn` in `other` + /// may (be) override(/den) by another in `self`. /// /// # Errors /// - /// If other's entrypoint is its module-root (see [Self::link_hugr]) + /// If other's entrypoint is its module-root (recommend using [Self::link_module] instead) /// /// If other's entrypoint calls (perhaps transitively) the function containing said entrypoint /// (an exception is made if the called+containing function is public and there is already /// an equivalent in `self` - to which the call is redirected). /// - /// If other's entrypoint calls a public function in `other` which does not yet exist in `self` - /// and `allow_new_pub` is false + /// If other's entrypoint calls a public function in `other` which + /// * has a name or signature different to any in `self`, and `allow_new_names` is `None` + /// * has a name equal to that in `self`, but a different signature, and `allow_new_names` is + /// `Some(`[`SignatureConflictHandling::ErrorDontInsert`]`)` + /// + /// If other's entrypoint calls a public [`FuncDefn`] in `other` which has the same name + /// and signature as a public [`FuncDefn`] in `self` and `allow_new_impls` is + /// `Some(`[`MultipleImplHandling::ErrorDontInsert`]`)` + /// + /// [`FuncDefn`]: crate::ops::FuncDefn fn insert_link_hugr( &mut self, parent: Self::Node, mut other: Hugr, allow_new_names: Option, - // allow_new = false, SigConflict::Error = error on new name, or same name w/ different sig - // allow_new = false, SigConflict::UseBoth = error on new name, allow same name w/different sig - WEIRD - // allow_new = true, SigConflict::Error = allow new name, but error if different sig - // allow_new = true, SigConflict::UseBoth = allow new name, even if different sig allow_new_impls: Option, ) -> Result, String> { - // Could extend NameLinkingPolicy with option on both? - // allow_new_names = None -> only adds new impls of existing names - prob. needs reachability - // allow_new_impls = None -> converts all incoming impls to decls - // but, makes no sense for both to be None, as does nothing... let entrypoint_func = { let mut n = other.entrypoint(); loop { @@ -320,7 +339,6 @@ pub enum NodeLinkingDirective { /// to leave the newly-inserted node instead. (Typically, this `Vec` would contain /// at most one [FuncDefn], or perhaps-multiple, aliased, [FuncDecl]s.) /// - /// [FuncDecl]: crate::ops::FuncDecl /// [FuncDefn]: crate::ops::FuncDefn /// [EdgeKind::Const]: crate::types::EdgeKind::Const /// [EdgeKind::Function]: crate::types::EdgeKind::Function @@ -493,34 +511,10 @@ impl NameLinkingPolicy { &self, target: &T, source: &S, - ) -> Result, NameLinkingError> { - let pub_funcs = self.to_node_linking_public(target, source)?; - if self.filter_private { - find_reachable(source, pub_funcs.keys().copied(), |n| { - Ok(pub_funcs[&n].clone()) - }) - } else { - let mut res = pub_funcs; - for n in source.children(source.module_root()) { - if matches!(link_sig(source, n), Some(LinkSig::Private)) { - let prev = res.insert(n, NodeLinkingDirective::add()); - debug_assert_eq!(prev, None); - } - } - Ok(res) - } - } - - /// Builds an explicit map of [NodeLinkingDirective]s that implements this policy for a given - /// source (inserted) and target (inserted-into) Hugr, for [Visibility::Public] functions only. - #[allow(clippy::type_complexity)] - fn to_node_linking_public( - &self, - target: &T, - source: &S, ) -> Result, NameLinkingError> { let existing = gather_existing(target); - let mut res = NodeLinkingDirectives::new(); + // If filter_private, then this is just the public functions, otherwise all: + let mut dirvs = NodeLinkingDirectives::new(); let NameLinkingPolicy { sig_conflict, @@ -529,7 +523,7 @@ impl NameLinkingPolicy { } = self; for n in source.children(source.module_root()) { let dirv = match link_sig(source, n) { - None | Some(LinkSig::Private) => continue, + Some(LinkSig::Private) if !self.filter_private => NodeLinkingDirective::add(), Some(LinkSig::Public { name, is_defn, sig }) => { if let Some((ex_ns, ex_sig)) = existing.get(name) { match *sig_conflict { @@ -549,11 +543,15 @@ impl NameLinkingPolicy { NodeLinkingDirective::add() } } + _ => continue, }; - res.insert(n, dirv); + dirvs.insert(n, dirv); + } + if self.filter_private { + find_reachable(source, dirvs.keys().copied(), |n| Ok(dirvs[&n].clone())) + } else { + Ok(dirvs) } - - Ok(res) } } From 86bbe5b7720a28a687b9317ce1115daf23e7ed3f Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 5 Sep 2025 20:12:01 +0100 Subject: [PATCH 06/52] WIP notes --- hugr-core/src/hugr/linking.rs | 70 +++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 98fdaad462..62aa2fd77b 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -189,6 +189,76 @@ pub trait HugrLinking: HugrMut { // * RejectNewNames (i.e. the none here; more aggressive than any existing enum variant) // * MakeAllNewNamesPrivate // * MakeNewConflictsPrivate (i.e. when name exists but with different sig) + + // there is already SigConfHand::ErrorDontInsert,UseBoth. So... + // NoNewNames(skip_unused: bool, make_private_or_error) (also same name but different sig) + // NewNames(if new type - skip unused: bool, make_private_or_error_or_invalid) + + // skip_unreachable: bool, // can be new in this PR...no, maybe this is ALWAYS true for insert_link, and NEVER true for link? + // new_name: AllowOrHideOrError, // call this ConflictResolution, add this field and the Hide variant in this PR + // conflicting_sig: AllowOrHideOrError + // + // currently have MultiImplHandling::UseNew, ::UseExisting, ::UseBoth, ::Error + // instead: NewImplHandling::ConvertToDecls**, PreferExisting, OverrideExisting, Both(ConflictResolution) + // ** doesn't make sense for link_hugr...unless new_name == Allow and that's all we're doing? + + // enum NewMemberHandling { + // Allow // bool skip_unreachable makes sense only for insert_link...but assume true iff insert_link + // MakePrivate {skip_unreachable:bool} + // ErrorDontInsert {skip_unreachable:bool} + // } + + // implementation... + // * can build "what's there" in each Hugr + // * work out minimum FuncDefns to add: start with + // - for link_hugr, consider all public functions (defns and decls) + // if name is new -> bail if new_name==ErrorDontInsert, include if new_name==Allow, **skip if MakePrivate** + // if name exists and sig conflicts, as previous for conflicting_sig + // and sig is same -> include if at least one is decl or new_impls==OverrideExisting or Both(Allow) + // else, both are defns; error if Both(ErrorDontInsert) + // ** skip if Both(MakePrivate)** + // if new_impls==PreferExisting, need to record in NodeLinkingDirectives but NOT callgraph-start-points + // (insert_link_xxxx_by_node will mutate Hugr or ignore descendants according to xxxx) + // **skip if**s add rather than skip if skip_(private_)unreachable is false, but it may never be + // - for insert_link, just the entrypoint subtree + // that's probably a mutable NodeLinkingDirectives, but also use to + // walk the callgraph...for any node not in the dirvs + // - check as previous, EXCEPT + //. if name new and new_name==MakePrivate, or sig conflicts and conflicting_sig==MakePrivate, or an impl and Both(MakePrivate) + // then don't skip, add. + + // So...function from starting points (either all pub funcs or just entrypoint subtree) + // does above conditional skipping on MakePrivate but adding to Dfs otherwise + // then walks callgraph, doing above conditional including carrying on through MakePrivates :) + // results in NodeLinkingDirectives + // + // Ah, multiple meanings of skip_unreachable: + // * for link_hugr, applies to private functions (if false, include all of those) + // do we want to allow skipping unreachable new public names?? + // * for insert_link, applies to all funcs private and public...but, separately? + // Surely we should always skip private unreachable, as otherwise the entrypoint would have to be overridden by UseExisting (!) + // - So, could have 'skip_unreachable_new_public' (prob. wants to default to TRUE for insert_link and FALSE for link_hugr) + // `skip_existing_public` would basically be ConvertNewToDecls but we *could* have an extra mode + // where reachable public funcs are OverrideExisting but unreachable are PreferExisting + // - And, could have `skip_private` - def. wants to default to TRUE for insert_link, maybe *required* to be TRUE; + // but could be an option for link_hugr OR we could quietly change behaviour. + // - Sounds like `skip_unreachable` applies only to new, public, functions + // But really this belongs in NewMemberHandling for new_names and conflicting_sig + // --> NewMemberHandling::Allow(skip=true) is fine for insert_link, mildly odd for link_hugr + // (but as long as link_hugr has no ConvertNewToDecls, only mildly odd, not totally pointless) + // NewMemberHandling::Error(skip_unreachable=true) is new behaviour, similarly slightly odd + // (and skip_unreachable=false would be odd for insert_link) + // NewMemberHandling::MakePrivate doesn't need the flag, can be always true? + // Consider Add, AddIfReached, Error, ErrorIfReached, MakePrivate(IfReached) + // CallGraph is not needed only if new_names == Add/Error, ConflictingSig==Add/Error, impls=Prefer/Override/Both(Add/Error) + // link_hugr with ConvertToDecls...does it make ANY sense? + // new_names == Add, will add decls; AddIfReached==ErrorIfReached==MakePrivate(IfReached)==no-op; Error may error (so check); MakePrivateEvenIfNotReached might add private decls + // conflicting_sig, similarly + // so a lot of no-ops, but not inconsistent/impossible. + + // WHAT ABOUT CALLBACKS RATHER THAN ALL OF THESE ???? + + /// /// If `allow_new_impls` is `None`, then any required [`FuncDefn`] in `other` is treated /// as a [`FuncDecl`] (either being linked with a function existing in `self`, or added if From 531eda680c98421aef04be3c1623f37e47ff0c16 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 5 Sep 2025 21:15:34 +0100 Subject: [PATCH 07/52] WIP new variants NewFuncHandling, use NameLinkingPolicy in insert_link_hugr --- hugr-core/src/hugr/linking.rs | 55 ++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 6850609780..17e2cd3b1b 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -282,8 +282,7 @@ pub trait HugrLinking: HugrMut { &mut self, parent: Self::Node, mut other: Hugr, - allow_new_names: Option, - allow_new_impls: Option, + policy: &NameLinkingPolicy ) -> Result, String> { let entrypoint_func = { let mut n = other.entrypoint(); @@ -443,11 +442,14 @@ impl NodeLinkingDirective { #[derive(Clone, Debug, PartialEq, Eq)] pub struct NameLinkingPolicy { // TODO: consider pub-funcs-to-add? (With others, optionally filtered by callgraph, made private) + new_names: NewFuncHandling, // ALAN default to Add for link_module but AddIfReached for insert_link.... sig_conflict: NewFuncHandling, // TODO consider Set of names where to prefer new? Or optional map from name? multi_impls: MultipleImplHandling, /// Whether to filter private functions down to only those required (reached from public) /// false means just to copy all private functions. + // ALAN remove this? it's just to preserve behaviour of old link_module that was + // (explicitly) never guaranteed in the docs. filter_private: bool, // TODO Renames to apply to public functions in the inserted Hugr. These take effect // before [error_on_conflicting_sig] or [take_existing_and_new_impls]. @@ -465,10 +467,21 @@ pub struct NameLinkingPolicy { pub enum NewFuncHandling { /// Do not link the Hugrs together; fail with a [NameLinkingError] instead. RaiseError, + /// If the new function is reachable, then error; otherwise, just skip it. + ErrorIfReached, /// Add the new function alongside the existing one in the target Hugr, /// preserving (separately) uses of both. (The Hugr will be invalid because /// of [duplicate names](crate::hugr::ValidationError::DuplicateExport).) Add, + /// If the new function is reachable, then add it (as per [Self::Add]); + /// otherwise, skip it. + AddIfReached, + /// If the new function is reachable, then add it but make it [Private]; + /// otherwise, just skip it. (This always avoids making the Hugr invalid.) + /// + /// [Private]: crate::Visibility::Private + // ALAN note this really is MakePrivateIfReached, but I don't see much point in the other. + MakePrivate } /// What to do when both target and inserted Hugr @@ -590,7 +603,12 @@ impl NameLinkingPolicy { Some(LinkSig::Public { name, is_defn, sig }) => { if let Some((ex_ns, ex_sig)) = existing.get(name) { match *sig_conflict { - _ if sig == *ex_sig => directive(name, n, is_defn, ex_ns, multi_impls)?, + _ if sig == *ex_sig => { + match directive(name, n, is_defn, ex_ns, multi_impls)? { + Some(dirv) => dirv, + None => continue + } + } NewFuncHandling::RaiseError => { return Err(NameLinkingError::SignatureConflict { name: name.clone(), @@ -630,8 +648,9 @@ fn directive( new_defn: bool, ex_ns: &Either)>, multi_impls: &MultipleImplHandling, -) -> Result, NameLinkingError> { - Ok(match (new_defn, ex_ns) { + reached: bool +) -> Result>, NameLinkingError> { + Ok(Some(match (new_defn, ex_ns) { (false, Either::Right(_)) => NodeLinkingDirective::add(), // another alias (false, Either::Left(defn)) => NodeLinkingDirective::UseExisting(*defn), // resolve decl (true, Either::Right((decl, decls))) => { @@ -640,16 +659,24 @@ fn directive( (true, &Either::Left(defn)) => match multi_impls { MultipleImplHandling::UseExisting => NodeLinkingDirective::UseExisting(defn), MultipleImplHandling::UseNew => NodeLinkingDirective::replace([defn]), - MultipleImplHandling::NewFunc(NewFuncHandling::RaiseError) => { - return Err(NameLinkingError::MultipleImpls( - name.to_owned(), - new_n, - defn, - )); - } - MultipleImplHandling::NewFunc(NewFuncHandling::Add) => NodeLinkingDirective::add(), + MultipleImplHandling::NewFunc(nf) => { + if matches!(nf, NewFuncHandling::ErrorIfReached | NewFuncHandling::AddIfReached) && !reached { + return Ok(None) + } + match nf { + NewFuncHandling::RaiseError | NewFuncHandling::ErrorIfReached => { + return Err(NameLinkingError::MultipleImpls( + name.to_owned(), + new_n, + defn, + )); + } + NewFuncHandling::Add | NewFuncHandling::AddIfReached => NodeLinkingDirective::add(), + NewFuncHandling::MakePrivate => todo!("ALAN") + } + }, }, - }) + })) } fn find_reachable( From 123a90c8d318f3aa3a11b6bd4129e7127c5023cf Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 12 Sep 2025 18:49:40 +0100 Subject: [PATCH 08/52] Add call-graph to hugr-core (non-exhaustive) --- hugr-core/src/call_graph.rs | 106 ++++++++++++++++++++++++++++++++++++ hugr-core/src/lib.rs | 1 + 2 files changed, 107 insertions(+) create mode 100644 hugr-core/src/call_graph.rs diff --git a/hugr-core/src/call_graph.rs b/hugr-core/src/call_graph.rs new file mode 100644 index 0000000000..8ffc58469f --- /dev/null +++ b/hugr-core/src/call_graph.rs @@ -0,0 +1,106 @@ +//! Data structure for call graphs of a Hugr +use std::collections::HashMap; + +use crate::{HugrView, Node, core::HugrNode, ops::OpType}; +use petgraph::Graph; + +/// Weight for an edge in a [`CallGraph`] +#[derive(Clone, Debug, PartialEq, Eq)] +#[non_exhaustive] +pub enum CallGraphEdge { + /// Edge corresponds to a [Call](OpType::Call) node (specified) in the Hugr + Call(N), + /// Edge corresponds to a [`LoadFunction`](OpType::LoadFunction) node (specified) in the Hugr + LoadFunction(N), +} + +/// Weight for a petgraph-node in a [`CallGraph`] +pub enum CallGraphNode { + /// petgraph-node corresponds to a [`FuncDecl`](OpType::FuncDecl) node (specified) in the Hugr + FuncDecl(N), + /// petgraph-node corresponds to a [`FuncDefn`](OpType::FuncDefn) node (specified) in the Hugr + FuncDefn(N), + /// petgraph-node corresponds to the root node of the hugr, that is not + /// a [`FuncDefn`](OpType::FuncDefn). Note that it will not be a [Module](OpType::Module) + /// either, as such a node could not have outgoing edges, so is not represented in the petgraph. + NonFuncRoot, +} + +/// Details the [`Call`]s and [`LoadFunction`]s in a Hugr. +/// +/// Each node in the `CallGraph` corresponds to a [`FuncDefn`] or [`FuncDecl`] in the Hugr; +/// each edge corresponds to a [`Call`]/[`LoadFunction`] of the edge's target, contained in +/// the edge's source. +/// +/// For Hugrs whose entrypoint is neither a [Module](OpType::Module) nor a [`FuncDefn`], the +/// call graph will have an additional [`CallGraphNode::NonFuncRoot`] corresponding to the Hugr's +/// entrypoint, with no incoming edges. +/// +/// [`Call`]: OpType::Call +/// [`FuncDecl`]: OpType::FuncDecl +/// [`FuncDefn`]: OpType::FuncDefn +/// [`LoadFunction`]: OpType::LoadFunction +pub struct CallGraph { + g: Graph, CallGraphEdge>, + node_to_g: HashMap>, +} + +impl CallGraph { + /// Makes a new `CallGraph` for a Hugr. + pub fn new(hugr: &impl HugrView) -> Self { + let mut g = Graph::default(); + let mut node_to_g = hugr + .children(hugr.module_root()) + .filter_map(|n| { + let weight = match hugr.get_optype(n) { + OpType::FuncDecl(_) => CallGraphNode::FuncDecl(n), + OpType::FuncDefn(_) => CallGraphNode::FuncDefn(n), + _ => return None, + }; + Some((n, g.add_node(weight))) + }) + .collect::>(); + if !hugr.entrypoint_optype().is_module() && !node_to_g.contains_key(&hugr.entrypoint()) { + node_to_g.insert(hugr.entrypoint(), g.add_node(CallGraphNode::NonFuncRoot)); + } + for (func, cg_node) in &node_to_g { + traverse(hugr, *cg_node, *func, &mut g, &node_to_g); + } + fn traverse( + h: &impl HugrView, + enclosing_func: petgraph::graph::NodeIndex, + node: N, // Nonstrict-descendant of `enclosing_func`` + g: &mut Graph, CallGraphEdge>, + node_to_g: &HashMap>, + ) { + for ch in h.children(node) { + if h.get_optype(ch).is_func_defn() { + continue; + } + traverse(h, enclosing_func, ch, g, node_to_g); + let weight = match h.get_optype(ch) { + OpType::Call(_) => CallGraphEdge::Call(ch), + OpType::LoadFunction(_) => CallGraphEdge::LoadFunction(ch), + _ => continue, + }; + if let Some(target) = h.static_source(ch) { + g.add_edge(enclosing_func, *node_to_g.get(&target).unwrap(), weight); + } + } + } + CallGraph { g, node_to_g } + } + + /// Allows access to the petgraph + #[must_use] + pub fn graph(&self) -> &Graph, CallGraphEdge> { + &self.g + } + + /// Convert a Hugr [Node] into a petgraph node index. + /// Result will be `None` if `n` is not a [`FuncDefn`](OpType::FuncDefn), + /// [`FuncDecl`](OpType::FuncDecl) or the [HugrView::entrypoint]. + pub fn node_index(&self, n: N) -> Option> { + self.node_to_g.get(&n).copied() + } +} diff --git a/hugr-core/src/lib.rs b/hugr-core/src/lib.rs index 862b8dee8a..1a471a822e 100644 --- a/hugr-core/src/lib.rs +++ b/hugr-core/src/lib.rs @@ -10,6 +10,7 @@ #![cfg_attr(coverage_nightly, feature(coverage_attribute))] pub mod builder; +pub mod call_graph; pub mod core; pub mod envelope; pub mod export; From 1d83e12a0a07ffee0a61f0da9db7105f2c6f65d0 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sat, 13 Sep 2025 14:51:20 +0100 Subject: [PATCH 09/52] WIP fn process --- hugr-core/src/hugr/linking.rs | 134 +++++++++++++++++++++++++++++----- 1 file changed, 116 insertions(+), 18 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 17e2cd3b1b..07cc7b08e1 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -9,6 +9,7 @@ use itertools::{Either, Itertools}; use crate::{ Hugr, HugrView, Node, + call_graph::CallGraph, core::HugrNode, hugr::{HugrMut, hugrmut::InsertedForest, internal::HugrMutInternals}, ops::{FuncDecl, OpType}, @@ -191,7 +192,7 @@ pub trait HugrLinking: HugrMut { // skip_unreachable: bool, // can be new in this PR...no, maybe this is ALWAYS true for insert_link, and NEVER true for link? // new_name: AllowOrHideOrError, // call this ConflictResolution, add this field and the Hide variant in this PR // conflicting_sig: AllowOrHideOrError - // + // // currently have MultiImplHandling::UseNew, ::UseExisting, ::UseBoth, ::Error // instead: NewImplHandling::ConvertToDecls**, PreferExisting, OverrideExisting, Both(ConflictResolution) // ** doesn't make sense for link_hugr...unless new_name == Allow and that's all we're doing? @@ -220,7 +221,7 @@ pub trait HugrLinking: HugrMut { // - check as previous, EXCEPT //. if name new and new_name==MakePrivate, or sig conflicts and conflicting_sig==MakePrivate, or an impl and Both(MakePrivate) // then don't skip, add. - + // So...function from starting points (either all pub funcs or just entrypoint subtree) // does above conditional skipping on MakePrivate but adding to Dfs otherwise // then walks callgraph, doing above conditional including carrying on through MakePrivates :) @@ -252,7 +253,6 @@ pub trait HugrLinking: HugrMut { // WHAT ABOUT CALLBACKS RATHER THAN ALL OF THESE ???? - /// /// If `allow_new_impls` is `None`, then any required [`FuncDefn`] in `other` is treated /// as a [`FuncDecl`] (either being linked with a function existing in `self`, or added if @@ -282,7 +282,7 @@ pub trait HugrLinking: HugrMut { &mut self, parent: Self::Node, mut other: Hugr, - policy: &NameLinkingPolicy + policy: &NameLinkingPolicy, ) -> Result, String> { let entrypoint_func = { let mut n = other.entrypoint(); @@ -329,9 +329,7 @@ pub trait HugrLinking: HugrMut { "Conflicting signature for function {name} callable from entrypoint" )); } - Some(NewFuncHandling::Add) => { - NodeLinkingDirective::add() - } + Some(NewFuncHandling::Add) => NodeLinkingDirective::add(), } } } @@ -478,10 +476,10 @@ pub enum NewFuncHandling { AddIfReached, /// If the new function is reachable, then add it but make it [Private]; /// otherwise, just skip it. (This always avoids making the Hugr invalid.) - /// + /// /// [Private]: crate::Visibility::Private // ALAN note this really is MakePrivateIfReached, but I don't see much point in the other. - MakePrivate + MakePrivate, } /// What to do when both target and inserted Hugr @@ -531,6 +529,8 @@ pub enum NameLinkingError { tgt_node: TN, tgt_sig: Box, }, + #[error("Node {src_node} adds new name {name} which was specified to be an error")] + NoNewNames { name: String, src_node: SN }, /// A [Visibility::Public] function in the source, whose body is being added /// to the target, contained the entrypoint (which needs to be added /// in a different place). @@ -548,6 +548,7 @@ impl NameLinkingPolicy { /// [FuncDefn]: crate::ops::FuncDefn pub fn err_on_conflict(multi_impls: impl Into) -> Self { Self { + new_names: NewFuncHandling::Add, multi_impls: multi_impls.into(), sig_conflict: NewFuncHandling::RaiseError, filter_private: false, @@ -559,9 +560,10 @@ impl NameLinkingPolicy { /// a (potentially-invalid) Hugr is always produced. pub fn keep_both_invalid() -> Self { Self { + new_names: NewFuncHandling::Add, multi_impls: MultipleImplHandling::NewFunc(NewFuncHandling::Add), sig_conflict: NewFuncHandling::Add, - filter_private: false + filter_private: false, } } @@ -588,11 +590,101 @@ impl NameLinkingPolicy { target: &T, source: &S, ) -> Result, NameLinkingError> { + self.to_node_linking_for(target, source, false) + } + + // TODO return Result ? Does NLD::add mean, traverse, but NLD::use_ex mean, don't traverse? + // TODO(2): add NodeLinkingDirective::MakePrivate?? What about ConvertToDecl ?? + fn process( + &self, + existing: &HashMap<&String, PubFuncs>, + sn: SN, + new: LinkSig, + reached: bool, + ) -> Result> { + let (nfh, err) = match new { + LinkSig::Private => return Ok(reached), + LinkSig::Public { + name, + is_defn: new_is_defn, + sig: new_sig, + } => match existing.get(name) { + None => ( + self.new_names, + NameLinkingError::NoNewNames { + name: name.clone(), + src_node: sn, + }, + ), + Some((existing, ex_sig)) => match (*ex_sig == new_sig, existing) { + (false, ex) => ( + self.sig_conflict, + NameLinkingError::SignatureConflict { + name: name.clone(), + src_node: sn, + src_sig: new_sig.clone().into(), + tgt_node: *ex.as_ref().left_or_else(|(n, _)| n), + tgt_sig: (*ex_sig).clone().into(), + }, + ), + (true, Either::Left(n)) => match (new_is_defn, self.multi_impls) { + (false, _) => return Ok(false), // but add to dirvs + (true, MultipleImplHandling::NewFunc(nfh)) => { + (nfh, NameLinkingError::MultipleImpls(name.clone(), sn, *n)) + } + (true, MultipleImplHandling::UseExisting) => return Ok(false), // do not traverse into new body, ignore. TODO need to record somehow that we must NodeLinkingDirective::UseExisting. + (true, MultipleImplHandling::UseNew) => return Ok(true), + }, + (true, Either::Right((n, _ns))) => return Ok(true), //if decl, just add to dirvs + }, + }, + }; + match nfh { + NewFuncHandling::RaiseError => Err(err), + NewFuncHandling::ErrorIfReached if reached => Err(err), + NewFuncHandling::Add => Ok(true), + NewFuncHandling::AddIfReached | NewFuncHandling::MakePrivate if reached => Ok(true), // TODO and record MakePrivate + _ => Ok(false), + } + } + + #[allow(clippy::type_complexity)] + pub fn to_node_linking_for( + &self, + target: &T, + source: &S, + use_entrypoint: bool, + ) -> Result, NameLinkingError> { + let mut source_funcs = source_funcs.into_iter(); let existing = gather_existing(target); - // If filter_private, then this is just the public functions, otherwise all: + let cg = CallGraph::new(&source); + // Can't use petgraph Dfs as we need to avoid traversing through some nodes, + // and we need to maintain our own `visited` map anyway + let mut to_visit = VecDeque::from_iter(use_entrypoint.then_some(source.entrypoint())); let mut dirvs = NodeLinkingDirectives::new(); + for sn in source.children(source.module_root()) { + if let Some(ls) = link_sig(source, sn) { + if self.process(&existing, ls, false) { + to_visit.push_back(sn); + } + } + } + + while let Some(sn) = to_visit.pop_front() { + let Entry::Vacant(ve) = dirvs.entry(sn) else { + continue; + }; + // Hmmm, this will skip consts, we could consider as private + let Some(ls) = link_sig(source, sn) else { + continue; + }; + if self.process(&existing, ls, true) { + ve.insert(something); + } + } let NameLinkingPolicy { + new_names, sig_conflict, multi_impls, filter_private: _, @@ -606,7 +698,7 @@ impl NameLinkingPolicy { _ if sig == *ex_sig => { match directive(name, n, is_defn, ex_ns, multi_impls)? { Some(dirv) => dirv, - None => continue + None => continue, } } NewFuncHandling::RaiseError => { @@ -648,7 +740,7 @@ fn directive( new_defn: bool, ex_ns: &Either)>, multi_impls: &MultipleImplHandling, - reached: bool + reached: bool, ) -> Result>, NameLinkingError> { Ok(Some(match (new_defn, ex_ns) { (false, Either::Right(_)) => NodeLinkingDirective::add(), // another alias @@ -660,8 +752,12 @@ fn directive( MultipleImplHandling::UseExisting => NodeLinkingDirective::UseExisting(defn), MultipleImplHandling::UseNew => NodeLinkingDirective::replace([defn]), MultipleImplHandling::NewFunc(nf) => { - if matches!(nf, NewFuncHandling::ErrorIfReached | NewFuncHandling::AddIfReached) && !reached { - return Ok(None) + if matches!( + nf, + NewFuncHandling::ErrorIfReached | NewFuncHandling::AddIfReached + ) && !reached + { + return Ok(None); } match nf { NewFuncHandling::RaiseError | NewFuncHandling::ErrorIfReached => { @@ -671,10 +767,12 @@ fn directive( defn, )); } - NewFuncHandling::Add | NewFuncHandling::AddIfReached => NodeLinkingDirective::add(), - NewFuncHandling::MakePrivate => todo!("ALAN") + NewFuncHandling::Add | NewFuncHandling::AddIfReached => { + NodeLinkingDirective::add() + } + NewFuncHandling::MakePrivate => todo!("ALAN"), } - }, + } }, })) } From c8d2948ffab43825fdd6b7078ec30bdcbe244c9e Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 10:07:32 +0100 Subject: [PATCH 10/52] process returns Option, remove old find_reachable function --- hugr-core/src/call_graph.rs | 14 +- hugr-core/src/hugr/linking.rs | 282 +++++++++++----------------------- 2 files changed, 104 insertions(+), 192 deletions(-) diff --git a/hugr-core/src/call_graph.rs b/hugr-core/src/call_graph.rs index 8ffc58469f..0a3f69d524 100644 --- a/hugr-core/src/call_graph.rs +++ b/hugr-core/src/call_graph.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use crate::{HugrView, Node, core::HugrNode, ops::OpType}; -use petgraph::Graph; +use petgraph::{Graph, visit::EdgeRef}; /// Weight for an edge in a [`CallGraph`] #[derive(Clone, Debug, PartialEq, Eq)] @@ -103,4 +103,16 @@ impl CallGraph { pub fn node_index(&self, n: N) -> Option> { self.node_to_g.get(&n).copied() } + + pub fn callees(&self, n: N) -> impl Iterator, &CallGraphNode)> { + let g = self.graph(); + self.node_index(n).into_iter().flat_map(move |n| { + self.graph().edges(n).map(|e| { + ( + g.edge_weight(e.id()).unwrap(), + g.node_weight(e.target()).unwrap(), + ) + }) + }) + } } diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index dc69471990..0b05dc5ca5 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1,20 +1,16 @@ //! Directives and errors relating to linking Hugrs. use std::{ - collections::{HashMap, VecDeque}, + collections::{HashMap, VecDeque, hash_map::Entry}, fmt::Display, + iter::once, }; use itertools::{Either, Itertools}; -use crate::{ - Hugr, HugrView, Node, - call_graph::CallGraph, - core::HugrNode, - hugr::{HugrMut, hugrmut::InsertedForest, internal::HugrMutInternals}, - ops::{FuncDecl, OpType}, - types::PolyFuncType, -}; +use crate::call_graph::{CallGraph, CallGraphNode}; +use crate::hugr::{HugrMut, hugrmut::InsertedForest, internal::HugrMutInternals}; +use crate::{Hugr, HugrView, Node, core::HugrNode, ops::OpType, types::PolyFuncType}; /// Methods that merge Hugrs, adding static edges between old and inserted nodes. /// @@ -289,7 +285,7 @@ pub trait HugrLinking: HugrMut { fn insert_link_hugr( &mut self, parent: Self::Node, - mut other: Hugr, + other: Hugr, policy: &NameLinkingPolicy, ) -> Result, String> { let entrypoint_func = { @@ -305,65 +301,25 @@ pub trait HugrLinking: HugrMut { //if entrypoint_func == other.entrypoint() { // Do we need to check this? Ok if parent is self.module_root() ?? // return Err(format!("Entrypoint is a top-level function")) //} - let existing = gather_existing(self); - let mut defns_to_make_decls = Vec::new(); - let per_node = find_reachable(&other, [other.entrypoint()], |n| { - if n == other.entrypoint() { - Ok(NodeLinkingDirective::add()) - } else if n == entrypoint_func { - // ALAN TODO If there's a matching function in `self` then this is OK - Err(format!( - "Adding function {entrypoint_func} would double-copy the entrypoint-subtree" - )) - } else { - Ok(match link_sig(&other, n).unwrap() { - LinkSig::Private => NodeLinkingDirective::add(), - LinkSig::Public { name, is_defn, sig } => match existing.get(name) { - Some((defn_or_decl, ex_sig)) => { - if sig == *ex_sig { - match allow_new_impls.as_ref() { - None => NodeLinkingDirective::UseExisting( - *defn_or_decl.as_ref().left_or_else(|(n, _)| n), - ), - Some(multi_impls) => { - directive(name, n, is_defn, defn_or_decl, multi_impls) - .map_err(|e| e.to_string())? - } - } - } else { - match allow_new_names { - None | Some(NewFuncHandling::RaiseError) => { - return Err(format!( - "Conflicting signature for function {name} callable from entrypoint" - )); - } - Some(NewFuncHandling::Add) => NodeLinkingDirective::add(), - } - } - } - None => { - if allow_new_names.is_none() { - return Err(format!("New name {name}")); - } - if is_defn && allow_new_impls.is_none() { - defns_to_make_decls.push(n) - } - NodeLinkingDirective::add() - } - }, - }) + let pol = policy + .to_node_linking_for(&*self, &other, true) + .map_err(|e| e.to_string())?; + if matches!( + pol.get(&entrypoint_func), + Some(LinkAction::LinkNode(NodeLinkingDirective::Add { .. })) + ) { + if entrypoint_func != other.entrypoint() { + return Err(format!( + "Entrypoint is contained in function {entrypoint_func} which would be inserted" + )); } - })?; - for n in defns_to_make_decls { - while let Some(c) = other.first_child(n) { - other.remove_subtree(c) - } - let o = other.optype_mut(n); - let OpType::FuncDefn(fd) = o else { panic!() }; - *o = FuncDecl::new(fd.func_name().clone(), fd.signature().clone()).into(); } + let per_node = pol + .into_iter() + .map(|(k, LinkAction::LinkNode(v))| (k, v)) + .collect(); Ok(self - .add_hugr_link_nodes(Some(parent), other, per_node) + .insert_link_hugr_by_node(Some(parent), other, per_node) .expect("NodeLinkingPolicy was constructed to avoid any error")) } } @@ -483,12 +439,12 @@ pub enum NewFuncHandling { /// If the new function is reachable, then add it (as per [Self::Add]); /// otherwise, skip it. AddIfReached, - /// If the new function is reachable, then add it but make it [Private]; - /// otherwise, just skip it. (This always avoids making the Hugr invalid.) - /// - /// [Private]: crate::Visibility::Private + // /// If the new function is reachable, then add it but make it [Private]; + // /// otherwise, just skip it. (This always avoids making the Hugr invalid.) + // /// + // /// [Private]: crate::Visibility::Private // ALAN note this really is MakePrivateIfReached, but I don't see much point in the other. - MakePrivate, + // MakePrivate, } /// What to do when both target and inserted Hugr @@ -617,17 +573,16 @@ impl NameLinkingPolicy { self.to_node_linking_for(target, source, false) } - // TODO return Result ? Does NLD::add mean, traverse, but NLD::use_ex mean, don't traverse? - // TODO(2): add NodeLinkingDirective::MakePrivate?? What about ConvertToDecl ?? fn process( &self, existing: &HashMap<&String, PubFuncs>, sn: SN, new: LinkSig, reached: bool, - ) -> Result> { + ) -> Result>, NameLinkingError> { + let just_add = LinkAction::LinkNode(NodeLinkingDirective::add()); let (nfh, err) = match new { - LinkSig::Private => return Ok(reached), + LinkSig::Private => return Ok(reached.then_some(just_add)), LinkSig::Public { name, is_defn: new_is_defn, @@ -640,35 +595,57 @@ impl NameLinkingPolicy { src_node: sn, }, ), - Some((existing, ex_sig)) => match (*ex_sig == new_sig, existing) { - (false, ex) => ( - self.sig_conflict, - NameLinkingError::SignatureConflict { - name: name.clone(), - src_node: sn, - src_sig: new_sig.clone().into(), - tgt_node: *ex.as_ref().left_or_else(|(n, _)| n), - tgt_sig: (*ex_sig).clone().into(), - }, - ), - (true, Either::Left(n)) => match (new_is_defn, self.multi_impls) { - (false, _) => return Ok(false), // but add to dirvs - (true, MultipleImplHandling::NewFunc(nfh)) => { - (nfh, NameLinkingError::MultipleImpls(name.clone(), sn, *n)) + Some((existing, ex_sig)) => { + if *ex_sig == new_sig { + match (existing, new_is_defn, self.multi_impls) { + (Either::Left(n), false, _) => { + return Ok(Some(LinkAction::LinkNode( + NodeLinkingDirective::UseExisting(*n), + ))); + } + (Either::Left(n), true, MultipleImplHandling::NewFunc(nfh)) => { + (nfh, NameLinkingError::MultipleImpls(name.clone(), sn, *n)) + } + (Either::Left(n), true, MultipleImplHandling::UseExisting) => { + return Ok(Some(LinkAction::LinkNode( + NodeLinkingDirective::UseExisting(*n), + ))); + } + (Either::Left(n), true, MultipleImplHandling::UseNew) => { + return Ok(Some(LinkAction::LinkNode( + NodeLinkingDirective::replace([*n]), + ))); + } + (Either::Right((n, ns)), _, _) => { + return Ok(Some( + // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) + LinkAction::LinkNode(NodeLinkingDirective::replace( + once(n).chain(ns).copied(), + )), + )); + } } - (true, MultipleImplHandling::UseExisting) => return Ok(false), // do not traverse into new body, ignore. TODO need to record somehow that we must NodeLinkingDirective::UseExisting. - (true, MultipleImplHandling::UseNew) => return Ok(true), - }, - (true, Either::Right((n, _ns))) => return Ok(true), //if decl, just add to dirvs - }, + } else { + ( + self.sig_conflict, + NameLinkingError::SignatureConflict { + name: name.clone(), + src_node: sn, + src_sig: new_sig.clone().into(), + tgt_node: *existing.as_ref().left_or_else(|(n, _)| n), + tgt_sig: (*ex_sig).clone().into(), + }, + ) + } + } }, }; match nfh { NewFuncHandling::RaiseError => Err(err), NewFuncHandling::ErrorIfReached if reached => Err(err), - NewFuncHandling::Add => Ok(true), - NewFuncHandling::AddIfReached | NewFuncHandling::MakePrivate if reached => Ok(true), // TODO and record MakePrivate - _ => Ok(false), + NewFuncHandling::AddIfReached if !reached => Ok(None), + //NewFuncHandling::MakePrivate if reached => Ok(true), // TODO and record MakePrivate + _ => Ok(Some(just_add)), } } @@ -688,67 +665,32 @@ impl NameLinkingPolicy { let mut dirvs = LinkActions::new(); for sn in source.children(source.module_root()) { if let Some(ls) = link_sig(source, sn) { - if self.process(&existing, sn, ls, false)? { + if self.process(&existing, sn, ls, false)?.is_some() { to_visit.push_back(sn); } } } - + // Note: we could optimize the case where self.filter_private is false, + // by adding directly to results above, skipping this reachability traversal while let Some(sn) = to_visit.pop_front() { let Entry::Vacant(ve) = dirvs.entry(sn) else { continue; }; // Hmmm, this will skip consts, we could consider as private - let Some(ls) = link_sig(source, sn) else { - continue; - }; - if self.process(&existing, sn, ls, true)? { - ve.insert(something); - } - } - - let NameLinkingPolicy { - new_names, - sig_conflict, - multi_impls, - filter_private: _, - } = self; - for n in source.children(source.module_root()) { - let dirv = match link_sig(source, n) { - Some(LinkSig::Private) if !self.filter_private => NodeLinkingDirective::add(), - Some(LinkSig::Public { name, is_defn, sig }) => { - if let Some((ex_ns, ex_sig)) = existing.get(name) { - match *sig_conflict { - _ if sig == *ex_sig => { - match directive(name, n, is_defn, ex_ns, multi_impls)? { - Some(dirv) => dirv, - None => continue, - } - } - NewFuncHandling::RaiseError => { - return Err(NameLinkingError::SignatureConflict { - name: name.clone(), - src_node: n, - src_sig: Box::new(sig.clone()), - tgt_node: *ex_ns.as_ref().left_or_else(|(n, _)| n), - tgt_sig: Box::new((*ex_sig).clone()), - }); - } - NewFuncHandling::Add => NodeLinkingDirective::add(), - } - } else { - NodeLinkingDirective::add() - } + if let Some(ls) = link_sig(source, sn) { + let Some(act) = self.process(&existing, sn, ls, true)? else { + unreachable!("process never returns `None` when given reached==true") + }; + let LinkAction::LinkNode(dirv) = act; // for now + if let NodeLinkingDirective::Add { .. } = dirv { + to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { + CallGraphNode::FuncDecl(n) | CallGraphNode::FuncDefn(n) => *n, + CallGraphNode::NonFuncRoot => unreachable!("cannot call non-func"), + })); } - _ => continue, - }; - dirvs.insert(n, LinkAction::LinkNode(dirv)); - } - if self.filter_private { - find_reachable(source, dirvs.keys().copied(), |n| Ok(dirvs[&n].clone())) - } else { - Ok(dirvs) + } } + Ok(dirvs) } } @@ -793,55 +735,13 @@ fn directive( } NewFuncHandling::Add | NewFuncHandling::AddIfReached => { NodeLinkingDirective::add() - } - NewFuncHandling::MakePrivate => todo!("ALAN"), + } //NewFuncHandling::MakePrivate => todo!("ALAN"), } } }, })) } -fn find_reachable( - h: &H, - starts: impl IntoIterator, - mut pub_callback: impl FnMut(H::Node) -> Result, E>, -) -> Result, E> { - let mut queue = VecDeque::from_iter(starts); - let mut res = HashMap::new(); - while let Some(n) = queue.pop_front() { - if res.contains_key(&n) { - continue; - } - debug_assert!(h.get_parent(n) == Some(h.module_root()) || n == h.entrypoint()); - let nld = match link_sig(h, n) { - // This handles the case where `starts` includes the entrypoint. - None | Some(LinkSig::Private) => NodeLinkingDirective::add(), - Some(LinkSig::Public { .. }) => pub_callback(n)?, - }; - - if matches!(nld, NodeLinkingDirective::Add { .. }) { - // Would be great to use a CallGraph here but that is in hugr-passes - // and we cannot have a cyclic dependency between crates !! - // Also call-graph does not deal with (potentially-module-level) Constants. - for d in h.descendants(n) { - if matches!( - h.get_optype(d), - OpType::LoadConstant(_) | OpType::LoadFunction(_) | OpType::Call(_) - ) { - if let Some(static_) = h.static_source(d) { - if h.get_parent(static_) == Some(h.module_root()) { - queue.push_back(static_) - } - } - } - } - } - - res.insert(n, nld); - } - Ok(res) -} - type PubFuncs<'a, N> = (Either)>, &'a PolyFuncType); enum LinkSig<'a> { From bc03040be0822c0bed227fc32fe8b1feabd3efef Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 12:02:50 +0100 Subject: [PATCH 11/52] process returns Result and only-if-reached --- hugr-core/src/hugr/linking.rs | 56 +++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 0b05dc5ca5..039df5a7ae 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -573,16 +573,17 @@ impl NameLinkingPolicy { self.to_node_linking_for(target, source, false) } + /// The result is Ok((action, bool)) where the bool being true + /// means the action is ONLY needed if the function is reached. fn process( &self, existing: &HashMap<&String, PubFuncs>, sn: SN, new: LinkSig, - reached: bool, - ) -> Result>, NameLinkingError> { + ) -> (Result, NameLinkingError>, bool) { let just_add = LinkAction::LinkNode(NodeLinkingDirective::add()); let (nfh, err) = match new { - LinkSig::Private => return Ok(reached.then_some(just_add)), + LinkSig::Private => return (Ok(just_add), self.filter_private), LinkSig::Public { name, is_defn: new_is_defn, @@ -599,30 +600,34 @@ impl NameLinkingPolicy { if *ex_sig == new_sig { match (existing, new_is_defn, self.multi_impls) { (Either::Left(n), false, _) => { - return Ok(Some(LinkAction::LinkNode( - NodeLinkingDirective::UseExisting(*n), - ))); + return ( + Ok(LinkAction::LinkNode(NodeLinkingDirective::UseExisting(*n))), + false, + ); } (Either::Left(n), true, MultipleImplHandling::NewFunc(nfh)) => { (nfh, NameLinkingError::MultipleImpls(name.clone(), sn, *n)) } (Either::Left(n), true, MultipleImplHandling::UseExisting) => { - return Ok(Some(LinkAction::LinkNode( - NodeLinkingDirective::UseExisting(*n), - ))); + return ( + Ok(LinkAction::LinkNode(NodeLinkingDirective::UseExisting(*n))), + false, + ); } (Either::Left(n), true, MultipleImplHandling::UseNew) => { - return Ok(Some(LinkAction::LinkNode( - NodeLinkingDirective::replace([*n]), - ))); + return ( + Ok(LinkAction::LinkNode(NodeLinkingDirective::replace([*n]))), + false, + ); } (Either::Right((n, ns)), _, _) => { - return Ok(Some( - // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) - LinkAction::LinkNode(NodeLinkingDirective::replace( + // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) + return ( + Ok(LinkAction::LinkNode(NodeLinkingDirective::replace( once(n).chain(ns).copied(), - )), - )); + ))), + false, + ); } } } else { @@ -641,11 +646,11 @@ impl NameLinkingPolicy { }, }; match nfh { - NewFuncHandling::RaiseError => Err(err), - NewFuncHandling::ErrorIfReached if reached => Err(err), - NewFuncHandling::AddIfReached if !reached => Ok(None), + NewFuncHandling::RaiseError => (Err(err), false), + NewFuncHandling::ErrorIfReached => (Err(err), true), + NewFuncHandling::Add => (Ok(just_add), false), + NewFuncHandling::AddIfReached => (Ok(just_add), true), //NewFuncHandling::MakePrivate if reached => Ok(true), // TODO and record MakePrivate - _ => Ok(Some(just_add)), } } @@ -665,7 +670,9 @@ impl NameLinkingPolicy { let mut dirvs = LinkActions::new(); for sn in source.children(source.module_root()) { if let Some(ls) = link_sig(source, sn) { - if self.process(&existing, sn, ls, false)?.is_some() { + // Note we'll call process() again below, so a bit inefficient + let (_, only_if_reached) = self.process(&existing, sn, ls); + if !only_if_reached { to_visit.push_back(sn); } } @@ -678,10 +685,7 @@ impl NameLinkingPolicy { }; // Hmmm, this will skip consts, we could consider as private if let Some(ls) = link_sig(source, sn) { - let Some(act) = self.process(&existing, sn, ls, true)? else { - unreachable!("process never returns `None` when given reached==true") - }; - let LinkAction::LinkNode(dirv) = act; // for now + let LinkAction::LinkNode(dirv) = self.process(&existing, sn, ls).0?; if let NodeLinkingDirective::Add { .. } = dirv { to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { CallGraphNode::FuncDecl(n) | CallGraphNode::FuncDefn(n) => *n, From 65f512c18d348d5eb79027d15c046cf12f492ad0 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 12:12:56 +0100 Subject: [PATCH 12/52] Add LinkAction From --- hugr-core/src/hugr/linking.rs | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 039df5a7ae..55b9ab755f 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -581,7 +581,7 @@ impl NameLinkingPolicy { sn: SN, new: LinkSig, ) -> (Result, NameLinkingError>, bool) { - let just_add = LinkAction::LinkNode(NodeLinkingDirective::add()); + let just_add = NodeLinkingDirective::add().into(); let (nfh, err) = match new { LinkSig::Private => return (Ok(just_add), self.filter_private), LinkSig::Public { @@ -600,32 +600,22 @@ impl NameLinkingPolicy { if *ex_sig == new_sig { match (existing, new_is_defn, self.multi_impls) { (Either::Left(n), false, _) => { - return ( - Ok(LinkAction::LinkNode(NodeLinkingDirective::UseExisting(*n))), - false, - ); + return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); } (Either::Left(n), true, MultipleImplHandling::NewFunc(nfh)) => { (nfh, NameLinkingError::MultipleImpls(name.clone(), sn, *n)) } (Either::Left(n), true, MultipleImplHandling::UseExisting) => { - return ( - Ok(LinkAction::LinkNode(NodeLinkingDirective::UseExisting(*n))), - false, - ); + return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); } (Either::Left(n), true, MultipleImplHandling::UseNew) => { - return ( - Ok(LinkAction::LinkNode(NodeLinkingDirective::replace([*n]))), - false, - ); + return (Ok(NodeLinkingDirective::replace([*n]).into()), false); } (Either::Right((n, ns)), _, _) => { // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) return ( - Ok(LinkAction::LinkNode(NodeLinkingDirective::replace( - once(n).chain(ns).copied(), - ))), + Ok(NodeLinkingDirective::replace(once(n).chain(ns).copied()) + .into()), false, ); } @@ -811,11 +801,11 @@ pub type NodeLinkingDirectives = HashMap>; /// A separate enum from [NodeLinkingDirective] to allow [NameLinkingPolicy::to_node_linking] /// to specify a greater range of actions than that supported by /// [HugrLinking::insert_link_hugr_by_node] and [HugrLinking::insert_link_view_by_node]. -#[derive(Clone, Debug, Hash, PartialEq, Eq)] +#[derive(Clone, Debug, Hash, PartialEq, Eq, derive_more::From)] #[non_exhaustive] pub enum LinkAction { /// Just apply the specified [NodeLinkingDirective]. - LinkNode(NodeLinkingDirective), + LinkNode(#[from] NodeLinkingDirective), } /// Details the concrete actions to implement a specific source Hugr into a specific target Hugr. From daaac984e5ccc1dfc4c3565dd12e5df691c1a028 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 12:21:50 +0100 Subject: [PATCH 13/52] Remove enacted comment --- hugr-core/src/hugr/linking.rs | 95 ++--------------------------------- 1 file changed, 4 insertions(+), 91 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 55b9ab755f..73afc01c94 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -175,102 +175,15 @@ pub trait HugrLinking: HugrMut { /// /// `parent` is the parent node under which to insert the entrypoint. /// - /// If `allow_new_names` is `None`, then any FuncDefn/FuncDecl in `other` and reachable - /// from the entrypoint, must have a matching name and signature with one in `self`; - /// if `Some(s)` then `s` details how to handle the case where the name matches but the - /// signature does not. - // Note the use of an Option here, rather than a bool and a separate SignatureConflictHandling, - // rules out raiseing an error on an entirely new name but *accepting* a new+conflicting - // signature for an existing name; the latter is an invalid Hugr and would have to result in - // one of the functions being renamed *OR MADE PRIVATE*, so the latter is a possible case. - // TODO: consider extending(+renaming?) SignatureConflictHandling with new variants - // (and eliding the option here): - // * RejectNewNames (i.e. the none here; more aggressive than any existing enum variant) - // * MakeAllNewNamesPrivate - // * MakeNewConflictsPrivate (i.e. when name exists but with different sig) - - // there is already SigConfHand::ErrorDontInsert,UseBoth. So... - // NoNewNames(skip_unused: bool, make_private_or_error) (also same name but different sig) - // NewNames(if new type - skip unused: bool, make_private_or_error_or_invalid) - - // skip_unreachable: bool, // can be new in this PR...no, maybe this is ALWAYS true for insert_link, and NEVER true for link? - // new_name: AllowOrHideOrError, // call this ConflictResolution, add this field and the Hide variant in this PR - // conflicting_sig: AllowOrHideOrError - // - // currently have MultiImplHandling::UseNew, ::UseExisting, ::UseBoth, ::Error - // instead: NewImplHandling::ConvertToDecls**, PreferExisting, OverrideExisting, Both(ConflictResolution) - // ** doesn't make sense for link_hugr...unless new_name == Allow and that's all we're doing? - - // enum NewMemberHandling { - // Allow // bool skip_unreachable makes sense only for insert_link...but assume true iff insert_link - // MakePrivate {skip_unreachable:bool} - // ErrorDontInsert {skip_unreachable:bool} - // } - - // implementation... - // * can build "what's there" in each Hugr - // * work out minimum FuncDefns to add: start with - // - for link_hugr, consider all public functions (defns and decls) - // if name is new -> bail if new_name==ErrorDontInsert, include if new_name==Allow, **skip if MakePrivate** - // if name exists and sig conflicts, as previous for conflicting_sig - // and sig is same -> include if at least one is decl or new_impls==OverrideExisting or Both(Allow) - // else, both are defns; error if Both(ErrorDontInsert) - // ** skip if Both(MakePrivate)** - // if new_impls==PreferExisting, need to record in NodeLinkingDirectives but NOT callgraph-start-points - // (insert_link_xxxx_by_node will mutate Hugr or ignore descendants according to xxxx) - // **skip if**s add rather than skip if skip_(private_)unreachable is false, but it may never be - // - for insert_link, just the entrypoint subtree - // that's probably a mutable NodeLinkingDirectives, but also use to - // walk the callgraph...for any node not in the dirvs - // - check as previous, EXCEPT - //. if name new and new_name==MakePrivate, or sig conflicts and conflicting_sig==MakePrivate, or an impl and Both(MakePrivate) - // then don't skip, add. - - // So...function from starting points (either all pub funcs or just entrypoint subtree) - // does above conditional skipping on MakePrivate but adding to Dfs otherwise - // then walks callgraph, doing above conditional including carrying on through MakePrivates :) - // results in NodeLinkingDirectives - // - // Ah, multiple meanings of skip_unreachable: - // * for link_hugr, applies to private functions (if false, include all of those) - // do we want to allow skipping unreachable new public names?? - // * for insert_link, applies to all funcs private and public...but, separately? - // Surely we should always skip private unreachable, as otherwise the entrypoint would have to be overridden by UseExisting (!) - // - So, could have 'skip_unreachable_new_public' (prob. wants to default to TRUE for insert_link and FALSE for link_hugr) - // `skip_existing_public` would basically be ConvertNewToDecls but we *could* have an extra mode - // where reachable public funcs are OverrideExisting but unreachable are PreferExisting - // - And, could have `skip_private` - def. wants to default to TRUE for insert_link, maybe *required* to be TRUE; - // but could be an option for link_hugr OR we could quietly change behaviour. - // - Sounds like `skip_unreachable` applies only to new, public, functions - // But really this belongs in NewMemberHandling for new_names and conflicting_sig - // --> NewMemberHandling::Allow(skip=true) is fine for insert_link, mildly odd for link_hugr - // (but as long as link_hugr has no ConvertNewToDecls, only mildly odd, not totally pointless) - // NewMemberHandling::Error(skip_unreachable=true) is new behaviour, similarly slightly odd - // (and skip_unreachable=false would be odd for insert_link) - // NewMemberHandling::MakePrivate doesn't need the flag, can be always true? - // Consider Add, AddIfReached, Error, ErrorIfReached, MakePrivate(IfReached) - // CallGraph is not needed only if new_names == Add/Error, ConflictingSig==Add/Error, impls=Prefer/Override/Both(Add/Error) - // link_hugr with ConvertToDecls...does it make ANY sense? - // new_names == Add, will add decls; AddIfReached==ErrorIfReached==MakePrivate(IfReached)==no-op; Error may error (so check); MakePrivateEvenIfNotReached might add private decls - // conflicting_sig, similarly - // so a lot of no-ops, but not inconsistent/impossible. - - // WHAT ABOUT CALLBACKS RATHER THAN ALL OF THESE ???? - - /// - /// If `allow_new_impls` is `None`, then any required [`FuncDefn`] in `other` is treated - /// as a [`FuncDecl`] (either being linked with a function existing in `self`, or added if - /// `allow_new_names` is `Some`). - /// If `allow_new_impls` is Some, then that details how such a `FuncDefn` in `other` - /// may (be) override(/den) by another in `self`. - /// /// # Errors /// /// If other's entrypoint is its module-root (recommend using [Self::link_module] instead) /// /// If other's entrypoint calls (perhaps transitively) the function containing said entrypoint - /// (an exception is made if the called+containing function is public and there is already - /// an equivalent in `self` - to which the call is redirected). + /// (an exception is made if the called+containing function is public and is being replaced + /// by an equivalent in `self` via [MultipleImplHandling::UseExisting], in which case + /// the call is redirected to the existing function, and the new one is not inserted + /// except the entrypoint subtree.) /// /// If other's entrypoint calls a public function in `other` which /// * has a name or signature different to any in `self`, and `allow_new_names` is `None` From f2a4cae76a3c493ceaa664531c8b77b8d9b5fb9a Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 17:19:56 +0100 Subject: [PATCH 14/52] correct test wrt filter_private, but some other failures --- hugr-core/src/hugr/linking.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 74bb4d2085..bf773f768c 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1203,13 +1203,19 @@ mod test { } #[rstest] - #[case(MultipleImplHandling::UseNew, vec![11], vec![5, 11])] // Existing constant is not removed - #[case(MultipleImplHandling::UseExisting, vec![5], vec![5])] - #[case(NewFuncHandling::Add.into(), vec![5, 11], vec![5,11])] - #[case(NewFuncHandling::RaiseError.into(), vec![], vec![])] + #[case(MultipleImplHandling::UseNew, vec![11], false, vec![5, 11])] // Existing constant is not removed + #[should_panic] // ALAN TODO FIX - UnconnectedPort as Const not copied + #[case(MultipleImplHandling::UseNew, vec![11], true, vec![5, 11])] + #[case(MultipleImplHandling::UseExisting, vec![5], true, vec![5])] + #[case(MultipleImplHandling::UseExisting, vec![5], false, vec![5, 11])] + #[should_panic] // ALAN TODO FIX + #[case(NewFuncHandling::Add.into(), vec![5, 11], true, vec![5,11])] + #[case(NewFuncHandling::Add.into(), vec![5, 11], false, vec![5,11])] + #[case(NewFuncHandling::RaiseError.into(), vec![], true, vec![])] // filter_private ignored fn impl_conflict( #[case] multi_impls: MultipleImplHandling, #[case] expect_used: Vec, + #[case] filter_private: bool, #[case] expect_exist: Vec, ) { fn build_hugr(cst: u64) -> Hugr { @@ -1226,7 +1232,12 @@ mod test { let mut host = backup.clone(); let inserted = build_hugr(11); - let pol = NameLinkingPolicy::keep_both_invalid().on_multiple_impls(multi_impls); + let pol = NameLinkingPolicy { + new_names: NewFuncHandling::RaiseError, + sig_conflict: NewFuncHandling::RaiseError, + multi_impls, + filter_private + }; let res = host.link_module(inserted, &pol); if multi_impls == NewFuncHandling::RaiseError.into() { assert!(matches!(res, Err(NameLinkingError::MultipleImpls(n, _, _)) if n == "foo")); From 29bebd86baee30a2f10c15c743bb18a4baf9f9de Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 21:11:48 +0100 Subject: [PATCH 15/52] fmt --- hugr-core/src/hugr/linking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index bf773f768c..6a1414deba 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1236,7 +1236,7 @@ mod test { new_names: NewFuncHandling::RaiseError, sig_conflict: NewFuncHandling::RaiseError, multi_impls, - filter_private + filter_private, }; let res = host.link_module(inserted, &pol); if multi_impls == NewFuncHandling::RaiseError.into() { From f9ba896606de003b401d8454adc6bf7a1480010c Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 19:11:45 +0100 Subject: [PATCH 16/52] hugr-core CallGraph: rename NonFunc(Root=>Entrypoint) --- hugr-core/src/call_graph.rs | 11 +++++++---- hugr-core/src/hugr/linking.rs | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/hugr-core/src/call_graph.rs b/hugr-core/src/call_graph.rs index 2d24b9f908..2a0de938d8 100644 --- a/hugr-core/src/call_graph.rs +++ b/hugr-core/src/call_graph.rs @@ -20,10 +20,10 @@ pub enum CallGraphNode { FuncDecl(N), /// petgraph-node corresponds to a [`FuncDefn`](OpType::FuncDefn) node (specified) in the Hugr FuncDefn(N), - /// petgraph-node corresponds to the root node of the hugr, that is not + /// petgraph-node corresponds to the [HugrView::entrypoint], that is not /// a [`FuncDefn`](OpType::FuncDefn). Note that it will not be a [Module](OpType::Module) - /// either, as such a node could not have outgoing edges, so is not represented in the petgraph. - NonFuncRoot, + /// either, as such a node could not have edges, so is not represented in the petgraph. + NonFuncEntrypoint, } /// Details the [`Call`]s and [`LoadFunction`]s in a Hugr. @@ -61,7 +61,10 @@ impl CallGraph { }) .collect::>(); if !hugr.entrypoint_optype().is_module() && !node_to_g.contains_key(&hugr.entrypoint()) { - node_to_g.insert(hugr.entrypoint(), g.add_node(CallGraphNode::NonFuncRoot)); + node_to_g.insert( + hugr.entrypoint(), + g.add_node(CallGraphNode::NonFuncEntrypoint), + ); } for (func, cg_node) in &node_to_g { traverse(hugr, *cg_node, *func, &mut g, &node_to_g); diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 6a1414deba..1c8a1d9eb0 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -608,7 +608,7 @@ impl NameLinkingPolicy { if let NodeLinkingDirective::Add { .. } = dirv { to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { CallGraphNode::FuncDecl(n) | CallGraphNode::FuncDefn(n) => *n, - CallGraphNode::NonFuncRoot => unreachable!("cannot call non-func"), + CallGraphNode::NonFuncEntrypoint => unreachable!("cannot call non-func"), })); } ve.insert(act); From 612fba1cfc4f51770ae5a686211a5075aaf3a1ee Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 16 Sep 2025 21:11:04 +0100 Subject: [PATCH 17/52] Add Constants to CallGraph (no tests, TODO rename to StaticGraph) --- hugr-core/src/call_graph.rs | 18 ++++++++++++++---- hugr-core/src/hugr/linking.rs | 6 +++--- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/hugr-core/src/call_graph.rs b/hugr-core/src/call_graph.rs index 2a0de938d8..87d1ebb09b 100644 --- a/hugr-core/src/call_graph.rs +++ b/hugr-core/src/call_graph.rs @@ -12,6 +12,8 @@ pub enum CallGraphEdge { Call(N), /// Edge corresponds to a [`LoadFunction`](OpType::LoadFunction) node (specified) in the Hugr LoadFunction(N), + /// Edge corresponds to a [LoadConstant](OpType::LoadConstant) node (specified) in the Hugr + LoadConstant(N), } /// Weight for a petgraph-node in a [`CallGraph`] @@ -24,6 +26,9 @@ pub enum CallGraphNode { /// a [`FuncDefn`](OpType::FuncDefn). Note that it will not be a [Module](OpType::Module) /// either, as such a node could not have edges, so is not represented in the petgraph. NonFuncEntrypoint, + /// petgraph-node corresponds to a constant; will have no outgoing edges, and incoming + /// edges will be [CallGraphEdge::Const] + Const(N), } /// Details the [`Call`]s and [`LoadFunction`]s in a Hugr. @@ -55,6 +60,7 @@ impl CallGraph { let weight = match hugr.get_optype(n) { OpType::FuncDecl(_) => CallGraphNode::FuncDecl(n), OpType::FuncDefn(_) => CallGraphNode::FuncDefn(n), + OpType::Const(_) => CallGraphNode::Const(n), _ => return None, }; Some((n, g.add_node(weight))) @@ -77,17 +83,21 @@ impl CallGraph { node_to_g: &HashMap>, ) { for ch in h.children(node) { - if h.get_optype(ch).is_func_defn() { - continue; - } traverse(h, enclosing_func, ch, g, node_to_g); let weight = match h.get_optype(ch) { OpType::Call(_) => CallGraphEdge::Call(ch), OpType::LoadFunction(_) => CallGraphEdge::LoadFunction(ch), + OpType::LoadConstant(_) => CallGraphEdge::LoadConstant(ch), _ => continue, }; if let Some(target) = h.static_source(ch) { - g.add_edge(enclosing_func, *node_to_g.get(&target).unwrap(), weight); + if h.get_parent(target) == Some(h.module_root()) { + g.add_edge(enclosing_func, node_to_g[&target], weight); + } else { + assert!(!node_to_g.contains_key(&target)); + assert!(h.get_optype(ch).is_load_constant()); + assert!(h.get_optype(target).is_const()); + } } } } diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 1c8a1d9eb0..7ac25f2ddd 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -607,7 +607,9 @@ impl NameLinkingPolicy { let LinkAction::LinkNode(dirv) = &act; if let NodeLinkingDirective::Add { .. } = dirv { to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { - CallGraphNode::FuncDecl(n) | CallGraphNode::FuncDefn(n) => *n, + CallGraphNode::FuncDecl(n) + | CallGraphNode::FuncDefn(n) + | CallGraphNode::Const(n) => *n, CallGraphNode::NonFuncEntrypoint => unreachable!("cannot call non-func"), })); } @@ -1204,11 +1206,9 @@ mod test { #[rstest] #[case(MultipleImplHandling::UseNew, vec![11], false, vec![5, 11])] // Existing constant is not removed - #[should_panic] // ALAN TODO FIX - UnconnectedPort as Const not copied #[case(MultipleImplHandling::UseNew, vec![11], true, vec![5, 11])] #[case(MultipleImplHandling::UseExisting, vec![5], true, vec![5])] #[case(MultipleImplHandling::UseExisting, vec![5], false, vec![5, 11])] - #[should_panic] // ALAN TODO FIX #[case(NewFuncHandling::Add.into(), vec![5, 11], true, vec![5,11])] #[case(NewFuncHandling::Add.into(), vec![5, 11], false, vec![5,11])] #[case(NewFuncHandling::RaiseError.into(), vec![], true, vec![])] // filter_private ignored From cd6ae7d2c5dc5abaa00a6ee80068d7538663813f Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 17 Sep 2025 16:50:19 +0100 Subject: [PATCH 18/52] Comment --- hugr-core/src/hugr/linking.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 7ac25f2ddd..03d80349fb 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -601,7 +601,6 @@ impl NameLinkingPolicy { let Entry::Vacant(ve) = res.entry(sn) else { continue; }; - // Hmmm, this will skip consts, we could consider as private if let Some(ls) = link_sig(source, sn) { let act = self.process(&existing, sn, ls).0?; let LinkAction::LinkNode(dirv) = &act; From e5e1ad281711fd07e7e5a27c0b591fd258e9a4b4 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 17 Sep 2025 16:52:21 +0100 Subject: [PATCH 19/52] Properly handle entrypoint (no NLD, just traverse) --- hugr-core/src/hugr/linking.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 03d80349fb..a9c98b5ddb 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -598,22 +598,27 @@ impl NameLinkingPolicy { // Note: we could optimize the case where self.filter_private is false, // by adding directly to results above, skipping this reachability traversal while let Some(sn) = to_visit.pop_front() { - let Entry::Vacant(ve) = res.entry(sn) else { - continue; - }; - if let Some(ls) = link_sig(source, sn) { - let act = self.process(&existing, sn, ls).0?; - let LinkAction::LinkNode(dirv) = &act; - if let NodeLinkingDirective::Add { .. } = dirv { - to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { - CallGraphNode::FuncDecl(n) - | CallGraphNode::FuncDefn(n) - | CallGraphNode::Const(n) => *n, - CallGraphNode::NonFuncEntrypoint => unreachable!("cannot call non-func"), - })); + if !(use_entrypoint && sn == source.entrypoint()) { + let Entry::Vacant(ve) = res.entry(sn) else { + continue; + }; + if let Some(ls) = link_sig(source, sn) { + let act = self.process(&existing, sn, ls).0?; + let LinkAction::LinkNode(dirv) = &act; + let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); + ve.insert(act); + if !traverse { + continue; + } } - ve.insert(act); } + // For entrypoint, *just* traverse + to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { + CallGraphNode::FuncDecl(n) + | CallGraphNode::FuncDefn(n) + | CallGraphNode::Const(n) => *n, + CallGraphNode::NonFuncEntrypoint => unreachable!("cannot call non-func"), + })); } Ok(res) } From 0b0640d953b8cc7d952696c09e1c2ff322359271 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 14:55:11 +0000 Subject: [PATCH 20/52] clippy --- hugr-core/src/hugr/linking.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 7c7b2018e6..9b17e411ba 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -219,13 +219,11 @@ pub trait HugrLinking: HugrMut { .map_err(|e| e.to_string())?; if matches!( pol.get(&entrypoint_func), - Some(LinkAction::LinkNode(NodeLinkingDirective::Add { .. })) + Some(LinkAction::LinkNode(NodeLinkingDirective::Add { .. })) if entrypoint_func != other.entrypoint() ) { - if entrypoint_func != other.entrypoint() { - return Err(format!( - "Entrypoint is contained in function {entrypoint_func} which would be inserted" - )); - } + return Err(format!( + "Entrypoint is contained in function {entrypoint_func} which would be inserted" + )); } let per_node = pol .into_iter() From d8b6615ce6a7a46f23da25484f46fef9f229c675 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 15:04:48 +0000 Subject: [PATCH 21/52] remove filter_private --- hugr-core/src/hugr/linking.rs | 39 ++++++++++++----------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 9b17e411ba..6277de7400 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -318,11 +318,6 @@ pub struct NameLinkingPolicy { new_names: OnNewFunc, // ALAN default to Add for link_module but AddIfReached for insert_link.... sig_conflict: OnNewFunc, multi_defn: OnMultiDefn, - /// Whether to filter private functions down to only those required (reached from public) - /// false means just to copy all private functions. - // ALAN remove this? it's just to preserve behaviour of old link_module that was - // (explicitly) never guaranteed in the docs. - filter_private: bool, } /// Specifies what to do with a function in some situation - used in @@ -418,7 +413,6 @@ impl NameLinkingPolicy { new_names: OnNewFunc::Add, multi_defn: multi_defn.into(), sig_conflict: OnNewFunc::RaiseError, - filter_private: false, } } @@ -430,7 +424,6 @@ impl NameLinkingPolicy { new_names: OnNewFunc::Add, multi_defn: OnMultiDefn::NewFunc(OnNewFunc::Add), sig_conflict: OnNewFunc::Add, - filter_private: false, } } @@ -498,7 +491,7 @@ impl NameLinkingPolicy { ) -> (Result, NameLinkingError>, bool) { let just_add = NodeLinkingDirective::add().into(); let (nfh, err) = match new { - LinkSig::Private => return (Ok(just_add), self.filter_private), + LinkSig::Private => return (Ok(just_add), true), LinkSig::Public { name, is_defn: new_is_defn, @@ -588,8 +581,6 @@ impl NameLinkingPolicy { } } } - // Note: we could optimize the case where self.filter_private is false, - // by adding directly to results above, skipping this reachability traversal while let Some(sn) = to_visit.pop_front() { if !(use_entrypoint && sn == source.entrypoint()) { let Entry::Vacant(ve) = res.entry(sn) else { @@ -1104,7 +1095,12 @@ mod test { }; let inserted = { - let mut main_b = FunctionBuilder::new("main", Signature::new(vec![], i64_t())).unwrap(); + let mut main_b = FunctionBuilder::new_vis( + "main", + Signature::new(vec![], i64_t()), + Visibility::Public, + ) + .unwrap(); let mut mb = main_b.module_root_builder(); let foo1 = mb.declare("foo", foo_sig.clone().into()).unwrap(); let foo2 = mb.declare("foo", foo_sig.clone().into()).unwrap(); @@ -1129,12 +1125,8 @@ mod test { h }; - let pol = NameLinkingPolicy { - new_names: OnNewFunc::RaiseError, - sig_conflict, - multi_defn, - filter_private: false, - }; + let pol = + NameLinkingPolicy::err_on_conflict(multi_defn).on_signature_conflict(sig_conflict); let mut target2 = target.clone(); target.link_module_view(&inserted, &pol).unwrap(); @@ -1206,17 +1198,13 @@ mod test { } #[rstest] - #[case(OnMultiDefn::UseNew, vec![11], false, vec![5, 11])] // Existing constant is not removed - #[case(OnMultiDefn::UseNew, vec![11], true, vec![5, 11])] - #[case(OnMultiDefn::UseExisting, vec![5], true, vec![5])] - #[case(OnMultiDefn::UseExisting, vec![5], false, vec![5, 11])] - #[case(OnNewFunc::Add.into(), vec![5, 11], true, vec![5,11])] - #[case(OnNewFunc::Add.into(), vec![5, 11], false, vec![5,11])] - #[case(OnNewFunc::RaiseError.into(), vec![], true, vec![])] // filter_private ignored + #[case(OnMultiDefn::UseNew, vec![11], vec![5, 11])] // Existing constant is not removed + #[case(OnMultiDefn::UseExisting, vec![5], vec![5])] + #[case(OnNewFunc::Add.into(), vec![5, 11], vec![5,11])] + #[case(OnNewFunc::RaiseError.into(), vec![], vec![])] fn impl_conflict( #[case] multi_defn: OnMultiDefn, #[case] expect_used: Vec, - #[case] filter_private: bool, #[case] expect_exist: Vec, ) { fn build_hugr(cst: u64) -> Hugr { @@ -1237,7 +1225,6 @@ mod test { new_names: OnNewFunc::RaiseError, sig_conflict: OnNewFunc::RaiseError, multi_defn, - filter_private, }; let res = host.link_module(inserted, &pol); if multi_defn == OnNewFunc::RaiseError.into() { From 8fe11ff8ad9c30acf6b12adbae25e475ac7a0d4b Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 15:20:02 +0000 Subject: [PATCH 22/52] rename accessor, remove commented-out MakePrivate --- hugr-core/src/hugr/linking.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 6277de7400..d9affa3006 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -340,12 +340,6 @@ pub enum OnNewFunc { /// If the new function is reachable, then add it (as per [Self::Add]); /// otherwise, skip it. AddIfReached, - // /// If the new function is reachable, then add it but make it [Private]; - // /// otherwise, just skip it. (This always avoids making the Hugr invalid.) - // /// - // /// [Private]: crate::Visibility::Private - // ALAN note this really is MakePrivateIfReached, but I don't see much point in the other. - // MakePrivate, } /// What to do when both target and inserted Hugr @@ -466,7 +460,7 @@ impl NameLinkingPolicy { /// Tells how to behave when the source Hugr adds a ([Visibility::Public]) /// name not already in the target. - pub fn get_new_names(self) -> OnNewFunc { + pub fn get_on_new_names(&self) -> OnNewFunc { self.new_names } @@ -549,7 +543,6 @@ impl NameLinkingPolicy { OnNewFunc::ErrorIfReached => (Err(err), true), OnNewFunc::Add => (Ok(just_add), false), OnNewFunc::AddIfReached => (Ok(just_add), true), - //OnNewFunc::MakePrivate if reached => (Ok(LinkAction::MakePrivate), true) } } From d48cbb2cdae9407ea21facee64e267eb80944203 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 15:22:00 +0000 Subject: [PATCH 23/52] Update docs. re filter-private always on --- hugr-core/src/hugr/linking.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index d9affa3006..a0715be90f 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -111,9 +111,7 @@ pub trait HugrLinking: HugrMut { /// Insert module-children from another Hugr into this one according to a [NameLinkingPolicy]. /// /// All [Visibility::Public] module-children are inserted, or linked, according to the - /// specified policy; private children will also be inserted, at least including all those - /// used by the copied public children. (At present all module-children are inserted, - /// but this is expected to change in the future.) + /// specified policy; private children used by the public children will also be copied. /// /// # Errors /// @@ -141,9 +139,7 @@ pub trait HugrLinking: HugrMut { /// Copy module-children from another Hugr into this one according to a [NameLinkingPolicy]. /// /// All [Visibility::Public] module-children are copied, or linked, according to the - /// specified policy; private children will also be copied, at least including all those - /// used by the copied public children. (At present all module-children are inserted, - /// but this is expected to change in the future.) + /// specified policy; private children used by the public children will also be copied. /// /// # Errors /// From 44ab7e675b01f35b481f9555401d37a2c50dd24d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 15:37:30 +0000 Subject: [PATCH 24/52] Early return on LinkSig::Private --- hugr-core/src/hugr/linking.rs | 101 +++++++++++++++++----------------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index a0715be90f..503807b4e8 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -480,59 +480,60 @@ impl NameLinkingPolicy { new: LinkSig, ) -> (Result, NameLinkingError>, bool) { let just_add = NodeLinkingDirective::add().into(); - let (nfh, err) = match new { - LinkSig::Private => return (Ok(just_add), true), - LinkSig::Public { - name, - is_defn: new_is_defn, - sig: new_sig, - } => match existing.get(name) { - None => ( - self.new_names, - NameLinkingError::NoNewNames { - name: name.to_string(), - src_node: sn, - }, - ), - Some((existing, ex_sig)) => { - if *ex_sig == new_sig { - match (existing, new_is_defn, self.multi_defn) { - (Either::Left(n), false, _) => { - return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); - } - (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ( - nfh, - NameLinkingError::MultipleDefn(name.to_string(), sn, *n), - ), - (Either::Left(n), true, OnMultiDefn::UseExisting) => { - return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); - } - (Either::Left(n), true, OnMultiDefn::UseNew) => { - return (Ok(NodeLinkingDirective::replace([*n]).into()), false); - } - (Either::Right((n, ns)), _, _) => { - // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) - return ( - Ok(NodeLinkingDirective::replace(once(n).chain(ns).copied()) - .into()), - false, - ); - } + let LinkSig::Public { + name, + is_defn: new_is_defn, + sig: new_sig, + } = new + else { + return (Ok(just_add), true); + }; + let (nfh, err) = match existing.get(name) { + None => ( + self.new_names, + NameLinkingError::NoNewNames { + name: name.to_string(), + src_node: sn, + }, + ), + Some((existing, ex_sig)) => { + if *ex_sig == new_sig { + match (existing, new_is_defn, self.multi_defn) { + (Either::Left(n), false, _) => { + return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); + } + (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ( + nfh, + NameLinkingError::MultipleDefn(name.to_string(), sn, *n), + ), + (Either::Left(n), true, OnMultiDefn::UseExisting) => { + return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); + } + (Either::Left(n), true, OnMultiDefn::UseNew) => { + return (Ok(NodeLinkingDirective::replace([*n]).into()), false); + } + (Either::Right((n, ns)), _, _) => { + // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) + return ( + Ok(NodeLinkingDirective::replace(once(n).chain(ns).copied()) + .into()), + false, + ); } - } else { - ( - self.sig_conflict, - NameLinkingError::SignatureConflict { - name: name.to_string(), - src_node: sn, - src_sig: new_sig.clone().into(), - tgt_node: target_node(existing), - tgt_sig: (*ex_sig).clone().into(), - }, - ) } + } else { + ( + self.sig_conflict, + NameLinkingError::SignatureConflict { + name: name.to_string(), + src_node: sn, + src_sig: new_sig.clone().into(), + tgt_node: target_node(existing), + tgt_sig: (*ex_sig).clone().into(), + }, + ) } - }, + } }; match nfh { OnNewFunc::RaiseError => (Err(err), false), From d04f0bb43f7ccb9406b95035b6ace48d9895f9ed Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 15:37:32 +0000 Subject: [PATCH 25/52] rewrite process to take reached as a bool --- hugr-core/src/hugr/linking.rs | 43 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 503807b4e8..b7f6935e50 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -478,7 +478,8 @@ impl NameLinkingPolicy { existing: &HashMap<&str, PubFuncs>, sn: SN, new: LinkSig, - ) -> (Result, NameLinkingError>, bool) { + reached: bool, + ) -> Option, NameLinkingError>> { let just_add = NodeLinkingDirective::add().into(); let LinkSig::Public { name, @@ -486,7 +487,7 @@ impl NameLinkingPolicy { sig: new_sig, } = new else { - return (Ok(just_add), true); + return reached.then_some(Ok(just_add)); }; let (nfh, err) = match existing.get(name) { None => ( @@ -500,25 +501,22 @@ impl NameLinkingPolicy { if *ex_sig == new_sig { match (existing, new_is_defn, self.multi_defn) { (Either::Left(n), false, _) => { - return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); + return Some(Ok(NodeLinkingDirective::UseExisting(*n).into())); } (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ( nfh, NameLinkingError::MultipleDefn(name.to_string(), sn, *n), ), (Either::Left(n), true, OnMultiDefn::UseExisting) => { - return (Ok(NodeLinkingDirective::UseExisting(*n).into()), false); + return Some(Ok(NodeLinkingDirective::UseExisting(*n).into())); } (Either::Left(n), true, OnMultiDefn::UseNew) => { - return (Ok(NodeLinkingDirective::replace([*n]).into()), false); + return Some(Ok(NodeLinkingDirective::replace([*n]).into())); } (Either::Right((n, ns)), _, _) => { // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) - return ( - Ok(NodeLinkingDirective::replace(once(n).chain(ns).copied()) - .into()), - false, - ); + let nodes = once(n).chain(ns).copied(); + return Some(Ok(NodeLinkingDirective::replace(nodes).into())); } } } else { @@ -535,11 +533,12 @@ impl NameLinkingPolicy { } } }; + match nfh { - OnNewFunc::RaiseError => (Err(err), false), - OnNewFunc::ErrorIfReached => (Err(err), true), - OnNewFunc::Add => (Ok(just_add), false), - OnNewFunc::AddIfReached => (Ok(just_add), true), + OnNewFunc::RaiseError => Some(Err(err)), + OnNewFunc::ErrorIfReached => reached.then_some(Err(err)), + OnNewFunc::Add => Some(Ok(just_add)), + OnNewFunc::AddIfReached => reached.then_some(Ok(just_add)), } } @@ -565,8 +564,7 @@ impl NameLinkingPolicy { for sn in source.children(source.module_root()) { if let Some(ls) = link_sig(source, sn) { // Note we'll call process() again below, so a bit inefficient - let (_, only_if_reached) = self.process(&existing, sn, ls); - if !only_if_reached { + if self.process(&existing, sn, ls, false).is_some() { to_visit.push_back(sn); } } @@ -577,12 +575,13 @@ impl NameLinkingPolicy { continue; }; if let Some(ls) = link_sig(source, sn) { - let act = self.process(&existing, sn, ls).0?; - let LinkAction::LinkNode(dirv) = &act; - let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); - ve.insert(act); - if !traverse { - continue; + if let Some(act) = self.process(&existing, sn, ls, true).transpose()? { + let LinkAction::LinkNode(dirv) = &act; + let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); + ve.insert(act); + if !traverse { + continue; + } } } } From b9982eb8154079a1cd98bae025b96d66305a020a Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 15:51:38 +0000 Subject: [PATCH 26/52] Use reachability only if starting from entrypoint OR private, rm XXXIfReached --- hugr-core/src/hugr/linking.rs | 65 +++++++++++++++-------------------- 1 file changed, 27 insertions(+), 38 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index b7f6935e50..085b47c8cb 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -311,7 +311,7 @@ impl NodeLinkingDirective { /// Describes ways to link a "Source" Hugr being inserted into a target Hugr. #[derive(Clone, Debug, PartialEq, Eq)] pub struct NameLinkingPolicy { - new_names: OnNewFunc, // ALAN default to Add for link_module but AddIfReached for insert_link.... + new_names: OnNewFunc, sig_conflict: OnNewFunc, multi_defn: OnMultiDefn, } @@ -327,15 +327,10 @@ pub struct NameLinkingPolicy { pub enum OnNewFunc { /// Do not link the Hugrs together; fail with a [NameLinkingError] instead. RaiseError, - /// If the new function is reachable, then error; otherwise, just skip it. - ErrorIfReached, /// Add the new function alongside the existing one in the target Hugr, /// preserving (separately) uses of both. (The Hugr will be invalid because /// of [duplicate names](crate::hugr::ValidationError::DuplicateExport).) Add, - /// If the new function is reachable, then add it (as per [Self::Add]); - /// otherwise, skip it. - AddIfReached, } /// What to do when both target and inserted Hugr @@ -478,8 +473,7 @@ impl NameLinkingPolicy { existing: &HashMap<&str, PubFuncs>, sn: SN, new: LinkSig, - reached: bool, - ) -> Option, NameLinkingError>> { + ) -> Result, NameLinkingError> { let just_add = NodeLinkingDirective::add().into(); let LinkSig::Public { name, @@ -487,7 +481,7 @@ impl NameLinkingPolicy { sig: new_sig, } = new else { - return reached.then_some(Ok(just_add)); + return Ok(just_add); }; let (nfh, err) = match existing.get(name) { None => ( @@ -501,22 +495,22 @@ impl NameLinkingPolicy { if *ex_sig == new_sig { match (existing, new_is_defn, self.multi_defn) { (Either::Left(n), false, _) => { - return Some(Ok(NodeLinkingDirective::UseExisting(*n).into())); + return Ok(NodeLinkingDirective::UseExisting(*n).into()); } (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ( nfh, NameLinkingError::MultipleDefn(name.to_string(), sn, *n), ), (Either::Left(n), true, OnMultiDefn::UseExisting) => { - return Some(Ok(NodeLinkingDirective::UseExisting(*n).into())); + return Ok(NodeLinkingDirective::UseExisting(*n).into()); } (Either::Left(n), true, OnMultiDefn::UseNew) => { - return Some(Ok(NodeLinkingDirective::replace([*n]).into())); + return Ok(NodeLinkingDirective::replace([*n]).into()); } (Either::Right((n, ns)), _, _) => { // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) let nodes = once(n).chain(ns).copied(); - return Some(Ok(NodeLinkingDirective::replace(nodes).into())); + return Ok(NodeLinkingDirective::replace(nodes).into()); } } } else { @@ -535,10 +529,8 @@ impl NameLinkingPolicy { }; match nfh { - OnNewFunc::RaiseError => Some(Err(err)), - OnNewFunc::ErrorIfReached => reached.then_some(Err(err)), - OnNewFunc::Add => Some(Ok(just_add)), - OnNewFunc::AddIfReached => reached.then_some(Ok(just_add)), + OnNewFunc::RaiseError => Err(err), + OnNewFunc::Add => Ok(just_add), } } @@ -546,7 +538,7 @@ impl NameLinkingPolicy { /// (host) Hugr. /// /// `use_entrypoint` tells whether the entrypoint subtree will be inserted (alongside - /// any linking). + /// any linking). If so, unreached public functions are ignored. #[allow(clippy::type_complexity)] pub fn to_node_linking_for( &self, @@ -554,35 +546,32 @@ impl NameLinkingPolicy { source: &S, use_entrypoint: bool, ) -> Result, NameLinkingError> { - //let mut source_funcs = source_funcs.into_iter(); let existing = gather_existing(target); let cg = CallGraph::new(&source); // Can't use petgraph Dfs as we need to avoid traversing through some nodes, // and we need to maintain our own `visited` map anyway - let mut to_visit = VecDeque::from_iter(use_entrypoint.then_some(source.entrypoint())); - let mut res = LinkActions::new(); - for sn in source.children(source.module_root()) { - if let Some(ls) = link_sig(source, sn) { - // Note we'll call process() again below, so a bit inefficient - if self.process(&existing, sn, ls, false).is_some() { - to_visit.push_back(sn); - } - } + let mut to_visit = VecDeque::new(); + if use_entrypoint { + to_visit.push_back(source.entrypoint()); + } else { + to_visit.extend( + source + .children(source.module_root()) + .filter(|&sn| matches!(link_sig(source, sn), Some(LinkSig::Public { .. }))), + ); } + let mut res = LinkActions::new(); while let Some(sn) = to_visit.pop_front() { if !(use_entrypoint && sn == source.entrypoint()) { - let Entry::Vacant(ve) = res.entry(sn) else { + let (Entry::Vacant(ve), Some(ls)) = (res.entry(sn), link_sig(source, sn)) else { continue; }; - if let Some(ls) = link_sig(source, sn) { - if let Some(act) = self.process(&existing, sn, ls, true).transpose()? { - let LinkAction::LinkNode(dirv) = &act; - let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); - ve.insert(act); - if !traverse { - continue; - } - } + let act = self.process(&existing, sn, ls)?; + let LinkAction::LinkNode(dirv) = &act; + let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); + ve.insert(act); + if !traverse { + continue; } } // For entrypoint, *just* traverse From d5622d8fd189070695a3767afd5c8c8c584d7333 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 16:21:08 +0000 Subject: [PATCH 27/52] hide to_node_linking_helper, pub to_node_linking_for_entrypoint (no bools) --- hugr-core/src/hugr/linking.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 085b47c8cb..2304b9e283 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -211,7 +211,7 @@ pub trait HugrLinking: HugrMut { // return Err(format!("Entrypoint is a top-level function")) //} let pol = policy - .to_node_linking_for(&*self, &other, true) + .to_node_linking_for_entrypoint(&*self, &other) .map_err(|e| e.to_string())?; if matches!( pol.get(&entrypoint_func), @@ -463,7 +463,7 @@ impl NameLinkingPolicy { target: &T, source: &S, ) -> Result, NameLinkingError> { - self.to_node_linking_for(target, source, false) + self.to_node_linking_helper(target, source, false) } /// The result is Ok((action, bool)) where the bool being true @@ -534,13 +534,19 @@ impl NameLinkingPolicy { } } - /// Computes how this policy will act on a specified source (inserted) and target - /// (host) Hugr. - /// - /// `use_entrypoint` tells whether the entrypoint subtree will be inserted (alongside - /// any linking). If so, unreached public functions are ignored. + /// Computes how this policy will act when inserting the entrypoint-subtree of a + /// specified source Hugr into a target (host) Hugr (as per [LinkHugr::insert_link_hugr]). + #[allow(clippy::type_complexity)] + pub fn to_node_linking_for_entrypoint( + &self, + target: &T, + source: &S, + ) -> Result, NameLinkingError> { + self.to_node_linking_helper(target, source, true) + } + #[allow(clippy::type_complexity)] - pub fn to_node_linking_for( + fn to_node_linking_helper( &self, target: &T, source: &S, From 35a4a5260dfb162c38c4e5f4fd6faa995297c4fa Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 16:31:10 +0000 Subject: [PATCH 28/52] Use closure rather than returning (nfh, err) --- hugr-core/src/hugr/linking.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 2304b9e283..394f25492e 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -483,8 +483,12 @@ impl NameLinkingPolicy { else { return Ok(just_add); }; - let (nfh, err) = match existing.get(name) { - None => ( + let ret = |onf: OnNewFunc, e| match onf { + OnNewFunc::RaiseError => Err(e), + OnNewFunc::Add => Ok(just_add), + }; + match existing.get(name) { + None => ret( self.new_names, NameLinkingError::NoNewNames { name: name.to_string(), @@ -495,26 +499,26 @@ impl NameLinkingPolicy { if *ex_sig == new_sig { match (existing, new_is_defn, self.multi_defn) { (Either::Left(n), false, _) => { - return Ok(NodeLinkingDirective::UseExisting(*n).into()); + Ok(NodeLinkingDirective::UseExisting(*n).into()) } - (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ( + (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ret( nfh, NameLinkingError::MultipleDefn(name.to_string(), sn, *n), ), (Either::Left(n), true, OnMultiDefn::UseExisting) => { - return Ok(NodeLinkingDirective::UseExisting(*n).into()); + Ok(NodeLinkingDirective::UseExisting(*n).into()) } (Either::Left(n), true, OnMultiDefn::UseNew) => { - return Ok(NodeLinkingDirective::replace([*n]).into()); + Ok(NodeLinkingDirective::replace([*n]).into()) } (Either::Right((n, ns)), _, _) => { // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) let nodes = once(n).chain(ns).copied(); - return Ok(NodeLinkingDirective::replace(nodes).into()); + Ok(NodeLinkingDirective::replace(nodes).into()) } } } else { - ( + ret( self.sig_conflict, NameLinkingError::SignatureConflict { name: name.to_string(), @@ -526,11 +530,6 @@ impl NameLinkingPolicy { ) } } - }; - - match nfh { - OnNewFunc::RaiseError => Err(err), - OnNewFunc::Add => Ok(just_add), } } From c3cd9ce9dee8a981c31e53f72056da74e693cfa7 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 16:43:14 +0000 Subject: [PATCH 29/52] Rewrite process more sequentially --- hugr-core/src/hugr/linking.rs | 72 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 394f25492e..ae78f90749 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -483,53 +483,49 @@ impl NameLinkingPolicy { else { return Ok(just_add); }; - let ret = |onf: OnNewFunc, e| match onf { + let chk_add = |onf: OnNewFunc, e| match onf { OnNewFunc::RaiseError => Err(e), OnNewFunc::Add => Ok(just_add), }; - match existing.get(name) { - None => ret( + let Some((existing, ex_sig)) = existing.get(name) else { + return chk_add( self.new_names, NameLinkingError::NoNewNames { name: name.to_string(), src_node: sn, }, - ), - Some((existing, ex_sig)) => { - if *ex_sig == new_sig { - match (existing, new_is_defn, self.multi_defn) { - (Either::Left(n), false, _) => { - Ok(NodeLinkingDirective::UseExisting(*n).into()) - } - (Either::Left(n), true, OnMultiDefn::NewFunc(nfh)) => ret( - nfh, - NameLinkingError::MultipleDefn(name.to_string(), sn, *n), - ), - (Either::Left(n), true, OnMultiDefn::UseExisting) => { - Ok(NodeLinkingDirective::UseExisting(*n).into()) - } - (Either::Left(n), true, OnMultiDefn::UseNew) => { - Ok(NodeLinkingDirective::replace([*n]).into()) - } - (Either::Right((n, ns)), _, _) => { - // Replace all existing decls. (If the new node is a decl, we only need to add, so tidy as we go.) - let nodes = once(n).chain(ns).copied(); - Ok(NodeLinkingDirective::replace(nodes).into()) - } - } - } else { - ret( - self.sig_conflict, - NameLinkingError::SignatureConflict { - name: name.to_string(), - src_node: sn, - src_sig: new_sig.clone().into(), - tgt_node: target_node(existing), - tgt_sig: (*ex_sig).clone().into(), - }, - ) - } + ); + }; + if *ex_sig != new_sig { + return chk_add( + self.sig_conflict, + NameLinkingError::SignatureConflict { + name: name.to_string(), + src_node: sn, + src_sig: new_sig.clone().into(), + tgt_node: target_node(existing), + tgt_sig: (*ex_sig).clone().into(), + }, + ); + } + let ex_defn = match existing { + Either::Right((n, ns)) => { + // Replace all existing decls. (If the new node is a decl, we only need to add, but tidy as we go.) + let nodes = once(n).chain(ns).copied(); + return Ok(NodeLinkingDirective::replace(nodes).into()); } + Either::Left(n) => *n, + }; + if !new_is_defn { + return Ok(NodeLinkingDirective::UseExisting(ex_defn).into()); + } + match self.multi_defn { + OnMultiDefn::NewFunc(nfh) => chk_add( + nfh, + NameLinkingError::MultipleDefn(name.to_string(), sn, ex_defn), + ), + OnMultiDefn::UseExisting => Ok(NodeLinkingDirective::UseExisting(ex_defn).into()), + OnMultiDefn::UseNew => Ok(NodeLinkingDirective::replace([ex_defn]).into()), } } From aeb2ea288e5d09d533e579c841db2f4d71974048 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 21:08:55 +0000 Subject: [PATCH 30/52] Use errors; add insert_link_from_view, commoning into to_node_linking_for_entrypoint --- hugr-core/src/hugr/linking.rs | 179 ++++++++++++++++++++++++++-------- 1 file changed, 136 insertions(+), 43 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index ae78f90749..861a53bc29 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -196,31 +196,8 @@ pub trait HugrLinking: HugrMut { parent: Self::Node, other: Hugr, policy: &NameLinkingPolicy, - ) -> Result, String> { - let entrypoint_func = { - let mut n = other.entrypoint(); - loop { - let p = other.get_parent(n).ok_or("Entrypoint is module root")?; - if p == other.module_root() { - break n; - }; - n = p; - } - }; - //if entrypoint_func == other.entrypoint() { // Do we need to check this? Ok if parent is self.module_root() ?? - // return Err(format!("Entrypoint is a top-level function")) - //} - let pol = policy - .to_node_linking_for_entrypoint(&*self, &other) - .map_err(|e| e.to_string())?; - if matches!( - pol.get(&entrypoint_func), - Some(LinkAction::LinkNode(NodeLinkingDirective::Add { .. })) if entrypoint_func != other.entrypoint() - ) { - return Err(format!( - "Entrypoint is contained in function {entrypoint_func} which would be inserted" - )); - } + ) -> Result, NameLinkingError> { + let pol = policy.to_node_linking_for_entrypoint(&*self, &other)?; let per_node = pol .into_iter() .map(|(k, LinkAction::LinkNode(v))| (k, v)) @@ -229,6 +206,48 @@ pub trait HugrLinking: HugrMut { .insert_link_hugr_by_node(Some(parent), other, per_node) .expect("NodeLinkingPolicy was constructed to avoid any error")) } + + /// Inserts the entrypoint-subtree of another Hugr into this one, + /// including copying any private functions it calls and using linking + /// to resolve any public functions. + /// + /// `parent` is the parent node under which to insert the entrypoint. + /// + /// # Errors + /// + /// If other's entrypoint is its module-root (recommend using [Self::link_module] instead) + /// + /// If other's entrypoint calls (perhaps transitively) the function containing said entrypoint + /// (an exception is made if the called+containing function is public and is being replaced + /// by an equivalent in `self` via [OnMultiDefn::UseExisting], in which case + /// the call is redirected to the existing function, and the new one is not inserted + /// except the entrypoint subtree.) + /// + /// If other's entrypoint calls a public function in `other` which + /// * has a name or signature different to any in `self`, and `allow_new_names` is `None` + /// * has a name equal to that in `self`, but a different signature, and `allow_new_names` is + /// `Some(`[`SignatureConflictHandling::ErrorDontInsert`]`)` + /// + /// If other's entrypoint calls a public [`FuncDefn`] in `other` which has the same name + /// and signature as a public [`FuncDefn`] in `self` and `allow_new_impls` is + /// `Some(`[`OnMultiDefn::ErrorDontInsert`]`)` + /// + /// [`FuncDefn`]: crate::ops::FuncDefn + fn insert_link_from_view( + &mut self, + parent: Self::Node, + other: &H, + policy: &NameLinkingPolicy, + ) -> Result, NameLinkingError> { + let pol = policy.to_node_linking_for_entrypoint(&*self, &other)?; + let per_node = pol + .into_iter() + .map(|(k, LinkAction::LinkNode(v))| (k, v)) + .collect(); + Ok(self + .insert_link_view_by_node(Some(parent), other, per_node) + .expect("NodeLinkingPolicy was constructed to avoid any error")) + } } impl HugrLinking for T {} @@ -379,12 +398,15 @@ pub enum NameLinkingError { #[allow(missing_docs)] NoNewNames { name: String, src_node: SN }, /// A [Visibility::Public] function in the source, whose body is being added - /// to the target, contained the entrypoint (which needs to be added - /// in a different place). + /// to the target, contained the entrypoint which is being added in a different place + /// (in a call to [LinkHugr::insert_link_hugr] or [LinkHugr::insert_link_from_view]). /// /// [Visibility::Public]: crate::Visibility::Public #[error("The entrypoint is contained within function {_0} which will be added as {_1:?}")] AddFunctionContainingEntrypoint(SN, NodeLinkingDirective), + /// The source Hugr's entrypoint is its module-root, in a call to [LinkHugr::insert_link_hugr] or [LinkHugr::insert_link_from_view]. + #[error("The source Hugr's entrypoint is its module-root")] + InsertEntrypointIsModuleRoot, } impl NameLinkingPolicy { @@ -537,7 +559,32 @@ impl NameLinkingPolicy { target: &T, source: &S, ) -> Result, NameLinkingError> { - self.to_node_linking_helper(target, source, true) + let entrypoint_func = { + let mut n = source.entrypoint(); + loop { + let p = source + .get_parent(n) + .ok_or(NameLinkingError::InsertEntrypointIsModuleRoot)?; + if p == source.module_root() { + break n; + }; + n = p; + } + }; + //if entrypoint_func == other.entrypoint() { // Do we need to check this? Ok if parent is self.module_root() ?? + // return Err(format!("Entrypoint is a top-level function")) + //} + let pol = self.to_node_linking_helper(target, source, true)?; + if let Some(LinkAction::LinkNode(add @ NodeLinkingDirective::Add { .. })) = + pol.get(&entrypoint_func) + && entrypoint_func != source.entrypoint() + { + return Err(NameLinkingError::AddFunctionContainingEntrypoint( + entrypoint_func, + add.clone(), + )); + } + Ok(pol) } #[allow(clippy::type_complexity)] @@ -554,6 +601,21 @@ impl NameLinkingPolicy { let mut to_visit = VecDeque::new(); if use_entrypoint { to_visit.push_back(source.entrypoint()); + // Also add public defns not reachable from entrypoint that implement existing decls + // (ALAN this is a big API question. They might only be reachable from (the entrypoint via) + // functions in the target hugr...or not reachable at all; and they may reach other functions + // that cause problems) + to_visit.extend( + source + .children(source.module_root()) + .filter(|&sn| { + match link_sig(source, sn) { + Some(LinkSig::Public { is_defn: true, name, sig }) => + matches!(existing.get(name), Some((Either::Right(_), ex_sig)) if ex_sig == &sig), + _ => false, + } + }), + ); } else { to_visit.extend( source @@ -1053,7 +1115,7 @@ mod test { let i64_t = || INT_TYPES[6].to_owned(); let foo_sig = Signature::new_endo(i64_t()); let bar_sig = Signature::new(vec![i64_t(); 2], i64_t()); - let mut target = { + let def_foo = { let mut fb = FunctionBuilder::new_vis("foo", foo_sig.clone(), Visibility::Public).unwrap(); let mut mb = fb.module_root_builder(); @@ -1073,13 +1135,8 @@ mod test { h }; - let inserted = { - let mut main_b = FunctionBuilder::new_vis( - "main", - Signature::new(vec![], i64_t()), - Visibility::Public, - ) - .unwrap(); + let main_def_bar = { + let mut main_b = FunctionBuilder::new("main", Signature::new(vec![], i64_t())).unwrap(); let mut mb = main_b.module_root_builder(); let foo1 = mb.declare("foo", foo_sig.clone().into()).unwrap(); let foo2 = mb.declare("foo", foo_sig.clone().into()).unwrap(); @@ -1106,19 +1163,38 @@ mod test { let pol = NameLinkingPolicy::err_on_conflict(multi_defn).on_signature_conflict(sig_conflict); - let mut target2 = target.clone(); + // Insert def_foo into main_def_bar + let mut has_main1 = main_def_bar.clone(); + has_main1.link_module_view(&def_foo, &pol).unwrap(); + let mut has_main2 = main_def_bar.clone(); + has_main2.link_module(def_foo.clone(), &pol).unwrap(); + // Insert main_def_bar into def_foo + let mut no_main1 = def_foo.clone(); + no_main1.link_module_view(&main_def_bar, &pol).unwrap(); + let mut no_main2 = def_foo.clone(); + no_main2.link_module(main_def_bar.clone(), &pol).unwrap(); + // Insert main_def_bar into def_foo, explicitly adding main + let mut has_main3 = def_foo.clone(); + has_main3 + .insert_link_from_view(has_main3.module_root(), &main_def_bar, &pol) + .unwrap(); + let mut has_main4 = def_foo; + has_main4 + .insert_link_hugr(has_main4.module_root(), main_def_bar, &pol) + .unwrap(); - target.link_module_view(&inserted, &pol).unwrap(); - target2.link_module(inserted, &pol).unwrap(); - for tgt in [target, target2] { - tgt.validate().unwrap(); - let (decls, defns) = list_decls_defns(&tgt); + let mut count = 0; + for hugr in [&has_main1, &has_main2, &has_main3, &has_main4] { + eprintln!("ALAN count={count}"); + count += 1; + hugr.validate().unwrap(); + let (decls, defns) = list_decls_defns(hugr); assert_eq!(decls, HashMap::new()); assert_eq!( defns.values().copied().sorted().collect_vec(), ["bar", "foo", "main"] ); - let call_tgts = call_targets(&tgt); + let call_tgts = call_targets(&hugr); for (defn, name) in defns { if name != "main" { // Defns now have two calls each (was one to each alias) @@ -1126,6 +1202,23 @@ mod test { } } } + for hugr in [no_main1, no_main2] { + let (decls, defns) = list_decls_defns(&hugr); + assert_eq!(decls, HashMap::new()); + assert_eq!( + defns.values().copied().sorted().collect_vec(), + ["bar", "foo"] + ); + let call_tgts = call_targets(&hugr); + for (defn, name) in defns { + let expected = if name == "foo" { 0 } else { 2 }; + // Defns now have two calls each (was one to each alias) + assert_eq!( + call_tgts.values().filter(|tgt| **tgt == defn).count(), + expected + ); + } + } } #[rstest] From 8b9a23f78efafae3d2229145f4d6a21bc9cf7c3b Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 21:27:11 +0000 Subject: [PATCH 31/52] extend combines_decls_defn test --- hugr-core/src/hugr/linking.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 861a53bc29..c5ea9a8da6 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1183,10 +1183,7 @@ mod test { .insert_link_hugr(has_main4.module_root(), main_def_bar, &pol) .unwrap(); - let mut count = 0; for hugr in [&has_main1, &has_main2, &has_main3, &has_main4] { - eprintln!("ALAN count={count}"); - count += 1; hugr.validate().unwrap(); let (decls, defns) = list_decls_defns(hugr); assert_eq!(decls, HashMap::new()); From 0d476f1df024e29b933634a2a52416582f531dce Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 21:59:12 +0000 Subject: [PATCH 32/52] basic insert_link test, note TODO --- hugr-core/src/hugr/linking.rs | 49 ++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index c5ea9a8da6..c44f2164f1 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -233,6 +233,7 @@ pub trait HugrLinking: HugrMut { /// `Some(`[`OnMultiDefn::ErrorDontInsert`]`)` /// /// [`FuncDefn`]: crate::ops::FuncDefn + #[allow(clippy::type_complexity)] fn insert_link_from_view( &mut self, parent: Self::Node, @@ -840,7 +841,7 @@ mod test { use crate::builder::test::{dfg_calling_defn_decl, simple_dfg_hugr}; use crate::builder::{ Container, Dataflow, DataflowHugr, DataflowSubContainer, FunctionBuilder, HugrBuilder, - ModuleBuilder, + ModuleBuilder, endo_sig, }; use crate::extension::prelude::{ConstUsize, usize_t}; use crate::hugr::hugrmut::test::check_calls_defn_decl; @@ -1332,4 +1333,50 @@ mod test { .collect(); assert_eq!(all_consts, expect_exist); } + + #[test] + fn insert_link() { + let insert = { + let mut mb = ModuleBuilder::new(); + let reached = mb.declare("foo", endo_sig(usize_t()).into()).unwrap(); + let unreached = mb.declare("bar", endo_sig(usize_t()).into()).unwrap(); + let mut outer = mb.define_function("outer", endo_sig(usize_t())).unwrap(); + let [i] = outer.input_wires_arr(); + let [i] = outer.call(&unreached, &[], [i]).unwrap().outputs_arr(); + let mut dfb = outer.dfg_builder(endo_sig(usize_t()), [i]).unwrap(); + let [i] = dfb.input_wires_arr(); + let call = dfb.call(&reached, &[], [i]).unwrap(); + let dfg = dfb.finish_with_outputs(call.outputs()).unwrap(); + outer.finish_with_outputs(dfg.outputs()).unwrap(); + let mut h = mb.finish_hugr().unwrap(); + h.set_entrypoint(dfg.node()); + h + }; + let mut fb = FunctionBuilder::new("main", endo_sig(usize_t())).unwrap(); + let [i] = fb.input_wires_arr(); + let cst = fb.add_load_value(ConstUsize::new(42)); + let mut host = fb.finish_hugr_with_outputs([cst]).unwrap(); + + // TODO no good equivalent of pytest parametrized fixtures here... + // crate rstest_reuse is one way, but seems heavy for just this??? + let any_pol = NameLinkingPolicy::default(); + + let ins = host + .insert_link_from_view(host.entrypoint(), &insert, &any_pol) + .unwrap(); + let dfg = *ins.node_map.get(&insert.entrypoint()).unwrap(); + assert!(host.get_optype(dfg).is_dfg()); + host.connect(i.node(), i.source(), dfg, 0); + host.validate().unwrap(); + let (decls, defns) = list_decls_defns(&host); + assert_eq!(decls.values().collect_vec(), [&"foo"]); // unreached bar not copied + assert_eq!(defns.values().collect_vec(), [&"main"]); // as originally in host + let (call, tgt) = call_targets(&host).into_iter().exactly_one().unwrap(); + assert_eq!(host.get_parent(call), Some(dfg)); + assert_eq!( + host.get_parent(dfg), + Some(defns.into_keys().exactly_one().unwrap()) + ); + assert_eq!(tgt, decls.into_keys().exactly_one().unwrap()); + } } From e0f283252882b75349ecfaa2dc8efe91b79f590e Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 22:09:26 +0000 Subject: [PATCH 33/52] doc fixes --- hugr-core/src/call_graph.rs | 8 ++++---- hugr-core/src/hugr/linking.rs | 35 ++++++++++++++++++++++------------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/hugr-core/src/call_graph.rs b/hugr-core/src/call_graph.rs index 87d1ebb09b..df3b8a4275 100644 --- a/hugr-core/src/call_graph.rs +++ b/hugr-core/src/call_graph.rs @@ -27,7 +27,7 @@ pub enum CallGraphNode { /// either, as such a node could not have edges, so is not represented in the petgraph. NonFuncEntrypoint, /// petgraph-node corresponds to a constant; will have no outgoing edges, and incoming - /// edges will be [CallGraphEdge::Const] + /// edges will be [CallGraphEdge::LoadConstant] Const(N), } @@ -37,9 +37,9 @@ pub enum CallGraphNode { /// each edge corresponds to a [`Call`]/[`LoadFunction`] of the edge's target, contained in /// the edge's source. /// -/// For Hugrs whose entrypoint is neither a [Module](OpType::Module) nor a [`FuncDefn`], the -/// call graph will have an additional [`CallGraphNode::NonFuncRoot`] corresponding to the Hugr's -/// entrypoint, with no incoming edges. +/// For Hugrs whose entrypoint is neither a [Module](OpType::Module) nor a [`FuncDefn`], +/// the call graph will have an additional [`CallGraphNode::NonFuncEntrypoint`] +/// corresponding to the Hugr's entrypoint, with no incoming edges. /// /// [`Call`]: OpType::Call /// [`FuncDecl`]: OpType::FuncDecl diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index c44f2164f1..257d2019ca 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -182,15 +182,19 @@ pub trait HugrLinking: HugrMut { /// except the entrypoint subtree.) /// /// If other's entrypoint calls a public function in `other` which - /// * has a name or signature different to any in `self`, and `allow_new_names` is `None` - /// * has a name equal to that in `self`, but a different signature, and `allow_new_names` is - /// `Some(`[`SignatureConflictHandling::ErrorDontInsert`]`)` + /// * has a name or signature different to any in `self`, and [`on_new_names`] is + /// [`OnNewFunc::RaiseError`] + /// * has a name equal to that in `self`, but a different signature, and [`on_sig_conflict`] is + /// [`OnNewFunc::RaiseError`] /// /// If other's entrypoint calls a public [`FuncDefn`] in `other` which has the same name - /// and signature as a public [`FuncDefn`] in `self` and `allow_new_impls` is - /// `Some(`[`OnMultiDefn::ErrorDontInsert`]`)` + /// and signature as a public [`FuncDefn`] in `self` and [`on_multi_defn`] is + /// [`OnMultiDefn::NewFunc`] of [`OnNewFunc::RaiseError`] /// /// [`FuncDefn`]: crate::ops::FuncDefn + /// [`on_new_names`]: NameLinkingPolicy::get_on_new_names + /// [`on_multi_defn`]: NameLinkingPolicy::get_on_multiple_defn + /// [`on_sig_conflict`]: NameLinkingPolicy::get_signature_conflict fn insert_link_hugr( &mut self, parent: Self::Node, @@ -224,15 +228,19 @@ pub trait HugrLinking: HugrMut { /// except the entrypoint subtree.) /// /// If other's entrypoint calls a public function in `other` which - /// * has a name or signature different to any in `self`, and `allow_new_names` is `None` - /// * has a name equal to that in `self`, but a different signature, and `allow_new_names` is - /// `Some(`[`SignatureConflictHandling::ErrorDontInsert`]`)` + /// * has a name or signature different to any in `self`, and [`on_new_names`] is + /// [`OnNewFunc::RaiseError`] + /// * has a name equal to that in `self`, but a different signature, and [`on_sig_conflict`] is + /// [`OnNewFunc::RaiseError`] /// /// If other's entrypoint calls a public [`FuncDefn`] in `other` which has the same name - /// and signature as a public [`FuncDefn`] in `self` and `allow_new_impls` is - /// `Some(`[`OnMultiDefn::ErrorDontInsert`]`)` + /// and signature as a public [`FuncDefn`] in `self` and [`on_multi_defn`] is + /// [`OnMultiDefn::NewFunc`] of [`OnNewFunc::RaiseError`] /// /// [`FuncDefn`]: crate::ops::FuncDefn + /// [`on_new_names`]: NameLinkingPolicy::get_on_new_names + /// [`on_multi_defn`]: NameLinkingPolicy::get_on_multiple_defn + /// [`on_sig_conflict`]: NameLinkingPolicy::get_signature_conflict #[allow(clippy::type_complexity)] fn insert_link_from_view( &mut self, @@ -293,6 +301,7 @@ pub enum NodeLinkingDirective { /// at most one [FuncDefn], or perhaps-multiple, aliased, [FuncDecl]s.) /// /// [FuncDefn]: crate::ops::FuncDefn + /// [FuncDecl]: crate::ops::FuncDecl /// [EdgeKind::Const]: crate::types::EdgeKind::Const /// [EdgeKind::Function]: crate::types::EdgeKind::Function replace: Vec, @@ -400,12 +409,12 @@ pub enum NameLinkingError { NoNewNames { name: String, src_node: SN }, /// A [Visibility::Public] function in the source, whose body is being added /// to the target, contained the entrypoint which is being added in a different place - /// (in a call to [LinkHugr::insert_link_hugr] or [LinkHugr::insert_link_from_view]). + /// (in a call to [HugrLinking::insert_link_hugr] or [HugrLinking::insert_link_from_view]). /// /// [Visibility::Public]: crate::Visibility::Public #[error("The entrypoint is contained within function {_0} which will be added as {_1:?}")] AddFunctionContainingEntrypoint(SN, NodeLinkingDirective), - /// The source Hugr's entrypoint is its module-root, in a call to [LinkHugr::insert_link_hugr] or [LinkHugr::insert_link_from_view]. + /// The source Hugr's entrypoint is its module-root, in a call to [HugrLinking::insert_link_hugr] or [HugrLinking::insert_link_from_view]. #[error("The source Hugr's entrypoint is its module-root")] InsertEntrypointIsModuleRoot, } @@ -553,7 +562,7 @@ impl NameLinkingPolicy { } /// Computes how this policy will act when inserting the entrypoint-subtree of a - /// specified source Hugr into a target (host) Hugr (as per [LinkHugr::insert_link_hugr]). + /// specified source Hugr into a target (host) Hugr (as per [HugrLinking::insert_link_hugr]). #[allow(clippy::type_complexity)] pub fn to_node_linking_for_entrypoint( &self, From 5385efdde96bda07032e5067a7db361ecfeab9ea Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 22:25:40 +0000 Subject: [PATCH 34/52] ws layout --- hugr-core/src/hugr/linking.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 257d2019ca..a67dac49fb 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -183,7 +183,7 @@ pub trait HugrLinking: HugrMut { /// /// If other's entrypoint calls a public function in `other` which /// * has a name or signature different to any in `self`, and [`on_new_names`] is - /// [`OnNewFunc::RaiseError`] + /// [`OnNewFunc::RaiseError`] /// * has a name equal to that in `self`, but a different signature, and [`on_sig_conflict`] is /// [`OnNewFunc::RaiseError`] /// @@ -229,7 +229,7 @@ pub trait HugrLinking: HugrMut { /// /// If other's entrypoint calls a public function in `other` which /// * has a name or signature different to any in `self`, and [`on_new_names`] is - /// [`OnNewFunc::RaiseError`] + /// [`OnNewFunc::RaiseError`] /// * has a name equal to that in `self`, but a different signature, and [`on_sig_conflict`] is /// [`OnNewFunc::RaiseError`] /// @@ -414,7 +414,8 @@ pub enum NameLinkingError { /// [Visibility::Public]: crate::Visibility::Public #[error("The entrypoint is contained within function {_0} which will be added as {_1:?}")] AddFunctionContainingEntrypoint(SN, NodeLinkingDirective), - /// The source Hugr's entrypoint is its module-root, in a call to [HugrLinking::insert_link_hugr] or [HugrLinking::insert_link_from_view]. + /// The source Hugr's entrypoint is its module-root, in a call to + /// [HugrLinking::insert_link_hugr] or [HugrLinking::insert_link_from_view]. #[error("The source Hugr's entrypoint is its module-root")] InsertEntrypointIsModuleRoot, } From 97f8a047a1760425b0de23b69099a10cb999cc4c Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 17 Nov 2025 22:26:45 +0000 Subject: [PATCH 35/52] process->action_for, remove obsolete comment --- hugr-core/src/hugr/linking.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index a67dac49fb..dddbed8b89 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -499,9 +499,7 @@ impl NameLinkingPolicy { self.to_node_linking_helper(target, source, false) } - /// The result is Ok((action, bool)) where the bool being true - /// means the action is ONLY needed if the function is reached. - fn process( + fn action_for( &self, existing: &HashMap<&str, PubFuncs>, sn: SN, @@ -640,7 +638,7 @@ impl NameLinkingPolicy { let (Entry::Vacant(ve), Some(ls)) = (res.entry(sn), link_sig(source, sn)) else { continue; }; - let act = self.process(&existing, sn, ls)?; + let act = self.action_for(&existing, sn, ls)?; let LinkAction::LinkNode(dirv) = &act; let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); ve.insert(act); From 016c201d1677bcff82b69da44dbaf5ed05a8ffb7 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 18 Nov 2025 08:50:54 +0000 Subject: [PATCH 36/52] No matching defns for insert_link; complexify combines_decls_defn test --- hugr-core/src/hugr/linking.rs | 72 ++++++++++++----------------------- 1 file changed, 25 insertions(+), 47 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index dddbed8b89..fd4e33f9d2 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -610,21 +610,6 @@ impl NameLinkingPolicy { let mut to_visit = VecDeque::new(); if use_entrypoint { to_visit.push_back(source.entrypoint()); - // Also add public defns not reachable from entrypoint that implement existing decls - // (ALAN this is a big API question. They might only be reachable from (the entrypoint via) - // functions in the target hugr...or not reachable at all; and they may reach other functions - // that cause problems) - to_visit.extend( - source - .children(source.module_root()) - .filter(|&sn| { - match link_sig(source, sn) { - Some(LinkSig::Public { is_defn: true, name, sig }) => - matches!(existing.get(name), Some((Either::Right(_), ex_sig)) if ex_sig == &sig), - _ => false, - } - }), - ); } else { to_visit.extend( source @@ -1183,45 +1168,38 @@ mod test { let mut no_main2 = def_foo.clone(); no_main2.link_module(main_def_bar.clone(), &pol).unwrap(); // Insert main_def_bar into def_foo, explicitly adding main - let mut has_main3 = def_foo.clone(); - has_main3 - .insert_link_from_view(has_main3.module_root(), &main_def_bar, &pol) + let mut main_no_bar1 = def_foo.clone(); + main_no_bar1 + .insert_link_from_view(main_no_bar1.module_root(), &main_def_bar, &pol) .unwrap(); - let mut has_main4 = def_foo; - has_main4 - .insert_link_hugr(has_main4.module_root(), main_def_bar, &pol) + let mut main_no_bar2 = def_foo; + main_no_bar2 + .insert_link_hugr(main_no_bar2.module_root(), main_def_bar, &pol) .unwrap(); - for hugr in [&has_main1, &has_main2, &has_main3, &has_main4] { + for (hugr, exp_decls, exp_defns) in [ + (&has_main1, vec![], vec!["bar", "foo", "main"]), + (&has_main2, vec![], vec!["bar", "foo", "main"]), + (&main_no_bar1, vec!["bar", "bar"], vec!["foo", "main"]), + (&main_no_bar2, vec!["bar", "bar"], vec!["foo", "main"]), + (&no_main1, vec![], vec!["bar", "foo"]), + (&no_main2, vec![], vec!["bar", "foo"]), + ] { hugr.validate().unwrap(); let (decls, defns) = list_decls_defns(hugr); - assert_eq!(decls, HashMap::new()); - assert_eq!( - defns.values().copied().sorted().collect_vec(), - ["bar", "foo", "main"] - ); + assert_eq!(decls.values().copied().sorted().collect_vec(), exp_decls); + assert_eq!(defns.values().copied().sorted().collect_vec(), exp_defns); let call_tgts = call_targets(&hugr); - for (defn, name) in defns { - if name != "main" { - // Defns now have two calls each (was one to each alias) - assert_eq!(call_tgts.values().filter(|tgt| **tgt == defn).count(), 2); - } - } - } - for hugr in [no_main1, no_main2] { - let (decls, defns) = list_decls_defns(&hugr); - assert_eq!(decls, HashMap::new()); - assert_eq!( - defns.values().copied().sorted().collect_vec(), - ["bar", "foo"] - ); - let call_tgts = call_targets(&hugr); - for (defn, name) in defns { - let expected = if name == "foo" { 0 } else { 2 }; - // Defns now have two calls each (was one to each alias) + for (func, name) in defns.into_iter().chain(decls) { + let expected_calls = match name { + "bar" => 1 + exp_defns.contains(&"bar") as usize, // decls still separate + "foo" => (exp_defns.contains(&"main") as usize) * 2, // called from main + _ => 0, + }; assert_eq!( - call_tgts.values().filter(|tgt| **tgt == defn).count(), - expected + call_tgts.values().filter(|tgt| **tgt == func).count(), + expected_calls, + "for function {name}" ); } } From ddfd0a7dc39c357cda47dd7b573be11c30ec35fb Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 21 Nov 2025 10:49:26 +0000 Subject: [PATCH 37/52] test no_new_names_module --- hugr-core/src/hugr/linking.rs | 70 +++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index fd4e33f9d2..55fc17d9a8 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1325,10 +1325,13 @@ mod test { let insert = { let mut mb = ModuleBuilder::new(); let reached = mb.declare("foo", endo_sig(usize_t()).into()).unwrap(); - let unreached = mb.declare("bar", endo_sig(usize_t()).into()).unwrap(); + // This would conflict signature, but is not reached: + let unreached = mb + .declare("bar", inout_sig(vec![], usize_t()).into()) + .unwrap(); let mut outer = mb.define_function("outer", endo_sig(usize_t())).unwrap(); - let [i] = outer.input_wires_arr(); - let [i] = outer.call(&unreached, &[], [i]).unwrap().outputs_arr(); + // ...as this first call is outside the region we insert: + let [i] = outer.call(&unreached, &[], []).unwrap().outputs_arr(); let mut dfb = outer.dfg_builder(endo_sig(usize_t()), [i]).unwrap(); let [i] = dfb.input_wires_arr(); let call = dfb.call(&reached, &[], [i]).unwrap(); @@ -1343,12 +1346,10 @@ mod test { let cst = fb.add_load_value(ConstUsize::new(42)); let mut host = fb.finish_hugr_with_outputs([cst]).unwrap(); - // TODO no good equivalent of pytest parametrized fixtures here... - // crate rstest_reuse is one way, but seems heavy for just this??? - let any_pol = NameLinkingPolicy::default(); + let pol = NameLinkingPolicy::err_on_conflict(OnNewFunc::RaiseError); let ins = host - .insert_link_from_view(host.entrypoint(), &insert, &any_pol) + .insert_link_from_view(host.entrypoint(), &insert, &pol) .unwrap(); let dfg = *ins.node_map.get(&insert.entrypoint()).unwrap(); assert!(host.get_optype(dfg).is_dfg()); @@ -1365,4 +1366,59 @@ mod test { ); assert_eq!(tgt, decls.into_keys().exactly_one().unwrap()); } + + #[rstest] + fn no_new_names_module(#[values(Visibility::Public, Visibility::Private)] vis: Visibility) { + let pub_sig = endo_sig(usize_t()); + let bar_sig = Signature::new(vec![usize_t(); 2], usize_t()); + let existing = { + let mut mb = ModuleBuilder::new(); + mb.declare("pub_func", pub_sig.clone().into()).unwrap(); + mb.finish_hugr().unwrap() + }; + let (insert, new_func) = { + let mut mb = ModuleBuilder::new(); + let bar = mb + .declare_vis("new_func", bar_sig.into(), vis.clone()) + .unwrap(); + let mut pf = mb + .define_function_vis("pub_func", pub_sig.clone(), Visibility::Public) + .unwrap(); + let [i] = pf.input_wires_arr(); + let [i] = pf.call(&bar, &[], [i, i]).unwrap().outputs_arr(); + pf.finish_with_outputs([i]).unwrap(); + (mb.finish_hugr().unwrap(), bar.node()) + }; + let pol = NameLinkingPolicy::keep_both_invalid().on_new_names(OnNewFunc::RaiseError); + let mut host = existing.clone(); + let res = host.link_module(insert, &pol); + match vis { + Visibility::Public => { + assert_eq!( + res.err(), + Some(NameLinkingError::NoNewNames { + name: "new_func".to_string(), + src_node: new_func + }) + ); + assert_eq!(host, existing); + } + Visibility::Private => { + let ins = res.unwrap(); + host.validate().unwrap(); + let (decls, defns) = list_decls_defns(&host); + // Original pubfunc decl replaced by new definition + assert_eq!(defns.into_values().collect_vec(), ["pub_func"]); + // New private bar decl added + let new_added = ins.node_map[&new_func]; + assert_eq!(decls.into_iter().collect_vec(), [(new_added, "new_func")]); + assert_eq!( + host.get_optype(new_added) + .as_func_decl() + .map(FuncDecl::visibility), + Some(&Visibility::Private) + ); + } + } + } } From f874e9bb020ff882a580ec1c953f183752295a0e Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 21 Nov 2025 17:35:19 +0000 Subject: [PATCH 38/52] test no_new_names_entrypoint --- hugr-core/src/hugr/linking.rs | 78 +++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 55fc17d9a8..d29c51eb79 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -833,14 +833,14 @@ mod test { use super::{HugrLinking, NodeLinkingDirective, NodeLinkingError}; use crate::builder::test::{dfg_calling_defn_decl, simple_dfg_hugr}; use crate::builder::{ - Container, Dataflow, DataflowHugr, DataflowSubContainer, FunctionBuilder, HugrBuilder, - ModuleBuilder, endo_sig, + Container, DFGBuilder, Dataflow, DataflowHugr, DataflowSubContainer, FunctionBuilder, + HugrBuilder, ModuleBuilder, endo_sig, inout_sig, }; use crate::extension::prelude::{ConstUsize, usize_t}; use crate::hugr::hugrmut::test::check_calls_defn_decl; use crate::hugr::linking::{NameLinkingError, NameLinkingPolicy, OnMultiDefn, OnNewFunc}; use crate::hugr::{ValidationError, hugrmut::HugrMut}; - use crate::ops::{FuncDecl, OpTag, OpTrait, OpType, Value, handle::NodeHandle}; + use crate::ops::{Const, FuncDecl, OpTag, OpTrait, OpType, Value, handle::NodeHandle}; use crate::std_extensions::arithmetic::int_ops::IntOpDef; use crate::std_extensions::arithmetic::int_types::{ConstInt, INT_TYPES}; use crate::{Hugr, HugrView, Visibility, types::Signature}; @@ -1421,4 +1421,76 @@ mod test { } } } + + #[rstest] + fn no_new_names_entrypoint(#[values(true, false)] call_new: bool) { + let (insert, new_func) = { + let mut dfb = DFGBuilder::new(inout_sig(vec![], usize_t())).unwrap(); + let mut mb = dfb.module_root_builder(); + let cst_used = mb.add_constant(Value::from(ConstUsize::new(5))); + mb.add_constant(Value::from(ConstUsize::new(10))); + + let nf = mb.declare("new_func", endo_sig(usize_t()).into()).unwrap(); + let pf = mb + .define_function_vis("pub_func", endo_sig(usize_t()), Visibility::Public) + .unwrap(); + let [i] = pf.input_wires_arr(); + let pf = pf.finish_with_outputs([i]).unwrap(); + + let i = dfb.load_const(&cst_used); + let call = if call_new { + dfb.call(&nf, &[], [i]).unwrap() + } else { + dfb.call(pf.handle(), &[], [i]).unwrap() + }; + ( + dfb.finish_hugr_with_outputs(call.outputs()).unwrap(), + nf.node(), + ) + }; + + let (backup, ex_main) = { + let mut mb = ModuleBuilder::new(); + mb.declare("pub_func", endo_sig(usize_t()).into()).unwrap(); + let fb = mb.define_function("main", endo_sig(usize_t())).unwrap(); + let ins = fb.input_wires(); + let main = fb.finish_with_outputs(ins).unwrap(); + (mb.finish_hugr().unwrap(), main.node()) + }; + + let mut target = backup.clone(); + let res = target.insert_link_hugr( + ex_main, + insert, + &NameLinkingPolicy::err_on_conflict(OnNewFunc::RaiseError) + .on_new_names(OnNewFunc::RaiseError), + ); + if call_new { + assert_eq!( + res.err(), + Some(NameLinkingError::NoNewNames { + name: "new_func".to_string(), + src_node: new_func + }) + ); + assert_eq!(target, backup); + } else { + res.unwrap(); + target.validate().unwrap(); + let (decls, defns) = list_decls_defns(&target); + assert_eq!( + defns.into_values().sorted().collect_vec(), + ["main", "pub_func"] + ); + assert!(decls.is_empty()); + assert_eq!( + target + .nodes() + .filter_map(|n| target.get_optype(n).as_const()) + .map(Const::value) + .collect_vec(), + [&ConstUsize::new(5).into()] + ); + } + } } From 35361c0bbe248aaa95e07e668864dbaa2f5a2333 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sat, 22 Nov 2025 08:53:49 +0000 Subject: [PATCH 39/52] Use ModuleGraph, rm CallGraph --- hugr-core/src/call_graph.rs | 134 ---------------------------------- hugr-core/src/hugr/linking.rs | 12 ++- hugr-core/src/lib.rs | 1 - 3 files changed, 5 insertions(+), 142 deletions(-) delete mode 100644 hugr-core/src/call_graph.rs diff --git a/hugr-core/src/call_graph.rs b/hugr-core/src/call_graph.rs deleted file mode 100644 index df3b8a4275..0000000000 --- a/hugr-core/src/call_graph.rs +++ /dev/null @@ -1,134 +0,0 @@ -//! Data structure for call graphs of a Hugr -use std::collections::HashMap; - -use crate::{HugrView, Node, core::HugrNode, ops::OpType}; -use petgraph::{Graph, visit::EdgeRef}; - -/// Weight for an edge in a [`CallGraph`] -#[derive(Clone, Debug, PartialEq, Eq)] -#[non_exhaustive] -pub enum CallGraphEdge { - /// Edge corresponds to a [Call](OpType::Call) node (specified) in the Hugr - Call(N), - /// Edge corresponds to a [`LoadFunction`](OpType::LoadFunction) node (specified) in the Hugr - LoadFunction(N), - /// Edge corresponds to a [LoadConstant](OpType::LoadConstant) node (specified) in the Hugr - LoadConstant(N), -} - -/// Weight for a petgraph-node in a [`CallGraph`] -pub enum CallGraphNode { - /// petgraph-node corresponds to a [`FuncDecl`](OpType::FuncDecl) node (specified) in the Hugr - FuncDecl(N), - /// petgraph-node corresponds to a [`FuncDefn`](OpType::FuncDefn) node (specified) in the Hugr - FuncDefn(N), - /// petgraph-node corresponds to the [HugrView::entrypoint], that is not - /// a [`FuncDefn`](OpType::FuncDefn). Note that it will not be a [Module](OpType::Module) - /// either, as such a node could not have edges, so is not represented in the petgraph. - NonFuncEntrypoint, - /// petgraph-node corresponds to a constant; will have no outgoing edges, and incoming - /// edges will be [CallGraphEdge::LoadConstant] - Const(N), -} - -/// Details the [`Call`]s and [`LoadFunction`]s in a Hugr. -/// -/// Each node in the `CallGraph` corresponds to a [`FuncDefn`] or [`FuncDecl`] in the Hugr; -/// each edge corresponds to a [`Call`]/[`LoadFunction`] of the edge's target, contained in -/// the edge's source. -/// -/// For Hugrs whose entrypoint is neither a [Module](OpType::Module) nor a [`FuncDefn`], -/// the call graph will have an additional [`CallGraphNode::NonFuncEntrypoint`] -/// corresponding to the Hugr's entrypoint, with no incoming edges. -/// -/// [`Call`]: OpType::Call -/// [`FuncDecl`]: OpType::FuncDecl -/// [`FuncDefn`]: OpType::FuncDefn -/// [`LoadFunction`]: OpType::LoadFunction -pub struct CallGraph { - g: Graph, CallGraphEdge>, - node_to_g: HashMap>, -} - -impl CallGraph { - /// Makes a new `CallGraph` for a Hugr. - pub fn new(hugr: &impl HugrView) -> Self { - let mut g = Graph::default(); - let mut node_to_g = hugr - .children(hugr.module_root()) - .filter_map(|n| { - let weight = match hugr.get_optype(n) { - OpType::FuncDecl(_) => CallGraphNode::FuncDecl(n), - OpType::FuncDefn(_) => CallGraphNode::FuncDefn(n), - OpType::Const(_) => CallGraphNode::Const(n), - _ => return None, - }; - Some((n, g.add_node(weight))) - }) - .collect::>(); - if !hugr.entrypoint_optype().is_module() && !node_to_g.contains_key(&hugr.entrypoint()) { - node_to_g.insert( - hugr.entrypoint(), - g.add_node(CallGraphNode::NonFuncEntrypoint), - ); - } - for (func, cg_node) in &node_to_g { - traverse(hugr, *cg_node, *func, &mut g, &node_to_g); - } - fn traverse( - h: &impl HugrView, - enclosing_func: petgraph::graph::NodeIndex, - node: N, // Nonstrict-descendant of `enclosing_func`` - g: &mut Graph, CallGraphEdge>, - node_to_g: &HashMap>, - ) { - for ch in h.children(node) { - traverse(h, enclosing_func, ch, g, node_to_g); - let weight = match h.get_optype(ch) { - OpType::Call(_) => CallGraphEdge::Call(ch), - OpType::LoadFunction(_) => CallGraphEdge::LoadFunction(ch), - OpType::LoadConstant(_) => CallGraphEdge::LoadConstant(ch), - _ => continue, - }; - if let Some(target) = h.static_source(ch) { - if h.get_parent(target) == Some(h.module_root()) { - g.add_edge(enclosing_func, node_to_g[&target], weight); - } else { - assert!(!node_to_g.contains_key(&target)); - assert!(h.get_optype(ch).is_load_constant()); - assert!(h.get_optype(target).is_const()); - } - } - } - } - CallGraph { g, node_to_g } - } - - /// Allows access to the petgraph - #[must_use] - pub fn graph(&self) -> &Graph, CallGraphEdge> { - &self.g - } - - /// Convert a Hugr [Node] into a petgraph node index. - /// Result will be `None` if `n` is not a [`FuncDefn`](OpType::FuncDefn), - /// [`FuncDecl`](OpType::FuncDecl) or the [HugrView::entrypoint]. - pub fn node_index(&self, n: N) -> Option> { - self.node_to_g.get(&n).copied() - } - - /// Returns an iterator over the out-edges from the given Node. - /// - /// (If the node is not recognised as a function or the entrypoint, iterator will be empty.) - pub fn callees(&self, n: N) -> impl Iterator, &CallGraphNode)> { - let g = self.graph(); - self.node_index(n).into_iter().flat_map(move |n| { - self.graph().edges(n).map(|e| { - ( - g.edge_weight(e.id()).unwrap(), - g.node_weight(e.target()).unwrap(), - ) - }) - }) - } -} diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index a65f5bccf9..02811cc093 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -8,8 +8,8 @@ use std::{ use itertools::{Either, Itertools}; -use crate::call_graph::{CallGraph, CallGraphNode}; use crate::hugr::{HugrMut, hugrmut::InsertedForest, internal::HugrMutInternals}; +use crate::module_graph::{ModuleGraph, StaticNode}; use crate::{Hugr, HugrView, Node, Visibility, core::HugrNode, ops::OpType, types::PolyFuncType}; /// Methods that merge Hugrs, adding static edges between old and inserted nodes. @@ -605,7 +605,7 @@ impl NameLinkingPolicy { use_entrypoint: bool, ) -> Result, NameLinkingError> { let existing = gather_existing(target); - let cg = CallGraph::new(&source); + let g = ModuleGraph::new(&source); // Can't use petgraph Dfs as we need to avoid traversing through some nodes, // and we need to maintain our own `visited` map anyway let mut to_visit = VecDeque::new(); @@ -633,11 +633,9 @@ impl NameLinkingPolicy { } } // For entrypoint, *just* traverse - to_visit.extend(cg.callees(sn).map(|(_, nw)| match nw { - CallGraphNode::FuncDecl(n) - | CallGraphNode::FuncDefn(n) - | CallGraphNode::Const(n) => *n, - CallGraphNode::NonFuncEntrypoint => unreachable!("cannot call non-func"), + to_visit.extend(g.out_edges(sn).map(|(_, nw)| match nw { + StaticNode::FuncDecl(n) | StaticNode::FuncDefn(n) | StaticNode::Const(n) => *n, + _ => unreachable!("unknown / cannot call non-func entrypoint"), })); } Ok(res) diff --git a/hugr-core/src/lib.rs b/hugr-core/src/lib.rs index 5ed60a3d23..015be311f8 100644 --- a/hugr-core/src/lib.rs +++ b/hugr-core/src/lib.rs @@ -10,7 +10,6 @@ #![cfg_attr(coverage_nightly, feature(coverage_attribute))] pub mod builder; -pub mod call_graph; pub mod core; pub mod envelope; pub mod export; From d0de27108cbfa7d821abca83e295a096a7e20bbb Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sat, 22 Nov 2025 08:54:08 +0000 Subject: [PATCH 40/52] support MSRV --- hugr-core/src/hugr/linking.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 02811cc093..e1cb3ad4de 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -585,9 +585,9 @@ impl NameLinkingPolicy { // return Err(format!("Entrypoint is a top-level function")) //} let pol = self.to_node_linking_helper(target, source, true)?; - if let Some(LinkAction::LinkNode(add @ NodeLinkingDirective::Add { .. })) = - pol.get(&entrypoint_func) - && entrypoint_func != source.entrypoint() + if let Some(LinkAction::LinkNode(add @ NodeLinkingDirective::Add { .. })) = pol + .get(&entrypoint_func) + .filter(|_| entrypoint_func != source.entrypoint()) { return Err(NameLinkingError::AddFunctionContainingEntrypoint( entrypoint_func, From 62d0725f5891614b2675b35330456e670b24c651 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 25 Nov 2025 10:24:55 +0000 Subject: [PATCH 41/52] oops fix merge --- hugr-core/src/hugr/linking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index ce978cdad2..a283001978 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1,7 +1,7 @@ //! Directives and errors relating to linking Hugrs. use std::{ - collections::{BTreeMap, HashMap, VecDeque, hash_map::Entry}, + collections::{BTreeMap, HashMap, VecDeque, btree_map::Entry}, fmt::Display, iter::once, }; From 5ada90b866158379ffa288e8ea0d2136a7220e45 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 28 Nov 2025 12:50:39 +0000 Subject: [PATCH 42/52] remove type_complexities --- hugr-core/src/hugr/linking.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 8695299444..8dec609895 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -244,13 +244,12 @@ pub trait HugrLinking: HugrMut { /// [`on_new_names`]: NameLinkingPolicy::get_on_new_names /// [`on_multi_defn`]: NameLinkingPolicy::get_on_multiple_defn /// [`on_sig_conflict`]: NameLinkingPolicy::get_signature_conflict - #[allow(clippy::type_complexity)] - fn insert_link_from_view( + fn insert_link_from_view>( &mut self, parent: Self::Node, other: &H, policy: &NameLinkingPolicy, - ) -> Result, NameLinkingError> { + ) -> Result, NameLinkingError> { let pol = policy.to_node_linking_for_entrypoint(&*self, &other)?; let per_node = pol .into_iter() @@ -564,12 +563,11 @@ impl NameLinkingPolicy { /// Computes how this policy will act when inserting the entrypoint-subtree of a /// specified source Hugr into a target (host) Hugr (as per [HugrLinking::insert_link_hugr]). - #[allow(clippy::type_complexity)] - pub fn to_node_linking_for_entrypoint( + pub fn to_node_linking_for_entrypoint( &self, - target: &T, - source: &S, - ) -> Result, NameLinkingError> { + target: &(impl HugrView + ?Sized), + source: &impl HugrView, + ) -> Result, NameLinkingError> { let entrypoint_func = { let mut n = source.entrypoint(); loop { @@ -598,13 +596,12 @@ impl NameLinkingPolicy { Ok(pol) } - #[allow(clippy::type_complexity)] - fn to_node_linking_helper( + fn to_node_linking_helper( &self, - target: &T, - source: &S, + target: &(impl HugrView + ?Sized), + source: &impl HugrView, use_entrypoint: bool, - ) -> Result, NameLinkingError> { + ) -> Result, NameLinkingError> { let existing = gather_existing(target); let g = ModuleGraph::new(&source); // Can't use petgraph Dfs as we need to avoid traversing through some nodes, From e9c7d97d85f2f63ee6b8d2ce289cb783355727e2 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sun, 30 Nov 2025 08:58:02 +0000 Subject: [PATCH 43/52] renaming --- hugr-core/src/hugr/linking.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 8dec609895..3afddf78f8 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -204,7 +204,7 @@ pub trait HugrLinking: HugrMut { other: Hugr, policy: &NameLinkingPolicy, ) -> Result, NameLinkingError> { - let pol = policy.to_node_linking_for_entrypoint(&*self, &other)?; + let pol = policy.link_actions_with_entrypoint(&*self, &other)?; let per_node = pol .into_iter() .map(|(k, LinkAction::LinkNode(v))| (k, v)) @@ -250,7 +250,7 @@ pub trait HugrLinking: HugrMut { other: &H, policy: &NameLinkingPolicy, ) -> Result, NameLinkingError> { - let pol = policy.to_node_linking_for_entrypoint(&*self, &other)?; + let pol = policy.link_actions_with_entrypoint(&*self, &other)?; let per_node = pol .into_iter() .map(|(k, LinkAction::LinkNode(v))| (k, v)) @@ -460,7 +460,7 @@ impl NameLinkingPolicy { /// [Public] function with the same name but different signatures. /// /// [Public]: crate::Visibility::Public - pub fn get_signature_conflict(&self) -> OnNewFunc { + pub fn get_on_signature_conflict(&self) -> OnNewFunc { self.sig_conflict } @@ -497,7 +497,7 @@ impl NameLinkingPolicy { target: &(impl HugrView + ?Sized), source: &impl HugrView, ) -> Result, NameLinkingError> { - self.to_node_linking_helper(target, source, false) + self.link_actions_helper(target, source, false) } fn action_for( @@ -563,7 +563,7 @@ impl NameLinkingPolicy { /// Computes how this policy will act when inserting the entrypoint-subtree of a /// specified source Hugr into a target (host) Hugr (as per [HugrLinking::insert_link_hugr]). - pub fn to_node_linking_for_entrypoint( + pub fn link_actions_with_entrypoint( &self, target: &(impl HugrView + ?Sized), source: &impl HugrView, @@ -580,11 +580,8 @@ impl NameLinkingPolicy { n = p; } }; - //if entrypoint_func == other.entrypoint() { // Do we need to check this? Ok if parent is self.module_root() ?? - // return Err(format!("Entrypoint is a top-level function")) - //} - let pol = self.to_node_linking_helper(target, source, true)?; - if let Some(LinkAction::LinkNode(add @ NodeLinkingDirective::Add { .. })) = pol + let actions = self.link_actions_helper(target, source, true)?; + if let Some(LinkAction::LinkNode(add @ NodeLinkingDirective::Add { .. })) = actions .get(&entrypoint_func) .filter(|_| entrypoint_func != source.entrypoint()) { @@ -593,10 +590,10 @@ impl NameLinkingPolicy { add.clone(), )); } - Ok(pol) + Ok(actions) } - fn to_node_linking_helper( + fn link_actions_helper( &self, target: &(impl HugrView + ?Sized), source: &impl HugrView, From 39de0fe9cb936cc707dcdcfb71dbc7b55dca96d7 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sun, 30 Nov 2025 09:20:39 +0000 Subject: [PATCH 44/52] doc updates --- hugr-core/src/hugr/linking.rs | 116 ++++++++++++++++++++-------------- 1 file changed, 67 insertions(+), 49 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 3afddf78f8..078f1f264f 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -168,36 +168,41 @@ pub trait HugrLinking: HugrMut { .expect("NodeLinkingPolicy was constructed to avoid any error")) } - /// Inserts the entrypoint-subtree of another Hugr into this one, - /// including copying any private functions it calls and using linking - /// to resolve any public functions. + /// Inserts the entrypoint-subtree of another Hugr into this one, along with + /// any private functions it calls and using linking to resolve any public functions. /// /// `parent` is the parent node under which to insert the entrypoint. /// /// # Errors /// - /// If other's entrypoint is its module-root (recommend using [Self::link_module] instead) + /// * [NameLinkingError::InsertEntrypointIsModuleRoot] if `other`'s entrypoint is its + /// module-root, (recommend using [Self::link_module] instead). /// - /// If other's entrypoint calls (perhaps transitively) the function containing said entrypoint - /// (an exception is made if the called+containing function is public and is being replaced - /// by an equivalent in `self` via [OnMultiDefn::UseExisting], in which case - /// the call is redirected to the existing function, and the new one is not inserted - /// except the entrypoint subtree.) + /// * [NameLinkingError::AddFunctionContainingEntrypoint] if `other`'s entrypoint + /// calls (perhaps transitively) the function containing said entrypoint. + /// An exception is made if the called+containing function is public and is being replaced + /// by an equivalent in `self` via [OnMultiDefn::UseExisting], in which case + /// the call is redirected to the existing function (the part of the new function + /// outside the entrypoint subtree is not inserted). /// - /// If other's entrypoint calls a public function in `other` which - /// * has a name or signature different to any in `self`, and [`on_new_names`] is - /// [`OnNewFunc::RaiseError`] - /// * has a name equal to that in `self`, but a different signature, and [`on_sig_conflict`] is - /// [`OnNewFunc::RaiseError`] + /// * [NameLinkingError::NoNewNames] if `other`'s entrypoint calls (perhaps + /// transitively) a public function in `other` which has a name different to any in + /// `self` and [`on_new_names`] is [`OnNewFunc::RaiseError`]. /// - /// If other's entrypoint calls a public [`FuncDefn`] in `other` which has the same name - /// and signature as a public [`FuncDefn`] in `self` and [`on_multi_defn`] is - /// [`OnMultiDefn::NewFunc`] of [`OnNewFunc::RaiseError`] + /// * [NameLinkingError::SignatureConflict] if `other`' entrypoint calls (perhaps + /// transitively) a public function in `other` that has a name equal to one in + /// `self`, but a different signature, and [`on_signature_conflict`] is + /// [`OnNewFunc::RaiseError`]. + /// + /// * [NameLinkingError::MultipleDefn] if `other`'s entrypoint calls (perhaps + /// transitively) a public [`FuncDefn`] in `other` which has the same name + /// and signature as a public [`FuncDefn`] in `self` and [`on_multiple_defn`] is + /// [`OnMultiDefn::NewFunc`] or [`OnNewFunc::RaiseError`]. /// /// [`FuncDefn`]: crate::ops::FuncDefn - /// [`on_new_names`]: NameLinkingPolicy::get_on_new_names - /// [`on_multi_defn`]: NameLinkingPolicy::get_on_multiple_defn - /// [`on_sig_conflict`]: NameLinkingPolicy::get_signature_conflict + /// [`on_new_names`]: NameLinkingPolicy::on_new_names + /// [`on_multiple_defn`]: NameLinkingPolicy::on_multiple_defn + /// [`on_signature_conflict`]: NameLinkingPolicy::on_signature_conflict fn insert_link_hugr( &mut self, parent: Self::Node, @@ -214,36 +219,41 @@ pub trait HugrLinking: HugrMut { .expect("NodeLinkingPolicy was constructed to avoid any error")) } - /// Inserts the entrypoint-subtree of another Hugr into this one, - /// including copying any private functions it calls and using linking - /// to resolve any public functions. + /// Inserts (copies) the entrypoint-subtree of another Hugr into this one, along with + /// any private functions it calls and using linking to resolve any public functions. /// /// `parent` is the parent node under which to insert the entrypoint. /// /// # Errors /// - /// If other's entrypoint is its module-root (recommend using [Self::link_module] instead) + /// * [NameLinkingError::InsertEntrypointIsModuleRoot] if `other`'s entrypoint is its + /// module-root, (recommend using [Self::link_module] instead). + /// + /// * [NameLinkingError::AddFunctionContainingEntrypoint] if `other`'s entrypoint + /// calls (perhaps transitively) the function containing said entrypoint. + /// An exception is made if the called+containing function is public and is being replaced + /// by an equivalent in `self` via [OnMultiDefn::UseExisting], in which case + /// the call is redirected to the existing function (the part of the new function + /// outside the entrypoint subtree is not inserted). /// - /// If other's entrypoint calls (perhaps transitively) the function containing said entrypoint - /// (an exception is made if the called+containing function is public and is being replaced - /// by an equivalent in `self` via [OnMultiDefn::UseExisting], in which case - /// the call is redirected to the existing function, and the new one is not inserted - /// except the entrypoint subtree.) + /// * [NameLinkingError::NoNewNames] if `other`'s entrypoint calls (perhaps + /// transitively) a public function in `other` which has a name different to any in + /// `self` and [`on_new_names`] is [`OnNewFunc::RaiseError`]. /// - /// If other's entrypoint calls a public function in `other` which - /// * has a name or signature different to any in `self`, and [`on_new_names`] is - /// [`OnNewFunc::RaiseError`] - /// * has a name equal to that in `self`, but a different signature, and [`on_sig_conflict`] is - /// [`OnNewFunc::RaiseError`] + /// * [NameLinkingError::SignatureConflict] if `other`' entrypoint calls (perhaps + /// transitively) a public function in `other` that has a name equal to one in + /// `self`, but a different signature, and [`on_signature_conflict`] is + /// [`OnNewFunc::RaiseError`]. /// - /// If other's entrypoint calls a public [`FuncDefn`] in `other` which has the same name - /// and signature as a public [`FuncDefn`] in `self` and [`on_multi_defn`] is - /// [`OnMultiDefn::NewFunc`] of [`OnNewFunc::RaiseError`] + /// * [NameLinkingError::MultipleDefn] if `other`'s entrypoint calls (perhaps + /// transitively) a public [`FuncDefn`] in `other` which has the same name + /// and signature as a public [`FuncDefn`] in `self` and [`on_multiple_defn`] is + /// [`OnMultiDefn::NewFunc`] or [`OnNewFunc::RaiseError`]. /// /// [`FuncDefn`]: crate::ops::FuncDefn - /// [`on_new_names`]: NameLinkingPolicy::get_on_new_names - /// [`on_multi_defn`]: NameLinkingPolicy::get_on_multiple_defn - /// [`on_sig_conflict`]: NameLinkingPolicy::get_signature_conflict + /// [`on_new_names`]: NameLinkingPolicy::on_new_names + /// [`on_multiple_defn`]: NameLinkingPolicy::on_multiple_defn + /// [`on_signature_conflict`]: NameLinkingPolicy::on_signature_conflict fn insert_link_from_view>( &mut self, parent: Self::Node, @@ -448,44 +458,52 @@ impl NameLinkingPolicy { } /// Sets how to behave when both target and inserted Hugr have a - /// [Public] function with the same name but different signatures. + /// ([Visibility::Public]) function with the same name but different signatures. /// - /// [Public]: crate::Visibility::Public + /// See [Self::get_on_signature_conflict]. pub fn on_signature_conflict(mut self, sc: OnNewFunc) -> Self { self.sig_conflict = sc; self } - /// Tells how to behave when both target and inserted Hugr have a - /// [Public] function with the same name but different signatures. + /// Returns how this policy will behave when both target and inserted Hugr have a + /// ([Visibility::Public]) function with the same name but different signatures. /// - /// [Public]: crate::Visibility::Public + /// Can be changed via [Self::on_signature_conflict]. pub fn get_on_signature_conflict(&self) -> OnNewFunc { self.sig_conflict } /// Sets how to behave when both target and inserted Hugr have a /// [FuncDefn](crate::ops::FuncDefn) with the same name and signature. + /// + /// See [Self::get_on_multiple_defn]. pub fn on_multiple_defn(mut self, multi_defn: OnMultiDefn) -> Self { self.multi_defn = multi_defn; self } - /// Tells how to behave when both target and inserted Hugr have a + /// Returns how this policy will behave when both target and inserted Hugr have a /// [FuncDefn](crate::ops::FuncDefn) with the same name and signature. + /// + /// Can be changed via [Self::on_multiple_defn]. pub fn get_on_multiple_defn(&self) -> OnMultiDefn { self.multi_defn } /// Sets how to behave when the source Hugr adds a ([Visibility::Public]) /// name not already in the target. + /// + /// See [Self::get_on_new_names]. pub fn on_new_names(mut self, nn: OnNewFunc) -> Self { self.new_names = nn; self } - /// Tells how to behave when the source Hugr adds a ([Visibility::Public]) + /// Returns how this policy behaves when the source Hugr adds a [Visibility::Public] /// name not already in the target. + /// + /// Can be changed via [Self::on_new_names]. pub fn get_on_new_names(&self) -> OnNewFunc { self.new_names } @@ -561,8 +579,8 @@ impl NameLinkingPolicy { } } - /// Computes how this policy will act when inserting the entrypoint-subtree of a - /// specified source Hugr into a target (host) Hugr (as per [HugrLinking::insert_link_hugr]). + /// Computes concrete actions to link the entrypoint-subtree of a specific source + /// (inserted) Hugr into a specific target (host) Hugr according to this policy. pub fn link_actions_with_entrypoint( &self, target: &(impl HugrView + ?Sized), From 239f3580b14a4380d257f3ea1562c6a84b66c5b5 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sun, 30 Nov 2025 09:25:24 +0000 Subject: [PATCH 45/52] rename in test --- hugr-core/src/hugr/linking.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 078f1f264f..b87d4fe88b 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1343,10 +1343,10 @@ mod test { fn insert_link() { let insert = { let mut mb = ModuleBuilder::new(); - let reached = mb.declare("foo", endo_sig(usize_t()).into()).unwrap(); + let reached = mb.declare("reached", endo_sig(usize_t()).into()).unwrap(); // This would conflict signature, but is not reached: let unreached = mb - .declare("bar", inout_sig(vec![], usize_t()).into()) + .declare("unreached", inout_sig(vec![], usize_t()).into()) .unwrap(); let mut outer = mb.define_function("outer", endo_sig(usize_t())).unwrap(); // ...as this first call is outside the region we insert: @@ -1375,7 +1375,7 @@ mod test { host.connect(i.node(), i.source(), dfg, 0); host.validate().unwrap(); let (decls, defns) = list_decls_defns(&host); - assert_eq!(decls.values().collect_vec(), [&"foo"]); // unreached bar not copied + assert_eq!(decls.values().collect_vec(), [&"reached"]); // unreached not copied assert_eq!(defns.values().collect_vec(), [&"main"]); // as originally in host let (call, tgt) = call_targets(&host).into_iter().exactly_one().unwrap(); assert_eq!(host.get_parent(call), Some(dfg)); From 444fc6f619ce62d567ba35bc9ad70ab3b4a48532 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 1 Dec 2025 14:39:23 +0000 Subject: [PATCH 46/52] indent --- hugr-core/src/hugr/linking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index b87d4fe88b..222ed81f0b 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -187,7 +187,7 @@ pub trait HugrLinking: HugrMut { /// /// * [NameLinkingError::NoNewNames] if `other`'s entrypoint calls (perhaps /// transitively) a public function in `other` which has a name different to any in - /// `self` and [`on_new_names`] is [`OnNewFunc::RaiseError`]. + /// `self` and [`on_new_names`] is [`OnNewFunc::RaiseError`]. /// /// * [NameLinkingError::SignatureConflict] if `other`' entrypoint calls (perhaps /// transitively) a public function in `other` that has a name equal to one in @@ -238,7 +238,7 @@ pub trait HugrLinking: HugrMut { /// /// * [NameLinkingError::NoNewNames] if `other`'s entrypoint calls (perhaps /// transitively) a public function in `other` which has a name different to any in - /// `self` and [`on_new_names`] is [`OnNewFunc::RaiseError`]. + /// `self` and [`on_new_names`] is [`OnNewFunc::RaiseError`]. /// /// * [NameLinkingError::SignatureConflict] if `other`' entrypoint calls (perhaps /// transitively) a public function in `other` that has a name equal to one in From e8a54a26f770c3fcff52f6100fb491865b2ff016 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 1 Dec 2025 17:43:32 +0000 Subject: [PATCH 47/52] return InsertionResult not InsertedForest --- hugr-core/src/hugr/linking.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 222ed81f0b..85a97764d5 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -8,7 +8,11 @@ use std::{ use itertools::{Either, Itertools}; -use crate::hugr::{HugrMut, hugrmut::InsertedForest, internal::HugrMutInternals}; +use crate::hugr::{ + HugrMut, + hugrmut::{InsertedForest, InsertionResult}, + internal::HugrMutInternals, +}; use crate::module_graph::{ModuleGraph, StaticNode}; use crate::{Hugr, HugrView, Node, Visibility, core::HugrNode, ops::OpType, types::PolyFuncType}; @@ -208,15 +212,21 @@ pub trait HugrLinking: HugrMut { parent: Self::Node, other: Hugr, policy: &NameLinkingPolicy, - ) -> Result, NameLinkingError> { + ) -> Result, NameLinkingError> { let pol = policy.link_actions_with_entrypoint(&*self, &other)?; let per_node = pol .into_iter() .map(|(k, LinkAction::LinkNode(v))| (k, v)) .collect(); - Ok(self + let ep = other.entrypoint(); + let node_map = self .insert_link_hugr_by_node(Some(parent), other, per_node) - .expect("NodeLinkingPolicy was constructed to avoid any error")) + .expect("NodeLinkingPolicy was constructed to avoid any error") + .node_map; + Ok(InsertionResult { + inserted_entrypoint: node_map[&ep], + node_map, + }) } /// Inserts (copies) the entrypoint-subtree of another Hugr into this one, along with @@ -259,15 +269,20 @@ pub trait HugrLinking: HugrMut { parent: Self::Node, other: &H, policy: &NameLinkingPolicy, - ) -> Result, NameLinkingError> { + ) -> Result, NameLinkingError> { let pol = policy.link_actions_with_entrypoint(&*self, &other)?; let per_node = pol .into_iter() .map(|(k, LinkAction::LinkNode(v))| (k, v)) .collect(); - Ok(self + let node_map = self .insert_link_view_by_node(Some(parent), other, per_node) - .expect("NodeLinkingPolicy was constructed to avoid any error")) + .expect("NodeLinkingPolicy was constructed to avoid any error") + .node_map; + Ok(InsertionResult { + inserted_entrypoint: node_map[&other.entrypoint()], + node_map, + }) } } From e9bd5fa67469b5d776ab577c83b3984161aaf822 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 2 Dec 2025 17:15:10 +0000 Subject: [PATCH 48/52] refactor closure using ? --- hugr-core/src/hugr/linking.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 85a97764d5..7c28b2043b 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -707,11 +707,9 @@ fn link_sig(h: &H, n: H::Node) -> Option> { fn gather_existing<'a, H: HugrView + ?Sized>(h: &'a H) -> HashMap<&'a str, PubFuncs<'a, H::Node>> { let left_if = |b| if b { Either::Left } else { Either::Right }; h.children(h.module_root()) - .filter_map(|n| { - link_sig(h, n).and_then(|link_sig| match link_sig { - LinkSig::Public { name, is_defn, sig } => Some((name, (left_if(is_defn)(n), sig))), - LinkSig::Private => None, - }) + .filter_map(|n| match link_sig(h, n)? { + LinkSig::Public { name, is_defn, sig } => Some((name, (left_if(is_defn)(n), sig))), + LinkSig::Private => None, }) .into_grouping_map() .aggregate(|acc: Option>, name, (new, sig2)| { From 7d9e211de30c0d3426e9ebeacad1edc3e504568c Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 2 Dec 2025 17:30:42 +0000 Subject: [PATCH 49/52] Refactor loop in link_actions_helper --- hugr-core/src/hugr/linking.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 7c28b2043b..5bc486ec9a 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -638,7 +638,7 @@ impl NameLinkingPolicy { // and we need to maintain our own `visited` map anyway let mut to_visit = VecDeque::new(); if use_entrypoint { - to_visit.push_back(source.entrypoint()); + to_visit.extend(used_nodes(&g, source.entrypoint())); } else { to_visit.extend( source @@ -648,23 +648,17 @@ impl NameLinkingPolicy { } let mut res = LinkActions::new(); while let Some(sn) = to_visit.pop_front() { - if !(use_entrypoint && sn == source.entrypoint()) { - let (Entry::Vacant(ve), Some(ls)) = (res.entry(sn), link_sig(source, sn)) else { - continue; - }; - let act = self.action_for(&existing, sn, ls)?; - let LinkAction::LinkNode(dirv) = &act; - let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); - ve.insert(act); - if !traverse { - continue; - } + let Entry::Vacant(ve) = res.entry(sn) else { + continue; // Seen already (used by many) + }; + let ls = link_sig(source, sn).expect("Only funcs/consts ever enqueued"); + let act = self.action_for(&existing, sn, ls)?; + let LinkAction::LinkNode(dirv) = &act; + let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); + ve.insert(act); + if traverse { + to_visit.extend(used_nodes(&g, sn)); } - // For entrypoint, *just* traverse - to_visit.extend(g.out_edges(sn).map(|(_, nw)| match nw { - StaticNode::FuncDecl(n) | StaticNode::FuncDefn(n) | StaticNode::Const(n) => *n, - _ => unreachable!("unknown / cannot call non-func entrypoint"), - })); } Ok(res) } @@ -676,6 +670,13 @@ impl Default for NameLinkingPolicy { } } +fn used_nodes(g: &ModuleGraph, n: N) -> impl Iterator + '_ { + g.out_edges(n).map(|(_, nw)| match nw { + StaticNode::FuncDecl(n) | StaticNode::FuncDefn(n) | StaticNode::Const(n) => *n, + _ => unreachable!("unknown / cannot call non-func entrypoint"), + }) +} + type PubFuncs<'a, N> = (Either)>, &'a PolyFuncType); fn target_node(ns: &Either)>) -> N { From a0d2789df30fc189d06195151e5cd065b1ada3ac Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 2 Dec 2025 17:46:44 +0000 Subject: [PATCH 50/52] various private renames, inline just_add --- hugr-core/src/hugr/linking.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 5bc486ec9a..882bd6ecb9 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -533,31 +533,31 @@ impl NameLinkingPolicy { self.link_actions_helper(target, source, false) } - fn action_for( + // Also for source constants (considered private so just added) + fn action_for_source_func( &self, - existing: &HashMap<&str, PubFuncs>, - sn: SN, + existing_in_target: &HashMap<&str, PubFuncs>, + src_node: SN, new: LinkSig, ) -> Result, NameLinkingError> { - let just_add = NodeLinkingDirective::add().into(); let LinkSig::Public { name, is_defn: new_is_defn, sig: new_sig, } = new else { - return Ok(just_add); + return Ok(NodeLinkingDirective::add().into()); }; let chk_add = |onf: OnNewFunc, e| match onf { OnNewFunc::RaiseError => Err(e), - OnNewFunc::Add => Ok(just_add), + OnNewFunc::Add => Ok(NodeLinkingDirective::add().into()), }; - let Some((existing, ex_sig)) = existing.get(name) else { + let Some((existing, ex_sig)) = existing_in_target.get(name) else { return chk_add( self.new_names, NameLinkingError::NoNewNames { name: name.to_string(), - src_node: sn, + src_node, }, ); }; @@ -566,7 +566,7 @@ impl NameLinkingPolicy { self.sig_conflict, NameLinkingError::SignatureConflict { name: name.to_string(), - src_node: sn, + src_node, src_sig: new_sig.clone().into(), tgt_node: target_node(existing), tgt_sig: (*ex_sig).clone().into(), @@ -587,7 +587,7 @@ impl NameLinkingPolicy { match self.multi_defn { OnMultiDefn::NewFunc(nfh) => chk_add( nfh, - NameLinkingError::MultipleDefn(name.to_string(), sn, ex_defn), + NameLinkingError::MultipleDefn(name.to_string(), src_node, ex_defn), ), OnMultiDefn::UseExisting => Ok(NodeLinkingDirective::UseExisting(ex_defn).into()), OnMultiDefn::UseNew => Ok(NodeLinkingDirective::replace([ex_defn]).into()), @@ -632,7 +632,7 @@ impl NameLinkingPolicy { source: &impl HugrView, use_entrypoint: bool, ) -> Result, NameLinkingError> { - let existing = gather_existing(target); + let in_target = gather_existing(target); let g = ModuleGraph::new(&source); // Can't use petgraph Dfs as we need to avoid traversing through some nodes, // and we need to maintain our own `visited` map anyway @@ -652,7 +652,7 @@ impl NameLinkingPolicy { continue; // Seen already (used by many) }; let ls = link_sig(source, sn).expect("Only funcs/consts ever enqueued"); - let act = self.action_for(&existing, sn, ls)?; + let act = self.action_for_source_func(&in_target, sn, ls)?; let LinkAction::LinkNode(dirv) = &act; let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); ve.insert(act); From b3999eeee9a7e78eb580adc1c077fdb7a296ea37 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 2 Dec 2025 20:38:09 +0000 Subject: [PATCH 51/52] reduce change --- hugr-core/src/hugr/linking.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 854db76777..24f962dd26 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -1,18 +1,12 @@ //! Directives and errors relating to linking Hugrs. -use std::{ - collections::{BTreeMap, HashMap, VecDeque, btree_map::Entry}, - fmt::Display, - iter::once, -}; +use std::collections::{BTreeMap, HashMap, VecDeque, btree_map::Entry}; +use std::{fmt::Display, iter::once}; use itertools::{Either, Itertools}; -use crate::hugr::{ - HugrMut, - hugrmut::{InsertedForest, InsertionResult}, - internal::HugrMutInternals, -}; +use crate::hugr::hugrmut::{InsertedForest, InsertionResult}; +use crate::hugr::{HugrMut, internal::HugrMutInternals}; use crate::module_graph::{ModuleGraph, StaticNode}; use crate::{Hugr, HugrView, Node, Visibility, core::HugrNode, ops::OpType, types::PolyFuncType}; @@ -327,8 +321,8 @@ pub enum NodeLinkingDirective { /// to leave the newly-inserted node instead. (Typically, this `Vec` would contain /// at most one [FuncDefn], or perhaps-multiple, aliased, [FuncDecl]s.) /// - /// [FuncDefn]: crate::ops::FuncDefn /// [FuncDecl]: crate::ops::FuncDecl + /// [FuncDefn]: crate::ops::FuncDefn /// [EdgeKind::Const]: crate::types::EdgeKind::Const /// [EdgeKind::Function]: crate::types::EdgeKind::Function replace: Vec, From 72c13bbb3db117cefa8a3eb812ecab10045c52a7 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 3 Dec 2025 10:44:33 +0000 Subject: [PATCH 52/52] Move used_nodes inside --- hugr-core/src/hugr/linking.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 24f962dd26..505eba7584 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/hugr-core/src/hugr/linking.rs @@ -615,8 +615,14 @@ impl NameLinkingPolicy { ) -> Result, NameLinkingError> { let in_target = gather_existing(target); let g = ModuleGraph::new(&source); - // Can't use petgraph Dfs as we need to avoid traversing through some nodes, - // and we need to maintain our own `visited` map anyway + fn used_nodes(g: &ModuleGraph, n: N) -> impl Iterator + '_ { + g.out_edges(n).map(|(_, nw)| match nw { + StaticNode::FuncDecl(n) | StaticNode::FuncDefn(n) | StaticNode::Const(n) => *n, + _ => unreachable!("unknown / cannot call non-func entrypoint"), + }) + } + // Can't use petgraph traversal as we need to avoid traversing through some nodes, + // and we need to maintain our own `res` map of visited nodes anyway let mut to_visit = VecDeque::new(); if use_entrypoint { to_visit.extend(used_nodes(&g, source.entrypoint())); @@ -632,7 +638,7 @@ impl NameLinkingPolicy { let Entry::Vacant(ve) = res.entry(sn) else { continue; // Seen already (used by many) }; - let ls = link_sig(source, sn).expect("Only funcs/consts ever enqueued"); + let ls = link_sig(source, sn).expect("used_nodes returns only funcs+consts"); let act = self.action_for_source_func(&in_target, sn, ls)?; let LinkAction::LinkNode(dirv) = &act; let traverse = matches!(dirv, NodeLinkingDirective::Add { .. }); @@ -655,13 +661,6 @@ impl Default for NameLinkingPolicy { } } -fn used_nodes(g: &ModuleGraph, n: N) -> impl Iterator + '_ { - g.out_edges(n).map(|(_, nw)| match nw { - StaticNode::FuncDecl(n) | StaticNode::FuncDefn(n) | StaticNode::Const(n) => *n, - _ => unreachable!("unknown / cannot call non-func entrypoint"), - }) -} - type PubFuncs<'a, N> = (Either)>, &'a PolyFuncType); fn target_node(ns: &Either)>) -> N {