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

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

closes #2356, adding 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.

Does not use callgraph/reachability; this is coming in #2555. WAIT TO MERGE at the same time as #2555 to avoid the behaviour change of not inserting unreachable private functions.

There are a few other suggestions for even further into the future in #2687.

Questions for review now:

factory methods for NameLinkingPolicy? There are 8 possible policies now and more fields will be added (e.g. in feat: insert_link_hugr adds entrypoint subtree and links, with reachability #2555), so an explicit new doesn't work well. Is there a good default? Or a better set of factory methods? (They should at least be called new_, yes.)
Do tests need to be more systematic about covering the possible policy variants?

…r/from_view

…ink_nodes

…ing NOTESTS

This reverts commit caecc43.

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: insert_link_hugr adds entrypoint subtree and links, with reachability #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.

Behaviour change is breaking in my book, but I reckon the best plan is to avoid releasing inbetween this PR and #2555 being merged after it ;)

hugr-core/src/hugr/linking.rs

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

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

My view here is that having to specify + ?Sized is the annoying non-default thing that should be minimized....unfortunately trait LinkHugrs default impls don't compile without the first one.

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

hugr-core/src/hugr/linking.rs

Copilot · 2025-11-17T13:28:33Z

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}");


The error message 'Invalid Hugr: different signatures for {name}' should use :? formatting for the name variable to ensure proper display. Change to \"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-11-17T13:28:33Z

hugr-core/src/hugr/linking.rs

+                    true => "Multiple FuncDefns",
+                    false => "FuncDefn and FuncDecl(s)",
+                };
+                panic!("Invalid Hugr: {err} for {name}");


The panic message should use :? formatting for the name variable. Change to panic!(\"Invalid Hugr: {err} for {name:?}\");.

Suggested change

panic!("Invalid Hugr: {err} for {name}");

panic!("Invalid Hugr: {err} for {name:?}");

hugr-core/src/hugr/linking.rs

acl-cqc · 2025-11-17T22:05:52Z

hugr-core/src/hugr/linking.rs

+
+    #[rstest]
+    fn combines_decls_defn(
+        #[values(OnNewFunc::RaiseError, OnNewFunc::Add)] sig_conflict: OnNewFunc,


This mass of #[values....] is because none of these actually matter, the test is the same in all cases...

I'd like to define a pytest-like "parametrized fixture" #[fixture] fn any_policy(#[values....] ....) -> NameLinkingPolicy and then have every test using any_policy get run multiple times, but rstest doesn't offer such a facility. There are crates that do: https://crates.io/crates/rstest_reuse (a small addition) and https://crates.io/crates/rustest (a complete replacement of rstest)....

doug-q

Looks Great!

Just nits.

doug-q · 2025-11-24T11:12:00Z

hugr-core/src/hugr/linking.rs

    }
 }

+/// Describes ways to link a "Source" Hugr being inserted into a target Hugr.


NIT this seems inconsistent, i would choose from source target and "Source" "Target"

Took me a moment to grok that one....lol, love it, will do

doug-q · 2025-11-24T11:13:05Z

hugr-core/src/hugr/linking.rs

+///
+/// [FuncDefn]: crate::ops::FuncDefn
+/// [Visibility::Public]: crate::Visibility::Public


Suggested change

///

/// [FuncDefn]: crate::ops::FuncDefn

/// [Visibility::Public]: crate::Visibility::Public

doug-q · 2025-11-24T11:14:36Z

hugr-core/src/hugr/linking.rs

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


I think this could also apply to FuncDecls.

?? If their names and sigs are the same, we can just merge them, they are identical (aliases) - of course we cannot merge impls (ok, we could check same nodes in same order, build a map between the indices, and then check the edges matched...even that is quite a bit of work and would miss reorderings of nodes, which brings us to graph isomorphism).

Or do you mean, so you can control whether aliases are combined vs kept separate?

I was thinking about controlling when they are combined vs kept separate.
But this is fine as is

The other way to look at it is that switching between impls of the same-name-and-signature FuncDefn doesn't invalidate the Hugr, whereas other changes invalidate....?

doug-q · 2025-11-24T11:16:55Z

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>(


Not a big fan of to_node_linking. It makes me think that we are converting into a NodeLinking, which isn't a thing. What about link_actions

doug-q · 2025-11-24T11:17:42Z

hugr-core/src/hugr/linking.rs

+    ///
+    /// [Visibility::Public]: crate::Visibility::Public
+    /// [FuncDefn]: crate::ops::FuncDefn
+    fn link_module(


I think we should be explicit that entrypoint is ignored

hugr-core/src/hugr/linking.rs

doug-q · 2025-11-24T11:21:27Z

hugr-core/src/hugr/linking.rs

+/// Details the concrete actions to link a specific source Hugr into a specific target Hugr.
+///
+/// Computed from a [NameLinkingPolicy] and contains all actions required to implement
+/// that policy (for those specific Hugrs).


Suggested change

/// that policy (for those specific Hugrs).

/// that policy for those specific Hugrs.

doug-q · 2025-11-24T11:21:44Z

hugr-core/src/hugr/linking.rs

+///
+/// Computed from a [NameLinkingPolicy] and contains all actions required to implement
+/// that policy (for those specific Hugrs).
+pub type LinkActions<SN, TN> = HashMap<SN, LinkAction<TN>>;


Can we use BTreeMap so that determinism is never an issue?

I've switched this one to BTreeMap because it's externally visible and so you might print it. The ordering should not affect the result of linking (unless you hand-construct a map of actions which modify each other or something), so I've not switched any of the other HashMaps....

I would prefer to use BTreeMap everywhere.
When we have determinism bugs we will grep for Hash everywhere.

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 mentioned this pull request Sep 16, 2025

feat: insert_link_hugr adds entrypoint subtree and links, with reachability #2555

Open

acl-cqc commented Sep 16, 2025

View reviewed changes

hugr-core/src/hugr/linking.rs Outdated Show resolved Hide resolved

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

Copilot AI reviewed Sep 17, 2025

View reviewed changes

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

hugr-core/src/hugr/linking.rs Outdated Show resolved Hide resolved

hugr-core/src/hugr/linking.rs Outdated Show resolved Hide resolved

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

lmondada reviewed Sep 17, 2025

View reviewed changes

acl-cqc added 7 commits November 17, 2025 11:38

(Copilot-suggested) doc expected future LinkAction

8746a8c

Copilot-suggested fn target_node(&PubFuncs)...take further?

cfd4595

No further...target_node(&Either...)

dd06d44

NewFuncHandling => OnNewFunc, MultiImplHandling => OnMultiDefn

e9ae392

remove TODOs

298c177

Remove 'shall we' comment on error - we shall not

528b3c7

doc expectations re. change of behaviour

8b642fd

acl-cqc mentioned this pull request Nov 17, 2025

Further linking ideas (speculative) #2687

Open

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

7e87298

acl-cqc requested a review from Copilot November 17, 2025 13:27

Copilot AI reviewed Nov 17, 2025

View reviewed changes

acl-cqc added 5 commits November 17, 2025 13:31

copilot assert

4b5730e

More copilot: &String -> &str

203923b

Correct comment implement->link *1

3f5f925

Tidy let-else+panic into match+2*panic

d45db9d

Rename impl->defn accessors + MultipleImpls->Defn error variant

fd6ad94

acl-cqc commented Nov 17, 2025

View reviewed changes

acl-cqc added 2 commits November 21, 2025 19:34

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

da2efb3

NameLinkingError is non_exhaustive

7925437

doug-q approved these changes Nov 24, 2025

View reviewed changes

acl-cqc added 2 commits November 24, 2025 21:01

comments+renames from review

0bd5147

Use BTreeMap for LinkActions

8a58970

acl-cqc added the wait to merge label Nov 25, 2025

Remove 3* type_complexity, sort some imports

c0102f2

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

	panic!("Invalid Hugr: {err} for {name}");
	panic!("Invalid Hugr: {err} for {name:?}");

	///
	/// [FuncDefn]: crate::ops::FuncDefn
	/// [Visibility::Public]: crate::Visibility::Public

	/// that policy (for those specific Hugrs).
	/// that policy for those specific Hugrs.

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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lmondada left a comment

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 Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-q left a comment • 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!

acl-cqc Nov 24, 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!

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!

Reviewers

Assignees

acl-cqc commented Aug 25, 2025 •

edited

Loading

doug-q left a comment •

edited

Loading

acl-cqc Nov 24, 2025 •

edited

Loading