Make HugrMut a trait #132

acl-cqc · 2023-06-07T18:01:30Z

This turned out significantly harder than I'd expected, I'd not realized how much parameterization over hugr/ref/hugrmut there was in the builder. But I think I got there....

Anything returning a (new) HugrMut has to return an actual Hugr, it's the only concrete implementation of HugrMut we have.
Container having both base() and hugr() methods now seems a bit strange. But one is mutable and one is not....so left as is for now (TODO's can be removed if we are happy)?
Other things are variously parameterized over HugrMut and HugrMutRef...this was quite hard to satisfy rustc and I'm not sure I've got all these as consistent as I could have. The acid test will be when we have mutable views onto subregions and want to use HugrMut methods on those, but that's not in this PR!
Using Hugr::default() for a new module-rooted hugr is perhaps slightly tricky, could expose something there. (This was what the old HugrMut was using.)
Also made HugrMut be only pub(crate), but that's a separate commit (1bb79d2)

acl-cqc · 2023-06-07T18:02:27Z

src/builder/build_traits.rs

@@ -641,11 +640,11 @@ fn wire_up<T: Dataflow + ?Sized>(
 /// Return None if Const kind
 /// Panics if port not valid for Op or port is not Const/Value
 fn check_classical_value(
-    base: &HugrMut,
+    base: &impl AsRef<Hugr>,


This could probably also be &impl HugrMut, but AsRef<Hugr> seems more general (? or is it)

At present HugrMut has AsRef<Hugr> as a prereq, so that'd be strictly less general - some AsRef<Hugr> things don't have the HugrMut methods (but probably could have if we wanted). However, in the future I hope that HugrMut will not require being able to retrieve an actual Hugr (even a reference), rather, HugrMut will be able to return an immutable view onto what might be only part of a Hugr. So if we wanted to plan ahead for that, I should change this here to HugrMut, but that will be another PR(-series)...

acl-cqc · 2023-06-08T08:27:37Z

@aborgna-q @ss2165 please co-ordinate who reviews this! :)

aborgna-q

LGTM

I'd centralize the AsRef traits in the main hugr module.
Probably also dropping DerefHugr in HugrView in favour of AsRef<Hugr> so that everything is uniform.

src/hugr/hugrmut.rs

aborgna-q · 2023-06-08T09:08:02Z

src/hugr/hugrmut.rs

+    /// Build the HUGR, returning an error if the graph is not valid.
+    fn finish(self) -> Result<Hugr, ValidationError>;


This shouldn't be in the trait.
We can already dereference; finishing the hugr should be done by whatever builder struct you are using (or if you already have a Hugr just call validate).

Yes, I meant to call this out, sorry - it looked a bit dubious. Removing it entirely has meant that many of the impl<T: HugrMut> Container for SomeBuilder<T> have become impl Container for SomeBuilder<Hugr> which is to say, you can only call .finish_hugr when building a whole Hugr. Given that we are not doing mutable subregions yet, this is good :)

src/builder.rs

aborgna-q · 2023-06-08T09:11:53Z

src/builder.rs

+/// Trait allowing treating type as (im)mutable reference to [`Hugr`]
+pub trait HugrMutRef: AsMut<Hugr> + AsRef<Hugr> {}
+impl<H: HugrMut> HugrMutRef for H {}
+impl HugrMutRef for &mut Hugr {}


Can we implement AsMut and AsRef instead?

At present HugrMutRef is a trait that one implements on top of AsMut and AsRef. AFAICS, the only purpose is to make it easy to parameterize (i.e. over T: HugrMutRef rather than T: AsMut<Hugr> + AsRef<Hugr>).

So, removed via bulk replace. (I have not gone through and checked whether any of them can be just : AsMut<Hugr> or : AsRef<Hugr> rather than + of both.)

This is a bit messy textually even if I think makes sense. (Sadly you can't define type HugrMutRef = AsRef<Hugr> + AsMut<Hugr> - type must really be a type not merely a bound.)

However, have kept in separate commit b48e2f6 in case you think this is too messy...

@aborgna-q can you have a quick look at that b48e2f6 in case you think better of it....

That works!

src/builder/build_traits.rs

acl-cqc · 2023-06-09T12:09:50Z

Somewhere there was a suggestion of dropping DerefHugr (in view.rs) in favour of AsRef - this might be good, but I think is separate to this PR.

ss2165

👍 builder code base much nicer with this

ss2165 · 2023-06-09T12:10:34Z

src/algorithm/nest_cfgs.rs

@@ -624,7 +624,7 @@ pub(crate) mod test {
        Ok((split, merge))
    }

-    fn build_then_else_merge_from_if<T: HugrMutRef>(


do these actually need both Mut and Ref or is the Mut requirement sufficient?

They need it because the impl block is also parameterized like that...as I say I think we could probably go through and replace quite a few AsRef + AsMuts with one or the other, but I've not done that thorough an analysis here; consistency is helpful in itself and we can reconsider later if we really want.

Closes #132 BREAKING CHANGE: `PreludeCodegen::emit_panic` now takes a single configurable value.

acl-cqc added 3 commits June 7, 2023 18:50

Make HugrMut a trait. Ouch

748a0a4

Move add_other_edge impl into trait as default

6e46bc3

Make HugrMut pub(crate) not pub

1bb79d2

acl-cqc commented Jun 7, 2023

View reviewed changes

fix docs

2c945ed

acl-cqc requested review from aborgna-q and ss2165 June 8, 2023 08:27

aborgna-q approved these changes Jun 8, 2023

View reviewed changes

acl-cqc added 6 commits June 9, 2023 12:36

Merge remote-tracking branch 'origin/main' into refactor/trait_hugrmut

e6b7c53

Move AsRef+AsMut from hugrmut.rs to hugr.rs

558f70e

Remove HugrMut::finish; may only .finish_hugr() when building whole Hugr

9e1a007

Rename Container.{base->hugr_mut}()

a82c3ef

Remove HugrMutRef

b48e2f6

Remove TODOs

11fd6b5

ss2165 approved these changes Jun 9, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into refactor/trait_hugrmut

525fde2

acl-cqc merged commit 118c4af into main Jun 9, 2023

acl-cqc deleted the refactor/trait_hugrmut branch June 9, 2023 13:15

aborgna-q mentioned this pull request Jun 12, 2023

Refactor HugrMut #124

Closed

doug-q added a commit that referenced this pull request Oct 21, 2024

feat!: Support tket2.rotation.from_halfturns_unchecked (#133)

9e55d9a

Closes #132 BREAKING CHANGE: `PreludeCodegen::emit_panic` now takes a single configurable value.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make HugrMut a trait #132

Make HugrMut a trait #132

acl-cqc commented Jun 7, 2023 •

edited

Loading

acl-cqc Jun 7, 2023

acl-cqc Jun 8, 2023

acl-cqc commented Jun 8, 2023

aborgna-q left a comment •

edited

Loading

aborgna-q Jun 8, 2023

acl-cqc Jun 9, 2023

aborgna-q Jun 8, 2023

acl-cqc Jun 9, 2023

acl-cqc Jun 9, 2023

aborgna-q Jun 9, 2023

acl-cqc commented Jun 9, 2023

ss2165 left a comment •

edited

Loading

ss2165 Jun 9, 2023

acl-cqc Jun 9, 2023

		/// Build the HUGR, returning an error if the graph is not valid.
		fn finish(self) -> Result<Hugr, ValidationError>;

Make HugrMut a trait #132

Make HugrMut a trait #132

Conversation

acl-cqc commented Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jun 8, 2023

aborgna-q left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jun 9, 2023

ss2165 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jun 7, 2023 •

edited

Loading

aborgna-q left a comment •

edited

Loading

ss2165 left a comment •

edited

Loading