feat!: Make NamedOp private. Add MakeExtensionOp::name and MakeOpDef::opdef_name (#2138)

BREAKING CHANGE: `NamedOp` is no longer public. Use `OpType`'s `Display`
impl for a short string describing the OpType.
BREAKING CHANGE: New required trait method `opdef_name` on `MakeOpDef`
BREAKING CHANGE: New required trait method `name` on `MakeExtensionOp`

Author: doug-q

hugr-core/src/export.rs

@@ -447,7 +447,7 @@ impl<'a> Context<'a> {
             }
 
             OpType::OpaqueOp(op) => {
-                let node = self.make_named_global_ref(op.extension(), op.op_name());
+                let node = self.make_named_global_ref(op.extension(), op.unqualified_id());
                 let params = self
                     .bump
                     .alloc_slice_fill_iter(op.args().iter().map(|arg| self.export_type_arg(arg)));

hugr-core/src/extension.rs

@@ -466,8 +466,8 @@ trait CustomConcrete {
 impl CustomConcrete for OpaqueOp {
     type Identifier = OpName;
 
-    fn def_name(&self) -> &OpName {
-        self.op_name()
+    fn def_name(&self) -> &Self::Identifier {
+        self.unqualified_id()
     }
 
     fn type_args(&self) -> &[TypeArg] {

hugr-core/src/extension/prelude.rs

@@ -595,7 +595,12 @@ impl ConstFold for TupleOpDef {
         }
     }
 }
+
 impl MakeOpDef for TupleOpDef {
+    fn opdef_id(&self) -> OpName {
+        <&'static str>::from(self).into()
+    }
+
     fn init_signature(&self, _extension_ref: &Weak<Extension>) -> SignatureFunc {
         let rv = TypeRV::new_row_var_use(0, TypeBound::Any);
         let tuple_type = TypeRV::new_tuple(vec![rv.clone()]);
@@ -649,20 +654,18 @@ impl MakeTuple {
     }
 }
 
-impl NamedOp for MakeTuple {
-    fn name(&self) -> OpName {
-        TupleOpDef::MakeTuple.name()
+impl MakeExtensionOp for MakeTuple {
+    fn op_id(&self) -> OpName {
+        TupleOpDef::MakeTuple.opdef_id()
     }
-}
 
-impl MakeExtensionOp for MakeTuple {
     fn from_extension_op(ext_op: &crate::ops::ExtensionOp) -> Result<Self, OpLoadError>
     where
         Self: Sized,
     {
         let def = TupleOpDef::from_def(ext_op.def())?;
         if def != TupleOpDef::MakeTuple {
-            return Err(OpLoadError::NotMember(ext_op.def().name().to_string()))?;
+            return Err(OpLoadError::NotMember(ext_op.unqualified_id().to_string()))?;
         }
         let [TypeArg::Sequence { elems }] = ext_op.args() else {
             return Err(SignatureError::InvalidTypeArgs)?;
@@ -711,20 +714,18 @@ impl UnpackTuple {
     }
 }
 
-impl NamedOp for UnpackTuple {
-    fn name(&self) -> OpName {
-        TupleOpDef::UnpackTuple.name()
+impl MakeExtensionOp for UnpackTuple {
+    fn op_id(&self) -> OpName {
+        TupleOpDef::UnpackTuple.opdef_id()
     }
-}
 
-impl MakeExtensionOp for UnpackTuple {
     fn from_extension_op(ext_op: &crate::ops::ExtensionOp) -> Result<Self, OpLoadError>
     where
         Self: Sized,
     {
         let def = TupleOpDef::from_def(ext_op.def())?;
         if def != TupleOpDef::UnpackTuple {
-            return Err(OpLoadError::NotMember(ext_op.def().name().to_string()))?;
+            return Err(OpLoadError::NotMember(ext_op.unqualified_id().to_string()))?;
         }
         let [TypeArg::Sequence { elems }] = ext_op.args() else {
             return Err(SignatureError::InvalidTypeArgs)?;
@@ -760,28 +761,30 @@ impl MakeRegisteredOp for UnpackTuple {
     }
 }
 
+/// Name of the no-op operation.
+pub const NOOP_OP_ID: OpName = OpName::new_inline("Noop");
+
 #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
 /// A no-op operation definition.
 pub struct NoopDef;
 
-impl NamedOp for NoopDef {
-    fn name(&self) -> OpName {
-        "Noop".into()
-    }
-}
-
 impl std::str::FromStr for NoopDef {
     type Err = ();
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s == NoopDef.name() {
+        if s == NoopDef.op_id() {
             Ok(Self)
         } else {
             Err(())
         }
     }
 }
+
 impl MakeOpDef for NoopDef {
+    fn opdef_id(&self) -> OpName {
+        NOOP_OP_ID
+    }
+
     fn init_signature(&self, _extension_ref: &Weak<Extension>) -> SignatureFunc {
         let tv = Type::new_var_use(0, TypeBound::Any);
         PolyFuncType::new([TypeBound::Any.into()], Signature::new_endo(tv)).into()
@@ -836,13 +839,12 @@ impl Default for Noop {
         Self(Type::UNIT)
     }
 }
-impl NamedOp for Noop {
-    fn name(&self) -> OpName {
-        NoopDef.name()
-    }
-}
 
 impl MakeExtensionOp for Noop {
+    fn op_id(&self) -> OpName {
+        NoopDef.op_id()
+    }
+
     fn from_extension_op(ext_op: &crate::ops::ExtensionOp) -> Result<Self, OpLoadError>
     where
         Self: Sized,
@@ -875,17 +877,12 @@ pub struct BarrierDef;
 
 /// Name of the barrier operation.
 pub const BARRIER_OP_ID: OpName = OpName::new_inline("Barrier");
-impl NamedOp for BarrierDef {
-    fn name(&self) -> OpName {
-        BARRIER_OP_ID
-    }
-}
 
 impl std::str::FromStr for BarrierDef {
     type Err = ();
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s == BarrierDef.name() {
+        if s == BarrierDef.op_id() {
             Ok(Self)
         } else {
             Err(())
@@ -894,6 +891,10 @@ impl std::str::FromStr for BarrierDef {
 }
 
 impl MakeOpDef for BarrierDef {
+    fn opdef_id(&self) -> OpName {
+        BARRIER_OP_ID
+    }
+
     fn init_signature(&self, _extension_ref: &Weak<Extension>) -> SignatureFunc {
         PolyFuncTypeRV::new(
             vec![TypeParam::new_list(TypeBound::Any)],
@@ -940,11 +941,15 @@ impl Barrier {
 
 impl NamedOp for Barrier {
     fn name(&self) -> OpName {
-        BarrierDef.name()
+        BarrierDef.op_id()
     }
 }
 
 impl MakeExtensionOp for Barrier {
+    fn op_id(&self) -> OpName {
+        BarrierDef.op_id()
+    }
+
     fn from_extension_op(ext_op: &crate::ops::ExtensionOp) -> Result<Self, OpLoadError>
     where
         Self: Sized,

hugr-core/src/extension/prelude/generic.rs

@@ -9,7 +9,6 @@ use crate::extension::OpDef;
 use crate::extension::SignatureFunc;
 use crate::extension::{ConstFold, ExtensionId};
 use crate::ops::ExtensionOp;
-use crate::ops::NamedOp;
 use crate::ops::OpName;
 use crate::type_row;
 use crate::types::FuncValueType;
@@ -28,23 +27,17 @@ use super::{ConstUsize, PRELUDE_ID};
 use crate::types::type_param::TypeParam;
 
 /// Name of the operation for loading generic BoundedNat parameters.
-pub const LOAD_NAT_OP_ID: OpName = OpName::new_inline("load_nat");
+pub static LOAD_NAT_OP_ID: OpName = OpName::new_inline("load_nat");
 
 /// Definition of the load nat operation.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub struct LoadNatDef;
 
-impl NamedOp for LoadNatDef {
-    fn name(&self) -> OpName {
-        LOAD_NAT_OP_ID
-    }
-}
-
 impl FromStr for LoadNatDef {
     type Err = ();
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s == LoadNatDef.name() {
+        if s == Self.op_id() {
             Ok(Self)
         } else {
             Err(())
@@ -72,6 +65,10 @@ impl ConstFold for LoadNatDef {
 }
 
 impl MakeOpDef for LoadNatDef {
+    fn opdef_id(&self) -> OpName {
+        LOAD_NAT_OP_ID.clone()
+    }
+
     fn from_def(op_def: &OpDef) -> Result<Self, OpLoadError>
     where
         Self: Sized,
@@ -120,13 +117,11 @@ impl LoadNat {
     }
 }
 
-impl NamedOp for LoadNat {
-    fn name(&self) -> OpName {
-        LOAD_NAT_OP_ID
+impl MakeExtensionOp for LoadNat {
+    fn op_id(&self) -> OpName {
+        LoadNatDef.opdef_id()
     }
-}
 
-impl MakeExtensionOp for LoadNat {
     fn from_extension_op(ext_op: &ExtensionOp) -> Result<Self, OpLoadError>
     where
         Self: Sized,

hugr-core/src/extension/resolution/ops.rs

@@ -73,7 +73,7 @@ pub(crate) fn resolve_op_extensions<'e>(
     // Fail if the Extension is not in the registry, or if the Extension was
     // found but did not have the expected operation.
     let extension = extension.expect("OpaqueOp should have an extension");
-    let Some(def) = extension.get_op(opaque.op_name()) else {
+    let Some(def) = extension.get_op(opaque.unqualified_id()) else {
         return Err(OpaqueOpError::OpNotFoundInExtension {
             node,
             op: opaque.name().clone(),

hugr-core/src/extension/simple_op.rs

@@ -5,11 +5,7 @@ use std::sync::Weak;
 use strum::IntoEnumIterator;
 
 use crate::ops::{ExtensionOp, OpName, OpNameRef};
-use crate::{
-    ops::{NamedOp, OpType},
-    types::TypeArg,
-    Extension,
-};
+use crate::{ops::OpType, types::TypeArg, Extension};
 
 use super::{op_def::SignatureFunc, ExtensionBuildError, ExtensionId, OpDef, SignatureError};
 use delegate::delegate;
@@ -29,22 +25,24 @@ pub enum OpLoadError {
     WrongExtension(ExtensionId, ExtensionId),
 }
 
-impl<T> NamedOp for T
-where
-    for<'a> &'a T: Into<&'static str>,
-{
-    fn name(&self) -> OpName {
-        let s = self.into();
-        s.into()
-    }
-}
-
 /// Traits implemented by types which can add themselves to [`Extension`]s as
 /// [`OpDef`]s or load themselves from an [`OpDef`].
 ///
 /// Particularly useful with C-style enums that implement [strum::IntoEnumIterator],
 /// as then all definitions can be added to an extension at once.
-pub trait MakeOpDef: NamedOp {
+///
+/// [MakeExtensionOp] has a blanket impl for types that impl [MakeOpDef].
+pub trait MakeOpDef {
+    /// The [OpDef::name] which will be used when `Self`  is added to an [Extension]
+    /// or when `Self` is loaded from an [OpDef].
+    ///
+    /// This identifer must be unique within the extension with which the
+    /// [OpDef] is registered. An [ExtensionOp] instantiating this [OpDef] will
+    /// report `self.opdef_id()` as its [ExtensionOp::unqualified_id].
+    ///
+    /// [MakeExtensionOp::op_id] must match this function.
+    fn opdef_id(&self) -> OpName;
+
     /// Try to load one of the operations of this set from an [OpDef].
     fn from_def(op_def: &OpDef) -> Result<Self, OpLoadError>
     where
@@ -68,9 +66,9 @@ pub trait MakeOpDef: NamedOp {
         self.init_signature(&self.extension_ref())
     }
 
-    /// Description of the operation. By default, the same as `self.name()`.
+    /// Description of the operation. By default, the same as `self.opdef_id()`.
     fn description(&self) -> String {
-        self.name().to_string()
+        self.opdef_id().to_string()
     }
 
     /// Edit the opdef before finalising. By default does nothing.
@@ -87,7 +85,7 @@ pub trait MakeOpDef: NamedOp {
         extension_ref: &Weak<Extension>,
     ) -> Result<(), ExtensionBuildError> {
         let def = extension.add_op(
-            self.name(),
+            self.opdef_id(),
             self.description(),
             self.init_signature(extension_ref),
             extension_ref,
@@ -150,7 +148,14 @@ pub trait HasDef: MakeExtensionOp {
 
 /// Traits implemented by types which can be loaded from [`ExtensionOp`]s,
 /// i.e. concrete instances of [`OpDef`]s, with defined type arguments.
-pub trait MakeExtensionOp: NamedOp {
+pub trait MakeExtensionOp {
+    /// The [OpDef::name] of [ExtensionOp]s from which `Self` can be loaded.
+    ///
+    /// This identifer must be unique within the extension with which the
+    /// [OpDef] is registered. An [ExtensionOp] instantiating this [OpDef] will
+    /// report `self.opdef_id()` as its [ExtensionOp::unqualified_id].
+    fn op_id(&self) -> OpName;
+
     /// Try to load one of the operations of this set from an [OpDef].
     fn from_extension_op(ext_op: &ExtensionOp) -> Result<Self, OpLoadError>
     where
@@ -188,6 +193,10 @@ pub trait MakeExtensionOp: NamedOp {
 
 /// Blanket implementation for non-polymorphic operations - [OpDef]s with no type parameters.
 impl<T: MakeOpDef> MakeExtensionOp for T {
+    fn op_id(&self) -> OpName {
+        self.opdef_id()
+    }
+
     #[inline]
     fn from_extension_op(ext_op: &ExtensionOp) -> Result<Self, OpLoadError>
     where
@@ -243,7 +252,7 @@ impl<T: MakeExtensionOp> RegisteredOp<T> {
     /// Generate an [OpType].
     pub fn to_extension_op(&self) -> Option<ExtensionOp> {
         ExtensionOp::new(
-            self.extension.upgrade()?.get_op(&self.name())?.clone(),
+            self.extension.upgrade()?.get_op(&self.op_id())?.clone(),
             self.type_args(),
         )
         .ok()
@@ -252,7 +261,7 @@ impl<T: MakeExtensionOp> RegisteredOp<T> {
     delegate! {
         to self.op {
             /// Name of the operation - derived from strum serialization.
-            pub fn name(&self) -> OpName;
+            pub fn op_id(&self) -> OpName;
             /// Any type args which define this operation. Default is no type arguments.
             pub fn type_args(&self) -> Vec<TypeArg>;
         }
@@ -310,6 +319,10 @@ mod test {
     }
 
     impl MakeOpDef for DummyEnum {
+        fn opdef_id(&self) -> OpName {
+            <&'static str>::from(self).into()
+        }
+
         fn init_signature(&self, _extension_ref: &Weak<Extension>) -> SignatureFunc {
             Signature::new_endo(type_row![]).into()
         }
@@ -366,7 +379,7 @@ mod test {
         let o = DummyEnum::Dumb;
 
         assert_eq!(
-            DummyEnum::from_def(EXT.get_op(&o.name()).unwrap()).unwrap(),
+            DummyEnum::from_def(EXT.get_op(&o.opdef_id()).unwrap()).unwrap(),
             o
         );