-
Notifications
You must be signed in to change notification settings - Fork 13
fix!: model import loads Package extensions #2590
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2590 +/- ##
==========================================
+ Coverage 82.94% 82.95% +0.01%
==========================================
Files 254 254
Lines 47916 47915 -1
Branches 43427 43426 -1
==========================================
+ Hits 39743 39749 +6
+ Misses 6109 6103 -6
+ Partials 2064 2063 -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:
|
4e7736b to
2f8c8b9
Compare
acl-cqc
left a comment
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 to me, a couple of suggestions...
| packaged_extensions: ExtensionRegistry, | ||
| loaded_extensions: &ExtensionRegistry, | ||
| ) -> Result<Package, ImportError> { | ||
| let mut registry = loaded_extensions.clone(); |
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.
Reading through the rest of this I kept thinking, why are we not building this registry earlier....now I see (below) that we need to keep them separate so we can store the correct one in the Package, so ok.
Should we return this new/merged ExtensionRegistry, though?
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.
The extension resolution forms a merged registry of all the extensions actually used in the HUGR from this larger set of packaged + loaded, that is stored in Hugr.extensions
See the PR this is based on:
#2588
hugr-core/tests/model.rs
Outdated
| ); | ||
|
|
||
| #[test] | ||
| fn import_package_with_extensions() { |
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 it's plausible to make a roundtrip test where we serialize to hugr-model + extensions, and then import again (such that the roundtrip would have failed without this PR), I think that would be better?
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.
thanks, done
86b8f05 to
29d2fbc
Compare
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.
Should this empty file be here?
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.
thanks
29d2fbc to
3db03df
Compare
Closes #2589 BREAKING CHANGE: import_package takes packaged extensions argument
3db03df to
8b1f3d5
Compare
## 🤖 New release
* `hugr-model`: 0.22.4 -> 0.23.0 (✓ API compatible changes)
* `hugr-core`: 0.22.4 -> 0.23.0 (⚠ API breaking changes)
* `hugr-llvm`: 0.22.4 -> 0.23.0 (✓ API compatible changes)
* `hugr-passes`: 0.22.4 -> 0.23.0 (⚠ API breaking changes)
* `hugr-persistent`: 0.2.3 -> 0.3.0 (✓ API compatible changes)
* `hugr`: 0.22.4 -> 0.23.0 (✓ API compatible changes)
* `hugr-cli`: 0.22.4 -> 0.23.0 (✓ API compatible changes)
### ⚠ `hugr-core` breaking changes
```text
--- 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.43.0/src/lints/enum_variant_missing.ron
Failed in:
variant PackageEncodingError::ExtensionVersion, previously in file /tmp/.tmp8eoQo1/hugr-core/src/envelope/package_json.rs:89
--- failure function_parameter_count_changed: pub fn parameter count changed ---
Description:
A publicly-visible function now takes a different number of parameters.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.43.0/src/lints/function_parameter_count_changed.ron
Failed in:
hugr_core::import::import_package now takes 3 parameters instead of 2, in /tmp/.tmpBTXSed/hugr/hugr-core/src/import.rs:187
--- 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.43.0/src/lints/struct_missing.ron
Failed in:
struct hugr_core::extension::prelude::PRELUDE_REGISTRY, previously in file /tmp/.tmp8eoQo1/hugr-core/src/extension/prelude.rs:43
struct hugr_core::extension::PRELUDE_REGISTRY, previously in file /tmp/.tmp8eoQo1/hugr-core/src/extension/prelude.rs:43
struct hugr_core::std_extensions::logic::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/logic.rs:134
struct hugr_core::extension::prelude::PRELUDE, previously in file /tmp/.tmp8eoQo1/hugr-core/src/extension/prelude.rs:43
struct hugr_core::extension::PRELUDE, previously in file /tmp/.tmp8eoQo1/hugr-core/src/extension/prelude.rs:43
struct hugr_core::std_extensions::ptr::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/ptr.rs:112
struct hugr_core::std_extensions::arithmetic::int_types::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/arithmetic/int_types.rs:209
struct hugr_core::std_extensions::arithmetic::conversions::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/arithmetic/conversions.rs:174
struct hugr_core::std_extensions::collections::array::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/collections/array.rs:93
struct hugr_core::std_extensions::arithmetic::int_types::INT_TYPES, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/arithmetic/int_types.rs:49
struct hugr_core::std_extensions::arithmetic::float_types::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/arithmetic/float_types.rs:104
struct hugr_core::std_extensions::collections::static_array::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/collections/static_array.rs:142
struct hugr_core::std_extensions::collections::list::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/collections/list.rs:289
struct hugr_core::std_extensions::STD_REG, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions.rs:35
struct hugr_core::std_extensions::collections::value_array::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/collections/value_array.rs:99
struct hugr_core::std_extensions::collections::borrow_array::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/collections/borrow_array.rs:290
struct hugr_core::std_extensions::arithmetic::float_ops::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/arithmetic/float_ops.rs:115
struct hugr_core::std_extensions::arithmetic::int_ops::EXTENSION, previously in file /tmp/.tmp8eoQo1/hugr-core/src/std_extensions/arithmetic/int_ops.rs:254
```
### ⚠ `hugr-passes` breaking changes
```text
--- failure enum_missing: pub enum removed or renamed ---
Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum 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.43.0/src/lints/enum_missing.ron
Failed in:
enum hugr_passes::non_local::NonLocalEdgesError, previously in file /tmp/.tmp8eoQo1/hugr-passes/src/non_local.rs:57
```
<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.23.0](hugr-core-v0.22.4...hugr-core-v0.23.0)
- 2025-09-30
### Bug Fixes
- [**breaking**] Appease `cargo-audit` by replacing unmaintained
dependencies ([#2572](#2572))
- *(core)* check extension versions on model import
([#2580](#2580))
- [**breaking**] test extension version compatibility on ModelWithExts
([#2587](#2587))
- *(core)* check used extension versions against resolved extensions
([#2588](#2588))
- [**breaking**] model import loads Package extensions
([#2590](#2590))
### Miscellaneous Tasks
- [**breaking**] Cleanup deprecated definitions
([#2594](#2594))
### New Features
- add trait+funcs for linking Hugrs explicitly by Node
([#2521](#2521))
- Documentation and error hints
([#2523](#2523))
- Allow creating DFG builders from existing hugrs
([#2562](#2562))
- add_input/output for arbitrary DFGBuilders
([#2564](#2564))
- [**breaking**] Return error instead of panicking in
DFGWrapper::add_{in,out}put
([#2571](#2571))
- *(core)* inner acccesors for WithGenerator error
([#2583](#2583))
- Normalize CFGs ([#2591](#2591))
### Refactor
- [**breaking**] Replace lazy_static with std::sync::LazyLock
([#2567](#2567))
</blockquote>
## `hugr-llvm`
<blockquote>
##
[0.23.0](hugr-llvm-v0.22.4...hugr-llvm-v0.23.0)
- 2025-09-30
### Miscellaneous Tasks
- [**breaking**] Cleanup deprecated definitions
([#2594](#2594))
### Refactor
- [**breaking**] Replace lazy_static with std::sync::LazyLock
([#2567](#2567))
### Testing
- Add framework for LLVM execution tests involving panics
([#2568](#2568))
</blockquote>
## `hugr-passes`
<blockquote>
##
[0.23.0](hugr-passes-v0.22.4...hugr-passes-v0.23.0)
- 2025-09-30
### Bug Fixes
- DeadCodeElim keeps consumers of linear outputs
([#2560](#2560))
- [**breaking**] Appease `cargo-audit` by replacing unmaintained
dependencies ([#2572](#2572))
### Miscellaneous Tasks
- [**breaking**] Cleanup deprecated definitions
([#2594](#2594))
### New Features
- [**breaking**] DeadCodeElimPass reports error on non-existent
entry_points ([#2566](#2566))
- Normalize CFGs ([#2591](#2591))
### Refactor
- [**breaking**] Replace lazy_static with std::sync::LazyLock
([#2567](#2567))
</blockquote>
## `hugr-persistent`
<blockquote>
##
[0.3.0](hugr-persistent-v0.2.3...hugr-persistent-v0.3.0)
- 2025-09-30
### Miscellaneous Tasks
- [**breaking**] Cleanup deprecated definitions
([#2594](#2594))
### Refactor
- [**breaking**] Replace lazy_static with std::sync::LazyLock
([#2567](#2567))
</blockquote>
## `hugr`
<blockquote>
##
[0.23.0](hugr-v0.22.4...hugr-v0.23.0)
- 2025-09-30
### Bug Fixes
- DeadCodeElim keeps consumers of linear outputs
([#2560](#2560))
- [**breaking**] Appease `cargo-audit` by replacing unmaintained
dependencies ([#2572](#2572))
- *(core)* check extension versions on model import
([#2580](#2580))
- [**breaking**] test extension version compatibility on ModelWithExts
([#2587](#2587))
- *(core)* check used extension versions against resolved extensions
([#2588](#2588))
- [**breaking**] model import loads Package extensions
([#2590](#2590))
### Miscellaneous Tasks
- [**breaking**] Cleanup deprecated definitions
([#2594](#2594))
### New Features
- [**breaking**] DeadCodeElimPass reports error on non-existent
entry_points ([#2566](#2566))
- add trait+funcs for linking Hugrs explicitly by Node
([#2521](#2521))
- Documentation and error hints
([#2523](#2523))
- Allow creating DFG builders from existing hugrs
([#2562](#2562))
- add_input/output for arbitrary DFGBuilders
([#2564](#2564))
- [**breaking**] Return error instead of panicking in
DFGWrapper::add_{in,out}put
([#2571](#2571))
- *(core)* inner acccesors for WithGenerator error
([#2583](#2583))
- Normalize CFGs ([#2591](#2591))
### Refactor
- [**breaking**] Replace lazy_static with std::sync::LazyLock
([#2567](#2567))
</blockquote>
## `hugr-cli`
<blockquote>
##
[0.22.3](hugr-cli-v0.22.2...hugr-cli-v0.22.3)
- 2025-09-11
### New Features
- *(hugr-cli)* CliError::validate helper
([#2507](#2507))
</blockquote>
</p></details>
---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
Closes #2589
BREAKING CHANGE: import_package takes packaged extensions argument