Skip to content

Split bindgen simplified IR from parse/export logic#16144

Merged
raynelfss merged 1 commit intoQiskit:mainfrom
jakelishman:c/ctypes-refactor
May 7, 2026
Merged

Split bindgen simplified IR from parse/export logic#16144
raynelfss merged 1 commit intoQiskit:mainfrom
jakelishman:c/ctypes-refactor

Conversation

@jakelishman
Copy link
Copy Markdown
Member

While starting to write a Rust/PyO3 raw-FFI backend for the C API via the Python package, I realised that a lot of the logic was about to be duplicated from the ctypes output. Had I realised this sooner, I'd have folded this commit into ae919861 to avoid an immediate refactor.

I've not attempted to de-duplicate all logic that ever might occur (for example, lifting a Function is going to look mostly similar but not identical), because there's enough little tweaks between the languages to make the abstraction awkward, and we're unlikely to have enough languages to really warrant it. For example, the ctypes binding needs to know that any custom "path" name of an Enum can't actually be exported to that name, because we use Python-space true Enum classes in the ctypes bindings for input. This restriction isn't present in other languages.

AI/LLM disclosure

  • I didn't use LLM tooling, or only used it privately.
  • I used the following tool to help write this PR description:
  • I used the following tool to generate or modify code:

Footnotes

  1. ae91986: Add auto-generated ctypes bindings to C API (Add auto-generated ctypes bindings to C API #16067)

While starting to write a Rust/PyO3 raw-FFI backend for the C API via
the Python package, I realised that a lot of the logic was about to be
duplicated from the `ctypes` output.  Had I realised this sooner, I'd
have folded this commit into ae91986[^1] to avoid an immediate refactor.

I've not attempted to de-duplicate _all_ logic that ever might occur
(for example, lifting a `Function` is going to look _mostly_ similar but
not identical), because there's enough little tweaks between the
languages to make the abstraction awkward, and we're unlikely to have
enough languages to really warrant it.  For example, the `ctypes`
binding needs to know that any custom "path" name of an `Enum` can't
actually be exported to that name, because we use Python-space true
`Enum` classes in the `ctypes` bindings for input.  This restriction
isn't present in other languages.

[^1]: ae91986: Add auto-generated `ctypes` bindings to C API (Qiskitgh-16067)
@jakelishman jakelishman added this to the 2.5.0 milestone May 5, 2026
@jakelishman jakelishman added the Changelog: None Do not include in the GitHub Release changelog. label May 5, 2026
@jakelishman jakelishman requested a review from a team as a code owner May 5, 2026 21:10
@jakelishman jakelishman added the Rust This PR or issue is related to Rust code in the repository label May 5, 2026
@jakelishman jakelishman requested a review from ShellyGarion May 5, 2026 21:10
@github-project-automation github-project-automation Bot moved this to Ready in Qiskit 2.5 May 5, 2026
@qiskit-bot
Copy link
Copy Markdown
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 25402523711

Coverage decreased (-0.01%) to 87.575%

Details

  • Coverage decreased (-0.01%) from the base build.
  • Patch coverage: 9 uncovered changes across 2 files (185 of 194 lines covered, 95.36%).
  • 26 coverage regressions across 3 files.

Uncovered Changes

File Changed Covered %
crates/bindgen/src/simple_ir.rs 48 41 85.42%
crates/bindgen/src/render/ctypes.rs 143 141 98.6%

Coverage Regressions

26 previously-covered lines in 3 files lost coverage.

File Lines Losing Coverage Coverage
crates/qasm2/src/parse.rs 12 96.21%
crates/circuit/src/parameter/symbol_expr.rs 10 73.97%
crates/qasm2/src/lex.rs 4 91.77%

Coverage Stats

Coverage Status
Relevant Lines: 121863
Covered Lines: 106721
Line Coverage: 87.57%
Coverage Strength: 961580.41 hits per line

💛 - Coveralls

@ShellyGarion
Copy link
Copy Markdown
Member

This PR looks quite straightforward refactor to me.
It basically moves some functionality from ctypes.rs (previously called py_ctypes.rs) to simple_ir.rs,
to reduce future code duplication.
But since I'm not an expert with cbindgen, I would be happy to see if there are any further comments.

@jakelishman jakelishman requested a review from raynelfss May 6, 2026 12:06
Copy link
Copy Markdown
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

Ok, I took my sweet time going through the way this has been re-structured and it makes sense. It looks more clean than before (although one could argue that we now have an IR to the IR), but it looks like some of it might get replaced since Jake's PR to cbindgen has merged.

Since we need a more immediate solution, let's merge this now and update ReprType and co later.

@raynelfss raynelfss added this pull request to the merge queue May 7, 2026
@jakelishman
Copy link
Copy Markdown
Member Author

Yeah, sorry about the churn Ray: I'd hoped to slip this PR into the previous one you reviewed, but you reviewed that faster than I could finish off what I was doing to be sure that this path was what I needed for #16145.

Merged via the queue into Qiskit:main with commit b955d14 May 7, 2026
27 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 May 7, 2026
@jakelishman jakelishman deleted the c/ctypes-refactor branch May 7, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants