feat: Rewrite for peeling a TailLoop

[acl-cqc]

Replaces the TailLoop with a DFG containing the body plus a Conditional, inside one Case of which there is a copy of the original TailLoop. (Uses HugrMut::copy_descendants to copy incoming edges without copying their sources.)

Letting the new DFG take the Node of the old TailLoop allows to preserve all existing wiring (including e.g. order edges for nonlocals).

closes #2107

[codecov]

Codecov Report

Attention: Patch coverage is 96.13260% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.14%. Comparing base (6cab7f9) to head (82b4221).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/hugr/patch/peel_loop.rs	96.04%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2290      +/-   ##
==========================================
+ Coverage   82.08%   82.14%   +0.05%     
==========================================
  Files         243      244       +1     
  Lines       44091    44270     +179     
  Branches    39920    40099     +179     
==========================================
+ Hits        36193    36365     +172     
- Misses       5909     5915       +6     
- Partials     1989     1990       +1     

Flag	Coverage Δ
python	85.30% <ø> (ø)
rust	81.81% <96.13%> (+0.06%)	⬆️

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.

[aborgna-q]

Logic looks great. Just a small comment about the invalidation set.

[aborgna-q]

nit: Please use named fields for variants unless that aren't direct wrappers over another error.
derive_more's Error derive assumes things about where's the source / backtrace in the positional fields of tuple structs. Naming things instead is more readable.

Suggested change

	NotTailLoop(N, OpType),
	#[display("Node to inline {node} expected to be a TailLoop but actually got a {op}")]
	NotTailLoop {
	/// The node being inlined.
	node: N,
	/// The actual non-tail loop operations.
	op: OpType,
	},

[acl-cqc]

Ugh, this is the kind of thing that makes me wonder whether we should just use thiserror instead of derive_more - IIUC the argument against is just dependencies? This was +10 lines for, IMHO, no gain in readability :-(

However, you are right to flag my display message - "inline" a TailLoop, oops ;-)

[aborgna-q]

Mind you, tuple enums do work fine with derive_more (the special case is only for single element tuples) but named fields give a bit of info and specific docs to their fields (remember they are all public).

It just makes things easier to read from the docs, and avoids confusing error.0 lines (what's 0?).

[aborgna-q]

Do we need to include other nodes here too?

From the trait definition:

A set of nodes referenced by the rewrite. Modifying any of these nodes will invalidate it.
Two impl Rewrites can be composed if their invalidation sets are disjoint.

Changing a descendant does not invalidate this rewrite, but applying the patch would invalidate any rewrite that references the tailLoop output node. I think all other nodes are safe to modify.

[acl-cqc]

Yes, excellent spot, thanks @aborgna-q !

However, fixing this turned out to be pretty painful. Neither invalidation_set nor new previously took the Hugr (the output node had not been available until verify/apply). So I added that to new. Now it seems that new needs to fail if the op isn't a Tailloop (or at least, not a dataflow container with an Output node - ok to panic if invalid, but not if we've pointed the rewrite at say a leaf op). So try_new.

Then apply really has no error to report (unless the Hugr has changed since the rewrite was constructed - I mean, that could be a panic). For the time being I have kept the error struct, but this is a bit of a PITA; to ease use (clarify errors that should be handled), I am tempted to remove the PeelTailLoopError::NotTailLoop variant. This leaves PeelTailLoop empty - well it is non-exhaustive, but it being non-exhaustive means you can't match it away, so I'm then further tempted to change the error type to just Infallible so we can use something like unwrap_infallible...

What do you think?

[aborgna-q]

Ugh, that's quite annoying.

I think try_new should return the error rather than a random optype, as it gives context about why it failed.

Setting the patch error to Infallible seems right. Rather than adding a new dependency you can just do an infallible pattern match.

[acl-cqc]

Yah....so I've done that. ("Never" gets closer? cool.) Seems a bit off for try_new to return a non_exhaustive error but there.

I think this is still not great tho. The right thing to do is probably to make invalidation_set take the Hugr as argument. Or for this rewrite/PR, we could require the output node to be passed into new (and then error in verify if it isn't the output node)...I mean obviously not the great user experience we'd like but it might do until we can get through deprecation/change to invalidation_set ?

[aborgna-q]

Adding the hugr parameter to invalidation_set seems reasonable.

We should do a non-breaking bootstrap though, something like

    #[deprecated(since = "0.20.2", note = "Implement `invalidation_nodes` instead")]
    fn invalidation_set(&self) -> impl Iterator<Item = Self::Node> { }

    /// ...
    fn invalidation_nodes(
        &self,
        h: &impl HugrView<Node = Self::Node>,
    ) -> impl Iterator<Item = Self::Node> {
        let _ = h;
        #[expect(deprecated)]
        self.invalidation_set()
    }

add to the docs of invalidation_set that it will no longer be called directly so new traits should implement the new method instead and ignore the former.