quil-rs icon indicating copy to clipboard operation
quil-rs copied to clipboard

feat!: change `InstructionHandler` to be a trait and require it everywhere in Rust

Open antalsz opened this issue 2 months ago • 4 comments

This PR removes the InstructionHandler type that is defined in terms of boxed closures, and replaces it with an InstructionHandler trait. This has the potential to provide signficant performance improvements (and I think it's probably cleaner regardless).

This PR also removes every Rust method that could use InstructionHandler but doesn't, requiring a deliberate use of an instruction handler. However, because we don't expose instruction handlers to Python, this is not true for our Python bindings. Ideally, this should not be a change for Python users, but I would appreciate close review of this part.

antalsz avatar Oct 12 '25 04:10 antalsz

PR Preview Action v1.6.2 :---: |

:rocket: View preview at
https://rigetti.github.io/quil-rs/pr-preview/pr-484/

|
Built to branch quil-py-docs at 2025-11-07 21:08 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

github-actions[bot] avatar Oct 12 '25 04:10 github-actions[bot]

This has the potential to provide signficant performance improvements (and I think it's probably cleaner regardless).

Have you verified any potential improvements with benchmarks?

Shadow53 avatar Oct 28 '25 01:10 Shadow53

I marked this approved, but just noticed the CI checks failed due to an MSRV issue. Can you fix that up before merging?

asaites avatar Nov 10 '25 17:11 asaites

I marked this approved, but just noticed the CI checks failed due to an MSRV issue. Can you fix that up before merging?

@asaites This is a false positive – see rust-lang/rust-clippy#15792. We're not the only ones running into this with PyO3, either. There's not really a good fix except for ignoring the warning entirely, and I'm not sure that we want to do that.

antalsz avatar Nov 10 '25 17:11 antalsz