feat!: `MakeOpDef` has new `extension` method. #1266

ss2165 · 2024-07-05T14:57:05Z

Used to ensure try_from_name only succeeds when the extension is correct

BREAKING CHANGE:

MakeOpDef trait has new required extension method.
try_from_name takes the OpDef extension and checks it

Used to ensure `try_from_name` only succeeds when the extension is correct BREAKING CHANGE: - `MakeOpDef` trait has new required `extension_id` static method. - `try_from_name` takes an `&OpDef` rather than just names and checks the extension is expected

codecov · 2024-07-05T15:02:39Z

Codecov Report

Attention: Patch coverage is 42.42424% with 19 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (defd2bb) to head (8059d6c).
Report is 2 commits behind head on main.

Files	Patch %	Lines
hugr-core/src/extension/simple_op.rs	42.85%	7 Missing and 1 partial ⚠️
...-core/src/std_extensions/arithmetic/conversions.rs	0.00%	4 Missing ⚠️
...gr-core/src/std_extensions/arithmetic/float_ops.rs	0.00%	4 Missing ⚠️
hugr-core/src/std_extensions/logic.rs	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1266      +/-   ##
==========================================
- Coverage   87.15%   87.09%   -0.07%     
==========================================
  Files         103      103              
  Lines       19284    19311      +27     
  Branches    17136    17163      +27     
==========================================
+ Hits        16807    16818      +11     
- Misses       1699     1715      +16     
  Partials      778      778

Flag	Coverage Δ
rust	`86.56% <42.42%> (-0.08%)`	⬇️

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.

doug-q

I find this code(that was already here!) quite confusing, I may be off track here, but nevertheless:

All callsites of try_from_name get it out of an opdef. All implementations of MakeOpDef::from_def delegate to try_from_name.

How about:

try_from_name takes an &ExtensionId and a impl FromStr.
extension_id gets a &self parameter

from_def implementations are changed to pass the ExtensionId they each have access to.

This breaks from_def implementations that were previously not checking the extension(all of them).

It means types that implement MakeOpDef are free to be used with multiple extensions. As they were before.

I do think I weakly prefer putting extension_id on NamedOp, but this is also fine.

doug-q · 2024-07-05T15:16:28Z

hugr-core/src/extension/simple_op.rs

@@ -138,10 +143,19 @@ impl<T: MakeOpDef> MakeExtensionOp for T {

 /// Load an [MakeOpDef] from its name.
 /// See [strum_macros::EnumString].
-pub fn try_from_name<T>(name: &OpNameRef) -> Result<T, OpLoadError>
+pub fn try_from_name<T>(def: &OpDef) -> Result<T, OpLoadError>


I'm not sure this change makes sense. we already have MakeOpDef::from_def

from_def is abstract, MakeOpDef implementers need to have some way of loading themselves from a definition. It so happens that many can load themselves from (name, extension) because they implement FromStr, so we use a specific function for that case (try_from_name).

i dont mind passing name and extension explicitly like you suggest, so I've done that anyway. It's a bit clearer what the job of the function is.

doug-q · 2024-07-05T15:25:36Z

hugr-core/src/extension/simple_op.rs

@@ -51,6 +53,9 @@ pub trait MakeOpDef: NamedOp {
    /// Return the signature (polymorphic function type) of the operation.
    fn signature(&self) -> SignatureFunc;

+    /// The ID of the extension this operation is defined in.
+    fn extension_id() -> ExtensionId;


This should have where Self: Sized to maintain object-safety.

taking &self is a good idea, going with that

ss2165 · 2024-07-05T15:43:09Z

I find this code(that was already here!) quite confusing

Agreed, so do I, which is a lot worse given I wrote it. A clean-up is in order but don't want to do that in this PR.

doug-q

Let's rename expected_extension -> extension. I have some op: &impl MakeOpDef, i can ask it it's extension: op.extension().

doug-q · 2024-07-08T12:05:55Z

hugr-core/src/extension/simple_op.rs

@@ -51,6 +53,9 @@ pub trait MakeOpDef: NamedOp {
    /// Return the signature (polymorphic function type) of the operation.
    fn signature(&self) -> SignatureFunc;

+    /// The ID of the extension this operation is defined in.
+    fn expected_extension(&self) -> ExtensionId;


Suggested change

fn expected_extension(&self) -> ExtensionId;

fn extension(&self) -> ExtensionId;

doug-q

Thanks. I've taken the liberty of updating the PR title and commit message.

## 🤖 New release * `hugr`: 0.6.1 -> 0.7.0 * `hugr-core`: 0.3.1 -> 0.4.0 * `hugr-passes`: 0.3.0 -> 0.4.0 * `hugr-cli`: 0.1.2 -> 0.1.3 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.7.0 (2024-07-10) ### Bug Fixes - Bring back input_extensions serialized field in rust NodeSer ([#1275](#1275)) - [**breaking**] `ops::Module` now empty struct rather than unit struct ([#1271](#1271)) ### Features - Add `force_order` pass. ([#1285](#1285)) - [**breaking**] `MakeOpDef` has new `extension` method. ([#1266](#1266)) ### Refactor - [**breaking**] Remove `Value::Tuple` ([#1255](#1255)) - [**breaking**] Rename `HugrView` function type methods + simplify behaviour ([#1265](#1265)) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) ### Testing - Add a test for [#1257](#1257) ([#1260](#1260)) </blockquote> ## `hugr-core` <blockquote> ## 0.4.0 (2024-07-10) ### Bug Fixes - Bring back input_extensions serialized field in rust NodeSer ([#1275](#1275)) - [**breaking**] `ops::Module` now empty struct rather than unit struct ([#1271](#1271)) ### Features - [**breaking**] `MakeOpDef` has new `extension` method. ([#1266](#1266)) ### Refactor - [**breaking**] Remove `Value::Tuple` ([#1255](#1255)) - [**breaking**] Rename `HugrView` function type methods + simplify behaviour ([#1265](#1265)) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) ### Testing - Add a test for [#1257](#1257) ([#1260](#1260)) </blockquote> ## `hugr-passes` <blockquote> ## 0.4.0 (2024-07-10) ### Features - Add `force_order` pass. ([#1285](#1285)) ### Refactor - [**breaking**] Remove `Value::Tuple` ([#1255](#1255)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.3 (2024-07-10) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).

ss2165 requested a review from doug-q July 5, 2024 14:57

ss2165 requested a review from a team as a code owner July 5, 2024 14:57

doug-q reviewed Jul 5, 2024

View reviewed changes

MakeOpDef::extension not static

111a0df

ss2165 requested a review from doug-q July 5, 2024 16:07

ss2165 changed the title ~~feat!: MakeOpDef has new extension_id static method.~~ feat!: MakeOpDef has new extension_id method. Jul 5, 2024

doug-q reviewed Jul 8, 2024

View reviewed changes

rename expected_extension -> extension

8059d6c

ss2165 requested a review from doug-q July 8, 2024 12:24

doug-q changed the title ~~feat!: MakeOpDef has new extension_id method.~~ feat!: MakeOpDef has new extension method. Jul 8, 2024

doug-q approved these changes Jul 8, 2024

View reviewed changes

ss2165 added this pull request to the merge queue Jul 8, 2024

Merged via the queue into main with commit 75192f7 Jul 8, 2024
21 of 22 checks passed

ss2165 deleted the ss/check-extension branch July 8, 2024 12:32

hugrbot mentioned this pull request Jul 8, 2024

chore: release #1248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: `MakeOpDef` has new `extension` method. #1266

feat!: `MakeOpDef` has new `extension` method. #1266

ss2165 commented Jul 5, 2024 •

edited by doug-q

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

doug-q left a comment •

edited

Loading

doug-q Jul 5, 2024

ss2165 Jul 5, 2024

doug-q Jul 5, 2024

ss2165 Jul 5, 2024

ss2165 commented Jul 5, 2024

doug-q left a comment

doug-q Jul 8, 2024

doug-q left a comment

	fn expected_extension(&self) -> ExtensionId;
	fn extension(&self) -> ExtensionId;

feat!: MakeOpDef has new extension method. #1266

feat!: MakeOpDef has new extension method. #1266

Conversation

ss2165 commented Jul 5, 2024 • edited by doug-q Loading

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

doug-q left a comment • edited Loading

Choose a reason for hiding this comment

doug-q Jul 5, 2024

Choose a reason for hiding this comment

ss2165 Jul 5, 2024

Choose a reason for hiding this comment

doug-q Jul 5, 2024

Choose a reason for hiding this comment

ss2165 Jul 5, 2024

Choose a reason for hiding this comment

ss2165 commented Jul 5, 2024

doug-q left a comment

Choose a reason for hiding this comment

doug-q Jul 8, 2024

Choose a reason for hiding this comment

doug-q left a comment

Choose a reason for hiding this comment

feat!: `MakeOpDef` has new `extension` method. #1266

feat!: `MakeOpDef` has new `extension` method. #1266

ss2165 commented Jul 5, 2024 •

edited by doug-q

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

doug-q left a comment •

edited

Loading