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.

Sign up for GitHub

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

Jump to bottom

Refactor: move apply_rewrite from Hugr to HugrMut #519

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Refactor: move apply_rewrite from Hugr to HugrMut #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b22a86
Move apply_rewrite from Hugr to HugrMut
acl-cqc Sep 12, 2023
f6b906a
No need for .ok()s here
acl-cqc Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/hugr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,14 +206,6 @@ pub type Direction = portgraph::Direction;

/// Public API for HUGRs.
impl Hugr {
/// Applies a rewrite to the graph.
pub fn apply_rewrite<R, E>(
&mut self,
rw: impl Rewrite<ApplyResult = R, Error = E>,
) -> Result<R, E> {
rw.apply(self)
}

/// Run resource inference and pass the closure into validation
pub fn infer_and_validate(
&mut self,
Expand Down
10 changes: 9 additions & 1 deletion src/hugr/hugrmut.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{Hugr, Port};

use self::sealed::HugrMutInternals;

use super::NodeMetadata;
use super::{NodeMetadata, Rewrite};

/// Functions for low-level building of a HUGR.
pub trait HugrMut: HugrView + HugrMutInternals {
Expand Down Expand Up @@ -144,6 +144,14 @@ pub trait HugrMut: HugrView + HugrMutInternals {
self.valid_node(root)?;
self.hugr_mut().insert_from_view(root, other)
}

/// Applies a rewrite to the graph.
fn apply_rewrite<R, E>(&mut self, rw: impl Rewrite<ApplyResult = R, Error = E>) -> Result<R, E>
where
Self: Sized,
{
rw.apply(self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the other hugrmut methods we validate that we're not modifying the graph outside the view.
This isn't possible with the current Rewrite interface, so we cannot avoid rewriting arbitrary parts.

Is this something we want to allow?

Copy link
Collaborator

@aborgna-q aborgna-q Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Although I guess if we require the user to ensure that rw.validate(self) passes then this would be valid.

Would it make sense to have an assert_debug! here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that rw.apply(self) will have to work through methods of the impl HugrMut and so the checking comes there - i.e., when the rewrite tries to modify the view.

(If we relied on checking here, then just calling Rewrite::apply would allow to bypass the check!!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But are you suggesting (skipping all the typing boilerplate for now):

fn apply(rw: impl Rewrite) {
   debug_assert!(rw.verify(self));
   rw.apply(self)
}

? I do quite like that idea, but right now a lot of our Rewrite::verify impl's are unimplemented!....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could have an apply_verified as a shortcut? (not debug_assert just assert, then!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Yeah, I was suggesting the debug_assert you mentioned. If it's unimplemented I'd leave it out (maybe a TODO?).

This is definitely not a place where we want asserts. So I'd just leave it bare and let the inner calls do the checks.

}
}

/// Impl for non-wrapped Hugrs. Overwrites the recursive default-impls to directly use the hugr.
Expand Down
1 change: 1 addition & 0 deletions src/hugr/rewrite/outline_cfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ mod test {
build_cond_then_loop_cfg, build_conditional_in_loop_cfg,
};
use crate::extension::PRELUDE_REGISTRY;
use crate::hugr::HugrMut;
use crate::ops::handle::NodeHandle;
use crate::{HugrView, Node};
use cool_asserts::assert_matches;
Expand Down
2 changes: 1 addition & 1 deletion src/hugr/rewrite/simple_replace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ pub(in crate::hugr::rewrite) mod test {
use crate::extension::prelude::BOOL_T;
use crate::extension::{EMPTY_REG, PRELUDE_REGISTRY};
use crate::hugr::views::HugrView;
use crate::hugr::{Hugr, Node};
use crate::hugr::{Hugr, HugrMut, Node};
use crate::ops::OpTag;
use crate::ops::{OpTrait, OpType};
use crate::std_extensions::logic::test::and_op;
Expand Down
1 change: 1 addition & 0 deletions src/hugr/views/sibling_subgraph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ mod tests {
EMPTY_REG,
},
hugr::views::{HierarchyView, SiblingGraph},
hugr::HugrMut,
ops::{
handle::{FuncID, NodeHandle},
OpType,
Expand Down