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

doug-q · 2025-04-29T11:24:02Z

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

hugrbot · 2025-04-29T11:28:04Z

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary


--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/inherent_method_missing.ron

Failed in:
OpaqueOp::op_name, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops/custom.rs:259
OpaqueOp::op_name, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops/custom.rs:259
RegisteredOp::name, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/extension/simple_op.rs:255

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_method_added.ron

Failed in:
trait method hugr_core::extension::simple_op::MakeExtensionOp::op_id in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/extension/simple_op.rs:157

--- failure trait_missing: pub trait removed or renamed ---

Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_missing.ron

Failed in:
trait hugr_core::ops::NamedOp, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops.rs:358

--- failure trait_newly_sealed: pub trait became sealed ---

Description:
A publicly-visible trait became sealed, so downstream crates are no longer able to implement it
      ref: https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_newly_sealed.ron

Failed in:
trait hugr_core::extension::simple_op::MakeExtensionOp in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/extension/simple_op.rs:151
trait hugr_core::extension::simple_op::HasDef in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/extension/simple_op.rs:136

--- failure trait_removed_supertrait: supertrait removed or renamed ---

Description:
A supertrait was removed from a trait. Users of the trait can no longer assume it can also be used like its supertrait.
      ref: https://doc.rust-lang.org/reference/items/traits.html#supertraits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_removed_supertrait.ron

Failed in:
supertrait hugr_core::ops::NamedOp of trait MakeExtensionOp in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/extension/simple_op.rs:151
supertrait hugr_core::ops::NamedOp of trait MakeOpDef in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/extension/simple_op.rs:35

codecov · 2025-04-29T11:30:55Z

Codecov Report

Attention: Patch coverage is 87.56219% with 25 lines in your changes missing coverage. Please review.

Project coverage is 82.93%. Comparing base (731081f) to head (dca9eeb).
Report is 2 commits behind head on release-rs-v0.16.0.

Files with missing lines	Patch %	Lines
hugr-llvm/src/extension/int.rs	20.00%	4 Missing ⚠️
hugr-core/src/extension.rs	0.00%	2 Missing ⚠️
hugr-core/src/extension/prelude.rs	91.66%	2 Missing ⚠️
...e/src/std_extensions/collections/array/array_op.rs	71.42%	1 Missing and 1 partial ⚠️
hugr-core/src/std_extensions/collections/list.rs	75.00%	0 Missing and 2 partials ⚠️
hugr-passes/src/force_order.rs	88.23%	1 Missing and 1 partial ⚠️
hugr-core/src/extension/simple_op.rs	90.90%	0 Missing and 1 partial ⚠️
hugr-core/src/hugr/validate.rs	50.00%	1 Missing ⚠️
...rc/std_extensions/collections/array/array_clone.rs	85.71%	0 Missing and 1 partial ⚠️
...d_extensions/collections/array/array_conversion.rs	90.00%	0 Missing and 1 partial ⚠️
... and 7 more

Additional details and impacted files

@@                  Coverage Diff                   @@
##           release-rs-v0.16.0    #2138      +/-   ##
======================================================
+ Coverage               82.90%   82.93%   +0.02%     
======================================================
  Files                     227      227              
  Lines                   42288    42333      +45     
  Branches                38387    38432      +45     
======================================================
+ Hits                    35060    35107      +47     
+ Misses                   5360     5357       -3     
- Partials                 1868     1869       +1

Flag	Coverage Δ
python	`85.64% <ø> (ø)`
rust	`82.65% <87.56%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

acl-cqc

Thanks @doug - generally I like this, but I think we should standardize more the method naming - some places we have qualified_name, others name yet these are equivalent (I think).

You haven't mentioned why we are doing this - does this close #1496 ?

Also if the goal is to hide NamedOp, but then publically use &T: Into<&'static str> then I think that would be a retrograde step, but I don't think the latter really is intended for public use, so I think it would be better to clean that up instead...

so I'm 90% of the way to hitting approve but I think I'd like to see what you can do about those!

acl-cqc · 2025-04-30T08:46:58Z

hugr-core/src/extension/prelude.rs

 impl MakeOpDef for TupleOpDef {
+    fn opdef_name(&self) -> OpName {
+        <&Self as Into<&'static str>>::into(self).into()


Suggested change

<&Self as Into<&'static str>>::into(self).into()

<&'static str>::from(self).into()

acl-cqc · 2025-04-30T09:29:40Z

hugr-core/src/extension/simple_op.rs

 /// 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 {
+pub trait MakeOpDef {
+    /// TODO docs


In particular, is this qualified or unqualified?

acl-cqc · 2025-04-30T09:30:38Z

hugr-core/src/extension/simple_op.rs

@@ -70,7 +59,7 @@ pub trait MakeOpDef: NamedOp {

    /// Description of the operation. By default, the same as `self.name()`.


Suggested change

/// Description of the operation. By default, the same as `self.name()`.

/// Description of the operation. By default, the same as [Self::opdef_name].

Or default to empty string...

I prefer to leave this as is

acl-cqc · 2025-04-30T09:31:08Z

hugr-core/src/extension/simple_op.rs

@@ -150,7 +139,10 @@ 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 name of the operation


full or unqualified?

Even better, rename the method to qualified_name or unqualified_name

acl-cqc · 2025-04-30T09:32:28Z

hugr-core/src/extension/simple_op.rs

@@ -188,6 +180,10 @@ pub trait MakeExtensionOp: NamedOp {

 /// Blanket implementation for non-polymorphic operations - [OpDef]s with no type parameters.
 impl<T: MakeOpDef> MakeExtensionOp for T {
+    fn name(&self) -> OpName {
+        <Self as MakeOpDef>::opdef_name(self)


Either MakeOpDef::opdef_name(self) or doesn't just self.opdef_name() work?

Yes, this was necessary when opdef_name was called name

acl-cqc · 2025-04-30T09:41:16Z

hugr-core/src/ops/custom.rs

+
+    /// Returns the unqualified name of the operation. e.g. 'arithmetic.iadd'
+    pub fn qualified_name(&self) -> OpName {
+        self.name()


Is this NamedOp::name? So we could rename that to NamedOp::qualified_name, and then you'd have to do NamedOp::qualified_name(self), but at least that would be explicit

Alternatively, doc that OpNames are always qualified (right?), and then call this method name (i.e. make all .name()s qualified, and sometimes there is .unqualified_name() too - in that case, you might want to rename to name_unqualified because it'll be more obvious browsing the list of autocompletes!)

OpName is not always the name of an extension op, which is the only context were qualified/unqualified makes sense.
I do agree it would be better to move ExtensionOp's NamedOp::name implementation here, and call qualified_name from there instead.

acl-cqc · 2025-04-30T09:42:01Z

hugr-core/src/std_extensions/arithmetic/conversions.rs

@@ -45,6 +45,10 @@ pub enum ConvertOpDef {
 }

 impl MakeOpDef for ConvertOpDef {
+    fn opdef_name(&self) -> OpName {
+        <&Self as Into<&'static str>>::into(self).into()


acl-cqc · 2025-04-30T09:48:55Z

hugr-core/src/std_extensions/collections/array/array_scan.rs

-impl NamedOp for ArrayScanDef {
-    fn name(&self) -> OpName {
-        ARRAY_SCAN_OP_ID
+impl From<&ArrayScanDef> for &'static str {


I see, so is that how the <&Self as Into<&'static str>>:into(self) works. This is a bit of an odd declaration - do we parametrize anything <T>...where &T: Into<&'static str>? I am wondering if maybe we should just put this directly into the impl of MakeOpDef::opdef_name and do away with this rather odd conversion; what purpose does it serve?

(There are also things like ToString which are more standard - I haven't seen why we need a trait separate from MakeOpDef, but if we do, then how about using that?)

Ok, this seems to be the only place where we have to do this, so we must be getting this odd conversion some other way for the other places??

I didn't fix this up, will remove.

Where we use it, the <&'static str>::From<X> is convenient because it is derived by a strum macro IntoStaticStr

acl-cqc · 2025-04-30T09:50:43Z

hugr-core/src/std_extensions/collections/static_array.rs

@@ -218,6 +218,11 @@ impl StaticArrayOpDef {
 }

 impl MakeOpDef for StaticArrayOpDef {
+    fn opdef_name(&self) -> OpName {
+        let s: &str = self.into();


Make standardize this with the others (suggest <&'static str>::from(self).into()), but see prev. comment, why do we have that made impl From<...> for &'static str anyway?

acl-cqc · 2025-04-30T09:54:06Z

hugr-core/src/std_extensions/ptr.rs

@@ -234,8 +236,8 @@ pub(crate) mod test {

    use super::*;
    use crate::std_extensions::arithmetic::float_types::float64_type;
-    fn get_opdef(op: impl NamedOp) -> Option<&'static Arc<OpDef>> {
-        EXTENSION.get_op(&op.name())
+    fn get_opdef(op: impl Into<&'static str>) -> Option<&'static Arc<OpDef>> {


Ah, I think this is the (only) place where we're parametrizing over types that have this odd conversion. (So you can pass an actual &'static str here, or a reference to an op-thing that has that conversion). However, this is only a test method - consider using ToString here?

Yes I thought this was fine in a test method, but not fine in the API. will reconsider this one.

doug-q

Thanks Alan. Yes I agree all the names are confusing and somewhat incoherent.
To summarise(will move to commit message when we're done):

Now only OpType variants have a NamedOp impl. OpTypes have a name, it's that impl.
ExtensionOps/OpaqueOps have a qualified_name and an unqualified_name
MakeOpDefs have a opdef_name (it's unqualified)
MakeExtensionOps have a name. (it's unqualified)

What do you think about opdef_id and op_id instead?

doug-q · 2025-04-30T10:05:03Z

hugr-core/src/ops.rs

@@ -355,7 +355,7 @@ pub type OpNameRef = str;
 #[enum_dispatch]
 /// Trait for setting name of OpType variants.
 // Separate to OpTrait to allow simple definition via impl_op_name
-pub trait NamedOp {
+pub(crate) trait NamedOp {
    /// The name of the operation.
    fn name(&self) -> OpName;


It's neither. This trait is implemented by CFG, DFG, etc, as well as ExtensionOp (whose implementation is it's qualfied name).

doug-q · 2025-04-30T10:06:55Z

hugr-core/src/ops/custom.rs

+
+    /// Returns the unqualified name of the operation. e.g. 'arithmetic.iadd'
+    pub fn qualified_name(&self) -> OpName {
+        self.name()


OpName is not always the name of an extension op, which is the only context were qualified/unqualified makes sense.
I do agree it would be better to move ExtensionOp's NamedOp::name implementation here, and call qualified_name from there instead.

doug-q · 2025-04-30T10:08:22Z

hugr-core/src/std_extensions/collections/array/array_scan.rs

-impl NamedOp for ArrayScanDef {
-    fn name(&self) -> OpName {
-        ARRAY_SCAN_OP_ID
+impl From<&ArrayScanDef> for &'static str {


I didn't fix this up, will remove.

Where we use it, the <&'static str>::From<X> is convenient because it is derived by a strum macro IntoStaticStr

doug-q · 2025-04-30T10:09:54Z

hugr-core/src/std_extensions/ptr.rs

@@ -234,8 +236,8 @@ pub(crate) mod test {

    use super::*;
    use crate::std_extensions::arithmetic::float_types::float64_type;
-    fn get_opdef(op: impl NamedOp) -> Option<&'static Arc<OpDef>> {
-        EXTENSION.get_op(&op.name())
+    fn get_opdef(op: impl Into<&'static str>) -> Option<&'static Arc<OpDef>> {


Yes I thought this was fine in a test method, but not fine in the API. will reconsider this one.

acl-cqc · 2025-04-30T10:44:51Z

Thanks Alan. Yes I agree all the names are confusing and somewhat incoherent. To summarise(will move to commit message when we're done):

Now only OpType variants have a NamedOp impl. OpTypes have a name, it's that impl.

ExtensionOps/OpaqueOps have a qualified_name and an unqualified_name

MakeOpDefs have a opdef_name (it's unqualified)

MakeExtensionOps have a name. (it's unqualified)

Hah, so everything I thought I'd inferred was wrong ;).

If we doc that OpType::name is the name, qualified if an extension op, I think that's ok to coexist with qualified_name and unqualified_name for extension ops only. (That is, name is kind of inbetween qualified and unqualified. Alternatively I think it's fair to argue that "CFG" is the qualified name of a CFG!) However, in that case, MakeExtensionOp::name should be renamed (to unqualified_name or unqual_name or something). opdef_name might wanna be opdef_name_unqual or similar, but clearly opdef_name_qualified would also be a valid method so one wonders why there is only the one.

Of course the real way to fix this is to make Name a type with a qualifier/prefix and a final id; everything has only one method returning Name; and you extract just the unqualified bit if you want. (For core CFG/etc. the qualifier is empty, and Name::display omits the intervening . in such a case.) But I agree that that'd be a bigger change than you want to make here ;)

What do you think about opdef_id and op_id instead?

Not sure what you intend here - however, if you mean that an "id" is always qualified (or always unqualified), or indeed is that midpoint (so one has qualified names, unqualified names, and ids, only) then yes that could work.

acl-cqc · 2025-04-30T11:11:00Z

If the into-static-string thing works with that impl From<&ArrayScanDef> removed, then yeah, that's fine for a test method!

…pDef::opdef_name`

…pDef::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`

doug-q requested a review from a team as a code owner April 29, 2025 11:24

doug-q requested a review from acl-cqc April 29, 2025 11:24

doug-q changed the title ~~feat!: Make NamedOp private. Add MakeExtensionOp::name and `MakeO…~~ feat!: Make NamedOp private. Add MakeExtensionOp::name and MakeOpDef::opdef_name Apr 29, 2025

doug-q linked an issue Apr 30, 2025 that may be closed by this pull request

NamedOp::name should always return scoped names. #1496

Closed

acl-cqc reviewed Apr 30, 2025

View reviewed changes

doug-q commented Apr 30, 2025

View reviewed changes

doug-q force-pushed the doug/unnamedop branch from 1514059 to a1924e2 Compare May 6, 2025 12:12

doug-q requested a review from cqc-alec May 6, 2025 12:13

doug-q added 4 commits May 6, 2025 14:39

feat!: Make NamedOp private. Add MakeExtensionOp::name and `MakeO…

62223e3

…pDef::opdef_name`

tweak

b16c0cf

fix

14ac03d

address feedback, rename -> op{,def}_id

fa26111

doug-q force-pushed the doug/unnamedop branch from a1924e2 to fa26111 Compare May 6, 2025 13:52

tweak

dca9eeb

cqc-alec approved these changes May 6, 2025

View reviewed changes

doug-q added this pull request to the merge queue May 6, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 6, 2025

doug-q added this pull request to the merge queue May 7, 2025

Merged via the queue into release-rs-v0.16.0 with commit 56c110c May 7, 2025
25 checks passed

doug-q deleted the doug/unnamedop branch May 7, 2025 09:25

This was referenced May 7, 2025

chore: release v0.20.0 #2162

Closed

chore: release v0.20.0 #2181

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

doug-q commented Apr 29, 2025 •

edited

Loading

hugrbot commented Apr 29, 2025 •

edited

Loading

codecov bot commented Apr 29, 2025 •

edited

Loading

acl-cqc left a comment •

edited

Loading

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

doug-q May 6, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

doug-q Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

doug-q Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

doug-q Apr 30, 2025

acl-cqc Apr 30, 2025

acl-cqc Apr 30, 2025

doug-q Apr 30, 2025

doug-q left a comment

doug-q Apr 30, 2025

doug-q Apr 30, 2025

doug-q Apr 30, 2025

doug-q Apr 30, 2025

acl-cqc commented Apr 30, 2025

acl-cqc commented Apr 30, 2025

	<&Self as Into<&'static str>>::into(self).into()
	<&'static str>::from(self).into()

		@@ -70,7 +59,7 @@ pub trait MakeOpDef: NamedOp {

		/// Description of the operation. By default, the same as `self.name()`.

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

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

Conversation

doug-q commented Apr 29, 2025 • edited Loading

hugrbot commented Apr 29, 2025 • edited Loading

codecov bot commented Apr 29, 2025 • edited Loading

Codecov Report

acl-cqc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Apr 30, 2025

acl-cqc commented Apr 30, 2025

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

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

doug-q commented Apr 29, 2025 •

edited

Loading

hugrbot commented Apr 29, 2025 •

edited

Loading

codecov bot commented Apr 29, 2025 •

edited

Loading

acl-cqc left a comment •

edited

Loading