-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Constant values in hugr-model
#1838
Conversation
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
2bc0a18
to
565e18a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1838 +/- ##
==========================================
- Coverage 86.67% 86.55% -0.13%
==========================================
Files 194 194
Lines 35010 35156 +146
Branches 31823 31969 +146
==========================================
+ Hits 30346 30430 +84
- Misses 2939 2968 +29
- Partials 1725 1758 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks good, biggest question is about ->
placement
node_to_id: FxHashMap<Node, model::NodeId>, | ||
|
||
/// Mapping from node ids in the [`Hugr`] to the corresponding model nodes. | ||
id_to_node: FxHashMap<model::NodeId, Node>, |
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.
if these are meant to stay in sync would be better to have a dedicated struct holding both that has responsibility for their correctness.
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.
Ideally yes. I'm not entirely sure yet if the way that functions are exported is ideal here, so there might be a bit of churn around these things happening. In that light I'm not sure how much it is worth to polish.
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.
leave a todo perhaps
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.
TODO added.
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.
LGTM modulo CI fixes 👍
4cc2572
to
3a50604
Compare
## 🤖 New release * `hugr`: 0.14.1 -> 0.14.2 (✓ API compatible changes) * `hugr-core`: 0.14.1 -> 0.14.2 (✓ API compatible changes) * `hugr-model`: 0.16.0 -> 0.17.0 (⚠️ API breaking changes) * `hugr-llvm`: 0.14.1 -> 0.14.2 (✓ API compatible changes) * `hugr-passes`: 0.14.1 -> 0.14.2 (✓ API compatible changes) * `hugr-cli`: 0.14.1 -> 0.14.2 (✓ API compatible changes) ###⚠️ `hugr-model` breaking changes ``` --- failure enum_variant_added: enum variant added on exhaustive enum --- Description: A publicly-visible enum without #[non_exhaustive] has a new variant. ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_variant_added.ron Failed in: variant Term:Const in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:591 variant Term:ConstFunc in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:697 variant Term:ConstAdt in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:703 variant Term:Bytes in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:711 variant Term:BytesType in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:717 variant Term:Meta in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:720 variant Operation:Const in /tmp/.tmpszSQ4r/hugr/hugr-model/src/v0/mod.rs:409 --- failure enum_variant_missing: pub enum variant removed or renamed --- Description: A publicly-visible enum has at least one variant that 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.38.0/src/lints/enum_variant_missing.ron Failed in: variant Term::Quote, previously in file /tmp/.tmp8fke0a/hugr-model/src/v0/mod.rs:565 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct 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.38.0/src/lints/struct_missing.ron Failed in: struct hugr_model::v0::MetaItem, previously in file /tmp/.tmp8fke0a/hugr-model/src/v0/mod.rs:500 ``` <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr` <blockquote> ## [0.14.2](hugr-v0.14.1...hugr-v0.14.2) - 2025-01-20 ### Bug Fixes - Three bugfixes in model import and export. (#1844) ### Documentation - Fix typo in `DataflowParent` doc (#1865) ### New Features - Add CallGraph struct, and dead-function-removal pass (#1796) - `Value::some`, `::none`, and `SumType::new_option` helpers (#1828) - Constant values in `hugr-model` (#1838) - *(hugr-llvm)* Emit ipow (#1839) - Bytes literal in hugr-model. (#1845) - Improved representation for metadata in `hugr-model` (#1849) ### Testing - Add tests for constant value deserialization (#1822) </blockquote> ## `hugr-core` <blockquote> ## [0.14.2](hugr-core-v0.14.1...hugr-core-v0.14.2) - 2025-01-20 ### Bug Fixes - Three bugfixes in model import and export. (#1844) ### Documentation - Fix typo in `DataflowParent` doc (#1865) ### New Features - `Value::some`, `::none`, and `SumType::new_option` helpers (#1828) - Constant values in `hugr-model` (#1838) - *(hugr-llvm)* Emit ipow (#1839) - Bytes literal in hugr-model. (#1845) - Improved representation for metadata in `hugr-model` (#1849) ### Testing - Add tests for constant value deserialization (#1822) </blockquote> ## `hugr-model` <blockquote> ## [0.17.0](hugr-model-v0.16.0...hugr-model-v0.17.0) - 2025-01-20 ### Bug Fixes - Three bugfixes in model import and export. (#1844) ### New Features - Constant values in `hugr-model` (#1838) - Bytes literal in hugr-model. (#1845) - Improved representation for metadata in `hugr-model` (#1849) </blockquote> ## `hugr-llvm` <blockquote> ## [0.14.2](hugr-llvm-v0.14.1...hugr-llvm-v0.14.2) - 2025-01-20 ### New Features - *(hugr-llvm)* Emit more int ops (#1835) - Constant values in `hugr-model` (#1838) - *(hugr-llvm)* Emit ipow (#1839) ### Refactor - *(hugr-llvm)* [**breaking**] Optimise the llvm types used to represent hugr sums. (#1855) ### Testing - Fix failing inot test (#1841) </blockquote> ## `hugr-passes` <blockquote> ## [0.14.2](hugr-passes-v0.14.1...hugr-passes-v0.14.2) - 2025-01-20 ### New Features - Add CallGraph struct, and dead-function-removal pass (#1796) </blockquote> ## `hugr-cli` <blockquote> ## [0.14.1](hugr-cli-v0.14.0...hugr-cli-v0.14.1) - 2024-12-18 ### New Features - Print `hugr-cli`'s correct version when using '--version' (#1790) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: Agustín Borgna <[email protected]>
Import and export constant values via
hugr-model
.Includes terms corresponding to
Value::Function
and allows exporting; importing these is left for a later PR since sum/extension constants are the most important for now. In the future, we should move from the JSON/typetag based model for extension constants to one based on custom constructors.