feat: Split methods between `HugrMut` and `HugrMutInternals` #441

aborgna-q · 2023-08-22T15:00:26Z

Splits the mutable methods between HugrMutInternals & HugrMut.

When implemented by a (future) non-AsMut<Hugr>, the default impl checks whether the related nodes are valid in the view before calling the internal hugrs mutable primitives.

Other small changes:

Moved canonicalize_nodes from HugrMut to Hugr, since it doesn't make sense to call in on a view.
Added a root parameter to Hugr::canonical_order so we can get the canonical order for regions.
Added an assert to validate the root type when querying a hugrview
~~Add a lifetime to HugrInternals::Portgraph so the views can return clones instead of double references.~~
I had to roll it back due to the convex check requiring Copy on the portgraph view.

- split methods between HugrMutInternals & HugrMut - Add a hierarchy root to `Hugr::canonical_order` - check the root type when querying a hugrview - Add a lifetime to `HugrInternals::Portgraph` so the views can return clones instead of double references.

It clashes with the new convexCheck in portgraph since it requires `Copy` on the LinkView :/

co-authored-by: Seyon Sivarajah <seyon.sivarajah@quantinuum.com>

aborgna-q · 2023-08-22T15:27:57Z

I'll re-add the rolled-back lifetime parameters in HugrViewInternals::Portgraph once the fix is released on portgraph.
CQCL/portgraph#104

src/hugr.rs

src/hugr/hugrmut.rs

acl-cqc · 2023-08-23T09:45:24Z

src/hugr/views.rs

-    fn root(&self) -> Node;
+    /// Return the root node of this view.
+    #[inline]
+    fn root(&self) -> Node {


This is just a way of exposing the hidden-in-HugrInternals root_node, right?
(Might one call that root too and then do (self as HugrInternals).root() here?)

Yeah, I don't seriously suggest that, think I've got it now

The different name is to avoid ambiguity at local call sites. Users shouldn't see the HugrMutInternals, so there shouldn't be any confusion.

acl-cqc · 2023-08-23T09:50:41Z

src/hugr.rs

@@ -340,6 +384,12 @@ pub enum HugrError {
    /// An error occurred while manipulating the hierarchy.
    #[error("An error occurred while manipulating the hierarchy.")]
    HierarchyError(#[from] portgraph::hierarchy::AttachError),
+    /// The node doesn't exist.


I wonder about the utility of naming the single field within, but not too worried

I prefer to use named fields when the order is not relevant (what does x.0 mean?). But I have no strong opinion on this.

If there are multiple fields of the same type (e.g. expected and actual) then very much agree that named fields is the way to go. But here I don't think the name adds anything beyond what the type says already, and I really can't think of much else (of any type) you could put in an InvalidNode... (a validity-reason, perhaps. So ok if you prefer it this way!)

I changed it into a tuple struct for consistency with the other errors.

src/hugr/views/hierarchy.rs

acl-cqc · 2023-08-23T09:53:42Z

I'd also mention in the description - remove add_node and add_op (keeping only the variants thereof that specify a position in the hierarchy) - I think that's quite significant

src/hugr/hugrmut.rs

acl-cqc

Nice!! :)

acl-cqc

Nice! Like it, and think it does what I'm after, thanks @aborgna-q :-)

acl-cqc · 2023-08-26T19:59:45Z

src/hugr/hugrmut.rs

+        }
+
+        /// Add a node to the graph.
+        fn add_node(&mut self, nodetype: NodeType) -> Node {


This is the only implementation of add_node, I wonder if it really belongs here or might be better as a method on Hugr itself?

I thought for a moment that the same might be true for all of these HugrMutInternals methods, but no - add_node and add_op are the only ones that don't do (or aren't part of) a valid_xxx check, so I think that's a reasonable argument that they belong somewhere else.

acl-cqc · 2023-08-26T20:01:56Z

src/hugr/hugrmut.rs

-            *self.get_metadata_mut(node) = metadata;
+        /// Returns a [`HugrError::InvalidNode`] otherwise.
+        #[inline]
+        fn valid_node(&self, node: Node) -> Result<(), HugrError> {


Agreed with the implementation; the naming here is rather tricky. I don't really have a better suggestion (I thought about valid_parent instead of valid_node and valid_sibling instead of valid_descendant, and I think this is better in some places - but worse in others, so not really any better overall). So ok!

Hmmm, how about valid_non_root rather than valid_descendant? These mean pretty much the same thing, but descendant might be confusing in a flat view that has only children and nothing deeper? (And valid_non_root also describes quite well what the implementation actually checks...)

acl-cqc · 2023-08-26T20:46:57Z

src/hugr.rs

@@ -340,6 +384,12 @@ pub enum HugrError {
    /// An error occurred while manipulating the hierarchy.
    #[error("An error occurred while manipulating the hierarchy.")]
    HierarchyError(#[from] portgraph::hierarchy::AttachError),
+    /// The node doesn't exist.


If there are multiple fields of the same type (e.g. expected and actual) then very much agree that named fields is the way to go. But here I don't think the name adds anything beyond what the type says already, and I really can't think of much else (of any type) you could put in an InvalidNode... (a validity-reason, perhaps. So ok if you prefer it this way!)

## 🤖 New release * `quantinuum-hugr`: 0.1.0 <details><summary>Changelog</summary> <blockquote> ## 0.1.0 (2024-01-15) ### Bug Fixes - Subgraph boundaries with copies ([#440](#440)) - [**breaking**] Use internal tag for SumType enum serialisation ([#462](#462)) - Check kind before unwrap in insert_identity ([#475](#475)) - Allow for variables to get solved in inference ([#478](#478)) - In IdentityInsertion add noop to correct parent ([#477](#477)) - Failing release tests ([#485](#485)) - [**breaking**] Serialise `Value`, `PrimValue`, and `TypeArg` with internal tags ([#496](#496)) - Serialise custom constants with internal tag ([#502](#502)) - [**breaking**] Reduce max int width in arithmetic extension to 64 ([#504](#504)) - HugrView::get_function_type ([#507](#507)) - TODO in resolve_extension_ops: copy across input_extensions ([#510](#510)) - Use given input extensions in `define_function` ([#524](#524)) - Lessen requirements for hugrs in outline_cfg ([#528](#528)) - Make unification logic less strict ([#538](#538)) - Simple replace incorrectly copying metadata ([#545](#545)) - Account for self-referencial constraints ([#551](#551)) - Consider shunted metas when comparing equality ([#555](#555)) - Join labels in issue workflow ([#563](#563)) - Comment out broken priority code ([#562](#562)) - Handling of issues with no priority label ([#573](#573)) - Don't insert temporary wires when extracting a subgraph ([#582](#582)) - Improve convexity checking and fix test ([#585](#585)) - Ignore input->output links in SiblingSubgraph::try_new_dataflow_subgraph ([#589](#589)) - Enforce covariance of SiblingMut::RootHandle ([#594](#594)) - Erratic stack overflow in infer.rs (live_var) ([#638](#638)) - Work harder in variable instantiation ([#591](#591)) - Actually add the error type to prelude ([#672](#672)) - Serialise dynamically computed opaqueOp signatures ([#690](#690)) - FuncDefns don't require that their extensions match their children ([#688](#688)) - Binary compute_signature returning a PolyFuncType with binders ([#710](#710)) - Use correct number of args for int ops ([#723](#723)) - [**breaking**] Add serde tag to TypeParam enum ([#722](#722)) - Allow widening and narrowing to same width. ([#735](#735)) - Case node should not have an external signature ([#749](#749)) - [**breaking**] Normalize input/output value/static/other ports in `OpType` ([#783](#783)) - No dataflow_signature for block types ([#792](#792)) - Ignore unsupported test in miri ([#794](#794)) - Include schema rather than read file ([#807](#807)) ### Documentation - Add operation constraint to quantum extension ([#543](#543)) - Coverage check section in DEVELOPMENT.md ([#648](#648)) - Remove "quantum extension" from HUGR spec. ([#694](#694)) - Improve crate-level docs, including example code. ([#698](#698)) - Spec cleanups and clarifications ([#742](#742)) - Spec clarifications ([#738](#738)) - Spec updates ([#741](#741)) - [spec] Remove references to causal cone and Order edges from Input ([#762](#762)) - Mention experimental inference in readme ([#800](#800)) - Collection of spec updates for 0.1 ([#801](#801)) - Add schema v0 ([#805](#805)) - Update spec wrt. polymorphism ([#791](#791)) ### Features - Derive things for builder structs ([#229](#229)) - Return a slice of types from the signature ([#238](#238)) - Move `dot_string` to `HugrView` ([#271](#271)) - [**breaking**] Change `TypeParam::USize` to `TypeParam::BoundedNat` and use in int extensions ([#445](#445)) - TKET2 compatibility requirements ([#450](#450)) - Split methods between `HugrMut` and `HugrMutInternals` ([#441](#441)) - Add `HugrView::node_connections` to get all links between nodes ([#460](#460)) - Location information in extension inference error ([#464](#464)) - Add T, Tdg, X gates ([#466](#466)) - [**breaking**] Add `ApplyResult` associated type to `Rewrite` ([#472](#472)) - Implement rewrite `IdentityInsertion` ([#474](#474)) - Instantiate inferred extensions ([#461](#461)) - Default DFG builders to open extension sets ([#473](#473)) - Some helper methods ([#482](#482)) - Extension inference for conditional nodes ([#465](#465)) - Add ConvexChecker ([#487](#487)) - Add clippy lint for mut calls in a debug_assert ([#488](#488)) - Default more builder methods to open extension sets ([#492](#492)) - Port is serializable ([#489](#489)) - More general portgraph references ([#494](#494)) - Add extension deltas to CFG ops ([#503](#503)) - Implement petgraph traits on Hugr ([#498](#498)) - Make extension delta a parameter of CFG builders ([#514](#514)) - [**breaking**] SiblingSubgraph does not borrow Hugr ([#515](#515)) - Validate TypeArgs to ExtensionOp ([#509](#509)) - Derive Debug & Clone for `ExtensionRegistry`. ([#530](#530)) - Const nodes are built with some extension requirements ([#527](#527)) - Some python errors and bindings ([#533](#533)) - Insert_hugr/insert_view return node map ([#535](#535)) - Add `SiblingSubgraph::try_from_nodes_with_checker` ([#547](#547)) - PortIndex trait for undirected port parameters ([#553](#553)) - Insert/extract subgraphs from a HugrView ([#552](#552)) - Add `new_array` operation to prelude ([#544](#544)) - Add a `DataflowParentID` node handle ([#559](#559)) - Make TypeEnum and it's contents public ([#558](#558)) - Optional direction check when querying a port index ([#566](#566)) - Extension inference for CFGs ([#529](#529)) - Better subgraph verification errors ([#587](#587)) - Compute affected nodes for `SimpleReplacement` ([#600](#600)) - Move `SimpleReplace::invalidation_set` to the `Rewrite` trait ([#602](#602)) - [**breaking**] Resolve extension ops (mutating Hugr) in (infer_and_->)update_validate ([#603](#603)) - Add accessors for node index and const values ([#605](#605)) - [**breaking**] Expose the value of ConstUsize ([#621](#621)) - Nicer debug and display for core types ([#628](#628)) - [**breaking**] Static checking of Port direction ([#614](#614)) - Builder and HugrMut add_op_xxx default to open extensions ([#622](#622)) - [**breaking**] Add panicking integer division ops ([#625](#625)) - Add hashable `Angle` type to Quantum extension ([#608](#608)) - [**breaking**] Remove "rotations" extension. ([#645](#645)) - Port.as_directed to match on either direction ([#647](#647)) - Impl GraphRef for PetgraphWrapper ([#651](#651)) - Provide+implement Replace API ([#613](#613)) - Require the node's metadata to always be a Map ([#661](#661)) - [**breaking**] Polymorphic function types (inc OpDefs) using dyn trait ([#630](#630)) - Make prelude error type public ([#669](#669)) - Shorthand for retrieving custom constants from `Const`, `Value` ([#679](#679)) - [**breaking**] HugrView API improvements ([#680](#680)) - Make FuncDecl/FuncDefn polymorphic ([#692](#692)) - [**breaking**] Simplify `SignatureFunc` and add custom arg validation. ([#706](#706)) - [**breaking**] Drop the `pyo3` feature ([#717](#717)) - [**breaking**] `OpEnum` trait for common opdef functionality ([#721](#721)) - MakeRegisteredOp trait for easier registration ([#726](#726)) - Getter for `PolyFuncType::body` ([#727](#727)) - `Into<OpType>` for custom ops ([#731](#731)) - Always require a signature in `OpaqueOp` ([#732](#732)) - Values (and hence Consts) know their extensions ([#733](#733)) - [**breaking**] Use ConvexChecker trait ([#740](#740)) - Custom const for ERROR_TYPE ([#756](#756)) - Implement RemoveConst and RemoveConstIgnore ([#757](#757)) - Constant folding implemented for core and float extension ([#769](#769)) - Constant folding for arithmetic conversion operations ([#720](#720)) - DataflowParent trait for getting inner signatures ([#782](#782)) - Constant folding for logic extension ([#793](#793)) - Constant folding for list operations ([#795](#795)) - Add panic op to prelude ([#802](#802)) - Const::from_bool function ([#803](#803)) ### HugrMut - Validate nodes for set_metadata/get_metadata_mut, too ([#531](#531)) ### HugrView - Validate nodes, and remove Base ([#523](#523)) ### Miscellaneous Tasks - [**breaking**] Update portgraph 0.10 and pyo3 0.20 ([#612](#612)) - [**breaking**] Hike MSRV to 1.75 ([#761](#761)) ### Performance - Use lazy static definittion for prelude registry ([#481](#481)) ### Refactor - Move `rewrite` inside `hugr`, `Rewrite` -> `Replace` implementing new 'Rewrite' trait ([#119](#119)) - Use an excluded upper bound instead of max log width. ([#451](#451)) - Add extension info to `Conditional` and `Case` ([#463](#463)) - `ExtensionSolution` only consists of input extensions ([#480](#480)) - Remove builder from more DFG tests ([#490](#490)) - Separate hierarchy views ([#500](#500)) - [**breaking**] Use named struct for float constants ([#505](#505)) - Allow NodeType::new to take any Into<Option<ExtensionSet>> ([#511](#511)) - Move apply_rewrite from Hugr to HugrMut ([#519](#519)) - Use SiblingSubgraph in SimpleReplacement ([#517](#517)) - CFG takes a FunctionType ([#532](#532)) - Remove check_custom_impl by inlining into check_custom ([#604](#604)) - Insert_subgraph just return HashMap, make InsertionResult new_root compulsory ([#609](#609)) - [**breaking**] Rename predicate to TupleSum/UnitSum ([#557](#557)) - Simplify infer.rs/report_mismatch using early return ([#615](#615)) - Move the core types to their own module ([#627](#627)) - Change &NodeType->&OpType conversion into op() accessor ([#623](#623)) - Infer.rs 'fn results' ([#631](#631)) - Be safe ([#637](#637)) - NodeType constructors, adding new_auto ([#635](#635)) - Constraint::Plus stores an ExtensionSet, which is a BTreeSet ([#636](#636)) - [**breaking**] Remove `SignatureDescription` ([#644](#644)) - [**breaking**] Remove add_op_<posn> by generalizing add_node_<posn> with "impl Into" ([#642](#642)) - Rename accidentally-changed Extension::add_node_xxx back to add_op ([#659](#659)) - [**breaking**] Remove quantum extension ([#670](#670)) - Use type schemes in extension definitions wherever possible ([#678](#678)) - [**breaking**] Flatten `Prim(Type/Value)` in to parent enum ([#685](#685)) - [**breaking**] Rename `new_linear()` to `new_endo()`. ([#697](#697)) - Replace NodeType::signature() with io_extensions() ([#700](#700)) - Validate ExtensionRegistry when built, not as we build it ([#701](#701)) - [**breaking**] One way to add_op to extension ([#704](#704)) - Remove Signature struct ([#714](#714)) - Use `MakeOpDef` for int_ops ([#724](#724)) - [**breaking**] Use enum op traits for floats + conversions ([#755](#755)) - Avoid dynamic dispatch for non-folding operations ([#770](#770)) - Simplify removeconstignore verify ([#768](#768)) - [**breaking**] Unwrap BasicBlock enum ([#781](#781)) - Make clear const folding only for leaf ops ([#785](#785)) - [**breaking**] `s/RemoveConstIgnore/RemoveLoadConstant` ([#789](#789)) - Put extension inference behind a feature gate ([#786](#786)) ### SerSimpleType - Use Vec not TypeRow ([#381](#381)) ### SimpleReplace+OutlineCfg - Use HugrMut methods rather than .hierarchy/.graph ([#280](#280)) ### Testing - Update inference test to not use DFG builder ([#550](#550)) - Strengthen "failing_sccs_test", rename to "sccs" as it's not failing! ([#660](#660)) - [**breaking**] Improve coverage in signature and validate ([#643](#643)) - Use insta snapshots to add dot_string coverage ([#682](#682)) - Miri ignore file-opening test ([#684](#684)) - Unify the serialisation tests ([#730](#730)) - Add schema validation to roundtrips ([#806](#806)) ### `ConstValue - :F64` and `OpaqueOp::new` ([#206](#206)) ### Cleanup - Remove outdated comment ([#536](#536)) ### Cosmetic - Format + remove stray TODO ([#444](#444)) ### Doc - Crate name as README title + add reexport ([#199](#199)) ### S/EdgeKind - :Const/EdgeKind::Static/ ([#201](#201)) ### Simple_replace.rs - Use HugrMut::remove_node, includes clearing op_types ([#242](#242)) ### Spec - Remove "Draft 3" from title of spec document. ([#590](#590)) - Rephrase confusing paragraph about TailLoop inputs/outputs ([#567](#567)) ### Src/ops/validate.rs - Common-up some type row calculations ([#254](#254)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

aborgna-q and others added 5 commits August 21, 2023 23:18

feat: misc hugrmut changes

65002d3

- split methods between HugrMutInternals & HugrMut - Add a hierarchy root to `Hugr::canonical_order` - check the root type when querying a hugrview - Add a lifetime to `HugrInternals::Portgraph` so the views can return clones instead of double references.

Merge remote-tracking branch 'origin/main' into feat/hugrmut-split

f06543d

rollback HugrInternals::Portgraph lifetime param

5cbac2e

It clashes with the new convexCheck in portgraph since it requires `Copy` on the LinkView :/

fix doc

2eff599

Simplify the horrible boilerplate

f14907b

co-authored-by: Seyon Sivarajah <seyon.sivarajah@quantinuum.com>

aborgna-q requested a review from ss2165 August 22, 2023 15:00

ss2165 reviewed Aug 22, 2023

View reviewed changes

src/hugr.rs Show resolved Hide resolved

src/hugr/hugrmut.rs Outdated Show resolved Hide resolved

acl-cqc reviewed Aug 23, 2023

View reviewed changes

src/hugr/views/hierarchy.rs Outdated Show resolved Hide resolved

acl-cqc reviewed Aug 23, 2023

View reviewed changes

src/hugr/hugrmut.rs Outdated Show resolved Hide resolved

acl-cqc reviewed Aug 23, 2023

View reviewed changes

aborgna-q added 2 commits August 24, 2023 02:02

Inline init_graphs

6f90316

Disallow modifying the root in some hugrmut methods

87e8d89

aborgna-q requested a review from acl-cqc August 24, 2023 09:33

acl-cqc approved these changes Aug 29, 2023

View reviewed changes

aborgna-q added 5 commits August 29, 2023 10:11

Merge remote-tracking branch 'origin/main' into feat/hugrmut-split

99be6cc

Move add_op and add_node to Hugr

cb242e3

Fix merged imports

2419c75

s/valid_descendant/valid_non_root/

3ba5511

Convert hugr node error to tuple struct

bc2beb6

aborgna-q enabled auto-merge August 29, 2023 09:27

aborgna-q added this pull request to the merge queue Aug 29, 2023

Merged via the queue into main with commit 76402e3 Aug 29, 2023
6 checks passed

aborgna-q deleted the feat/hugrmut-split branch August 29, 2023 09:32

aborgna-q mentioned this pull request Aug 29, 2023

Rewrites work on any HugrMut #282

Merged

github-actions bot mentioned this pull request Jan 3, 2024

chore: release v0.1.0 #766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Split methods between `HugrMut` and `HugrMutInternals` #441

feat: Split methods between `HugrMut` and `HugrMutInternals` #441

aborgna-q commented Aug 22, 2023

aborgna-q commented Aug 22, 2023

acl-cqc Aug 23, 2023

acl-cqc Aug 23, 2023

aborgna-q Aug 24, 2023

acl-cqc Aug 23, 2023 •

edited

Loading

aborgna-q Aug 24, 2023

acl-cqc Aug 26, 2023

aborgna-q Aug 29, 2023

acl-cqc commented Aug 23, 2023

acl-cqc left a comment

acl-cqc left a comment

acl-cqc Aug 26, 2023

acl-cqc Aug 26, 2023

acl-cqc Aug 26, 2023

acl-cqc Aug 26, 2023

acl-cqc Aug 26, 2023

feat: Split methods between HugrMut and HugrMutInternals #441

feat: Split methods between HugrMut and HugrMutInternals #441

Conversation

aborgna-q commented Aug 22, 2023

aborgna-q commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Aug 23, 2023 • 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

acl-cqc commented Aug 23, 2023

acl-cqc left a comment

Choose a reason for hiding this comment

acl-cqc left a 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

feat: Split methods between `HugrMut` and `HugrMutInternals` #441

feat: Split methods between `HugrMut` and `HugrMutInternals` #441

acl-cqc Aug 23, 2023 •

edited

Loading