fix: add op's extension to signature check in `resolve_opaque_op` #1317

doug-q · 2024-07-16T13:44:32Z

We also remove an unsafe unwrap in resolve_opaque_op while we are here.

Co-authored-by: Alec Edgington <[email protected]>

codecov · 2024-07-16T15:03:38Z

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.46%. Comparing base (b832274) to head (13d898c).

Files	Patch %	Lines
hugr-core/src/ops/custom.rs	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1317      +/-   ##
==========================================
+ Coverage   87.31%   87.46%   +0.14%     
==========================================
  Files         108      107       -1     
  Lines       19784    19045     -739     
  Branches    17520    16782     -738     
==========================================
- Hits        17275    16658     -617     
+ Misses       1724     1680      -44     
+ Partials      785      707      -78

Flag	Coverage Δ
python	`91.42% <100.00%> (-0.01%)`	⬇️

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.

acl-cqc · 2024-07-16T15:16:35Z

hugr-core/src/ops/custom.rs

-            ExtensionOp::new(def.clone(), opaque.args.clone(), extension_registry).unwrap();
-        if opaque.signature != ext_op.signature {
+        let ext_op = ExtensionOp::new(def.clone(), opaque.args.clone(), extension_registry)?;
+        // ops always require their own extensions, so we do not force OpaqueOps


The comment I imagined was "OpaqueOps serialized prior to PR #1226 did not include their own extension. Add it manually for backwards compatibility when deserializing such early Hugr's".

Of course, my version doesn't allow that not all OpaqueOps result from serializing known ExtensionOps - you can also write OpaqueOps out by hand with any extension delta you like. So I guess your comment is fine**, but might be worth adding Especially for backwards compatibility with Hugrs serialized before PR#1226, say.

** indeed better, unless we want to make OpaqueOps automatically include such...

I have added some comments, see what you think. I think it's better for OpaqueOps not to include their extension in their serialised signature, just for smaller serialisation. I haven't made that change.

acl-cqc · 2024-07-16T15:18:29Z

hugr-core/src/ops/custom.rs

-        let ext_op =
-            ExtensionOp::new(def.clone(), opaque.args.clone(), extension_registry).unwrap();
-        if opaque.signature != ext_op.signature {
+        let ext_op = ExtensionOp::new(def.clone(), opaque.args.clone(), extension_registry)?;


The change from unwrap to ? here necessitates the addition of CustomOpError::SignatureError? Good move :)

acl-cqc

Looks good to me, thanks @doug-q

This reverts commit 026bfcb.

doug-q · 2024-07-17T07:01:44Z

I've reworked my approach, which uncovered two test failures which I have fixed. I have also, perhaps controversially, reverted #1303

acl-cqc

Yes, I think the adding in OpaqueOp is a good approach :)

hugr-core/src/ops/custom.rs

acl-cqc · 2024-07-17T09:08:47Z

hugr-py/src/hugr/std/float.py

 FLOAT_T = tys.Opaque(
-    extension=FLOAT_EXT_ID,
+    extension="arithmetic.float.types",


nit: don't really see any need to revert here

acl-cqc · 2024-07-17T09:09:59Z

hugr-core/src/hugr/rewrite/replace.rs

+                unknown_ext.clone(),
+            )
+            .unwrap();
+        let mut case1 = cond.case_builder(0).unwrap();


Why ? -> unwrap everywhere? It does make things a lot longer and look like you are changing more than you are.

unwrap is much better than ? in tests because you get a src location on failure. I changed them all to be able to fix the test and thought I might as well leave it in a debuggable state.

Co-authored-by: Alan Lawrence <[email protected]>

…esolve_opaque_op

## 🤖 New release * `hugr`: 0.8.0 -> 0.9.0 * `hugr-core`: 0.5.0 -> 0.6.0 * `hugr-passes`: 0.5.0 -> 0.6.0 * `hugr-cli`: 0.1.4 -> 0.2.0 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.9.0 (2024-07-19) ### Bug Fixes - Add op's extension to signature check in `resolve_opaque_op` ([#1317](#1317)) - Panic on `SimpleReplace` with multiports ([#1324](#1324)) ### Refactor - [**breaking**] Separate Signature from FuncValueType by parametrizing Type(/Row)/etc. ([#1138](#1138)) ### Testing - Verify order edges ([#1293](#1293)) - Add failing test case for [#1315](#1315) ([#1316](#1316)) </blockquote> ## `hugr-core` <blockquote> ## 0.6.0 (2024-07-19) ### Bug Fixes - Add op's extension to signature check in `resolve_opaque_op` ([#1317](#1317)) - Panic on `SimpleReplace` with multiports ([#1324](#1324)) ### Refactor - [**breaking**] Separate Signature from FuncValueType by parametrizing Type(/Row)/etc. ([#1138](#1138)) ### Testing - Verify order edges ([#1293](#1293)) - Add failing test case for [#1315](#1315) ([#1316](#1316)) </blockquote> ## `hugr-passes` <blockquote> ## 0.6.0 (2024-07-19) ### Refactor - [**breaking**] Separate Signature from FuncValueType by parametrizing Type(/Row)/etc. ([#1138](#1138)) </blockquote> ## `hugr-cli` <blockquote> ## 0.2.0 (2024-07-19) ### Refactor - [**breaking**] Separate Signature from FuncValueType by parametrizing Type(/Row)/etc. ([#1138](#1138)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Douglas Wilson <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [0.5.0](hugr-py-v0.4.0...hugr-py-v0.5.0) (2024-07-29) ### ⚠ BREAKING CHANGES * Eq type bound removed. References to `Eq` in serialized HUGRs will be treated as `Copyable`. * **hugr-core:** All Hugrs serialised with earlier versions will fail to deserialise * opaque type parameters replaced with string parameters. ### Features * **hugr-py:** `AsCustomOp` protocol for user-defined custom op types. ([#1290](#1290)) ([1db43eb](1db43eb)) * remove the `Eq` type bound. ([#1364](#1364)) ([1218d21](1218d21)) * replace opaque type arguments with String ([#1328](#1328)) ([24b2217](24b2217)), closes [#1308](#1308) * Serialization upgrade path ([#1327](#1327)) ([d493139](d493139)) ### Bug Fixes * add op's extension to signature check in `resolve_opaque_op` ([#1317](#1317)) ([01da7ba](01da7ba)) * **hugr-core:** bump serialisation version with no upgrade path ([#1352](#1352)) ([657cbb0](657cbb0)) * **hugr-py:** ops require their own extensions ([#1303](#1303)) ([026bfcb](026bfcb)), closes [#1301](#1301) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

doug-q and others added 5 commits July 16, 2024 14:23

test: Add failing test case for #1315

4f3cda8

add comment

7948129

Update hugr-core/src/ops/custom.rs

c520ece

Co-authored-by: Alec Edgington <[email protected]>

revert dup comment

d03d990

fix: add op's extension to signature check in resolve_opaque_op

143e507

doug-q requested a review from a team as a code owner July 16, 2024 13:44

doug-q requested a review from mark-koch July 16, 2024 13:44

doug-q changed the base branch from main to doug/test-1315 July 16, 2024 13:44

doug-q linked an issue Jul 16, 2024 that may be closed by this pull request

resolve_opaque_op fails when OpaqueOp does not explicitly require it's own extension. #1315

Closed

doug-q added 2 commits July 16, 2024 14:45

fmt

dd3bc4b

un-should_panic test

865ecfb

Base automatically changed from doug/test-1315 to main July 16, 2024 13:49

Merge branch 'main' into doug/resolve_opaque_op

118152d

acl-cqc reviewed Jul 16, 2024

View reviewed changes

acl-cqc approved these changes Jul 16, 2024

View reviewed changes

doug-q added 3 commits July 17, 2024 07:26

rework

3742ddd

Revert "fix(hugr-py): ops require their own extensions (#1303)"

db6534b

This reverts commit 026bfcb.

fix with_nonlinear_and_outputs and test_invalid

e119e26

doug-q requested a review from acl-cqc July 17, 2024 07:00

doug-q added 2 commits July 17, 2024 08:03

tidy

b5008f0

restore extension_reqs arg for endo

f319896

acl-cqc approved these changes Jul 17, 2024

View reviewed changes

doug-q and others added 4 commits July 17, 2024 11:26

Update hugr-core/src/ops/custom.rs

0aa0a30

Co-authored-by: Alan Lawrence <[email protected]>

put back improvement from reverted commit

1a5f330

Merge branch 'main' of gh:CQCL-DEV/hugr into doug/resolve_opaque_op

a0a0cab

Merge branch 'doug/resolve_opaque_op' of gh:CQCL-DEV/hugr into doug/r…

13d898c

…esolve_opaque_op

doug-q enabled auto-merge July 17, 2024 11:57

doug-q added this pull request to the merge queue Jul 17, 2024

Merged via the queue into main with commit 01da7ba Jul 17, 2024
22 checks passed

doug-q deleted the doug/resolve_opaque_op branch July 17, 2024 12:00

This was referenced Jul 17, 2024

chore(main): release hugr-py 0.5.0 #1291

Merged

chore: release #1314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add op's extension to signature check in `resolve_opaque_op` #1317

fix: add op's extension to signature check in `resolve_opaque_op` #1317

doug-q commented Jul 16, 2024

codecov bot commented Jul 16, 2024 •

edited

Loading

acl-cqc Jul 16, 2024 •

edited

Loading

doug-q Jul 17, 2024

acl-cqc Jul 16, 2024

acl-cqc left a comment

doug-q commented Jul 17, 2024

acl-cqc left a comment

acl-cqc Jul 17, 2024

acl-cqc Jul 17, 2024

doug-q Jul 17, 2024

fix: add op's extension to signature check in resolve_opaque_op #1317

fix: add op's extension to signature check in resolve_opaque_op #1317

Conversation

doug-q commented Jul 16, 2024

codecov bot commented Jul 16, 2024 • edited Loading

Codecov Report

acl-cqc Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

doug-q Jul 17, 2024

Choose a reason for hiding this comment

acl-cqc Jul 16, 2024

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

doug-q commented Jul 17, 2024

acl-cqc left a comment

Choose a reason for hiding this comment

acl-cqc Jul 17, 2024

Choose a reason for hiding this comment

acl-cqc Jul 17, 2024

Choose a reason for hiding this comment

doug-q Jul 17, 2024

Choose a reason for hiding this comment

fix: add op's extension to signature check in `resolve_opaque_op` #1317

fix: add op's extension to signature check in `resolve_opaque_op` #1317

codecov bot commented Jul 16, 2024 •

edited

Loading

acl-cqc Jul 16, 2024 •

edited

Loading