feat: Add method to link Hugr modules (linking pt3 of >=4) #2529

acl-cqc · 2025-08-25T11:33:19Z

Adds new methods link_module and link_module_view.

These ignore the entrypoint.
But take a 'policy' that acts on names of public functions to determine how to link the two Hugrs.

closes #2356

…r/from_view

…ink_nodes

…ing NOTESTS

This reverts commit caecc43.

acl-cqc · 2025-09-16T11:24:46Z

hugr-core/src/hugr/linking.rs

+/// Describes ways to link a "Source" Hugr being inserted into a target Hugr.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct NameLinkingPolicy {
+    // TODO: consider pub-funcs-to-add? (With others, optionally filtered by callgraph, made private)


I can remove these various (non-doc) comments about future ideas...

I think these comments would be better suited as an issue than as comments here.

It's more searchable and you can be as verbose as you want without bloating the source code.

acl-cqc · 2025-09-16T11:27:08Z

hugr-core/src/hugr/linking.rs

+/// [Visibility::Public]: crate::Visibility::Public
+#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
+#[non_exhaustive] // could consider e.g. disconnections
+pub enum MultipleImplHandling {


A possibility is to call this just NewImplHandling with the idea of adding in the future an option ::ConvertToDecls, this would avoid adding new code (except for the entrypoint subtree, when the next PR adds the feature to insert that alongside linking). Existing enum variants would then be PreferExisting, PreferNew, and a wrapper around NewFuncHandling (which allows to e.g. error upon any new impl)

ImplMergeOptions?

You can probably tell by now I dont find "Handling" particularly descriptive 😂

Yeah, "Handling" is not too good. I mean, "Options" only tells us the thing is an enum, but I guess this could be..."ImplMerging"? (probably DefnMerging)?

MergeBehaviour, MergePolicy, HowToMerge(Defns), ...

Looking at the other suggestions...ImplAddSomething? DefnAddOutcome? (These options are appropriate for impls/defns only not other things as that's the only situation where you have two things that can be merged)

DefnMerging::ConvertToDecls (later PR) is not great because that's about adding Defns not merging them...

BlahAddPolicy is not ridiculous - a NameLinkingPolicy is then largely composed of other/sub policies. But Policy may make this simple thing sound more complex than it is...

acl-cqc · 2025-09-16T11:28:55Z

hugr-core/src/hugr/linking.rs

+    /// multiple [FuncDefn]s according to `multi_impls`.
+    ///
+    /// [FuncDefn]: crate::ops::FuncDefn
+    pub fn err_on_conflict(multi_impls: impl Into<MultipleImplHandling>) -> Self {


Names for constructors, given that there'll be significant extension to this NameLinkingPolicy class in the future, are tricky and these are a bit dubious....

acl-cqc · 2025-09-16T11:30:23Z

hugr-core/src/hugr/linking.rs

+    /// [Public] function with the same name but different signatures.
+    ///
+    /// [Public]: crate::Visibility::Public
+    pub fn on_signature_conflict(mut self, sc: NewFuncHandling) -> Self {


I felt that the short expression style NameLinkingPolicy::foo().on_xyz(bar) was preferable for configuration rather than having to {let pol = NameLinkingPolicy::foo(); *pol.xyz() = bar; pol } over several lines. Any counter-opinions?

acl-cqc · 2025-09-16T11:34:44Z

hugr-core/src/hugr/linking.rs

+/// [Visibility::Public]: crate::Visibility::Public
+#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
+#[non_exhaustive] // could consider e.g. disconnections
+pub enum NewFuncHandling {


The "New" is perhaps a misnomer. It can be applied in the situation where the signature is "new" (for the same name, so a conflict); or when the impl (function body in a FuncDefn) is new (and there is an existing impl too). In the next PR one can apply it whenever the source Hugr adds a new name. Does that merit the New prefix here??

Maybe FuncCreationOptions?

Ah but you have further enums with the same naming pattern below...

This is basically "how to add a function to a Hugr". So your FuncCreation(Option) is not bad. FuncAddOptions, hmmm (I have a mild dislike of "Options" as you can probably also tell 😁). HowToAddFunc (!), FuncAdd(Policy/Method/Technique/How). Resolver, Resolution (it's not an algorithm, it is pretty much a decision at the end of an algorithm). Hmmmm. This seems harder to name than the merge one above....

Maybe something like FuncAddResult? I know it's not a rust enum Result {Ok, Err} but it's actually not that far off - those are basically the two options available now (::Add and ::RaiseError), but I'll be adding ::AddIfReached and ::ErrorIfReached in the next PR #2555, and there'll be an ::MakePrivate (really ::AddIfReachedButMakePrivate) after that....)

FuncAddOutcome ??

Oh, one thing I can say is it only applies to public functions. So could be PubAddOutcome or PubFunc.... or something. Not sure that helps.

acl-cqc · 2025-09-16T11:38:05Z

My commiserations to @lmondada to having been randomly picked again...you're gonna hate linking before long ;-).

I'm also adding @doug-q for a look at the interface but perhaps he should leave implementation code detail questions to Luca in order to share the burden.

acl-cqc · 2025-09-16T11:49:51Z

hugr-core/src/hugr/linking.rs


+/// Details a concrete action to link a specific node from source Hugr into a specific target Hugr.
+///
+/// A separate enum from [NodeLinkingDirective] to allow [NameLinkingPolicy::to_node_linking]


This may not be appropriate for a doc comment, maybe a non-doc comment or maybe just this review comment i.e. remove it...

acl-cqc · 2025-09-16T12:42:18Z

hugr-core/src/hugr/linking.rs

+    /// 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.
+    // Yes at present we copy all private children, i.e. a safe over-approximation!


I'm debating whether to

specify a vague "at least" bound on behaviour in the docs; and then quietly change the actual behaviour (from the maximum set within that bound, to the minimum) in the next PR

Add an extra flag in the next PR to preserve the exact behaviour introduced in this PR (i.e. add all the private functions, even unused ones, unless overridden). feat: (WIP) Add insert_link_hugr to add entrypoint subtree AND link #2555 currently takes this latter approach and it's not super-difficult but mildly annoying. (And do people really want those private funcs to be added?)

Thoughts welcome! :-)

Not adding unused private functions sounds like the right thing to do. Let's not bloat the Hugr unless we have a very good reason to.

If we do come up with a use case in the future for adding those private functions, it's an easy change.

Regarding docs: it's up to you. Behaviour change is not breaking, so even if there's a release in between it would not matter.

acl-cqc · 2025-09-16T14:14:58Z

hugr-core/src/hugr/linking.rs

+    #[error("Source ({_1}) and target ({_2}) both contained FuncDefn with same public name {_0}")]
+    MultipleImpls(String, SN, TN),
+    /// Source and target containing public functions with conflicting signatures
+    // TODO ALAN Should we indicate which were decls or defns? via an extra enum?


Opinions here?

I think the error is clear enough as is.

Copilot

Pull Request Overview

This PR adds functionality to link Hugr modules by introducing link_module and link_module_view methods that operate on module children based on a configurable linking policy. The implementation focuses on public functions and provides flexible conflict resolution strategies for handling naming and signature conflicts between modules.

Adds NameLinkingPolicy with configurable behavior for signature conflicts and multiple implementations
Implements link_module and link_module_view methods that apply the policy to merge module children
Provides comprehensive error handling and validation for various linking scenarios

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

hugr-core/src/hugr/linking.rs

Copilot · 2025-09-17T05:21:54Z

hugr-core/src/hugr/linking.rs

+                                    name: name.clone(),
+                                    src_node: n,
+                                    src_sig: Box::new(sig.clone()),
+                                    tgt_node: *ex_ns.as_ref().left_or_else(|(n, _)| n),


The expression *ex_ns.as_ref().left_or_else(|(n, _)| n) is complex and hard to read. Consider extracting this logic into a helper method or using a clearer pattern match to get the target node from the Either type.

Copilot · 2025-09-17T05:21:54Z

hugr-core/src/hugr/linking.rs

+            let Some((mut acc, sig1)) = acc else {
+                return Some((new.map_right(|n| (n, vec![])), sig2));
+            };
+            assert_eq!(sig1, sig2, "Invalid Hugr: different signatures for {name}");


Using assert_eq! with a custom message that includes {name} won't interpolate the variable. Use panic! with format string instead: panic!(\"Invalid Hugr: different signatures for {name}\");

Suggested change

assert_eq!(sig1, sig2, "Invalid Hugr: different signatures for {name}");

assert_eq!(sig1, sig2, "Invalid Hugr: different signatures for {}", name);

Copilot · 2025-09-17T05:21:54Z

hugr-core/src/hugr/linking.rs

+#[derive(Clone, Debug, Hash, PartialEq, Eq, derive_more::From)]
+#[non_exhaustive]
+pub enum LinkAction<TN> {
+    /// Just apply the specified [NodeLinkingDirective].
+    LinkNode(#[from] NodeLinkingDirective<TN>),
+}


[nitpick] The LinkAction enum currently has only one variant LinkNode. If this is intended for future extensibility, consider adding a comment explaining the planned additional variants. Otherwise, this abstraction layer may be unnecessary.

lmondada

I'll finish my review later. Here's the first half

lmondada · 2025-09-17T06:03:03Z

hugr-core/src/hugr/linking.rs

+    /// Computes how this policy will act on a specified source (inserted) and target
+    /// (host) Hugr.
+    #[allow(clippy::type_complexity)]
+    pub fn to_node_linking<T: HugrView + ?Sized, S: HugrView>(


Nit: presumably S does not need to be Sized either

acl-cqc added 30 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

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

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

7c41f73

…ing NOTESTS

test multi-replace / replace+add

e70015f

fix doc

7001f9c

NodeLinkingDirective::replace takes impl IntoIterator

698283c

Avoid test nondeterminism; clippy

18e1cf7

Add NameLinkingPolicy+friends, and LinkHugr::link_hugr

5e0cf33

Remove copy_private_funcs; fix test to expect main

c210f28

Accessor/factory methods (need docs), enum SignatureConflictHandling

68a1ca6

Optimize by turning HashMap into impl IntoIterator, avoid size_hint

10778ce

struct InsertedForest; use InsertForestResult alias more widely

826990d

N -> SN

f9eee44

Make DoubleCopy contain a named field

caecc43

Revert "Make DoubleCopy contain a named field"

bd12196

This reverts commit caecc43.

Use rstest::fixture

bdc0ff1

test tidy

58dccdd

add benchmark

b1c0d2a

lint benchmark and add to criterion group

88a4987

acl-cqc added 9 commits September 14, 2025 08:54

NameLinkingPolicy mutators are self->Self

158fcfd

NodeLinkingDirective is non-exhaustive

82759b5

Merge branch 'acl/insert_link_nodes' into HEAD

2c00827

Add struct LinkerActions

0d9db3a

Revert; add enum LinkAction (and map thereof)

0c49003

Review comment be more imperative

70b4e3f

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

3193ea4

Merge branch 'acl/insert_link_nodes' into acl/link_hugr

c3bcbc9

Drop Visibility::is_public, single callee and better to inline

a76c37c

acl-cqc commented Sep 16, 2025

View reviewed changes

Use derive_more::From

23e680a

acl-cqc commented Sep 16, 2025

View reviewed changes

acl-cqc added 2 commits September 16, 2025 12:35

Drop S: ?Sized on to_node_linking

2a608df

Add LinkAction From<NodeLinkingDirective>

5ca0e5f

acl-cqc marked this pull request as ready for review September 16, 2025 11:36

acl-cqc requested a review from a team as a code owner September 16, 2025 11:36

acl-cqc requested a review from lmondada September 16, 2025 11:36

acl-cqc requested a review from doug-q September 16, 2025 11:38

acl-cqc changed the title ~~feat: Add method to link Hugr modules (linking 3/4)~~ feat: Add method to link Hugr modules (linking pt3 of >=4) Sep 16, 2025

acl-cqc commented Sep 16, 2025

View reviewed changes

acl-cqc mentioned this pull request Sep 16, 2025

feat: (WIP) Add insert_link_hugr to add entrypoint subtree AND link #2555

Draft

2 tasks

acl-cqc commented Sep 16, 2025

View reviewed changes

lmondada requested a review from Copilot September 17, 2025 05:20

Copilot AI reviewed Sep 17, 2025

View reviewed changes

lmondada reviewed Sep 17, 2025

View reviewed changes

	assert_eq!(sig1, sig2, "Invalid Hugr: different signatures for {name}");
	assert_eq!(sig1, sig2, "Invalid Hugr: different signatures for {}", name);

feat: Add method to link Hugr modules (linking pt3 of >=4) #2529

Are you sure you want to change the base?

feat: Add method to link Hugr modules (linking pt3 of >=4) #2529

Uh oh!

Conversation

acl-cqc commented Aug 25, 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!

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!

acl-cqc Sep 17, 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!

acl-cqc commented Sep 16, 2025

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

lmondada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acl-cqc commented Aug 25, 2025 •

edited

Loading

acl-cqc Sep 17, 2025 •

edited

Loading