feat: add trait+funcs for linking Hugrs explicitly by Node (linking 2/4) #2521

acl-cqc · 2025-08-13T10:29:58Z

Closes #2355.

This adds a new trait HugrLinking: HugrMut with two methods that allow inserting a Hugr/View into self - the entrypoint subtree (anywhere) and/or the module-children (under the module-root of self). An error is raised if you're trying to insert the entrypoint-subtree into two places. The added nodes can be linked i.e. static edges added from module-children to uses, both from new to existing or vice versa. The new methods are default-implemented on top of HugrMut::insert_(view_)forest, and there's a blanket impl for any HugrMut, so this is just to keep them separated out.

Note: it would be possible to deprecate insert_hugr now: insert_link_nodes(other, Some(parent), HashMap::new()) does the same job (explicitly not copying any module-children), and returns an error, which is probably what we want. However, we'll be able to do a much better (more powerful) version in a couple of PRs time, so have left this for now.

Largely a respin of #2285 (which was approved), keeping a similar API, but without the behaviour changes to insert_hugr and insert_from_view of that PR, and reimplemented. However, this adds the ability for a new node (e.g. FuncDefn) to replace existing nodes (e.g. aliased FuncDecls), and the new stuff is moved out into the new trait.

…r/from_view

codecov · 2025-08-13T10:33:51Z

Codecov Report

❌ Patch coverage is 97.60192% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.93%. Comparing base (4b38008) to head (97b3bf1).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/builder/build_traits.rs	83.72%	5 Missing and 2 partials ⚠️
hugr-core/src/hugr/linking.rs	99.34%	2 Missing ⚠️
hugr-core/src/builder/module.rs	98.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2521      +/-   ##
==========================================
+ Coverage   82.75%   82.93%   +0.17%     
==========================================
  Files         252      253       +1     
  Lines       47065    47530     +465     
  Branches    42581    43046     +465     
==========================================
+ Hits        38950    39417     +467     
+ Misses       6055     6050       -5     
- Partials     2060     2063       +3

Flag	Coverage Δ
python	`91.41% <ø> (ø)`
rust	`82.04% <97.60%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ink_nodes

acl-cqc · 2025-08-13T12:33:41Z

Just realized I am not handling corner cases of same node in target Hugr being replaced by multiple new (::Added) nodes, or being replaced while also being ::UseExistingd...these will break ATM ✅

…ing NOTESTS

hugr-core/src/builder.rs

hugr-core/src/builder/build_traits.rs

acl-cqc · 2025-08-13T13:29:04Z

hugr-core/src/hugr/linking.rs

+    transfers: Transfers<SN, TGT::Node>,
+    node_map: &mut HashMap<SN, TGT::Node>,
+) {
+    // Resolve `use_existing` first in case the existing node is also replaced by


~~No test yet, coming up...~~(EDIT: added)

Alternatively we could error if the same existing-node is target of both a replace and a UseExisting.

lmondada · 2025-08-20T12:35:08Z

hugr-core/src/builder.rs

@@ -352,4 +363,33 @@ pub(crate) mod test {
        );
        hugr
    }
+
+    /// A DFG-entrypoint Hugr (no inputs, one bool_t output) containing two calls,
+    /// to a FuncDefn and a FuncDecl each bool_t->bool_t, and their handles.


...and their handles

I think some words are missing. You maybe meant "Returns the hugr and the function handles"?

oh i see you just copied this over. Might be worth fixing anyways :)

lmondada · 2025-08-20T12:55:53Z

hugr-core/src/builder/build_traits.rs

+        let node = self
+            .hugr_mut()
+            .insert_hugr_link_nodes(parent, hugr, defns)?[&ep];
+        wire_ins_return_outs(input_wires, node, self)
    }

    /// Copy a hugr-defined op into the sibling graph, wiring up the


I find this docstring confusing, what "hugr-defined op" does this refer to? The entrypoint I suppose:

Suggested change

/// Copy a hugr-defined op into the sibling graph, wiring up the

/// Copy the op designed by the HUGR entrypoint into the sibling graph, wiring up the

lmondada · 2025-08-20T13:05:41Z

hugr-core/src/hugr/hugrmut.rs

@@ -833,39 +840,7 @@ mod test {
                    .chain(call2.then_some((decl.node(), h.module_root())).into_iter()),
            );
            h.insert_forest(insert, roots).unwrap();


this is in your previous PR I think, but anyways:

in the docs of fn insert_forest, can you change any reference to root_parents to roots, to match the argument name?

I was going to make another comment about using rstest instead of the for loop, but I see that this seems already taken care of in #2518.

Yes, sorry, I'm rather behind on merging, hoping 2518 goes in soon!

Docs for insert_forest updated (now on main), should be showing up here

lmondada · 2025-08-20T13:10:43Z

hugr-core/src/hugr/linking.rs

+    hugr::{HugrMut, internal::HugrMutInternals},
+};
+
+/// Methods for linking Hugrs, i.e. merging the Hugrs and adding edges between old and inserted nodes.


A bit shorter so it fits in 80chars

Suggested change

/// Methods for linking Hugrs, i.e. merging the Hugrs and adding edges between old and inserted nodes.

/// Methods that merge Hugrs, adding edges between old and inserted nodes.

lmondada · 2025-08-20T13:18:29Z

hugr-core/src/hugr/linking.rs

+    ///
+    /// If `parent` is `Some` but not in the graph.
+    #[allow(clippy::type_complexity)]
+    fn insert_from_view_link_nodes<H: HugrView>(


These names make sense, but I wonder if we could shorten them a bit -- or do you plan on renaming to insert_hugr and insert_from_view once those are deleted?

I can suggest:

insert_link_from_view

insert_link_hugr

I think they are just as clear and sound like natural "evolutions" of the current API. Up to you :)

Later I'm going to add variants that do linking by name, using FuncDefn::func_name and FuncDecl::func_name of Visibility::Public functions; so I'd like either

method names that say ...node... here and I can replace node by name

method names that I can abbreviate (e.g. by removing node altogether), as linking by name is likely to be the common/usual case. (This is the hardcode explicit option if you want more control; and linking by name reduces to this; so it is possible there won't be many calls of these methods.)

Happy to abbreviate insert_from_view to insert_view....I suspect a second trait (trait NodeLinking and trait NameLinking, heh) is probably too many, insert_nodelinked_hugr/view not much better. Hmmm. Will have a think.

lmondada · 2025-08-20T13:20:42Z

hugr-core/src/hugr/linking.rs

+/// This is done by module-children from the inserted (source) Hugr replacing, or being replaced by,
+/// module-children already in the target Hugr; static edges from the replaced node,
+/// are transferred to come from the replacing node, and the replaced node(/subtree) then deleted.
+pub trait LinkHugr: HugrMut {


I'm fine with leaving insert_hugr and insert_from_view as they are for the moment, but could you add a note in their docs pointing users here if they are considering using them?

E.g. say something like "more specialised methods that handle copying multiple module root children and preserving edge connectivity can be found in [LinkHugr].

lmondada · 2025-08-20T13:27:18Z

hugr-core/src/hugr/linking.rs

+///
+/// This is done by module-children from the inserted (source) Hugr replacing, or being replaced by,
+/// module-children already in the target Hugr; static edges from the replaced node,
+/// are transferred to come from the replacing node, and the replaced node(/subtree) then deleted.


Have you thought of other names for this trait? What I dislike about LinkHugr is that it sounds like LinkView in portgraph, which is read-only; a lot of our traits for mutation have a Mut suffix, but I cannot think of a good name that would keep this convention.

InsertHugr or HugrInsert are pretty straight-forward, though? Maybe InsertHugrMut if you want to highlight that this is a "specialisation" of HugrMut...

Also, is there a reason this cannot be part of the HugrMut trait? Or at least provide a blanket implementation for any H: HugrMut?

I think there is a tradeoff between avoiding huge bloated traits and on the other hand a proliferation of traits... I find more traits make API exploration much harder as the available methods might get missed by Intellisense. A pointer from the HugrMut docs to here would already help with that (see my other comment).

Thanks, you make many good points here. I had not thought of LinkView but that's a good reason not to call this LinkHugr! Hmmm. Maybe trait HugrLinking? With a Mut suffix....hmmm....maybe InsertHugrMut is not bad, although next I'm planning to add link_module(_view) - and then finally insert_ variants that link by name, so those would be fine.

Separated out from HugrMut as per suggestion from @aborgna-q because that trait is getting pretty big and this is very much non-core functionality, and built entirely on top of HugrMut. I do see what you mean about Intellisense tho, hmmm.

But, there is a blanket implementation! :)

Excellent, I think HugrLinking is very good.

lmondada · 2025-08-20T13:30:43Z

hugr-core/src/hugr/linking.rs

+        check_calls_defn_decl(&h, false, false);
+
+        // Specify which decls to transfer. No real "linking" here though.
+        for (call1, call2) in [(false, false), (false, true), (true, false), (true, true)] {


here you could use rstest #[case] or #[values] as in the test_insert_forest case :)

lmondada

The design looks solid, I like it! My main discussion points are... naming, as usual :)

acl-cqc · 2025-08-25T10:09:32Z

Merged, and hope I've fixed all the thumbs-up'd bits, either here or in #2518 !

I've renamed to trait HugrLinking - if you can see something more consistent i.e. ...Mut then by all means, but I think this is reasonable.

Also renamed insert_hugr_link_nodes to add_hugr_link_nodes and insert_from_view_link_nodes to add_view_link_nodes, which I am less happy with. More ideas:

at one point these methods were _with_defns. I moved away from that because they are really module children which could also be decls and even constants. And it's not shorter (!)... it would be good to hint at module children though.
insert -> add saves a few characters and moreover, I take insert as being "add the entrypoint subtree" which these methods do not necessarily do (only if parent is Some). Obvs maybe_insert... does not fly ;)
I wonder about something like insert_link_hugr_explicit (or _manual) with the later "by name" variant just dropping the _explicit?

acl-cqc · 2025-09-02T15:46:31Z

Ok, #2518 now merged into main, and I hope that's fixed a number of issues here. As mentioned, I've gone with trait HugrLinking and add_hugr_link_nodes / add_view_link_nodes, and Dataflow builder versions have extra with_wires. ModuleBuilder I've gone with the slightly inconsistent link_hugr_by_node as later I expect just to add a "link_hugr", but it might be more consistent to call it add_hugr_link_nodes there too (but the followup would then be..."add_hugr_link"?? "add_link_hugr"?? hmmm, you see why 😉 )

lmondada · 2025-09-03T19:47:59Z

hugr-core/src/builder/module.rs

@@ -220,17 +221,46 @@ impl<T: AsMut<Hugr> + AsRef<Hugr>> ModuleBuilder<T> {

        Ok(AliasID::new(node, name, bound))
    }
+
+    /// Adds some module-children of another Hugr to this module, with


Suggested change

/// Adds some module-children of another Hugr to this module, with

/// Add some module-children of another Hugr to this module, with

slightly subjective, but I think we mostly use the imperative form.

lmondada · 2025-09-03T19:48:09Z

hugr-core/src/builder/module.rs

+        self.hugr_mut().add_hugr_link_nodes(None, other, children)
+    }
+
+    /// Copies module-children from a HugrView to this module, with


Suggested change

/// Copies module-children from a HugrView to this module, with

/// Copy module-children from a HugrView to this module, with

lmondada · 2025-09-03T19:50:18Z

hugr-core/src/hugr/linking.rs

+///
+/// This is done by module-children from the inserted (source) Hugr replacing, or being replaced by,
+/// module-children already in the target Hugr; static edges from the replaced node,
+/// are transferred to come from the replacing node, and the replaced node(/subtree) then deleted.


Excellent, I think HugrLinking is very good.

lmondada · 2025-09-03T19:57:51Z

hugr-core/src/hugr/linking.rs

@@ -33,12 +33,12 @@ pub trait LinkHugr: HugrMut {
    ///
    /// If `parent` is `Some` but not in the graph.
    #[allow(clippy::type_complexity)]
-    fn insert_from_view_link_nodes<H: HugrView>(
+    fn add_view_link_nodes<H: HugrView>(


Ok, good name proposals!

I prefer insert to add (add sounds to me like adding an element to a container, whereas insert is a more complex process?) I admit this is subjective, so up to you.

Given your explanation, I think a good suffix for the names would be _by_nodes. Names with by are pretty idiomatic.

If you say linking by name will be the most common use case, then I am not fussed if the method names for linking explicitly by node are quite verbose. As you suggest I would just drop the by_nodes suffix for the link-by-name versions.

So maybe insert_link_view_by_nodes and insert_link_hugr_by_nodes? I don't know at this point 😅

Ok, so

I think you're right about insert rather than add: I think "insert" is "put the entrypoint-subtree under a new parent" whereas "link" is "do stuff with the module children" (only). Using "add" for "maybe insert" is a faux pas, will just go with insert.

Let's also establish the "link....by_nodes" convention (foreshadowing upcoming+shorter "link...." that's by name), replacing current "...._link_nodes".

I also think rejecting names for being too long may be a bad move here. There's an old idea that the more a method does, the longer its name should be. These methods do quite a lot, so long names are at least ok....

Hence: I propose: HugrMut::insert_link_hugr_by_node (and s/hugr/view/), Dataflow builder add_link_hugr_by_node (where add in the builder means insert elsewhere but nvm), Module builder link_hugr_by_node (without the add: it only does stuff with module children).

Will enact shortly unless you object @lmondada...

oh, by_node singular or plural? I think I favour singular

Yes I like it!

Ok, done. insert_link_view_.... isn't great because there is also a thing called a LinkView but it is portgraph really.

And the Dataflow builder now has add_link_hugr_by_node_with_wires which is a lot of suffixes 👎 so better ideas welcome, but add.....with_wires_by_node is worse (it's not the wires that are by node it's the "link"!)

acl-cqc added 6 commits August 11, 2025 13:28

Add HugrMut::insert_forest / insert_view_forest, implement insert_hug…

1723b48

…r/from_view

insert_subgraph delegates to insert_forest (copying nodes list)

2e66542

errors; docs

02fea73

Tests

fdee944

fix test - edges removed rather than delocalized

0eaf3eb

Merge remote-tracking branch 'origin/main' into acl/insert_forest

05072b6

This was referenced Aug 13, 2025

feat: insert_hugr copies Module-level children, add insert..._link_nodes #2285

Closed

feat: [RFC] linking by name when inserting Hugrs #2481

Closed

acl-cqc added 7 commits August 13, 2025 12:04

More docs/links

f55c288

WIP

3621a7f

Dedup dfg_calling_defn_decl, start

0f3883b

Implement LinkHugr, split old insert_link_nodes -> check_directives+l…

865fdb8

…ink_nodes

Fix test, and note duplication

54e87a0

Common up insertion-checking in tests; expand doc

b522ff1

fix doclinks

5774665

acl-cqc force-pushed the acl/insert_link_nodes branch from 3a47eb3 to 5774665 Compare August 13, 2025 11:15

acl-cqc requested a review from aborgna-q August 13, 2025 11:24

acl-cqc marked this pull request as ready for review August 13, 2025 11:24

acl-cqc requested a review from a team as a code owner August 13, 2025 11:24

acl-cqc marked this pull request as draft August 13, 2025 12:32

Error on overlapping Add::replace; handle Add overlapping w/ UseExist…

7c41f73

…ing NOTESTS

acl-cqc commented Aug 13, 2025

View reviewed changes

hugr-core/src/builder.rs Show resolved Hide resolved

acl-cqc commented Aug 13, 2025

View reviewed changes

hugr-core/src/builder/build_traits.rs Show resolved Hide resolved

acl-cqc commented Aug 13, 2025

View reviewed changes

acl-cqc marked this pull request as ready for review August 13, 2025 13:29

acl-cqc added 3 commits August 13, 2025 16:17

test multi-replace / replace+add

e70015f

fix doc

7001f9c

NodeLinkingDirective::replace takes impl IntoIterator

698283c

acl-cqc requested review from lmondada and removed request for aborgna-q August 20, 2025 10:14

lmondada requested changes Aug 20, 2025

View reviewed changes

acl-cqc added 8 commits August 21, 2025 18:14

Builder docs for #2496

f8bdaa1

remove not-yet-existent doclink

17a8de4

Improve doc of dfg_calling_defn_decl

191119f

Merge branch 'acl/insert_forest' into acl/insert_link_Nodes

a3341df

Rename Builder add_(hugr_=>)view_with_wires_link_nodes

351fa78

Rename LinkHugr => HugrLinking

871e26e

Rename insert_(hugr/from_view)_link_nodes => add_(hugr/view)_link_nodes

ccc57ba

Review suggestions (doc improvements)

f3d0d84

acl-cqc added 2 commits August 25, 2025 11:11

doc: HugrLinking: add _static_ edges between old and inserted nodes

3438e5e

Improve docs: module-children, not necessarily funcdefn/decl

2a6f33a

acl-cqc mentioned this pull request Aug 25, 2025

feat: Add method to link Hugr modules (linking 3/4) #2529

Draft

acl-cqc changed the title ~~feat: add trait+funcs for linking Hugrs explicitly by Node~~ feat: add trait+funcs for linking Hugrs explicitly by Node (linking 2/4) Aug 25, 2025

acl-cqc added 2 commits September 1, 2025 18:13

insert_view_forest uses iter_batched and recreates host each time

685e19f

oops, fix borrow in bench

57e58e2

Base automatically changed from acl/insert_forest to main September 2, 2025 13:16

acl-cqc added 3 commits September 2, 2025 14:18

Merge remote-tracking branch 'origin/main' into acl/insert_forest

6850d9d

Merge branch 'acl/insert_forest' into acl/insert_link_nodes

eac8d74

Add ModuleBuilder::add_{hugr,view}_link_nodes

5979bf6

acl-cqc requested a review from lmondada September 2, 2025 15:46

ModuleBuilder::add_(hugr/view)_link_nodes => link_(hugr/view)_by_node

1a3e37a

lmondada approved these changes Sep 3, 2025

View reviewed changes

acl-cqc added 3 commits September 4, 2025 17:57

HugrMut methods are insert_link_(hugr/view)_by_node

cc861fa

Dataflow builder: add_link_(hugr/view)_by_node_with_wires

0de1326

doclinks

97b3bf1

	/// Copy a hugr-defined op into the sibling graph, wiring up the
	/// Copy the op designed by the HUGR entrypoint into the sibling graph, wiring up the

	/// Methods for linking Hugrs, i.e. merging the Hugrs and adding edges between old and inserted nodes.
	/// Methods that merge Hugrs, adding edges between old and inserted nodes.

	/// Adds some module-children of another Hugr to this module, with
	/// Add some module-children of another Hugr to this module, with

	/// Copies module-children from a HugrView to this module, with
	/// Copy module-children from a HugrView to this module, with

feat: add trait+funcs for linking Hugrs explicitly by Node (linking 2/4) #2521

Are you sure you want to change the base?

feat: add trait+funcs for linking Hugrs explicitly by Node (linking 2/4) #2521

Uh oh!

Conversation

acl-cqc commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

acl-cqc commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acl-cqc Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmondada left a comment

Choose a reason for hiding this comment

Uh oh!

acl-cqc commented Aug 25, 2025

Uh oh!

acl-cqc commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmondada Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acl-cqc commented Aug 13, 2025 •

edited

Loading

codecov bot commented Aug 13, 2025 •

edited

Loading

acl-cqc commented Aug 13, 2025 •

edited

Loading

acl-cqc Aug 13, 2025 •

edited

Loading

acl-cqc commented Sep 2, 2025 •

edited

Loading

lmondada Sep 3, 2025 •

edited

Loading