feat: traits for transforming Types/TypeArgs/etc.

[acl-cqc]

Allows fallible transformation (whereas Substitution is infallible), by implementing new trait TypeTransformer. At present this only allows replacing Extension types (with arbitrary types), but we could easily add new trait methods (with default {Ok(None)} to make no change) in the future.

Trait Transformable is more of an implementation detail, it allows commoning up lots of list traversal, I don't really foresee anyone external implementing it, but it doesn't seem worth going to any effort to stop them if they want to.

The (&mut self, ...) -> Result<bool, ...> interface means you may want to clone first in order to recover from an error, but avoids an extra traversal for change-checking (i.e. it's easier to build an (&self, ...) -> Result<Self, ...> interface on top of this, than the other way around).

[codecov]

Codecov Report

Attention: Patch coverage is 95.40230% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.72%. Comparing base (53b9505) to head (044ff32).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/types.rs	95.52%	1 Missing and 5 partials ⚠️
hugr-core/src/types/signature.rs	92.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1991      +/-   ##
==========================================
+ Coverage   83.65%   83.72%   +0.06%     
==========================================
  Files         209      209              
  Lines       38993    39167     +174     
  Branches    35666    35838     +172     
==========================================
+ Hits        32620    32791     +171     
+ Misses       4533     4530       -3     
- Partials     1840     1846       +6     

Flag	Coverage Δ
python	92.03% <ø> (∅)
rust	82.94% <95.40%> (∅)

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]

@doug-q I promise you were randomly picked, and it has nothing to do with my having mentioned you in the description!

[doug-q]

I like this a lot. I need to do another more careful pass before approving

[doug-q]

Very nice Alan.

I don't prefer &mut self over self. I don't have a strong opinion either way. Ideally we would not clone if we didn't have to, and ideally we would report whether there was any change. I expect that both of these optimisations are in practise irrelevant. If you would prefer the "functional" interface(or both interfaces, this is easy because of the trait-style interface), go for it.

[doug-q]

This case should be tested

[doug-q]

I haven't looked at the other callsites of get_type_def, but I don't like it. I would prefer to inline it here, and just "pub(crate)"-icise get_extension. If you disagree I'm fine with this.

[acl-cqc]

I'd look at it the other way - how about removing get_extension by inlining into get_type_def and "pub(crate)"-icise only get_type_def?

[acl-cqc]

There are three callers of get_type_def and they all call get_extension first with only a slight change in how they handle errors from get_extension (i.e. one caller panics....on errors from either, so that's fine to combine both errors)

[doug-q]

I agree that makes sense, but I do think get_extension is a perfectly reasonable thing to want to do. Up to you.

[acl-cqc]

OIC. The thing is that the caller has to hang onto the result Arc from get_extension for the lifetime of the &TypeDef. Ugh. I guess if Extension::get_type returned an Option<Arc<TypeDef>> rather than an Option<&TypeDef> then this would go away but otherwise we're a bit stuck.

[acl-cqc]

Ok, I don't think the implementation of get_type_def is great either, but it seems mad to make the caller do that work (looking up the CustomType's type-id in the extension). Could change it to an Option rather than Result if that's preferable but the SignatureError is appropriate in at least a couple of cases, so I'm inclined to leave it.

#2001 covers what I think is the way forward but no need to do that in this PR.

[acl-cqc]

I don't prefer &mut self over self. I don't have a strong opinion either way. Ideally we would not clone if we didn't have to, and ideally we would report whether there was any change. I expect that both of these optimisations are in practise irrelevant. If you would prefer the "functional" interface(or both interfaces, this is easy because of the trait-style interface), go for it.

Hmmm, I guess a lot depends on how important it is no "not clone if we didn't have to".

• (self, T) -> Result<Self, T::Error> is no better, in the event of an error you've lost the original
• (self, T) -> Result<Self, (Self, T::Error)> likewise, you've no guarantee that the one you get back is unchanged
• (&self, T) -> Result<Self, T::Error> is the only way you know you've still got the original (/in a consistent state), but clones - unnecessary if you have no intention of recovering from failure

The other advantage is that determining whether anything is changed requires an additional traversal (as we are not sharing with Arcs - otherwise a "pointer-eq" check might be an acceptable safe approximation).

And, the &mut self requires much less code for OpType, where there are 20 things that would need to be recreated after transforming the constituent elements, but implementing this over the top of functional-style substitution would require equality checking everything :-(

So the best alternative seems to be, clone even when not needed, and drop change-checking. Hmmm, boo, ok, maybe I'll drop this objection then...;)