Skip to content

Conversation

@aborgna-q
Copy link
Collaborator

@aborgna-q aborgna-q commented Apr 23, 2025

  • Allows HugrMut to be implemented for HugrViews with arbitrary node types
  • Removes HugrMutInternals::hugr_mut(&mut self) -> &mut Hugr, it can be implemented for more complex types. This is required for Replace HugrInternals::base_hugr with simpler methods #1926, but I haven't touched the read-only side yet.
  • Added a Node associated type to Rewrite. All existing rewrites only implement Rewrite<Node = Node> for now, expanding their type is left for a separate PR.

drive-by: Fix a couple bugs in rewrite implementations that assumed that SiblingMut contained transitive children.

BREAKING CHANGE: HugrMut is now implemented generically for any HugrView::Node type.
BREAKING CHANGE: SiblingMut has a new type parameter for the wrapped hugr type.

@aborgna-q aborgna-q requested a review from a team as a code owner April 23, 2025 14:46
@aborgna-q aborgna-q requested a review from mark-koch April 23, 2025 14:46
@hugrbot
Copy link
Collaborator

hugrbot commented Apr 23, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure method_requires_different_generic_type_params: method now requires a different number of generic type parameters ---

Description:
A method now requires a different number of generic type parameters than it used to. Uses of this method that supplied the previous number of generic types will be broken.
      ref: https://doc.rust-lang.org/reference/items/generics.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/method_requires_different_generic_type_params.ron

Failed in:
hugr_core::hugr::views::sibling::SiblingMut::try_new takes 0 generic types instead of 1, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/views/sibling.rs:256

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_associated_type_added.ron

Failed in:
trait associated type hugr_core::hugr::rewrite::Rewrite::Node in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/rewrite.rs:22
trait associated type hugr_core::hugr::Rewrite::Node in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/rewrite.rs:22

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_method_added.ron

Failed in:
trait method hugr_core::hugr::internal::HugrInternals::node_metadata_map in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/internal.rs:57

--- failure trait_method_missing: pub trait method removed or renamed ---

Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_method_missing.ron

Failed in:
method take_node_metadata of trait HugrMut, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/hugrmut.rs:71
method overwrite_node_metadata of trait HugrMut, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/hugrmut.rs:83
method extensions_mut of trait HugrMut, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/hugrmut.rs:309
method hugr_mut of trait HugrMutInternals, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/internal.rs:211

--- failure trait_requires_more_generic_type_params: trait now requires more generic type parameters ---

Description:
A trait now requires more generic type parameters than it used to. Uses of this trait that supplied the previously-required number of generic types will be broken. To fix this, consider supplying default values for newly-added generic types.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-parameter-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_requires_more_generic_type_params.ron

Failed in:
trait SiblingMut (0 -> 1 required generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/views/sibling.rs:242

--- failure type_requires_more_generic_type_params: type now requires more generic type parameters ---

Description:
A type now requires more generic type parameters than it used to. Uses of this type that supplied the previously-required number of generic types will be broken. To fix this, consider supplying default values for newly-added generic types.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-parameter-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/type_requires_more_generic_type_params.ron

Failed in:
Struct SiblingMut (0 -> 1 required generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/views/sibling.rs:242

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_associated_type_added.ron

Failed in:
trait associated type hugr_passes::composable::ComposablePass::Node in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/composable.rs:13
trait associated type hugr_passes::ComposablePass::Node in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/composable.rs:13

- Allows `HugrMut` to be implemented for `HugrView`s with arbitrary node
  types
- Removes `HugrMutInternals::hugr_mut(&mut self) -> &mut Hugr`, it can
  be implemented for more complex types

drive-by: Fix a couple bugs in rewrite implementations that assumed that
`SiblingMut` contained transitive children.
@aborgna-q aborgna-q force-pushed the ab/hugrmut-generic-node branch from 8bf5f99 to 2c2745e Compare April 23, 2025 15:05
@aborgna-q aborgna-q changed the title feat: Hugrmut on generic nodes feat!: Hugrmut on generic nodes Apr 23, 2025
@aborgna-q aborgna-q added this to the hugr-rs 0.16 milestone Apr 23, 2025
@codecov
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 79.23977% with 71 lines in your changes missing coverage. Please review.

Project coverage is 83.46%. Comparing base (d8a5d67) to head (c70d989).
Report is 1 commits behind head on release-rs-v0.16.0.

Files with missing lines Patch % Lines
hugr-core/src/hugr/views/impls.rs 53.03% 31 Missing ⚠️
hugr-core/src/hugr/views/sibling.rs 73.77% 16 Missing ⚠️
hugr-core/src/hugr/internal.rs 87.71% 7 Missing ⚠️
hugr-core/src/hugr/views/root_checked.rs 70.83% 7 Missing ⚠️
hugr-core/src/hugr/hugrmut.rs 94.00% 3 Missing ⚠️
hugr-core/src/hugr/rewrite.rs 0.00% 3 Missing ⚠️
hugr-core/src/hugr/views/descendants.rs 25.00% 3 Missing ⚠️
hugr-passes/src/nest_cfgs.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           release-rs-v0.16.0    #2111      +/-   ##
======================================================
+ Coverage               83.33%   83.46%   +0.12%     
======================================================
  Files                     219      218       -1     
  Lines                   42205    42053     -152     
  Branches                38307    38258      -49     
======================================================
- Hits                    35173    35098      -75     
+ Misses                   5221     5147      -74     
+ Partials                 1811     1808       -3     
Flag Coverage Δ
python 85.42% <ø> (-0.31%) ⬇️
rust 83.26% <79.23%> (+0.17%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

"Received an invalid node {node} while mutating a HUGR:\n\n {}",
hugr.mermaid_string()
);
if cfg!(debug_assertions) && !hugr.valid_node(node) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm strictly speaking the docs promise that operations panic if nodes are invalid. Do we need to specify that this is only true in debug builds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is that now that we don't have the hugr_mut shortcut, stacked hugr wrappers will end up running the check at each and every layer.

Maybe we should leave them enabled and benchmark the performance impact later.

Comment on lines 311 to 314
/// # Errors
///
/// Returns a [`HugrError::InvalidTag`] if this would break the bound
/// (`Self::RootHandle`) on the root node's OpTag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all impls still do this check. Should the error still be documented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. The error is being dropped in the next PR but this got filtered into this one

///
/// Returns the old OpType.
///
/// TODO: Add a version which ignores input extensions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO no longer relevant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, we don't check the replaced signature

@aborgna-q aborgna-q force-pushed the ab/hugrmut-generic-node branch from 6066389 to c70d989 Compare April 23, 2025 16:16
@aborgna-q aborgna-q added this pull request to the merge queue Apr 23, 2025
Merged via the queue into release-rs-v0.16.0 with commit b209709 Apr 23, 2025
25 checks passed
@aborgna-q aborgna-q deleted the ab/hugrmut-generic-node branch April 23, 2025 16:37
aborgna-q added a commit that referenced this pull request May 7, 2025
- Allows `HugrMut` to be implemented for `HugrView`s with arbitrary node
types
- Removes `HugrMutInternals::hugr_mut(&mut self) -> &mut Hugr`, it can
be implemented for more complex types. This is required for #1926, but I
haven't touched the read-only side yet.
- Added a `Node` associated type to `Rewrite`. All existing rewrites
only implement `Rewrite<Node = Node>` for now, expanding their type is
left for a separate PR.

drive-by: Fix a couple bugs in rewrite implementations that assumed that
`SiblingMut` contained transitive children.

BREAKING CHANGE: `HugrMut` is now implemented generically for any
`HugrView::Node` type.
BREAKING CHANGE: `SiblingMut` has a new type parameter for the wrapped
hugr type.
This was referenced May 7, 2025
@hugrbot hugrbot mentioned this pull request May 14, 2025
This was referenced May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants