feat!: Disallow nonlocal value edges into FuncDefn's (#1061)

Closes #936 

Test for `ext` edges only in this PR. (Would be good to have one for
`dom` edges too but that can follow.)

BREAKING CHANGE: Hugr's with nonlocal ("Inter-Graph") edges that enter
FuncDefns will now fail validation

hugr/src/hugr/validate.rs

@@ -491,12 +491,28 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
         //
         // This search could be sped-up with a pre-computed LCA structure, but
         // for valid Hugrs this search should be very short.
+        //
+        // For Value edges only, we record any FuncDefn we went through; if there is
+        // any such, then that is an error, but we report that only if the dom/ext
+        // relation was otherwise ok (an error about an edge "entering" some ancestor
+        // node could be misleading if the source isn't where it's expected)
+        let mut err_entered_func = None;
         let from_parent_parent = self.hugr.get_parent(from_parent);
         for (ancestor, ancestor_parent) in
             iter::successors(to_parent, |&p| self.hugr.get_parent(p)).tuple_windows()
         {
+            if !is_static && self.hugr.get_optype(ancestor).is_func_defn() {
+                err_entered_func.get_or_insert(InterGraphEdgeError::ValueEdgeIntoFunc {
+                    to,
+                    to_offset,
+                    from,
+                    from_offset,
+                    func: ancestor,
+                });
+            }
             if ancestor_parent == from_parent {
                 // External edge.
+                err_entered_func.map_or(Ok(()), Err)?;
                 if !is_static {
                     // Must have an order edge.
                     self.hugr
@@ -527,7 +543,7 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
                         ancestor_parent_op: ancestor_parent_op.clone(),
                     });
                 }
-
+                err_entered_func.map_or(Ok(()), Err)?;
                 // Check domination
                 let dominator_tree = match self.dominators.get(&ancestor_parent) {
                     Some(tree) => tree,
@@ -771,6 +787,15 @@ pub enum InterGraphEdgeError {
         to_offset: Port,
         ty: EdgeKind,
     },
+    /// Inter-Graph edges may not enter into FuncDefns unless they are static
+    #[error("Inter-graph Value edges cannot enter into FuncDefns. Inter-graph edge from {from:?} ({from_offset:?}) to {to:?} ({to_offset:?} enters FuncDefn {func:?}")]
+    ValueEdgeIntoFunc {
+        from: Node,
+        from_offset: Port,
+        to: Node,
+        to_offset: Port,
+        func: Node,
+    },
     /// The grandparent of a dominator inter-graph edge must be a CFG container.
     #[error("The grandparent of a dominator inter-graph edge must be a CFG container. Found operation {ancestor_parent_op:?}. In a dominator inter-graph edge from {from:?} ({from_offset:?}) to {to:?} ({to_offset:?}).")]
     NonCFGAncestor {

hugr/src/hugr/validate/test.rs

@@ -258,6 +258,39 @@ fn test_ext_edge() {
     h.update_validate(&EMPTY_REG).unwrap();
 }
 
+#[test]
+fn no_ext_edge_into_func() -> Result<(), Box<dyn std::error::Error>> {
+    let b2b = FunctionType::new_endo(BOOL_T);
+    let mut h = DFGBuilder::new(FunctionType::new(BOOL_T, Type::new_function(b2b.clone())))?;
+    let [input] = h.input_wires_arr();
+
+    let mut dfg = h.dfg_builder(
+        FunctionType::new(vec![], Type::new_function(b2b.clone())),
+        None,
+        [],
+    )?;
+    let mut func = dfg.define_function("AndWithOuter", b2b.clone().into())?;
+    let [fn_input] = func.input_wires_arr();
+    let and_op = func.add_dataflow_op(and_op(), [fn_input, input])?; // 'ext' edge
+    let func = func.finish_with_outputs(and_op.outputs())?;
+    let loadfn = dfg.load_func(func.handle(), &[], &EMPTY_REG)?;
+    let dfg = dfg.finish_with_outputs([loadfn])?;
+    let res = h.finish_hugr_with_outputs(dfg.outputs(), &EMPTY_REG);
+    assert_eq!(
+        res,
+        Err(BuildError::InvalidHUGR(
+            ValidationError::InterGraphEdgeError(InterGraphEdgeError::ValueEdgeIntoFunc {
+                from: input.node(),
+                from_offset: input.source().into(),
+                to: and_op.node(),
+                to_offset: IncomingPort::from(1).into(),
+                func: func.node()
+            })
+        ))
+    );
+    Ok(())
+}
+
 #[test]
 fn test_local_const() {
     let mut h = closed_dfg_root_hugr(FunctionType::new(type_row![BOOL_T], type_row![BOOL_T]));

specification/hugr.md

@@ -554,8 +554,11 @@ parent(n<sub>2</sub>) when the edge's locality is:
 Each of these localities have additional constraints as follows:
 
 1. For Ext edges, we require parent(n<sub>1</sub>) ==
-   parent<sup>i</sup>(n<sub>2</sub>) for some i\>1, *and* for Value edges only there must be a order edge from n<sub>1</sub> to
-   parent<sup>i-1</sup>(n<sub>2</sub>).
+   parent<sup>i</sup>(n<sub>2</sub>) for some i\>1, *and* for Value edges only:
+     * there must be a order edge from n<sub>1</sub> to
+       parent<sup>i-1</sup>(n<sub>2</sub>).
+     * None of the parent<sup>j</sup>(n<sub>2</sub>), for i\>j\>=1,
+       may be a FuncDefn node
 
    The order edge records the
    ordering requirement that results, i.e. it must be possible to
@@ -568,6 +571,9 @@ Each of these localities have additional constraints as follows:
    For Static edges this order edge is not required since the source is
    guaranteed to causally precede the target.
 
+   The FuncDefn restriction means that FuncDefn really are static,
+   and do not capture runtime values from their environment.
+
 2. For Dom edges, we must have that parent<sup>2</sup>(n<sub>1</sub>)
    == parent<sup>i</sup>(n<sub>2</sub>) is a CFG-node, for some i\>1,
    **and** parent(n<sub>1</sub>) strictly dominates
@@ -576,6 +582,8 @@ Each of these localities have additional constraints as follows:
    i\>1 allows the node to target an arbitrarily-deep descendant of the
    dominated block, similar to an Ext edge.)
 
+   The same FuncDefn restriction also applies here, on the parent(<sup>j</sup>)(n<sub>2</sub>) for i\>j\>=1 (of course j=i is the CFG and j=i-1 is the basic block).
+
 Specifically, these rules allow for edges where in a given execution of
 the HUGR the source of the edge executes once, but the target may
 execute \>=0 times.