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!: ComposablePass trait allowing sequencing and validation #1895

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

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Jan 28, 2025

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

@hugrbot
Copy link
Collaborator

hugrbot commented Jan 28, 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_passes::validation::ValidationLevel, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/validation.rs:16
enum hugr_passes::validation::ValidatePassError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/validation.rs:28
enum hugr_passes::MonomorphizeError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/monomorphize.rs:282

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It 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_variant_missing.ron

Failed in:
variant ConstFoldError::ValidationError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/const_fold.rs:47
variant RemoveDeadFuncsError::ValidationError, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/dead_funcs.rs:31

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It 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/inherent_method_missing.ron

Failed in:
RemoveDeadFuncsPass::validation_level, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/dead_funcs.rs:73
DeadCodeElimPass::validation_level, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/dead_code.rs:91
DeadCodeElimPass::validation_level, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/dead_code.rs:91
MonomorphizePass::validation_level, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/monomorphize.rs:290
ConstantFoldPass::validation_level, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/const_fold.rs:57

--- failure method_requires_different_generic_type_params: method now requires a different number of generic type parameters ---

Description:
A method now requires a different number of generic type parameters than it used to. Uses of this method that supplied the previous number of generic types will be broken.
      ref: https://doc.rust-lang.org/reference/items/generics.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/method_requires_different_generic_type_params.ron

Failed in:
hugr_passes::RemoveDeadFuncsPass::run takes 0 generic types instead of 1, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dead_funcs.rs:87
hugr_passes::MonomorphizePass::run takes 0 generic types instead of 1, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/monomorphize.rs:263
hugr_passes::const_fold::ConstantFoldPass::run takes 0 generic types instead of 1, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/const_fold.rs:97

--- 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_passes::validation, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/validation.rs:1

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 88.39779% with 21 lines in your changes missing coverage. Please review.

Project coverage is 83.76%. Comparing base (fdcd48a) to head (80cedfc).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
hugr-passes/src/composable.rs 88.70% 10 Missing and 4 partials ⚠️
hugr-passes/src/monomorphize.rs 77.77% 1 Missing and 3 partials ⚠️
hugr-passes/src/const_fold.rs 60.00% 1 Missing and 1 partial ⚠️
hugr-passes/src/dead_funcs.rs 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1895      +/-   ##
==========================================
+ Coverage   83.74%   83.76%   +0.02%     
==========================================
  Files         204      208       +4     
  Lines       38585    39015     +430     
  Branches    35395    35723     +328     
==========================================
+ Hits        32312    32680     +368     
- Misses       4472     4498      +26     
- Partials     1801     1837      +36     
Flag Coverage Δ
python ∅ <ø> (∅)
rust ∅ <88.39%> (∅)

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.


fn sequence(
self,
other: impl ComposablePass<Err = Self::Err>,
Copy link
Contributor Author

@acl-cqc acl-cqc Mar 14, 2025

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)

@acl-cqc acl-cqc marked this pull request as ready for review March 14, 2025 14:40
@acl-cqc acl-cqc requested a review from a team as a code owner March 14, 2025 14:40
@acl-cqc acl-cqc requested a review from cqc-alec March 14, 2025 14:40
Copy link
Collaborator

@cqc-alec cqc-alec left a 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.

@acl-cqc acl-cqc added the breaking-change Changes that break semver label Mar 18, 2025
@acl-cqc
Copy link
Contributor Author

acl-cqc commented Mar 18, 2025

Breaking, so will leave merging until after end March

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better Pass Infrastructure
3 participants