feat!: Split Rewrite trait into VerifyPatch and ApplyPatch #2070
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-rs-v0.16.0 #2070 +/- ##
======================================================
- Coverage 83.45% 83.44% -0.02%
======================================================
Files 219 219
Lines 42141 42200 +59
Branches 38243 38302 +59
======================================================
+ Hits 35167 35212 +45
- Misses 5159 5172 +13
- Partials 1815 1816 +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:
|
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
aborgna-q
reviewed
Apr 8, 2025
hugr-core/src/hugr/patch.rs
Outdated
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> { |
There was a problem hiding this comment.
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.
acl-cqc
reviewed
Apr 21, 2025
There was a problem hiding this comment.
Like it. Like it a lot! :). Agree with Agustin's suggesting of calling ApplyPatch just Patch.
I've made a lot of driveby comments, more than is really fair, feel free to be selective!
I'd approve all of this except for the changes to Replacement. I remember we talked about this and I can't really remember what we said but I do see now that it is more complex than we talked then. However I've had a go and
- branch
acl/rewrite-traitfinds, I think, a better solution, via an enumDynEdgeSpecwhich stores the three possible kinds ofNewEdgeSpecand thus removes the need forDynNode. This saves about 35 lines overall and reduces change vs. the original (branchmain). - branch
acl/rewrite-trait2goes even further, as I realized that the previous branch does in fact makeenum WhichHugrcompletely redundant. You can see the steps there to remove WhichHugr (including the first commit e61f800 verifying that it is indeed redundant)
I suggest you merge in one or other of those two branches and then I'll approve. (Renaming/etc. is fine and probably advisable - DynEdgeSpec might be better called WhichEdgeSpec, the variants might be better named MuIn/MuOut/MuNew, whatever.)
hugr-core/src/hugr/patch.rs
Outdated
| type Node; | ||
|
|
||
| /// Checks whether the rewrite would succeed on the specified Hugr. | ||
| /// If this call succeeds, [self.apply] should also succeed on the same `h` |
There was a problem hiding this comment.
Do these doclinks work? I'd expect you to have to link to [ApplyPatch::apply]. It might also be worth clarifying in the comment, if ``self`` is an [ApplyPatch].
hugr-core/src/hugr/patch.rs
Outdated
| fn invalidation_set(&self) -> impl Iterator<Item = Self::Node>; | ||
| } | ||
|
|
||
| /// A patch that can be applied to a Hugr of type `H`. |
There was a problem hiding this comment.
Suggested change
| /// A patch that can be applied to a Hugr of type `H`. | |
| /// A patch that can be applied to a mutable Hugr of type `H`. |
hugr-core/src/hugr/patch.rs
Outdated
| /// ### 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. |
There was a problem hiding this comment.
Suggested change
| /// that this will require specifying which types `H` the patch applies to. | |
| /// that this will require specifying which type `H` the patch applies to. |
hugr-core/src/hugr/patch.rs
Outdated
| fn apply(self, h: &mut H) -> Result<Self::Outcome, Self::Error>; | ||
| } | ||
|
|
||
| /// A patch that can be applied to any HugrMut. |
There was a problem hiding this comment.
Suggested change
| /// A patch that can be applied to any HugrMut. | |
| /// A patch that can be applied to any [HugrMut]. |
| /// | ||
| /// [CFG]: OpType::CFG | ||
| type ApplyResult = (Node, Node); | ||
| type Outcome = (Node, Node); |
There was a problem hiding this comment.
complete driveby: This'd be better as a [Node; 2] if you fancied changing it
hugr-core/src/hugr/patch/replace.rs
Outdated
| .ok_or(ReplaceError::InvalidAdoptingParent(n)) | ||
| })?; | ||
| let mut transferred: HashSet<Node> = self.adoptions.values().copied().collect(); | ||
| let mut transferred: HashSet<_> = self.adoptions.values().copied().collect(); |
There was a problem hiding this comment.
Suggested change
| let mut transferred: HashSet<_> = self.adoptions.values().copied().collect(); | |
| let mut transferred: HashSet<HostNode> = self.adoptions.values().copied().collect(); |
| dfg_hugr: Hugr, | ||
| #[values(apply_simple, apply_replace)] applicator: impl Fn(&mut Hugr, SimpleReplacement), | ||
| ) { | ||
| use crate::hugr::patch::VerifyPatch; |
There was a problem hiding this comment.
Nit: Move this to the top? (An annoying artefact of rstest combined with VSCode)
It is currently impossible to obtain the map from nodes in the replacement graph to newly inserted nodes in `self` from the `ApplyResult` type for `SimpleReplacement`. This PR fixes this. BREAKING CHANGE: The `ApplyResult` associated type of `SimpleReplacement` is now a custom struct. The deleted nodes returned previously can be obtained using the `result.deleted_nodes` field.
|
Suggest you target this at branch |
lmondada
force-pushed
the
feat/rewrite-trait
branch
from
c7c6dc7 to
74eb7df
Compare
acl-cqc
approved these changes
Apr 28, 2025
| use super::HugrMut; | ||
|
|
||
| /// Verify that a patch application would succeed. | ||
| pub trait PatchVerification { |
There was a problem hiding this comment.
I'd consider Verification or Verifiable but this is OK by me :)
| /// | ||
| /// For `H: HugrMut`, prefer implementing [`PatchHugrMut`] instead. This | ||
| /// will automatically implement this trait. | ||
| pub trait Patch<H: HugrView>: PatchVerification<Node = H::Node> { |
There was a problem hiding this comment.
Do you have any cases where H is not a HugrMut? If not, then bounding by HugrMut would be more obvious, and we could always change to HugrView if such a case comes up later (non-breaking as HugrView is a supertrait)
There was a problem hiding this comment.
H can be a PersistentHugr, which won't implement HugrMut (at least in its current form)
hugr-passes/src/lower.rs
Outdated
| let mut repls = hugr.apply_rewrite(rw)?; | ||
| debug_assert_eq!(repls.len(), 1); | ||
| Ok(repls.remove(0)) | ||
| let mut removed_nodes = hugr.apply_patch(rw)?.removed_nodes; |
There was a problem hiding this comment.
might could drop the mut and use into_iter rather than drain
| } | ||
|
|
||
| /// Result of applying a [`SimpleReplacement`]. | ||
| pub struct SimpleReplacementOutcome<HostNode = Node> { |
There was a problem hiding this comment.
I was hoping you might find a shorter name but OK...
There was a problem hiding this comment.
PatchOutcome? Or just Outcome? I know you didn't like it when the type name equaled the associated type name, but it's what std does for e.g. vec::Iter.
Unlike a trait name, in this case I personally don't mind having a short and slightly ambiguous name. The user can always choose to only import the module and refer to it by simple_replace::Outcome
hugr-core/src/hugr/patch/replace.rs
Outdated
| /// Nodes in the existing Hugr that are not [Replacement::removal] | ||
| /// (or are on the RHS of an entry in [Replacement::adoptions]) | ||
| Retained, | ||
| pub enum DynEdgeSpec<HostNode> { |
There was a problem hiding this comment.
Suggest - strongly suggest - renaming this to something like WhichEdgeSpec - that agrees more with the comment, and that this replaces WhichHugr, and that we don't really do anything dyn with it
There was a problem hiding this comment.
WhichEdgeSpec is a good name
Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>
Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>
Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>
Merged
via the queue into
release-rs-v0.16.0
with commit Apr 29, 2025
db4b39f
24 of 25 checks passed
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 29, 2025
Oupsie, during one of the merge conflict resolutions I must have forgotten to remove the old `rewrite.rs` file. As you can see in the `hugr-core/src/hugr.rs` file, this is no longer a module and thus the file should be deleted. It has been renamed to `patch.rs` in #2070
aborgna-q
pushed a commit
that referenced
this pull request
May 7, 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: Replaced the `Rewrite` trait with `Patch`. `Rewrite::ApplyResult` is now `Patch::Outcome`. `Rewrite::verify` was split into a separate trait, and is now `PatchVerification::verify`. BREAKING CHANGE: Renamed `hugr.rewrite` module to `hugr.patch`. BREAKING CHANGE: Changed the type `OutlineCfg::ApplyResult` (now `OutlineCfg::Outcome`) from `(Node, Node)` to `[Node; 2]`. --------- Co-authored-by: Alan Lawrence <alan.lawrence@cambridgequantum.com> Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>
aborgna-q
pushed a commit
that referenced
this pull request
May 7, 2025
Oupsie, during one of the merge conflict resolutions I must have forgotten to remove the old `rewrite.rs` file. As you can see in the `hugr-core/src/hugr.rs` file, this is no longer a module and thus the file should be deleted. It has been renamed to `patch.rs` in #2070
This was referenced May 7, 2025
Closed
Closed
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 14, 2025
~The base of this PR will be moved to main once #2070 has been merged in.~ ~Status: The API and main `struct`s have been defined. Most of the core logic has yet to be implemented.~ Status: PR is ready. The trait implementations for `HugrView`, `VerifyPatch` and `ApplyPatch` will be in a separate PR once progress on those is unblocked. Closes #2096
Closed
This was referenced May 29, 2025
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits the
Rewritetrait into two (three) traits:VerifyPatchtrait that has thefn verifyandfn invalidation_setfunctionsApplyPatchtrait that has thefn applyfunction. This inheritsVerifyPatchand is the "rewriting" trait that should be used in most scenarios.In addition, there is a third trait
ApplyPatchHugrMutthat can be implemented by any patches that can be applied to anyHugrMut(as opposed to a specific typeH). This is strictly stronger thanApplyPatchand should be implemented instead ofApplyPatchwhere possible (see the docs of the traits).closes #588
closes #2052
BREAKING CHANGE: Replaced the
Rewritetrait withPatch.Rewrite::ApplyResultis nowPatch::Outcome.Rewrite::verifywas split into a separate trait, and is nowPatchVerification::verify.BREAKING CHANGE: Renamed
hugr.rewritemodule tohugr.patch.BREAKING CHANGE: Changed the type
OutlineCfg::ApplyResult(nowOutlineCfg::Outcome) from(Node, Node)to[Node; 2].