Merge remote-tracking branch 'origin/main' into new/poly_types_map, t…

…idy imports

src/extension/infer.rs

@@ -22,7 +22,7 @@ use super::validate::ExtensionError;
 
 use petgraph::graph as pg;
 
-use std::collections::{HashMap, HashSet};
+use std::collections::{HashMap, HashSet, VecDeque};
 
 use thiserror::Error;
 
@@ -521,57 +521,46 @@ impl UnificationContext {
     pub fn results(&self) -> Result<ExtensionSolution, InferExtensionError> {
         // Check that all of the metavariables associated with nodes of the
         // graph are solved
+        let depended_upon = {
+            let mut h: HashMap<Meta, Vec<Meta>> = HashMap::new();
+            for (m, m2) in self.constraints.iter().flat_map(|(m, cs)| {
+                cs.iter().flat_map(|c| match c {
+                    Constraint::Plus(_, m2) => Some((*m, self.resolve(*m2))),
+                    _ => None,
+                })
+            }) {
+                h.entry(m2).or_default().push(m);
+            }
+            h
+        };
+        // Calculate everything dependent upon a variable.
+        // Note it would be better to find metas ALL of whose dependencies were (transitively)
+        // on variables, but this is more complex, and hard to define if there are cycles
+        // of PLUS constraints, so leaving that as a TODO until we've handled such cycles.
+        let mut depends_on_var = HashSet::new();
+        let mut queue = VecDeque::from_iter(self.variables.iter());
+        while let Some(m) = queue.pop_front() {
+            if depends_on_var.insert(m) {
+                if let Some(d) = depended_upon.get(m) {
+                    queue.extend(d.iter())
+                }
+            }
+        }
+
         let mut results: ExtensionSolution = HashMap::new();
         for (loc, meta) in self.extensions.iter() {
             if let Some(rs) = self.get_solution(meta) {
                 if loc.1 == Direction::Incoming {
                     results.insert(loc.0, rs.clone());
                 }
-            } else if self.live_var(meta).is_some() {
-                // If it depends on some other live meta, that's bad news.
-                return Err(InferExtensionError::Unsolved { location: *loc });
-            }
-            // If it only depends on graph variables, then we don't have
-            // a *solution*, but it's fine
-        }
-        debug_assert!(self.live_metas().is_empty());
-        Ok(results)
-    }
-
-    // Get the live var associated with a meta.
-    // TODO: This should really be a list
-    fn live_var(&self, m: &Meta) -> Option<Meta> {
-        if self.variables.contains(m) || self.variables.contains(&self.resolve(*m)) {
-            return None;
-        }
-
-        // TODO: We should be doing something to ensure that these are the same check...
-        if self.get_solution(m).is_none() {
-            if let Some(cs) = self.get_constraints(m) {
-                for c in cs {
-                    match c {
-                        Constraint::Plus(_, m) => return self.live_var(m),
-                        _ => panic!("we shouldn't be here!"),
-                    }
+            } else {
+                // Unsolved nodes must be unsolved because they depend on graph variables.
+                if !depends_on_var.contains(&self.resolve(*meta)) {
+                    return Err(InferExtensionError::Unsolved { location: *loc });
                 }
             }
-            Some(*m)
-        } else {
-            None
         }
-    }
-
-    /// Return the set of "live" metavariables in the context.
-    /// "Live" here means a metavariable:
-    ///   - Is associated to a location in the graph in `UnifyContext.extensions`
-    ///   - Is still unsolved
-    ///   - Isn't a variable
-    fn live_metas(&self) -> HashSet<Meta> {
-        self.extensions
-            .values()
-            .filter_map(|m| self.live_var(m))
-            .filter(|m| !self.variables.contains(m))
-            .collect()
+        Ok(results)
     }
 
     /// Iterates over a set of metas (the argument) and tries to solve
@@ -665,12 +654,16 @@ mod test {
 
     use super::*;
     use crate::builder::test::closed_dfg_root_hugr;
+    use crate::builder::{DFGBuilder, Dataflow, DataflowHugr};
+    use crate::extension::prelude::QB_T;
     use crate::extension::ExtensionId;
     use crate::extension::{prelude::PRELUDE_REGISTRY, ExtensionSet};
     use crate::hugr::{validate::ValidationError, Hugr, HugrMut, HugrView, NodeType};
     use crate::macros::const_extension_ids;
-    use crate::ops::OpType;
+    use crate::ops::custom::{ExternalOp, OpaqueOp};
     use crate::ops::{self, dataflow::IOTrait, handle::NodeHandle, OpTrait};
+    use crate::ops::{LeafOp, OpType};
+
     use crate::type_row;
     use crate::types::{FunctionType, Type, TypeRow};
 
@@ -1539,4 +1532,53 @@ mod test {
 
         Ok(())
     }
+
+    /// This was stack-overflowing approx 50% of the time,
+    /// see https://github.com/CQCL/hugr/issues/633
+    #[test]
+    fn plus_on_self() -> Result<(), Box<dyn std::error::Error>> {
+        let ext = ExtensionId::new("unknown1").unwrap();
+        let delta = ExtensionSet::singleton(&ext);
+        let ft = FunctionType::new_linear(type_row![QB_T, QB_T]).with_extension_delta(&delta);
+        let mut dfg = DFGBuilder::new(ft.clone())?;
+
+        // While https://github.com/CQCL-DEV/hugr/issues/388 is unsolved,
+        // most operations have empty extension_reqs (not including their own extension).
+        // Define some that do.
+        let binop: LeafOp = ExternalOp::Opaque(OpaqueOp::new(
+            ext.clone(),
+            "2qb_op",
+            String::new(),
+            vec![],
+            Some(ft),
+        ))
+        .into();
+        let unary_sig = FunctionType::new_linear(type_row![QB_T])
+            .with_extension_delta(&ExtensionSet::singleton(&ext));
+        let unop: LeafOp = ExternalOp::Opaque(OpaqueOp::new(
+            ext,
+            "1qb_op",
+            String::new(),
+            vec![],
+            Some(unary_sig),
+        ))
+        .into();
+        // Constrain q1,q2 as PLUS(ext1, inputs):
+        let [q1, q2] = dfg
+            .add_dataflow_op(binop.clone(), dfg.input_wires())?
+            .outputs_arr();
+        // Constrain q1 as PLUS(ext2, q2):
+        let [q1] = dfg.add_dataflow_op(unop, [q1])?.outputs_arr();
+        // Constrain q1 as EQUALS(q2) by using both together
+        dfg.finish_hugr_with_outputs([q1, q2], &PRELUDE_REGISTRY)?;
+        // The combined q1+q2 variable now has two PLUS constraints - on itself and the inputs.
+        Ok(())
+    }
+
+    /// [plus_on_self] had about a 50% rate of failing with stack overflow.
+    /// So if we run 10 times, that would succeed about 1 run in 2^10, i.e. <0.1%
+    #[test]
+    fn plus_on_self_10_times() {
+        [0; 10].iter().for_each(|_| plus_on_self().unwrap())
+    }
 }

src/extension/op_def.rs

@@ -1,27 +1,20 @@
-use crate::Hugr;
 use std::cmp::min;
 use std::collections::hash_map::Entry;
+use std::collections::HashMap;
 use std::fmt::{Debug, Formatter};
 use std::sync::Arc;
 
+use smol_str::SmolStr;
+
 use super::{
     Extension, ExtensionBuildError, ExtensionId, ExtensionRegistry, ExtensionSet, SignatureError,
     TypeParametrised,
 };
 
-use crate::types::{PolyFuncType, SignatureDescription};
-
-use crate::types::FunctionType;
-
-use crate::types::type_param::{check_type_args, TypeArg};
-
 use crate::ops::custom::OpaqueOp;
-
-use std::collections::HashMap;
-
-use crate::types::type_param::TypeParam;
-
-use smol_str::SmolStr;
+use crate::types::type_param::{check_type_args, TypeArg, TypeParam};
+use crate::types::{FunctionType, PolyFuncType};
+use crate::Hugr;
 
 /// Trait for extensions to provide custom binary code for computing signature.
 pub trait CustomSignatureFunc: Send + Sync {
@@ -35,18 +28,6 @@ pub trait CustomSignatureFunc: Send + Sync {
         misc: &HashMap<String, serde_yaml::Value>,
         extension_registry: &ExtensionRegistry,
     ) -> Result<PolyFuncType, SignatureError>;
-
-    /// Describe the signature of a node, given the operation name,
-    /// values for the type parameters,
-    /// and 'misc' data from the extension definition YAML.
-    fn describe_signature(
-        &self,
-        _name: &SmolStr,
-        _arg_values: &[TypeArg],
-        _misc: &HashMap<String, serde_yaml::Value>,
-    ) -> SignatureDescription {
-        SignatureDescription::default()
-    }
 }
 
 // Note this is very much a utility, rather than definitive;
@@ -215,16 +196,6 @@ impl OpDef {
         Ok(res)
     }
 
-    /// Optional description of the ports in the signature.
-    pub fn signature_desc(&self, args: &[TypeArg]) -> SignatureDescription {
-        match &self.signature_func {
-            SignatureFunc::TypeScheme { .. } => todo!(),
-            SignatureFunc::CustomFunc { func, .. } => {
-                func.describe_signature(&self.name, args, &self.misc)
-            }
-        }
-    }
-
     pub(crate) fn should_serialize_signature(&self) -> bool {
         match self.signature_func {
             SignatureFunc::TypeScheme { .. } => true,

src/ops.rs

@@ -9,7 +9,7 @@ pub mod leaf;
 pub mod module;
 pub mod tag;
 pub mod validate;
-use crate::types::{EdgeKind, FunctionType, SignatureDescription, Type};
+use crate::types::{EdgeKind, FunctionType, Type};
 use crate::PortIndex;
 use crate::{Direction, Port};
 
@@ -189,12 +189,6 @@ pub trait OpTrait {
     fn signature(&self) -> FunctionType {
         Default::default()
     }
-    /// Optional description of the ports in the signature.
-    ///
-    /// Only dataflow operations have a non-empty signature.
-    fn signature_desc(&self) -> SignatureDescription {
-        Default::default()
-    }
 
     /// Get the static input type of this operation if it has one (only Some for
     /// [`LoadConstant`] and [`Call`])

src/ops/custom.rs

@@ -7,7 +7,7 @@ use thiserror::Error;
 use crate::extension::{ExtensionId, ExtensionRegistry, OpDef, SignatureError};
 use crate::hugr::hugrmut::sealed::HugrMutInternals;
 use crate::hugr::{HugrView, NodeType};
-use crate::types::{type_param::TypeArg, FunctionType, SignatureDescription};
+use crate::types::{type_param::TypeArg, FunctionType};
 use crate::{Hugr, Node};
 
 use super::tag::OpTag;
@@ -76,13 +76,6 @@ impl OpTrait for ExternalOp {
         }
     }
 
-    fn signature_desc(&self) -> SignatureDescription {
-        match self {
-            Self::Opaque(_) => SignatureDescription::default(),
-            Self::Extension(ExtensionOp { def, args, .. }) => def.signature_desc(args),
-        }
-    }
-
     fn tag(&self) -> OpTag {
         OpTag::Leaf
     }

src/ops/leaf.rs

@@ -10,7 +10,7 @@ use crate::types::type_param::TypeArg;
 use crate::types::PolyFuncType;
 use crate::{
     extension::{ExtensionId, ExtensionSet},
-    types::{EdgeKind, FunctionType, SignatureDescription, Type, TypeRow},
+    types::{EdgeKind, FunctionType, Type, TypeRow},
 };
 
 /// Dataflow operations with no children.
@@ -182,15 +182,6 @@ impl OpTrait for LeafOp {
         }
     }
 
-    /// Optional description of the ports in the signature.
-    fn signature_desc(&self) -> SignatureDescription {
-        match self {
-            LeafOp::CustomOp(ext) => ext.signature_desc(),
-            // TODO: More port descriptions
-            _ => Default::default(),
-        }
-    }
-
     fn other_input(&self) -> Option<EdgeKind> {
         Some(EdgeKind::StateOrder)
     }

src/std_extensions.rs

@@ -6,4 +6,3 @@ pub mod arithmetic;
 pub mod collections;
 pub mod logic;
 pub mod quantum;
-pub mod rotation;

src/types.rs

@@ -12,7 +12,7 @@ pub mod type_row;
 pub use check::{ConstTypeError, CustomCheckFailure};
 pub use custom::CustomType;
 pub use poly_func::PolyFuncType;
-pub use signature::{FunctionType, Signature, SignatureDescription};
+pub use signature::{FunctionType, Signature};
 pub use type_param::TypeArg;
 pub use type_row::TypeRow;