Add RootChecked to guarantee root node type

[acl-cqc]

• Factor out RootTagged as a subtrait of HugrView to allow separate implementation. Most things can still just use HugrView (as it happens everything that implements HugrView also implements RootTagged, but this is not required).
  • HugrMut a subtrait of RootTagged not just HugrView
• Then add a new RootChecked struct storing any AsRef<Hugr> with a fixed RootHandle.
  • It allows .as_ref(), throwing away the extra information in the RootHandle
  • If the underlying view is a HugrMut then it is too
  • Use trait-default implementations of Hugr(View,(Mut)Internals), as the latter check that replace_op does not break the bound on root-type
  • But do not provide .as_mut() as that would allow bypassing and invalidating the extra info in the RootHandle
  • Like SiblingMut, check that nested instances only narrow the bound.
• Change the overridden-for-&(mut) Hugr impls of Hugr(Mut)Internals to only work for RootHandle=Node to ensure the lack of checking in replace_op there is safe.

[ss2165]

Seems fine to me, but I would suggest waiting for Agustin's review too

[aborgna-q]

I agree with doing root-checked references, but from knowing how the rest of the codebase went we will also need a mutable versions at some point.

What do you think about having a

pub struct RootTagged<H, Root>(H, PhantomData<Root>);

impl<H: AsRef<Hugr>, Root: NodeHandle> RootTagged<H, Root> { ... }

instead? (and derive AsMut too).

[acl-cqc]

I agree with doing root-checked references, but from knowing how the rest of the codebase went we will also need a mutable versions at some point.

What do you think about having....

impl<H: AsRef<Hugr>, Root: NodeHandle> RootTagged<H, Root> { ... }

instead? (and derive AsMut too).

Not sure I understand the lifetime stuff well enough, but don't we need the explicit 'a lifetime to make this useful? Hmmm, I'll have a play and see what I find out.

AsMut (or HugrMut for the WholeHugrView as it stands) both seem reasonable, yes can do (but will try to figure out the AsRef-vs-'a before I do that)

[acl-cqc]

don't we need the explicit 'a lifetime to make this useful?

Hmmm, probably not. The likely use case here is when you have an (&)Hugr and want to pass it to something that takes an &impl HugrView<NodeHandle=...>, see for example #247. Returning a WholeHugrView with a particular lifetime specified in the type is...possible, but likely not the main use case.

So yeah, maybe we don't need the lifetime after all....but, that said, what's the advantage of using AsRef<Hugr>? You gain the ability to store a whole Hugr in a WholeHugrView, if you want to. In general I doubt that's the main use case either, but again, probably has uses in other/different corner cases...

[acl-cqc]

I agree with doing root-checked references, but from knowing how the rest of the codebase went we will also need a mutable versions at some point.

Yup, now done :).

What do you think about having a

pub struct RootTagged<H, Root>(H, PhantomData<Root>);

Ok, I persuade myself that the use case of returning a RootTagged<Hugr, DfgId> (owning the Hugr) is useful - otherwise you would need to return a Hugr and a WholeHugrView of that same thing, which is generally impossible. So, good call.

That said, it's a shame to see try_from has to go, and it's a bit of a pain having to do RootTagged<Hugr, XXXId> when you want to specify just the XXX. But the only way around that that I can see is something like:

struct RootTagFactory<H>(PhantomData<H>);
impl<H: AsRef<Hugr>> RootTagFactory<H> {
  fn try_new<Root: NodeHandle>(h: H) -> Result<RootTagged<H, Root>> {....}
}

so you can then do RootTagFactory::try_new::<CfgID>(h). (I think? Right?)....I am not sure that's worth it.

[acl-cqc]

Hmmm, these were a bit drive-by, but they make searching for these quasi-blanket impls much easier. (A one-line search hit impl<T> HugrMut for T is really not very informative!)

[acl-cqc]

CI failing on existing code due to updates in clippy. Let's fix those in another PR.

[acl-cqc]

Note this contrary to your initial idea @aborgna-q. If you try to impl AsMut, you'll have to remove the impl of HugrMutInternals (and HugrMut) here, and then the test will fail. The test may be enough to justify removing this comment, possibly.

[aborgna-q]

Uhm, this is an easy pit to fall in.

Since the auto-impl for HugrMutInternals requires RootTagged, why not check the root type before dereferencing with as_mut there?
The generic constraints in impl<T: RootTagged + AsMut<Hugr>> HugrMutInts for T kind of tell you that you're adding the extra check on top of the reference.

[acl-cqc]

Hmmm, the answer might be - premature optimization, i.e. that the impl HugrMutInternals for T: AsMut<Hugr> then gets to skip the check....so ok, let's not prematurely optimize, SGTM ;)

Note there was an unexpected subtle consequence that I had to rule out in 07e4f10

[acl-cqc]

(and this does let anyone call .as_mut().replace_op(...) and bypass the check, which I'm not very keen on....)

[acl-cqc]

So overall, I have done as you say, but I'm not sure why this is better, TBH

[acl-cqc]

See 2be330d, which HEAD reverts - I actually think I prefer the former; it needs an extra impl HugrMut (but, empty!) so that seems minimally harmful to me

[aborgna-q]

Uhm, this is an easy pit to fall in.

Since the auto-impl for HugrMutInternals requires RootTagged, why not check the root type before dereferencing with as_mut there?
The generic constraints in impl<T: RootTagged + AsMut<Hugr>> HugrMutInts for T kind of tell you that you're adding the extra check on top of the reference.

[aborgna-q]

👍

[acl-cqc]

I'm now allowing making one RootChecked on top of another, or indeed on top of a SiblingMut, so long as the root-type bound is only narrowed, rather than only on top of something whose RootHandle was Any, as before. And hiding .as_mut() (which would allow breaking the bound) by using the trait-default HugrMut methods rather than the versions overridden for operating directly on a Hugr.