Skip to content

feat!: Allow generic Nodes in HugrMut insert operations #2075

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

aborgna-q
Copy link
Collaborator

insert_hugr, insert_from_view, and insert_subgraph were written before we made Node a type generic, and incorrectly assumed the return type maps were always hugr::Nodes.

The methods were either unusable or incorrect when using generic HugrViews source/targets with non-base node types.

This PR fixes that, and additionally allows us us to have SiblingSubgraph::extract_subgraph work for generic HugrViews.

BREAKING CHANGE: Added Node type parameters to extraction operations in HugrMut.

@aborgna-q aborgna-q requested a review from a team as a code owner April 10, 2025 15:30
@aborgna-q aborgna-q requested a review from tatiana-s April 10, 2025 15:30
@aborgna-q aborgna-q added the breaking-change Changes that break semver label Apr 10, 2025
@aborgna-q aborgna-q added this to the hugr-rs 0.16 milestone Apr 10, 2025
Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 81.96721% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (release-rs-v0.16.0@209b2ea). Learn more about missing BASE report.

Files with missing lines Patch % Lines
hugr-core/src/hugr/hugrmut.rs 83.05% 10 Missing ⚠️
hugr-core/src/builder/build_traits.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##             release-rs-v0.16.0    #2075   +/-   ##
=====================================================
  Coverage                      ?   82.90%           
=====================================================
  Files                         ?      217           
  Lines                         ?    41554           
  Branches                      ?    37732           
=====================================================
  Hits                          ?    34451           
  Misses                        ?     5299           
  Partials                      ?     1804           
Flag Coverage Δ
python 85.40% <ø> (?)
rust 82.65% <81.96%> (?)

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.

@hugrbot
Copy link
Collaborator

hugrbot commented Apr 10, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

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

Description:
A trait method now requires a different number of generic type parameters than it used to. Calls or implementations of this trait method using 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.40.0/src/lints/trait_method_requires_different_generic_type_params.ron

Failed in:
Container::add_hugr_view (0 -> 1 generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/builder/build_traits.rs:122
HugrMut::insert_from_view (0 -> 1 generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/hugrmut.rs:241
HugrMut::insert_subgraph (0 -> 1 generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/hugrmut.rs:264

Copy link
Contributor

@tatiana-s tatiana-s left a comment

Choose a reason for hiding this comment

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

I am not too familiar with this part of the codebase but generally looks good to me 👍

@aborgna-q aborgna-q force-pushed the ab/insert-generic-subgraph branch from c443bec to c99d022 Compare April 15, 2025 13:57
@aborgna-q aborgna-q changed the base branch from main to release-rs-v0.16.0 April 15, 2025 13:58
@aborgna-q aborgna-q force-pushed the ab/insert-generic-subgraph branch from c99d022 to 63da208 Compare April 15, 2025 14:21
@aborgna-q aborgna-q merged commit 81447ec into release-rs-v0.16.0 Apr 15, 2025
25 checks passed
@aborgna-q aborgna-q deleted the ab/insert-generic-subgraph branch April 15, 2025 14:58
aborgna-q added a commit that referenced this pull request May 7, 2025
`insert_hugr`, `insert_from_view`, and `insert_subgraph` were written
before we made `Node` a type generic, and incorrectly assumed the return
type maps were always `hugr::Node`s.

The methods were either unusable or incorrect when using generic
`HugrView`s source/targets with non-base node types.

This PR fixes that, and additionally allows us us to have
`SiblingSubgraph::extract_subgraph` work for generic `HugrViews`.

BREAKING CHANGE: Added Node type parameters to extraction operations in
`HugrMut`.
This was referenced May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break semver wait to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants