feat!: Add Visibility to FuncDefn/FuncDecl. (#2143)

Note: modified to come after #2412 as I want those Schema checks in
place to check this works!
closes #2354, #1752.
Includes cherry-pick of #2343.

* Add `enum Visibility`. In fact *2 (hugr-model version is not
serde-able so cannot be used in hugr-core).
* `FuncDefn` and `FuncDecl` gain this. (A good use-case for a private
FuncDecl is building cyclic functions via builder `define_declaration`.)
* I've not added to `AliasDecl` or `AliasDefn` - I could, it would be
consistent, but we hardly use them....
* `validate` checks that the *public* children of a Module have unique
names (except multiple FuncDecls may alias).

* `FuncDefn::new` defaults to private (also builder `define_function` /
`FunctionBuilder::new`, and json-deserialize in both python and rust).
There is a `_vis` taking an explicit Visibility parameter.
* `FuncDecl::new` defaults to public (also builder `declare_function`,
and json-deserialize*2). Again there is `_vis`.
* hugr-model import/export + roundtrip + AST printing via new keyword
`pub`. Note since `hugr-core` does not have visibility on aliases or
extension operations I've defaulted these to private to avoid any change
to textual output.
* add field to hugr-py, using "string"-enum (`Literal["Public",
"Private"]`)
* I've attempted to add hugr-model hugr-py bindings, but there are no
tests of these yet.

Note all changes to hugr-passes delayed until a follow-up PR.

BREAKING CHANGE: hugr-model: Symbol has an extra field

Author: acl-cqc

hugr-core/src/builder/dataflow.rs

@@ -8,14 +8,9 @@ use std::marker::PhantomData;
 
 use crate::hugr::internal::HugrMutInternals;
 use crate::hugr::{HugrView, ValidationError};
-use crate::ops::{self, OpParent};
-use crate::ops::{DataflowParent, Input, Output};
-use crate::{Direction, IncomingPort, OutgoingPort, Wire};
-
+use crate::ops::{self, DataflowParent, FuncDefn, Input, OpParent, Output};
 use crate::types::{PolyFuncType, Signature, Type};
-
-use crate::Node;
-use crate::{Hugr, hugr::HugrMut};
+use crate::{Direction, Hugr, IncomingPort, Node, OutgoingPort, Visibility, Wire, hugr::HugrMut};
 
 /// Builder for a [`ops::DFG`] node.
 #[derive(Debug, Clone, PartialEq)]
@@ -152,17 +147,35 @@ impl<B, T> DFGWrapper<B, T> {
 pub type FunctionBuilder<B> = DFGWrapper<B, BuildHandle<FuncID<true>>>;
 
 impl FunctionBuilder<Hugr> {
-    /// Initialize a builder for a `FuncDefn` rooted HUGR
+    /// Initialize a builder for a [`FuncDefn`](ops::FuncDefn)-rooted HUGR;
+    /// the function will be private. (See also [Self::new_vis].)
+    ///
     /// # Errors
     ///
     /// Error in adding DFG child nodes.
     pub fn new(
         name: impl Into<String>,
         signature: impl Into<PolyFuncType>,
     ) -> Result<Self, BuildError> {
-        let signature: PolyFuncType = signature.into();
-        let body = signature.body().clone();
-        let op = ops::FuncDefn::new(name, signature);
+        Self::new_with_op(FuncDefn::new(name, signature))
+    }
+
+    /// Initialize a builder for a FuncDefn-rooted HUGR, with the specified
+    /// [Visibility].
+    ///
+    /// # Errors
+    ///
+    /// Error in adding DFG child nodes.
+    pub fn new_vis(
+        name: impl Into<String>,
+        signature: impl Into<PolyFuncType>,
+        visibility: Visibility,
+    ) -> Result<Self, BuildError> {
+        Self::new_with_op(FuncDefn::new_vis(name, signature, visibility))
+    }
+
+    fn new_with_op(op: FuncDefn) -> Result<Self, BuildError> {
+        let body = op.signature().body().clone();
 
         let base = Hugr::new_with_entrypoint(op).expect("FuncDefn entrypoint should be valid");
         let root = base.entrypoint();

hugr-core/src/builder/module.rs

@@ -4,15 +4,14 @@ use super::{
     dataflow::{DFGBuilder, FunctionBuilder},
 };
 
-use crate::hugr::ValidationError;
 use crate::hugr::internal::HugrMutInternals;
 use crate::hugr::views::HugrView;
 use crate::ops;
-use crate::types::{PolyFuncType, Type, TypeBound};
-
 use crate::ops::handle::{AliasID, FuncID, NodeHandle};
+use crate::types::{PolyFuncType, Type, TypeBound};
+use crate::{Hugr, Node, Visibility};
+use crate::{hugr::ValidationError, ops::FuncDefn};
 
-use crate::{Hugr, Node};
 use smol_str::SmolStr;
 
 /// Builder for a HUGR module.
@@ -69,25 +68,61 @@ impl<T: AsMut<Hugr> + AsRef<Hugr>> ModuleBuilder<T> {
         f_id: &FuncID<false>,
     ) -> Result<FunctionBuilder<&mut Hugr>, BuildError> {
         let f_node = f_id.node();
-        let decl =
-            self.hugr()
-                .get_optype(f_node)
-                .as_func_decl()
-                .ok_or(BuildError::UnexpectedType {
-                    node: f_node,
-                    op_desc: "crate::ops::OpType::FuncDecl",
-                })?;
-        let name = decl.func_name().clone();
-        let sig = decl.signature().clone();
-        let body = sig.body().clone();
-        self.hugr_mut()
-            .replace_op(f_node, ops::FuncDefn::new(name, sig));
+        let opty = self.hugr_mut().optype_mut(f_node);
+        let ops::OpType::FuncDecl(decl) = opty else {
+            return Err(BuildError::UnexpectedType {
+                node: f_node,
+                op_desc: "crate::ops::OpType::FuncDecl",
+            });
+        };
+
+        let body = decl.signature().body().clone();
+        *opty = ops::FuncDefn::new_vis(
+            decl.func_name(),
+            decl.signature().clone(),
+            decl.visibility().clone(),
+        )
+        .into();
+
+        let db = DFGBuilder::create_with_io(self.hugr_mut(), f_node, body)?;
+        Ok(FunctionBuilder::from_dfg_builder(db))
+    }
+
+    /// Add a [`ops::FuncDefn`] node of the specified visibility.
+    /// Returns a builder to define the function body graph.
+    ///
+    /// # Errors
+    ///
+    /// This function will return an error if there is an error in adding the
+    /// [`ops::FuncDefn`] node.
+    pub fn define_function_vis(
+        &mut self,
+        name: impl Into<String>,
+        signature: impl Into<PolyFuncType>,
+        visibility: Visibility,
+    ) -> Result<FunctionBuilder<&mut Hugr>, BuildError> {
+        self.define_function_op(FuncDefn::new_vis(name, signature, visibility))
+    }
+
+    fn define_function_op(
+        &mut self,
+        op: FuncDefn,
+    ) -> Result<FunctionBuilder<&mut Hugr>, BuildError> {
+        let body = op.signature().body().clone();
+        let f_node = self.add_child_node(op);
+
+        // Add the extensions used by the function types.
+        self.use_extensions(
+            body.used_extensions().unwrap_or_else(|e| {
+                panic!("Build-time signatures should have valid extensions. {e}")
+            }),
+        );
 
         let db = DFGBuilder::create_with_io(self.hugr_mut(), f_node, body)?;
         Ok(FunctionBuilder::from_dfg_builder(db))
     }
 
-    /// Declare a function with `signature` and return a handle to the declaration.
+    /// Declare a [Visibility::Public] function with `signature` and return a handle to the declaration.
     ///
     /// # Errors
     ///
@@ -97,10 +132,26 @@ impl<T: AsMut<Hugr> + AsRef<Hugr>> ModuleBuilder<T> {
         &mut self,
         name: impl Into<String>,
         signature: PolyFuncType,
+    ) -> Result<FuncID<false>, BuildError> {
+        self.declare_vis(name, signature, Visibility::Public)
+    }
+
+    /// Declare a function with the specified `signature` and [Visibility],
+    /// and return a handle to the declaration.
+    ///
+    /// # Errors
+    ///
+    /// This function will return an error if there is an error in adding the
+    /// [`crate::ops::OpType::FuncDecl`] node.
+    pub fn declare_vis(
+        &mut self,
+        name: impl Into<String>,
+        signature: PolyFuncType,
+        visibility: Visibility,
     ) -> Result<FuncID<false>, BuildError> {
         let body = signature.body().clone();
         // TODO add param names to metadata
-        let declare_n = self.add_child_node(ops::FuncDecl::new(name, signature));
+        let declare_n = self.add_child_node(ops::FuncDecl::new_vis(name, signature, visibility));
 
         // Add the extensions used by the function types.
         self.use_extensions(
@@ -112,8 +163,8 @@ impl<T: AsMut<Hugr> + AsRef<Hugr>> ModuleBuilder<T> {
         Ok(declare_n.into())
     }
 
-    /// Add a [`ops::FuncDefn`] node and returns a builder to define the function
-    /// body graph.
+    /// Adds a [`ops::FuncDefn`] node and returns a builder to define the function
+    /// body graph. The function will be private. (See [Self::define_function_vis].)
     ///
     /// # Errors
     ///
@@ -124,19 +175,7 @@ impl<T: AsMut<Hugr> + AsRef<Hugr>> ModuleBuilder<T> {
         name: impl Into<String>,
         signature: impl Into<PolyFuncType>,
     ) -> Result<FunctionBuilder<&mut Hugr>, BuildError> {
-        let signature: PolyFuncType = signature.into();
-        let body = signature.body().clone();
-        let f_node = self.add_child_node(ops::FuncDefn::new(name, signature));
-
-        // Add the extensions used by the function types.
-        self.use_extensions(
-            body.used_extensions().unwrap_or_else(|e| {
-                panic!("Build-time signatures should have valid extensions. {e}")
-            }),
-        );
-
-        let db = DFGBuilder::create_with_io(self.hugr_mut(), f_node, body)?;
-        Ok(FunctionBuilder::from_dfg_builder(db))
+        self.define_function_op(FuncDefn::new(name, signature))
     }
 
     /// Add a [`crate::ops::OpType::AliasDefn`] node and return a handle to the Alias.

hugr-core/src/core.rs

@@ -276,6 +276,46 @@ impl<N: HugrNode> std::fmt::Display for Wire<N> {
     }
 }
 
+/// Marks [FuncDefn](crate::ops::FuncDefn)s and [FuncDecl](crate::ops::FuncDecl)s as
+/// to whether they should be considered for linking.
+#[derive(
+    Clone,
+    Debug,
+    derive_more::Display,
+    PartialEq,
+    Eq,
+    PartialOrd,
+    Ord,
+    serde::Serialize,
+    serde::Deserialize,
+)]
+#[cfg_attr(test, derive(proptest_derive::Arbitrary))]
+#[non_exhaustive]
+pub enum Visibility {
+    /// Function is visible or exported
+    Public,
+    /// Function is hidden, for use within the hugr only
+    Private,
+}
+
+impl From<hugr_model::v0::Visibility> for Visibility {
+    fn from(value: hugr_model::v0::Visibility) -> Self {
+        match value {
+            hugr_model::v0::Visibility::Private => Self::Private,
+            hugr_model::v0::Visibility::Public => Self::Public,
+        }
+    }
+}
+
+impl From<Visibility> for hugr_model::v0::Visibility {
+    fn from(value: Visibility) -> Self {
+        match value {
+            Visibility::Public => hugr_model::v0::Visibility::Public,
+            Visibility::Private => hugr_model::v0::Visibility::Private,
+        }
+    }
+}
+
 /// Enum for uniquely identifying the origin of linear wires in a circuit-like
 /// dataflow region.
 ///

hugr-core/src/export.rs

@@ -20,6 +20,7 @@ use crate::{
 };
 
 use fxhash::{FxBuildHasher, FxHashMap};
+use hugr_model::v0::Visibility;
 use hugr_model::v0::{
     self as model,
     bumpalo::{Bump, collections::String as BumpString, collections::Vec as BumpVec},
@@ -230,16 +231,6 @@ impl<'a> Context<'a> {
         }
     }
 
-    /// Get the name of a function definition or declaration node. Returns `None` if not
-    /// one of those operations.
-    fn get_func_name(&self, func_node: Node) -> Option<&'a str> {
-        match self.hugr.get_optype(func_node) {
-            OpType::FuncDecl(func_decl) => Some(func_decl.func_name()),
-            OpType::FuncDefn(func_defn) => Some(func_defn.func_name()),
-            _ => None,
-        }
-    }
-
     fn with_local_scope<T>(&mut self, node: table::NodeId, f: impl FnOnce(&mut Self) -> T) -> T {
         let prev_local_scope = self.local_scope.replace(node);
         let prev_local_constraints = std::mem::take(&mut self.local_constraints);
@@ -338,8 +329,11 @@ impl<'a> Context<'a> {
             }
 
             OpType::FuncDefn(func) => self.with_local_scope(node_id, |this| {
-                let name = this.get_func_name(node).unwrap();
-                let symbol = this.export_poly_func_type(name, func.signature());
+                let symbol = this.export_poly_func_type(
+                    func.func_name(),
+                    func.visibility().clone().into(),
+                    func.signature(),
+                );
                 regions = this.bump.alloc_slice_copy(&[this.export_dfg(
                     node,
                     model::ScopeClosure::Closed,
@@ -349,15 +343,21 @@ impl<'a> Context<'a> {
             }),
 
             OpType::FuncDecl(func) => self.with_local_scope(node_id, |this| {
-                let name = this.get_func_name(node).unwrap();
-                let symbol = this.export_poly_func_type(name, func.signature());
+                let symbol = this.export_poly_func_type(
+                    func.func_name(),
+                    func.visibility().clone().into(),
+                    func.signature(),
+                );
                 table::Operation::DeclareFunc(symbol)
             }),
 
             OpType::AliasDecl(alias) => self.with_local_scope(node_id, |this| {
                 // TODO: We should support aliases with different types and with parameters
                 let signature = this.make_term_apply(model::CORE_TYPE, &[]);
+                // Visibility is not spec'd in hugr-core
+                let visibility = this.bump.alloc(Visibility::default()); // good to common up!?
                 let symbol = this.bump.alloc(table::Symbol {
+                    visibility,
                     name: &alias.name,
                     params: &[],
                     constraints: &[],
@@ -370,7 +370,10 @@ impl<'a> Context<'a> {
                 let value = this.export_type(&alias.definition);
                 // TODO: We should support aliases with different types and with parameters
                 let signature = this.make_term_apply(model::CORE_TYPE, &[]);
+                // Visibility is not spec'd in hugr-core
+                let visibility = this.bump.alloc(Visibility::default()); // good to common up!?
                 let symbol = this.bump.alloc(table::Symbol {
+                    visibility,
                     name: &alias.name,
                     params: &[],
                     constraints: &[],
@@ -545,7 +548,8 @@ impl<'a> Context<'a> {
 
         let symbol = self.with_local_scope(node, |this| {
             let name = this.make_qualified_name(opdef.extension_id(), opdef.name());
-            this.export_poly_func_type(name, poly_func_type)
+            // Visibility of OpDef's has no effect
+            this.export_poly_func_type(name, Visibility::default(), poly_func_type)
         });
 
         let meta = {
@@ -796,13 +800,14 @@ impl<'a> Context<'a> {
     pub fn export_poly_func_type<RV: MaybeRV>(
         &mut self,
         name: &'a str,
+        visibility: Visibility,
         t: &PolyFuncTypeBase<RV>,
     ) -> &'a table::Symbol<'a> {
         let mut params = BumpVec::with_capacity_in(t.params().len(), self.bump);
         let scope = self
             .local_scope
             .expect("exporting poly func type outside of local scope");
-
+        let visibility = self.bump.alloc(visibility);
         for (i, param) in t.params().iter().enumerate() {
             let name = self.bump.alloc_str(&i.to_string());
             let r#type = self.export_term(param, Some((scope, i as _)));
@@ -814,6 +819,7 @@ impl<'a> Context<'a> {
         let body = self.export_func_type(t.body());
 
         self.bump.alloc(table::Symbol {
+            visibility,
             name,
             params: params.into_bump_slice(),
             constraints,