feat: insert_hugr copies Module-level children, _with_defns links by Node

[acl-cqc]

• Add HugrMut::insert_hugr_with_defns and HugrMut::insert_from_view_with_defns that take a map from module-level children of the inserted Hugr (i.e. outside the entrypoint / the bit that gets inserted at the "desired location") to an instruction to either Add them to the target Hugr, or replace (edges from) them with a node existing in the target Hugr.
  • ReplaceWith allows linking e.g. FuncDecls in the inserted Hugr to FuncDefns in the target Hugr
  • We can imagine adding an enum variant that uses a subtree in the inserted Hugr (e.g. FuncDefn) to replace a module-level child in the target Hugr (e.g. FuncDecl), too (but not done in this PR)
• insert_hugr defaults to copying all module-level children; insert_from_view defaults to not inserting any. (The difference because insert_hugr consumes your Hugr, whereas with ..._from_view you still have it.)
  • Since SimpleReplacement uses insert_hugr we get back the ability to SR in FuncDefns lost in feat!: No nested FuncDefns (or AliasDefns) #2256 (I've added a test; the same should work for NodeTemplate::CompoundOp but I haven't tested this.)
• Similarly add to builder add_hugr(_view)_with_wires_and_defns (we could also have a non-dataflow add_hugr_with_defns but I skipped this as a less-common case; we could also add a NodeTemplate::CompoundOpWithDefns with an explicit map.)

Naming: _with_defns obviously isn't quite right, they could be decls too (even module-level constants!). _with_linkage seems slightly premature but maybe??

The plan is that when we add link_name to FuncDefn/FuncDecl, the "default" for insert_hugr will be to Replace, etc., defns/decls with the same link_name. (Not clear what to do by default if both target and inserted have a FuncDefn with the same link_name, but we could favour either one, add both and make an invalid hugr, or panic.)

However there is an alternative possibility, to end this PR with just a set of module children to copy (i.e. 0db0ccd - see diff) and then to add a method on a Hugr(Mut) that merges together FuncDefns/Decls with the same link_name. This raises similar questions, e.g. does it panic on two FuncDefns with the same link_name, and is not as flexible for funcs without link_name. I guess it would probably have all the same options as InsertDefnMode here.

closes #2355

[codecov]

Codecov Report

Attention: Patch coverage is 95.56314% with 13 lines in your changes missing coverage. Please review.

Project coverage is 82.12%. Comparing base (5bbe0cd) to head (960e9a0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/builder/build_traits.rs	82.50%	5 Missing and 2 partials ⚠️
hugr-core/src/hugr/hugrmut.rs	98.63%	2 Missing and 1 partial ⚠️
hugr-core/src/hugr/views/rerooted.rs	0.00%	2 Missing ⚠️
hugr-core/src/hugr/views/impls.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2285      +/-   ##
==========================================
+ Coverage   82.01%   82.12%   +0.10%     
==========================================
  Files         245      245              
  Lines       45474    45730     +256     
  Branches    41210    41466     +256     
==========================================
+ Hits        37297    37555     +258     
+ Misses       6172     6167       -5     
- Partials     2005     2008       +3     

Flag	Coverage Δ
python	85.38% <ø> (ø)
rust	81.78% <95.56%> (+0.11%)	⬆️

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.

[acl-cqc]

Another possibility here is to say, if the inserted Hugr's entrypoint is its module-root, then

• The parent node under which to insert must be the target Hugr's module root (or else panic)
• The inserted Hugr's module root will be elided - i.e. all the children will be copied from one module root to the other

I think this makes a likely-to-become-fairly-common case, of trying to combine two Hugrs, a whole lot easier. It's not breaking in the "API-compatible" sense but does change behaviour (so might break tests). Thoughts?

[doug-q]

I think the current implementation which does not special case the insert-module-into-module case is simplest and therefore best.

I do think we should provide tools to make "linking two modules" as you describe as easy as possible, but not here.

[doug-q]

This looks right, but I admit that I don't entirely grok this.

[doug-q]

I think the current implementation which does not special case the insert-module-into-module case is simplest and therefore best.

I do think we should provide tools to make "linking two modules" as you describe as easy as possible, but not here.

[doug-q]

This comment needs to be cleaned up, I think the error should remain as-is. This(insert-module-into-module) is a reasonable thing to do, but not like this!

[acl-cqc]

This looks right, but I admit that I don't entirely grok this.

Hmmmm, ok, well. Spelling out the future in a bit more detail....

So the idea was that the new method is enough to express every plausible policy using link-name, and the next PR will add the link-name attribute/field and some of those - some subset of

• Don’t copy any module children (breaking any static edges to under the entrypoint)
• Copy all module children - maybe producing an invalid hugr if link-names are duplicated between inserted and host
• Copy module children, combining any non-conflicting children with the same link-name (two decls, or a decl and a defn with the same signature). The question is then what to do about the conflicts…
  • The possible conflicts are: decls/defns with different signatures (and the same name); or two defns with the same name - checking identify of the subtrees does not seem a good plan but if the signatures match this is not necessarily a “conflict” per se
  • Options for dealing with conflicts:
    • keep both (=> invalid hugr)
    • ignore the child from the inserted hugr, break its outgoing edges (=>invalid hugr)
    • Replace one with the other (either invalid hugr or just quietly discarding some subtree/body)
    • Raise error and don’t do insertion at all
    • Panic (not really an option - but since the plain insert_hugr does not return an error, this may be the only option left for it, or a breaking change. I’d prefer to find some nice default for insert_hugr/insert_from_view that avoids panic, though)

This direction raises two questions for this PR:

1. Are we OK with this “incremental” approach of insert_hugr_xxx doing a reasonable amount of work to keep us in a sane state (or erroring if it can’t, whatever), as opposed to the alternative where we just add everything (including multiple conflicting defns/decls) and then have a resolve_linking(&mut Hugr) method that tries to sort out everything afterwards (and errors if there are conflicts such that it can’t)? My argument is that the latter alternative makes it much harder to recover from any error because you’d need to do masses of book-keeping yourself to know what came from where.
2. Assuming we are happy with that, is the new insert_..._with_defns API right. If the above bullet points are just an enum LinkMode of options, then rather than insert_hugr_with_defns we might just want a LinkMode::ExplicitByNode(HashMap<Node, InsertDefnMode>) perhaps with some renaming (InsertDefnMode -> DefnLinkMode?). In which case that enum could be introduced in this PR (non-exhaustive, one variant) to avoid the second PR being breaking. Even if we want to keep insert_...with_defns as the leaf implementation, it might still want renaming….

[acl-cqc]

I.e. we might want a solution for #2356 as part of this too