-
Notifications
You must be signed in to change notification settings - Fork 13
fix: Preserve offset for CFG edges when serializing to JSON #2606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2606 +/- ##
=======================================
Coverage 83.07% 83.07%
=======================================
Files 255 255
Lines 48422 48436 +14
Branches 43932 43946 +14
=======================================
+ Hits 40225 40240 +15
Misses 6103 6103
+ Partials 2094 2093 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hugr-core/src/hugr/serialize.rs
Outdated
| let is_cfg_edge = op.is_dataflow_block(); | ||
| let offset = (is_value_port || is_static_input || is_cfg_edge).then_some(offset as u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too restraining.
The bug happens any time we have more than one non-df ports, so it would be safer to do
| let is_cfg_edge = op.is_dataflow_block(); | |
| let offset = (is_value_port || is_static_input || is_cfg_edge).then_some(offset as u32); | |
| let multiple_other_ports = op.non_df_port_count(dir) > 1; | |
| let offset = (is_value_port || is_static_input || multiple_other_ports).then_some(offset as u32); |
Alternatively, always encode the offset 🤷
It's not a breaking serialization change either way since the decoding logic works fine if the offset is already defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my first idea was to encode the offset always, but this broke some round-trip tests in hugr-py for hugrs containing order edges.
I will try your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, seems like hugr-py uses None only for order edges. (The fn comment seems wrong here, only order edges have offset -1)
hugr/hugr-py/src/hugr/hugr/base.py
Lines 971 to 980 in 70ee1cb
| def _constrain_offset(self, p: P) -> PortOffset | None: | |
| """Constrain an offset to be a valid encoded port offset. | |
| Order edges and control flow edges should be encoded without an offset. | |
| """ | |
| if p.offset < 0: | |
| assert p.offset == -1, "Only order edges are allowed with offset < 0" | |
| return None | |
| else: | |
| return p.offset |
If we wanted to follow that here then we can check
let other_port_is_not_order = op.other_port_kind(dir) != EdgeKind::StateOrder;
let offset = (is_value_port || is_static_input || other_port_is_not_order).then_some(offset as u32);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, thanks. I've gone with your second suggestion. I tried the first, but ran into some round-tripping failures using testfiles exported by guppylang. With this second idea, all seems good. (Also checked that it still fixes #2592 .)
## 🤖 New release * `hugr-model`: 0.23.0 -> 0.24.0 * `hugr-core`: 0.23.0 -> 0.24.0 (✓ API compatible changes) * `hugr-llvm`: 0.23.0 -> 0.24.0 (✓ API compatible changes) * `hugr-passes`: 0.23.0 -> 0.24.0 (✓ API compatible changes) * `hugr-persistent`: 0.3.0 -> 0.3.1 (✓ API compatible changes) * `hugr`: 0.23.0 -> 0.24.0 (✓ API compatible changes) * `hugr-cli`: 0.23.0 -> 0.24.0 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr-model` <blockquote> ## [0.23.0](hugr-model-v0.22.4...hugr-model-v0.23.0) - 2025-09-30 ### Bug Fixes - [**breaking**] Appease `cargo-audit` by replacing unmaintained dependencies ([#2572](#2572)) ### New Features - Documentation and error hints ([#2523](#2523)) </blockquote> ## `hugr-core` <blockquote> ## [0.24.0](hugr-core-v0.23.0...hugr-core-v0.24.0) - 2025-10-13 ### Bug Fixes - Preserve offset for CFG edges when serializing to JSON ([#2606](#2606)) ### New Features - LLVM lowering for borrow arrays using bitmasks ([#2574](#2574)) - *(py, core, llvm)* add `is_borrowed` op for BorrowArray ([#2610](#2610)) ### Refactor - [**breaking**] consistent inout order in borrow array ([#2621](#2621)) </blockquote> ## `hugr-llvm` <blockquote> ## [0.24.0](hugr-llvm-v0.23.0...hugr-llvm-v0.24.0) - 2025-10-13 ### New Features - LLVM lowering for borrow arrays using bitmasks ([#2574](#2574)) - *(py, core, llvm)* add `is_borrowed` op for BorrowArray ([#2610](#2610)) ### Refactor - [**breaking**] consistent inout order in borrow array ([#2621](#2621)) </blockquote> ## `hugr-passes` <blockquote> ## [0.24.0](hugr-passes-v0.23.0...hugr-passes-v0.24.0) - 2025-10-13 ### New Features - Add handler for copying / discarding borrow arrays to default lineariser ([#2602](#2602)) </blockquote> ## `hugr-persistent` <blockquote> ## [0.3.1](hugr-persistent-v0.3.0...hugr-persistent-v0.3.1) - 2025-10-13 ### Bug Fixes - *(test)* No extension serialisation in persistent-hugr testing ([#2612](#2612)) ### New Features - *(persistent)* Redesign CommitStateSpace, bound Commit lifetime ([#2534](#2534)) </blockquote> ## `hugr` <blockquote> ## [0.24.0](hugr-v0.23.0...hugr-v0.24.0) - 2025-10-13 ### Bug Fixes - Preserve offset for CFG edges when serializing to JSON ([#2606](#2606)) ### New Features - Add handler for copying / discarding borrow arrays to default lineariser ([#2602](#2602)) - LLVM lowering for borrow arrays using bitmasks ([#2574](#2574)) - *(py, core, llvm)* add `is_borrowed` op for BorrowArray ([#2610](#2610)) ### Refactor - [**breaking**] consistent inout order in borrow array ([#2621](#2621)) </blockquote> ## `hugr-cli` <blockquote> ## [0.24.0](hugr-cli-v0.23.0...hugr-cli-v0.24.0) - 2025-10-13 ### Documentation - update cli readme ([#2601](#2601)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
🤖 I have created a release *beep* *boop* --- ## [0.14.0](hugr-py-v0.13.1...hugr-py-v0.14.0) (2025-10-13) ### ⚠ BREAKING CHANGES * BorrowArray::{borrow, is_borrowed} return types have been swapped such that the array is first. ### Features * Add more options to `DotRenderer` config ([#2540](#2540)) ([45f7573](45f7573)) * Allow importing from model data to python. ([#2581](#2581)) ([4fb0a5e](4fb0a5e)) * **py, core, llvm:** add `is_borrowed` op for BorrowArray ([#2610](#2610)) ([1cd08ef](1cd08ef)), closes [#2569](#2569) ### Bug Fixes * Preserve offset for CFG edges when serializing to JSON ([#2606](#2606)) ([69a126d](69a126d)) ### Code Refactoring * consistent inout order in borrow array ([#2621](#2621)) ([8fc59f3](8fc59f3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
Fixes #2592 . (Verified by regenerating JSON file using CLI built on this branch and rerunning test code.)
Fixes #2600 . (Verified by test included here.)
I don't think this counts as a breaking change to the JSON serialization format.