Skip to content
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!: Split Rewrite trait into VerifyPatch and ApplyPatch #2070

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lmondada
Copy link
Contributor

@lmondada lmondada commented Apr 8, 2025

This PR splits the Rewrite trait into two (three) traits:

  • a VerifyPatch trait that has the fn verify and fn invalidation_set functions
  • a ApplyPatch trait that has the fn apply function. This inherits VerifyPatch and is the "rewriting" trait that should be used in most scenarios.

In addition, there is a third trait ApplyPatchHugrMut that can be implemented by any patches that can be applied to any HugrMut (as opposed to a specific type H). This is strictly stronger than ApplyPatch and should be implemented instead of ApplyPatch where possible (see the docs of the traits).

closes #588
closes #2052

BREAKING CHANGE: the Rewrite trait was replaced by ApplyPatch. The hugr/rewrite module is now called hugr/patch

@lmondada lmondada requested a review from a team as a code owner April 8, 2025 15:30
@lmondada lmondada requested review from acl-cqc, Copilot and aborgna-q and removed request for acl-cqc April 8, 2025 15:30
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Copy link

codecov bot commented Apr 8, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 68 lines in your changes missing coverage. Please review.

Project coverage is 82.89%. Comparing base (7626797) to head (eda21fa).

Files with missing lines Patch % Lines
hugr-core/src/hugr/patch.rs 10.00% 27 Missing ⚠️
hugr-core/src/hugr/patch/replace.rs 80.64% 20 Missing and 4 partials ⚠️
hugr-core/src/hugr/patch/outline_cfg.rs 36.36% 7 Missing ⚠️
hugr-core/src/hugr/patch/inline_call.rs 50.00% 3 Missing ⚠️
hugr-core/src/hugr/patch/inline_dfg.rs 57.14% 3 Missing ⚠️
hugr-core/src/hugr/patch/insert_identity.rs 40.00% 3 Missing ⚠️
hugr-core/src/hugr/patch/simple_replace.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2070      +/-   ##
==========================================
- Coverage   82.91%   82.89%   -0.02%     
==========================================
  Files         217      217              
  Lines       41529    41613      +84     
  Branches    37707    37791      +84     
==========================================
+ Hits        34433    34497      +64     
- Misses       5292     5311      +19     
- Partials     1804     1805       +1     
Flag Coverage Δ
python 85.40% <ø> (ø)
rust 82.64% <66.66%> (-0.02%) ⬇️

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 8, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- 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.40.0/src/lints/enum_missing.ron

Failed in:
enum hugr_core::hugr::rewrite::consts::RemoveError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/consts.rs:18
enum hugr_core::hugr::rewrite::outline_cfg::OutlineCfgError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/outline_cfg.rs:221
enum hugr_core::hugr::rewrite::replace::WhichHugr, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/replace.rs:433
enum hugr_core::hugr::rewrite::BoundaryPort, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/port_types.rs:14
enum hugr_core::hugr::rewrite::inline_call::InlineCallError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/inline_call.rs:17
enum hugr_core::hugr::rewrite::replace::NewEdgeKind, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/replace.rs:31
enum hugr_core::hugr::rewrite::replace::ReplaceError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/replace.rs:397
enum hugr_core::hugr::rewrite::simple_replace::SimpleReplacementError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/simple_replace.rs:351
enum hugr_core::hugr::rewrite::SimpleReplacementError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/simple_replace.rs:351
enum hugr_core::hugr::rewrite::insert_identity::IdentityInsertionError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/insert_identity.rs:38
enum hugr_core::hugr::rewrite::inline_dfg::InlineDFGError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/inline_dfg.rs:15

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/module_missing.ron

Failed in:
mod hugr_core::hugr::rewrite::insert_identity, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/insert_identity.rs:1
mod hugr_core::hugr::rewrite, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite.rs:1
mod hugr_core::hugr::rewrite::replace, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/replace.rs:1
mod hugr_core::hugr::rewrite::inline_dfg, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/inline_dfg.rs:1
mod hugr_core::hugr::rewrite::outline_cfg, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/outline_cfg.rs:1
mod hugr_core::hugr::rewrite::consts, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/consts.rs:1
mod hugr_core::hugr::rewrite::inline_call, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/inline_call.rs:1
mod hugr_core::hugr::rewrite::simple_replace, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/simple_replace.rs:1

--- 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.40.0/src/lints/struct_missing.ron

Failed in:
struct hugr_core::hugr::rewrite::outline_cfg::OutlineCfg, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/outline_cfg.rs:23
struct hugr_core::hugr::rewrite::replace::NewEdgeSpec, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/replace.rs:18
struct hugr_core::hugr::rewrite::inline_call::InlineCall, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/inline_call.rs:12
struct hugr_core::hugr::rewrite::replace::Replacement, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/replace.rs:57
struct hugr_core::hugr::rewrite::inline_dfg::InlineDFG, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/inline_dfg.rs:10
struct hugr_core::hugr::rewrite::insert_identity::IdentityInsertion, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/insert_identity.rs:18
struct hugr_core::hugr::rewrite::consts::RemoveConst, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/consts.rs:73
struct hugr_core::hugr::rewrite::simple_replace::SimpleReplacement, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/simple_replace.rs:26
struct hugr_core::hugr::rewrite::SimpleReplacement, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/simple_replace.rs:26
struct hugr_core::hugr::rewrite::Transactional, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite.rs:53
struct hugr_core::hugr::rewrite::consts::RemoveLoadConstant, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/consts.rs:13
struct hugr_core::hugr::rewrite::HostPort, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/port_types.rs:23
struct hugr_core::hugr::rewrite::ReplacementPort, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite/port_types.rs:27

--- failure trait_missing: pub trait removed or renamed ---

Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait 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.40.0/src/lints/trait_missing.ron

Failed in:
trait hugr_core::hugr::rewrite::Rewrite, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite.rs:19
trait hugr_core::hugr::Rewrite, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/rewrite.rs:19
trait hugr_core::hugr::rewrite::simple_replace::HugrMutInternals, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/internal.rs:209

--- failure trait_removed_supertrait: supertrait removed or renamed ---

Description:
A supertrait was removed from a trait. Users of the trait can no longer assume it can also be used like its supertrait.
      ref: https://doc.rust-lang.org/reference/items/traits.html#supertraits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/trait_removed_supertrait.ron

Failed in:
supertrait hugr_core::hugr::rewrite::simple_replace::HugrMutInternals of trait HugrMut in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/hugrmut.rs:22

@aborgna-q aborgna-q added the breaking-change Changes that break semver label Apr 8, 2025
@aborgna-q aborgna-q added this to the hugr-rs 0.16 milestone Apr 8, 2025
Comment on lines +39 to +50
/// A patch that can be applied to a Hugr of type `H`.
///
/// ### When to use
///
/// Use this trait whenever possible in bounds for the most generality. Note
/// that this will require specifying which types `H` the patch applies to.
///
/// ### When to implement
///
/// For `H: HugrMut`, prefer implementing [`ApplyPatchHugrMut`] instead. This
/// will automatically implement this trait.
pub trait ApplyPatch<H: HugrView>: VerifyPatch<Node = H::Node> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Naming suggestion: Since this is the main entrypoint, could it just be called Patch?

VerifyPatch is more niche, and the name still reflects its a verification for the things you'll do on this trait.

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.

Generic Rewrite trait over HugrMut type Rename 'Rewrite'
3 participants