-
Notifications
You must be signed in to change notification settings - Fork 14
feat!: ComposablePass trait allowing sequencing and validation #1895
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
Conversation
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-rs-v0.16.0 #1895 +/- ##
======================================================
+ Coverage 83.15% 83.26% +0.10%
======================================================
Files 218 218
Lines 41923 41948 +25
Branches 38101 38153 +52
======================================================
+ Hits 34861 34926 +65
+ Misses 5257 5217 -40
Partials 1805 1805
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:
|
hugr-passes/src/composable.rs
Outdated
|
||
fn sequence( | ||
self, | ||
other: impl ComposablePass<Err = Self::Err>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way to do this is
fn sequence_before<P2: ComposablePass>(self, other: P2) -> impl ComposablePass<Err = Self::Err>
where P2::Err : Into<Self::Err>
(and similarly sequence_after
) which avoids what may be a common-case use of map_err
(i.e. with Into
).
However, Infallible doesn't implement Into, so it's probably not much of a win. (I could add sequence_infallible(self, other: ComposablePass<Err=Infallible>)
I guess, but would then want that before/after versions of that too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I do think some more docs would be good. Regarding the suggestion to add add_entry_point
-- does it mean to apply the pass to everything below that point in the hierarchy but not above it? I think that sounds reasonable.
@aborgna-q do you mean |
@doug-q @aborgna-q @cqc-alec this has changed quite a bit since Alec approved - as new passes have been added to I contemplate adding to the trait
But I guess there is no immediate need - these would be non-breaking if we added them later. |
|
||
/// Returns a [ComposablePass] that does "`self` then `other`", so long as | ||
/// `other::Err` can be combined with ours. | ||
fn then<P: ComposablePass, E: ErrorCombiner<Self::Error, P::Error>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a possible method then2
that requires E: ErrorCombiner<P::Error, Self::Error>
i.e. that would allow when our error-type maps into P's (as opposed to then
which requires the other way around).
However, both ways also allow returning an Either
error and then2
would reverse that too (you probably wouldn't bother using then2
if you wanted an Either
but). So then I wonder, should we also reverse the order of results...and the doubt as to what's the right API has lead me to avoid providing then2
entirely. But I could definitely be persuaded....
(Sadly we can't allow ErrorCombiner
to work for both <A, B: Into<A>>
and <A: Into<B>, B>
because that would provide two conflicting impls when A
and B
are the same type :-( :-( )
hugr-passes/src/composable.rs
Outdated
} | ||
} | ||
|
||
// Note: in the short term two we could wish for more impls: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Note: in the short term two we could wish for more impls: | |
// Note: in the short term we could wish for more impls: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
fn from_second(b: B) -> Self; | ||
} | ||
|
||
impl<A: Error, B: Into<A>> ErrorCombiner<A, B> for A { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have
impl<E: Error, A: Into<E>, B: Into<E>> ErrorCombiner<A, B> for E
instead, which covers the Either impl.
But that breaks the tests below as it doesn't allow us to have ErrorCombiner<A, Infallible>
-.-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR...AFAICS Either does not implement From from either left or right, e.g. I get two errors here (one on each .into()
):
fn foo<A,B>(a:A, b:B, which: bool) -> Either<A,B> {
if which {a.into()} else {b.into()}
}
// Note: in the short term we could wish for two more impls: | ||
// impl<E:Error> ErrorCombiner<Infallible, E> for E | ||
// impl<E:Error> ErrorCombiner<E, Infallible> for E | ||
// however, these aren't possible as they conflict with | ||
// impl<A, B:Into<A>> ErrorCombiner<A,B> for A | ||
// when A=E=Infallible, boo :-(. | ||
// However this will become possible, indeed automatic, when Infallible is replaced | ||
// by ! (never_type) as (unlike Infallible) ! converts Into anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is covered by impl<A: Error, B: Error> ErrorCombiner<A, B> for Either<A, B>
, no?
It is required to compose DeadCodeElimPass
in test_then
.
Currently We have several "passes": monomorphization, dead function removal, constant folding. Each has its own code to allow setting a validation level (before and after that pass). This PR adds the ability chain (sequence) passes;, and to add validation before+after any pass or sequence; and commons up validation code. The top-level `constant_fold_pass` (etc.) functions are left as wrappers that do a single pass with validation only in test. I've left ConstFoldPass as always including DCE, but an alternative could be to return a sequence of the two - ATM that means a tuple `(ConstFoldPass, DeadCodeElimPass)`. I also wondered about including a method `add_entry_point` in ComposablePass (e.g. for ConstFoldPass, that means `with_inputs` but no inputs, i.e. all Top). I feel this is not applicable to *all* passes, but near enough. This could be done in a later PR but `add_entry_point` would need a no-op default for that to be a non-breaking change. So if we wouldn't be happy with the no-op default then I could just add it here... Finally...docs are extremely minimal ATM (this is hugr-passes), I am hoping that most of this is reasonably obvious (it doesn't really do a lot!), but please flag anything you think is particularly in need of a doc comment! BREAKING CHANGE: quite a lot of calls to current pass routines will break, specific cases include (a) `with_validation_level` should be done by wrapping a ValidatingPass around the receiver; (b) XXXPass::run() requires `use ...ComposablePass` (however, such calls will cease to do any validation). closes #1832
Currently We have several "passes": monomorphization, dead function removal, constant folding. Each has its own code to allow setting a validation level (before and after that pass).
This PR adds the ability chain (sequence) passes;, and to add validation before+after any pass or sequence; and commons up validation code. The top-level
constant_fold_pass
(etc.) functions are left as wrappers that do a single pass with validation only in test.I've left ConstFoldPass as always including DCE, but an alternative could be to return a sequence of the two - ATM that means a tuple
(ConstFoldPass, DeadCodeElimPass)
.I also wondered about including a method
add_entry_point
in ComposablePass (e.g. for ConstFoldPass, that meanswith_inputs
but no inputs, i.e. all Top). I feel this is not applicable to all passes, but near enough. This could be done in a later PR butadd_entry_point
would need a no-op default for that to be a non-breaking change. So if we wouldn't be happy with the no-op default then I could just add it here...Finally...docs are extremely minimal ATM (this is hugr-passes), I am hoping that most of this is reasonably obvious (it doesn't really do a lot!), but please flag anything you think is particularly in need of a doc comment!
BREAKING CHANGE: quite a lot of calls to current pass routines will break, specific cases include (a)
with_validation_level
should be done by wrapping a ValidatingPass around the receiver; (b) XXXPass::run() requiresuse ...ComposablePass
(however, such calls will cease to do any validation).closes #1832