feat: Add CallGraph struct, and dead-function-removal pass

[acl-cqc]

Closes #1753.

• remove_polyfuncs preserved but deprecated, some uses in tests replaced to give coverage here.
• Future (breaking) release to remove the automatic-remove_polyfuncs that currently follows monomorphization.

[codecov]

Codecov Report

Attention: Patch coverage is 84.07080% with 18 lines in your changes missing coverage. Please review.

Project coverage is 86.63%. Comparing base (1f841ae) to head (4a07dee).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-passes/src/dead_funcs.rs	80.95%	10 Missing and 2 partials ⚠️
hugr-passes/src/call_graph.rs	93.02%	2 Missing and 1 partial ⚠️
hugr-passes/src/monomorphize.rs	57.14%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1796      +/-   ##
==========================================
- Coverage   86.63%   86.63%   -0.01%     
==========================================
  Files         191      194       +3     
  Lines       34738    34885     +147     
  Branches    31571    31698     +127     
==========================================
+ Hits        30097    30224     +127     
- Misses       2940     2954      +14     
- Partials     1701     1707       +6     

Flag	Coverage Δ
python	92.31% <ø> (-0.11%)	⬇️
rust	86.06% <84.07%> (+<0.01%)	⬆️

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.

[ss2165]

can we deprecate remove_polyfuncs instead so we can include this in a non-breaking release?

[acl-cqc]

can we deprecate remove_polyfuncs instead so we can include this in a non-breaking release?

Yes, well, we can, and I have, but I admit I am not singing the praises of the tooling here - deprecate has a "since" tag for tools that might use it, but IIUC I have to guess what the next version number will be (i.e. the version in which it'll be deprecated); and the re-export causes problems (I have to suppress a warning there - I tried putting the #[deprecated] only on the re-export but that did not generate a warning even when I used the function via the re-export). If I'm doing this wrong @ss2165 then please shout...

[tatiana-s]

Generally looks good to me, just one comment mainly for my understanding.

[tatiana-s]

Just for clarification - is enclosing the node in the call graph corresponding to the node node in the hugr or something else?

[acl-cqc]

It's the (callgraph representation of) the nearest enclosing FuncDefn, so maybe the node's parent or an ancestor. I'll rename...

[doug-q]

As above, I don't think the second should panic.

I think the interface would be cleaner if entry_points, maybe with a different name: must be FuncDefn or FuncDecl nodes that are immediate children of the root.

Now the first panic goes away, and the third would be an error with the offending nodes.

[acl-cqc]

Changed to error, indeed. But FuncDefns inside a non-Module are invisible from outside (so unless you're gonna add new stuff inside the root - which you can do, but that's not linking, that's....arbitrary editing), so I've not allowed those as entry_points. I could be persuaded, it'd be easier from a code perspective not to check, but it feels wrong :-!

[doug-q]

We should remove dead FuncDecls too

[acl-cqc]

Done (no tests I admit)

[doug-q]

Did you consider tracking in the Node weight whether it's a FuncDecl or a FuncDefn?

[acl-cqc]

Did you consider tracking in the Node weight whether it's a FuncDecl or a FuncDefn?

No, good idea, done

[doug-q]

Great, thank you.

[acl-cqc]

Ok, so the big question here (@doug-q) is what happens to the "automatic" remove_polyfuncs on non-Module Hugrs. The uncertainty suggests that perhaps that was always a bad move, so options:

1. Remove it: no automatic dead/poly-function removal. It is always up to the caller. Definitely the easiest/least-code option, but also the most breaking.
2. Switch to removing all dead functions (for non-Module-rooted Hugrs). This is what the PR does currently. It seems a bit bizarre/random that dead non-polymorphic functions are removed by monomorphization (and this could potentially break some client code at runtime), but non-Module Hugrs are only really here for tests, production users are probably unaffected.
3. Keep remove_polyfuncs, as per before this PR. In that case ideally I'd like to automatically remove polymorphic FuncDefns from Modules too where they were not previously children of the Module (most will become children of the Module afterwards because of flattening), as this would be more consistent with non-Modules. That could be a follow-up, I guess for this PR we'd leave things, and/or we could still deprecate the public remove_polyfuncs (i.e. moving towards a situation where remove_polyfuncs lives on but as an internal thing for monomorphization, and external users have only remove_dead_funcs - which could be extended via a filter-callback if we wanted). This might be best in some way, but clearly involves the most code, and the step to remove "invisible" polyfuncs from Modules is slightly breaking.

Thoughts?

[doug-q]

Ok, so the big question here (@doug-q) is what happens to the "automatic" remove_polyfuncs on non-Module Hugrs. The uncertainty suggests that perhaps that was always a bad move, so options:

1. Remove it: no automatic dead/poly-function removal. It is always up to the caller. Definitely the easiest/least-code option, but also the most breaking.

I don't think breaking changes are a concern here. There are no users of this on non-module hugrs, so no users to break. I think this is my favoured option, I consider it not-onerous-at-all to ask callers to remove the dead polymorphic functions.

2. Switch to removing all dead functions (for non-Module-rooted Hugrs). This is what the PR does currently. It seems a bit bizarre/random that dead non-polymorphic functions are removed by monomorphization (and this could potentially break some client code at runtime), but non-Module Hugrs are only really here for tests, production users are probably unaffected.

Agree that non-polymorphic functions being removed is bizarre. I think it would be better to do no removal.

3. Keep remove_polyfuncs, as per before this PR. In that case ideally I'd like to automatically remove polymorphic FuncDefns from Modules too where they were not previously children of the Module (most will become children of the Module afterwards because of flattening), as this would be more consistent with non-Modules. That could be a follow-up, I guess for this PR we'd leave things, and/or we could still deprecate the public remove_polyfuncs (i.e. moving towards a situation where remove_polyfuncs lives on but as an internal thing for monomorphization, and external users have only remove_dead_funcs - which could be extended via a filter-callback if we wanted). This might be best in some way, but clearly involves the most code, and the step to remove "invisible" polyfuncs from Modules is slightly breaking.

I think it's fine to remove polymorphic FuncDefns where they were not previously children of the module. Having said that, I have a weak preference to not do so. To me it makes the pass strictly less useful:

• You still have to run a dead-func-removal (to get the module-level polymorphic functions. We can't do any better because we can't reason about public functions in general yet)
• If you cared about some interior function for some reason deleting it may be annoying
  Why would you care about said interior function? I don't know, but deleting it here does not seem to accomplish anything

Thoughts?

For dead func removal:

I would prefer a completely uniform treatment of Module and non-Module rooted HUGRs. In both cases:

• pass a list of roots(FuncDecl/FuncDefn nodes). They are not constrained to be children of the root
• Every function reachable from a root is kept alive
• The HUGR root is kept alive
• If a roots parent would be removed, first move the root to be a child of the root.

I'm also ok with the pass as-is, if we need the additional flexibility afforded by the above we can change it then.

For monomorphise:
I think we should do the simplest thing: no removing functions, documentation pointing to DeadFuncRemoval

[acl-cqc]

For dead func removal:

I would prefer a completely uniform treatment of Module and non-Module rooted HUGRs. In both cases:

• pass a list of roots(FuncDecl/FuncDefn nodes). They are not constrained to be children of the root
• Every function reachable from a root is kept alive
• The HUGR root is kept alive

Ok - sounds reasonable - I would probably move to adding a callback/filter at the same time, but either way, this can be done later as a non-breaking change (it just allows more things), so no urgency, I'll leave it for now.

For monomorphise: I think we should do the simplest thing: no removing functions, documentation pointing to DeadFuncRemoval

Ok, we agree that option 1 is the best target 👍 👍. I'm a bit hazy on the definition of breaking change (does that include runtime-breaking i.e. any behaviour change but not compile-breaking?), but erring on the side of caution I'll categorize dropping the automatic call to remove_polyfuncs as breaking, so not do that here - I've marked it with a TODO for the next release and kept the call to the now-deprecated remove_polyfuncs instead.