From a66c4b9fafc44f606f0a7abd1d5e6497563e6ea2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agust=C3=ADn=20Borgna?= <121866228+aborgna-q@users.noreply.github.com> Date: Thu, 17 Oct 2024 10:51:41 +0100 Subject: [PATCH] fix: Allocate ports on root nodes (#1585) Closes #1584 Not having ports in the root nodes caused problems when querying information about them, as shown in #1584. This PR changes the behaviour to instead allocate the ports, and validate that they are always disconnected. This should help make the API more homogeneous. --- hugr-core/src/hugr.rs | 4 +- hugr-core/src/hugr/validate.rs | 48 ++++++++------- hugr-core/src/hugr/validate/test.rs | 58 +++++++++++-------- hugr-core/src/hugr/views.rs | 3 +- ...ugr_core__hugr__views__tests__dot_cfg.snap | 3 +- ...ugr_core__hugr__views__tests__dot_dfg.snap | 4 +- ...re__hugr__views__tests__dot_empty_dfg.snap | 2 +- 7 files changed, 66 insertions(+), 56 deletions(-) diff --git a/hugr-core/src/hugr.rs b/hugr-core/src/hugr.rs index 2e21a2358..ed0fcca0a 100644 --- a/hugr-core/src/hugr.rs +++ b/hugr-core/src/hugr.rs @@ -176,9 +176,7 @@ impl Hugr { let mut graph = MultiPortGraph::with_capacity(nodes, ports); let hierarchy = Hierarchy::new(); let mut op_types = UnmanagedDenseMap::with_capacity(nodes); - let root = graph.add_node(0, 0); - // TODO: These extensions should be open in principle, but lets wait - // until extensions can be inferred for open sets until changing this + let root = graph.add_node(root_node.input_count(), root_node.output_count()); op_types[root] = root_node; Self { diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index ce4ae43a2..faf3e51af 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -166,11 +166,25 @@ impl<'a, 'b> ValidationContext<'a, 'b> { ))?; } // The Hugr can have only one root node. + + for dir in Direction::BOTH { + // Check that we have the correct amount of ports and edges. + let num_ports = self.hugr.graph.num_ports(node.pg_index(), dir); + if num_ports != op_type.port_count(dir) { + return Err(ValidationError::WrongNumberOfPorts { + node, + optype: op_type.clone(), + actual: num_ports, + expected: op_type.port_count(dir), + dir, + }); + } + } + if node == self.hugr.root() { - // The root node has no edges. - if self.hugr.graph.num_outputs(node.pg_index()) - + self.hugr.graph.num_inputs(node.pg_index()) - != 0 + // The root node cannot have connected edges + if self.hugr.all_linked_inputs(node).next().is_some() + || self.hugr.all_linked_outputs(node).next().is_some() { return Err(ValidationError::RootWithEdges { node }); } @@ -190,20 +204,6 @@ impl<'a, 'b> ValidationContext<'a, 'b> { allowed_children, }); } - - for dir in Direction::BOTH { - // Check that we have the correct amount of ports and edges. - let num_ports = self.hugr.graph.num_ports(node.pg_index(), dir); - if num_ports != op_type.port_count(dir) { - return Err(ValidationError::WrongNumberOfPorts { - node, - optype: op_type.clone(), - actual: num_ports, - expected: op_type.port_count(dir), - dir, - }); - } - } } // Thirdly that the node has correct children @@ -602,10 +602,14 @@ impl<'a, 'b> ValidationContext<'a, 'b> { } // Check port connections. - for dir in Direction::BOTH { - for (i, port_index) in self.hugr.graph.ports(node.pg_index(), dir).enumerate() { - let port = Port::new(dir, i); - self.validate_port(node, port, port_index, op_type, var_decls)?; + // + // Root nodes are ignored, as they cannot have connected edges. + if node != self.hugr.root() { + for dir in Direction::BOTH { + for (i, port_index) in self.hugr.graph.ports(node.pg_index(), dir).enumerate() { + let port = Port::new(dir, i); + self.validate_port(node, port, port_index, op_type, var_decls)?; + } } } diff --git a/hugr-core/src/hugr/validate/test.rs b/hugr-core/src/hugr/validate/test.rs index 05c60fb1b..ccfe1b7d0 100644 --- a/hugr-core/src/hugr/validate/test.rs +++ b/hugr-core/src/hugr/validate/test.rs @@ -25,7 +25,9 @@ use crate::types::{ CustomType, FuncValueType, PolyFuncType, PolyFuncTypeRV, Signature, Type, TypeBound, TypeRV, TypeRow, }; -use crate::{const_extension_ids, test_file, type_row, Direction, IncomingPort, Node}; +use crate::{ + const_extension_ids, test_file, type_row, Direction, IncomingPort, Node, OutgoingPort, +}; const NAT: Type = crate::extension::prelude::USIZE_T; @@ -68,36 +70,44 @@ fn add_df_children(b: &mut Hugr, parent: Node, copies: usize) -> (Node, Node, No #[test] fn invalid_root() { - let declare_op: OpType = ops::FuncDecl { - name: "main".into(), - signature: Default::default(), - } - .into(); - - let mut b = Hugr::default(); + let mut b = Hugr::new(LogicOp::Not); let root = b.root(); - assert_eq!(b.validate(&EMPTY_REG), Ok(())); + assert_eq!(b.validate(&PRELUDE_REGISTRY), Ok(())); + + // Change the number of ports in the root + b.set_num_ports(root, 1, 0); + assert_matches!( + b.validate(&PRELUDE_REGISTRY), + Err(ValidationError::WrongNumberOfPorts { node, .. }) => assert_eq!(node, root) + ); + b.set_num_ports(root, 2, 2); + + // Connect it to itself + b.connect(root, 0, root, 0); + assert_matches!( + b.validate(&PRELUDE_REGISTRY), + Err(ValidationError::RootWithEdges { node, .. }) => assert_eq!(node, root) + ); + b.disconnect(root, OutgoingPort::from(0)); // Add another hierarchy root - let other = b.add_node(ops::Module::new().into()); + let module = b.add_node(ops::Module::new().into()); assert_matches!( - b.validate(&EMPTY_REG), - Err(ValidationError::NoParent { node }) => assert_eq!(node, other) + b.validate(&PRELUDE_REGISTRY), + Err(ValidationError::NoParent { node }) => assert_eq!(node, module) ); - b.set_parent(other, root); - b.replace_op(other, declare_op).unwrap(); - b.add_ports(other, Direction::Outgoing, 1); - assert_eq!(b.validate(&EMPTY_REG), Ok(())); // Make the hugr root not a hierarchy root - { - let mut hugr = b.clone(); - hugr.root = other.pg_index(); - assert_matches!( - hugr.validate(&EMPTY_REG), - Err(ValidationError::RootNotRoot { node }) => assert_eq!(node, other) - ); - } + b.set_parent(root, module); + assert_matches!( + b.validate(&PRELUDE_REGISTRY), + Err(ValidationError::RootNotRoot { node }) => assert_eq!(node, root) + ); + + // Fix the root + b.root = module.pg_index(); + b.remove_node(root); + assert_eq!(b.validate(&PRELUDE_REGISTRY), Ok(())); } #[test] diff --git a/hugr-core/src/hugr/views.rs b/hugr-core/src/hugr/views.rs index 6a52f33f0..5779070fa 100644 --- a/hugr-core/src/hugr/views.rs +++ b/hugr-core/src/hugr/views.rs @@ -24,7 +24,7 @@ use itertools::{Itertools, MapInto}; use portgraph::render::{DotFormat, MermaidFormat}; use portgraph::{multiportgraph, LinkView, PortView}; -use super::internal::{HugrInternals, HugrMutInternals}; +use super::internal::HugrInternals; use super::{ Hugr, HugrError, HugrMut, NodeMetadata, NodeMetadataMap, ValidationError, DEFAULT_OPTYPE, }; @@ -509,7 +509,6 @@ pub trait ExtractHugr: HugrView + Sized { let old_root = hugr.root(); let new_root = hugr.insert_from_view(old_root, &self).new_root; hugr.set_root(new_root); - hugr.set_num_ports(new_root, 0, 0); hugr.remove_node(old_root); hugr } diff --git a/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_cfg.snap b/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_cfg.snap index e81a393d8..7f7d769be 100644 --- a/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_cfg.snap +++ b/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_cfg.snap @@ -3,7 +3,7 @@ source: hugr-core/src/hugr/views/tests.rs expression: h.dot_string() --- digraph { -0 [shape=plain label=<
(0) CFG
>] +0 [shape=plain label=<
0: usize
(0) CFG
0: usize
>] 1 [shape=plain label=<
0
(1) ExitBlock
>] 2 [shape=plain label=<
0
(2) DataflowBlock
01
>] 2:out0 -> 6:in0 [style="dashed"] @@ -44,4 +44,3 @@ hier8 [shape=plain label="8"] hier9 [shape=plain label="9"] hier10 [shape=plain label="10"] } - diff --git a/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_dfg.snap b/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_dfg.snap index a77760fe3..2acf952eb 100644 --- a/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_dfg.snap +++ b/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_dfg.snap @@ -1,9 +1,9 @@ --- -source: hugr-core/views/tests.rs +source: hugr-core/src/hugr/views/tests.rs expression: h.dot_string() --- digraph { -0 [shape=plain label=<
(0) DFG
>] +0 [shape=plain label=<
0: qubit1: qubit
(0) DFG
0: qubit1: qubit
>] 1 [shape=plain label=<
(1) Input
0: qubit1: qubit
>] 1:out0 -> 3:in0 [style=""] 1:out1 -> 3:in1 [style=""] diff --git a/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_empty_dfg.snap b/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_empty_dfg.snap index fe82c83fe..0bae7e511 100644 --- a/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_empty_dfg.snap +++ b/hugr-core/src/hugr/views/snapshots/hugr_core__hugr__views__tests__dot_empty_dfg.snap @@ -3,7 +3,7 @@ source: hugr-core/src/hugr/views/tests.rs expression: h.dot_string() --- digraph { -0 [shape=plain label=<
(0) DFG
>] +0 [shape=plain label=<
0: []+[]
(0) DFG
0: []+[]
>] 1 [shape=plain label=<
(1) Input
0: []+[]
>] 1:out0 -> 2:in0 [style=""] 2 [shape=plain label=<
0: []+[]
(2) Output
>]