feat!: impl HugrView for any &(mut) to a HugrView #1678

acl-cqc · 2024-11-25T09:19:19Z

closes #1636 . Uses macro to cover both &T and &mut T and also RootChecked.

While this "works", there are downsides: previously we had an impl<T:AsRef<Hugr>> HugrView for T so the "AsRef" covers Rc, Cow, etc. etc., whereas now we only have specific cases of & and &mut (admittedly for any HugrView not just an actual Hugr).

Really it seems that what we'd like is one impl<H: HugrView, T: AsRef<H>> HugrView for T. However there are about three issues here:

that conflicts with the base impl HugrView for Hugr because we have impl AsRef<Hugr> for Hugr, and we need that for the builder. We can actually remove that conflicting impl AsRef if we add a new trait (call it Buildable 😁) - see eeb9e73 - so that is soluble, and might even be good, if it were a step towards being able to use the builder on arbitrary HugrMut's e.g. limited to a sibling subtree. But it's mildly disruptive, so I've not undone that for this PR.
Secondly, dropping impl AsRef<Hugr> for Hugr, means that &Hugr doesn't impl AsRef<Hugr> either (!) - the way AsRef works is that &T inherits any AsRef-ness from T, there is no blanket impl<T> AsRef<T> for &T. Conversely Borrow has both blanket impl<T> Borrow<T> for &T and also impl<T> Borrow<T> for T, the latter again means we conflict with impl HugrView for Hugr). Possibly we could define our own trait/version of AsRef that we impl for Rc, Cow, & and &mut.... :-(
the foreign impl restriction, in that that would prevent anyone ever deriving AsRef<X> for Y for any X,Y that both implement HugrView, so rustc won't even let us impl<H: HugrView, T: AsRef<H>> HugrView for T. (Might this work for our own version of AsRef?) One possible solution here would be to say, every new implementation of HugrView, such as DescendantsGraph, should have its own impl<T: AsRef<DescendantsGraph>> HugrView for T, and then we provide the macro to do that (like the hugr_view_methods here - the same macro would also have to impl<T:AsRef<_>> HugrInternals for T too, which doesn't sound all that "internal", ick...)
- Also this would then mean that anywhere wanting to parametrize would have to do <T: HugrView + AsRef<T>>, which is awkward - I'm not sure whether we could avoid that with trait HugrView: AsRef<Self>, I haven't got that far. Perhaps if we have a custom AsRef and say:

trait HugrView: CustomAsRef<Self::BorrowedView> {
    type BorrowedView: HugrView

???
(I even wonder if we might use std::ops::Deref, but I started to try that and got into lots of trouble with lifetimes...)

Summary this PR has some shortcomings, but it does work. The CustomAsRef route might be better, but might not work at all, and/or may have other unforeseen problems - is it worth pursuing?

BREAKING CHANGE: types which implement AsRef - both library ones such as Rc and custom ones - no longer get a blanket impl of HugrView. Workaround by manually calling as_ref() and using the &Hugr yourself.

…grMut ?" This reverts commit 925940be354e0a1294b9d90b8da70778ebcaf867.

…cked

This reverts commit 633c5a8112322dda5060ade8dec2e5109f4fdaec.

…r fixes

codecov · 2024-11-25T09:21:29Z

Codecov Report

Attention: Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.33%. Comparing base (c5c8a6f) to head (e97b842).

Files with missing lines	Patch %	Lines
hugr-core/src/hugr/internal.rs	77.77%	2 Missing ⚠️
hugr-core/src/hugr/views.rs	93.93%	2 Missing ⚠️
hugr-core/src/hugr/views/root_checked.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1678      +/-   ##
==========================================
- Coverage   86.34%   86.33%   -0.01%     
==========================================
  Files         166      166              
  Lines       29747    29762      +15     
  Branches    26659    26674      +15     
==========================================
+ Hits        25684    25694      +10     
- Misses       2537     2542       +5     
  Partials     1526     1526

Flag	Coverage Δ
python	`92.42% <ø> (ø)`
rust	`85.62% <90.00%> (-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.

aborgna-q · 2024-11-25T16:36:51Z

hugr-core/src/hugr/views.rs

+macro_rules! hugr_view_methods {
+    () => {
+        delegate! {
+            to (**self) {
+
+                #[inline]
+                fn contains_node(&self, node: Node) -> bool;
+
+                #[inline]
+                fn node_count(&self) -> usize;
+
+                #[inline]
+                fn edge_count(&self) -> usize;
+
+                #[inline]
+                fn nodes(&self) -> impl Iterator<Item = Node> + Clone;
+
+                #[inline]
+                fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;
+
+                #[inline]
+                fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;
+
+                #[inline]
+                fn linked_ports(
+                    &self,
+                    node: Node,
+                    port: impl Into<Port>,
+                ) -> impl Iterator<Item = (Node, Port)> + Clone;
+
+                #[inline]
+                fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;
+
+                #[inline]
+                fn num_ports(&self, node: Node, dir: Direction) -> usize;
+
+                #[inline]
+                fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;
+
+                #[inline]
+                fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;
+
+                #[inline]
+                fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;
+            }
+        }
    }
 }

+impl<T: HugrView> HugrView for &T {
+    hugr_view_methods! {}
+}
+
+impl<T: HugrView> HugrView for &mut T {
+    hugr_view_methods! {}
+}


There's no need to add inlines. derive_more takes care of that already.

We normally single-line the methods when using derive!

With the above, I don't think the macro simplifies the code much.
Writing it explicitly avoids the extra layer of indirection.

Suggested change

macro_rules! hugr_view_methods {

() => {

delegate! {

to (**self) {

#[inline]

fn contains_node(&self, node: Node) -> bool;

#[inline]

fn node_count(&self) -> usize;

#[inline]

fn edge_count(&self) -> usize;

#[inline]

fn nodes(&self) -> impl Iterator<Item = Node> + Clone;

#[inline]

fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;

#[inline]

fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;

#[inline]

fn linked_ports(

&self,

node: Node,

port: impl Into<Port>,

) -> impl Iterator<Item = (Node, Port)> + Clone;

#[inline]

fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;

#[inline]

fn num_ports(&self, node: Node, dir: Direction) -> usize;

#[inline]

fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;

#[inline]

fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;

#[inline]

fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;

}

}

}

}

impl<T: HugrView> HugrView for &T {

hugr_view_methods! {}

}

impl<T: HugrView> HugrView for &mut T {

hugr_view_methods! {}

}

impl<T: HugrView> HugrView for &T {

delegate! {

to (**self) {

fn contains_node(&self, node: Node) -> bool;

fn node_count(&self) -> usize;

fn edge_count(&self) -> usize;

fn nodes(&self) -> impl Iterator<Item = Node> + Clone;

fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;

fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;

fn linked_ports(&self, node: Node, port: impl Into<Port>) -> impl Iterator<Item = (Node, Port)> + Clone;

fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;

fn num_ports(&self, node: Node, dir: Direction) -> usize;

fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;

fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;

fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;

}

}

}

impl<T: HugrView> HugrView for &mut T {

delegate! {

to (**self) {

fn contains_node(&self, node: Node) -> bool;

fn node_count(&self) -> usize;

fn edge_count(&self) -> usize;

fn nodes(&self) -> impl Iterator<Item = Node> + Clone;

fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;

fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;

fn linked_ports(&self, node: Node, port: impl Into<Port>) -> impl Iterator<Item = (Node, Port)> + Clone;

fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;

fn num_ports(&self, node: Node, dir: Direction) -> usize;

fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;

fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;

fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;

}

}

}

There's no need to add inlines. derive_more takes care of that already.

Sounds good, but how does derive_more get involved here? I took the [inline]s from the old impl<T: AsRef<Hugr>> HugrView for T {...

I've now managed to common up all three of the delegations with the same macro...

Sorry, s/derive_more/delegate/g -.-'

Ah....right. (That makes sense!)

Ok, so if delegate automatically adds [inline]....then (a) that shortens the macro :), and (b) yes it does reduce the utility of the macro a bit. OTOH there are now three instances of it, so...we can drop it....but do you really want to? :)

Note the macro is local to hugr::views, which I think works quite well, but we'd have to put Rc / Cow / Arc versions thereof in hugr::views::refs or something - not a bad place I guess. If we do those as well it'd be 5-6 instances of the macro.... (but they are easy to add, even in this PR)

They should all go on the same hugr::views::impl files imo, including these.
A file-local macro would be ok there, the file would only contain the same repeated calls.

aborgna-q · 2024-11-25T16:38:50Z

hugr-core/src/hugr/views/root_checked.rs

+    delegate! {
+        to self.as_ref() {
+            #[inline]
+            fn contains_node(&self, node: Node) -> bool;
+
+            #[inline]
+            fn node_count(&self) -> usize;
+
+            #[inline]
+            fn edge_count(&self) -> usize;
+
+            #[inline]
+            fn nodes(&self) -> impl Iterator<Item = Node> + Clone;
+
+            #[inline]
+            fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;
+
+            #[inline]
+            fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;
+
+            #[inline]
+            fn linked_ports(
+                &self,
+                node: Node,
+                port: impl Into<Port>,
+            ) -> impl Iterator<Item = (Node, Port)> + Clone;
+
+            #[inline]
+            fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;
+
+            #[inline]
+            fn num_ports(&self, node: Node, dir: Direction) -> usize;
+
+            #[inline]
+            fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;
+
+            #[inline]
+            fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;
+
+            #[inline]
+            fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;
+        }
+    }


Suggested change

delegate! {

to self.as_ref() {

#[inline]

fn contains_node(&self, node: Node) -> bool;

#[inline]

fn node_count(&self) -> usize;

#[inline]

fn edge_count(&self) -> usize;

#[inline]

fn nodes(&self) -> impl Iterator<Item = Node> + Clone;

#[inline]

fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;

#[inline]

fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;

#[inline]

fn linked_ports(

&self,

node: Node,

port: impl Into<Port>,

) -> impl Iterator<Item = (Node, Port)> + Clone;

#[inline]

fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;

#[inline]

fn num_ports(&self, node: Node, dir: Direction) -> usize;

#[inline]

fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;

#[inline]

fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;

#[inline]

fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;

}

}

delegate! {

to self.as_ref() {

fn contains_node(&self, node: Node) -> bool;

fn node_count(&self) -> usize;

fn edge_count(&self) -> usize;

fn nodes(&self) -> impl Iterator<Item = Node> + Clone;

fn node_ports(&self, node: Node, dir: Direction) -> impl Iterator<Item = Port> + Clone;

fn all_node_ports(&self, node: Node) -> impl Iterator<Item = Port> + Clone;

fn linked_ports(&self, node: Node, port: impl Into<Port>) -> impl Iterator<Item = (Node, Port)> + Clone;

fn node_connections(&self, node: Node, other: Node) -> impl Iterator<Item = [Port; 2]> + Clone;

fn num_ports(&self, node: Node, dir: Direction) -> usize;

fn children(&self, node: Node) -> impl DoubleEndedIterator<Item = Node> + Clone;

fn neighbours(&self, node: Node, dir: Direction) -> impl Iterator<Item = Node> + Clone;

fn all_neighbours(&self, node: Node) -> impl Iterator<Item = Node> + Clone;

}

}

aborgna-q · 2024-11-25T16:47:37Z

Any impl<any kind of bound> HugrView for T is a blanket implementation.
Unless you are in nightly, blanket implementations are unique so they prevent any other impl from being written (since an external definition would be able to satisfy the overlapping constraints otherwise).

I don't think the delegate block is annoying enough to merit much more workarounds. We can add the handful implementations to Rc, Cow, ... in addition to the ones here (in a submodule!) and be done with it.

…hecked

acl-cqc added 7 commits November 24, 2024 09:23

Add Buildable trait. TODO can we make it return a HugrView+HugrMut ?

eeb9e73

Revert "Add Buildable trait. TODO can we make it return a HugrView+Hu…

8acf6c2

…grMut ?" This reverts commit 925940be354e0a1294b9d90b8da70778ebcaf867.

HugrView for &T, but not &mut

896127d

macro for HugrView methods for both & and &mut. Problems with RootChe…

530f215

…cked

Try to parametrize hugr_view_meths macro, doesn't work

24c54c6

Revert "Try to parametrize hugr_view_meths macro, doesn't work"

ac5ab01

This reverts commit 633c5a8112322dda5060ade8dec2e5109f4fdaec.

Solve RootChecked by duplicating hugr_view_methods macro :(, few othe…

0290aa0

…r fixes

acl-cqc requested a review from aborgna-q November 25, 2024 09:19

acl-cqc requested a review from a team as a code owner November 25, 2024 09:19

aborgna-q reviewed Nov 25, 2024

View reviewed changes

acl-cqc added 3 commits November 25, 2024 22:29

Give hugr_view_methods extra ident to assign self --> common up RootC…

fc16a83

…hecked

Remove [inline]s inside delegate

aab834c

Merge remote-tracking branch 'origin/main' into acl/hugr_view_ref

e97b842

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: impl HugrView for any &(mut) to a HugrView #1678

feat!: impl HugrView for any &(mut) to a HugrView #1678

acl-cqc commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

aborgna-q Nov 25, 2024

acl-cqc Nov 25, 2024

aborgna-q Nov 26, 2024

acl-cqc Nov 26, 2024 •

edited

Loading

aborgna-q Nov 27, 2024

aborgna-q Nov 25, 2024

aborgna-q commented Nov 25, 2024

feat!: impl HugrView for any &(mut) to a HugrView #1678

Are you sure you want to change the base?

feat!: impl HugrView for any &(mut) to a HugrView #1678

Conversation

acl-cqc commented Nov 25, 2024 • edited Loading

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

aborgna-q Nov 25, 2024

Choose a reason for hiding this comment

acl-cqc Nov 25, 2024

Choose a reason for hiding this comment

aborgna-q Nov 26, 2024

Choose a reason for hiding this comment

acl-cqc Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

aborgna-q Nov 27, 2024

Choose a reason for hiding this comment

aborgna-q Nov 25, 2024

Choose a reason for hiding this comment

aborgna-q commented Nov 25, 2024

acl-cqc commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

acl-cqc Nov 26, 2024 •

edited

Loading