Add linting of vtable slots compared to exports by jakelishman · Pull Request #15778 · Qiskit/qiskit

jakelishman · 2026-03-07T00:35:01Z

A risk of having the definitions of C API functions (cext) separate to the crate defining the static vtable structure (cext-vtable) is that these can more easily get out of sync. This extends the capabilities of qiskit-bindgen-c to add linting of those two crates against each other, providing a convenience wrapper (tools/check_slots.sh) to run a suitable set of checks, and enforcing this in CI.

Depends on:

Add C API vtable to pyext #15761

I will also write a follow-on linter to this that checks for ABI stability between different Qiskit versions (some of the code structure of this patch already indicates where I'm going here - I cut this patch out of a WIP commit).

This isn't a public code feature for 2.4, just an internal QA tool for developers.

qiskit-bot · 2026-03-07T00:35:06Z

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

@Qiskit/terra-core

jan-an

Hi Jake, thank you so much for this PR.
Looks good to me, I just have some very minor questions / comments.

jan-an · 2026-03-17T23:08:58Z

      - name: Run lint
        run: |
          make cformat
+          tools/check_slots.bash


Given that this lint check may not be relevant for every PR but only when there are changes to files such as in crates/cext or crates/cext-vtable, would it make sense to want to decouple this to have that kind of control, say such as via a dedicated lint check for C API related check that is skipped for example, on a python-changes-only sort of PR but runs when there are relevant changes?

A related comment that perhaps answers my above question. I noticed that further into the PR that we also have more changes where we have separated standalone C API exports (qiskit.h) from Python-extension-specific exports (qiskit/funcs_py.h), meaning that this lint check would need to cover a larger set of potentially modifiable files. So running it on every PR is likely the simpler and safer choice?

Your point is totally valid - ideally we'd only do differential linting - but it's pretty hard to get working reliably in practice. You really don't want false negatives in your lint/test suite, and it's very easy to accidentally introduce false negatives when we do incremental checking. It's easier to err on the safe side and just always do it - the build should not be expensive.

jan-an

Just a few more additional minor comments and suggestions correcting typos. Thank you!

jan-an · 2026-03-18T10:18:07Z

+    .add_child(250, &circuit_library::FUNCTIONS);
+pub static FUNCTIONS_QI: ExportedFunctions =
+    ExportedFunctions::empty().add_child(0, &sparse_observable::FUNCTIONS);
+pub use transpiler::FUNCTIONS as FUNCTIONS_TRANSPILE;


Is there a reason FUNCTIONS_TRANSPILE is re-exported as an alias whereas the others are directly declared statically?

This is actually introduced in a parent commit of this one (you've reviewed a complete changeset, not just the single change of the last commit), and I talked about it a bit more here: #15761 (comment)

mrossinek

Overall this looks great. Just a few comments from my side 👍

mrossinek · 2026-03-19T08:56:28Z

+                ("circuit", &qiskit_cext_vtable::FUNCTIONS_CIRCUIT),
+                ("transpile", &qiskit_cext_vtable::FUNCTIONS_TRANSPILE),
+                ("qi", &qiskit_cext_vtable::FUNCTIONS_QI),


Just noting that, this hard-coded list of VTables to check against needs to be kept in sync with the root-level VTables of the crate that is being checked. While probably not ideal, this is the simplest solution for now. But this should be documented over in the cext-vtable crate accordingly.

It does, yeah. The trouble is that there are several associations of name: vtable used in various places (C header files, vtable creation, this linter, etc) and I didn't want the vtables to actually have to own them all. If we come up with something better, we can swap to it.

mrossinek · 2026-03-19T09:10:28Z

This is definitely beyond scope right now (i.e. for 2.4) but I think it would be great if we could add some tests to make sure that these lints actually catch the errors that they are supposed to catch.

One edge case that I have in mind immediately is for example duplicate exported functions but across different root-level vtables..

Fwiw this should work - I tested it locally and used it to catch errors - but yeah, it would be better to have tests.

mrossinek · 2026-03-19T09:16:29Z

+cargo run -p qiskit-bindgen-c -- lint-slots -c crates/cext
+```
+
+This checks various coherence properties between the declared `extern "C"` functions in


What is the point of allowing --cext-path be specified from the CLI while hard-coding qiskit-cext-vtable internally?

I'm not suggesting we relax the hard-coding of the latter, just questioning the flexibility on the former instead of hard-coding that, too.

because qiskit_cext_vtable is a compiled-in dependency, so cargo arranges for it to be compiled into the binary, but cbindgen needs to be pointed at the source tree on disk to parse qiskit-cext

mrossinek · 2026-03-19T09:16:58Z

+all functions have a slot and there are no duplicates; in other words, that each exported function
+is referenced exactly onces.
+
+Note that this command does not test for ABI compatibility between different Qiskit versions.


Is this something you plan to be able to lint in the future?

Yes - I have half of it on a local laptop, and it's just waiting for me to finish off writing a simple diff algorithm into it and pushing it up

mrossinek · 2026-03-19T09:18:24Z

+
+- `no-export`: assert that the function will not be in the slots list (such as for functions
+  deprecated before the introduction of the vtables).
+- `allow-duplicates`: allow functions to be in more than one slot (if, for example, we want to


Do you have a motivation for allowing this?

Not really a useful motivation: the reason is that I have a lint that forbids you from doing it, so I also added the escape valve to bypass the lint. I don't have a use in mind for bypassing that lint.

Fair enough I suppose. Although given that you don't actually support it below, not having this bypass for the time being might be the easier fix rather than supporting it within the 2.4 time window..

While Qiskitgh-15778 is not yet merged, we don't have CI enforcement that the vtable slots are always completely up-to-date. Several PRs adding new C API functions merged between the merge-base of Qiskitgh-15761[^1] and `main` at its time of merge (and subsequently). This PR uses the machinery in Qiskitgh-15778 to bring the slots completely up-to-date. [^1]: 888c877: Add C API vtable to `pyext`

While gh-15778 is not yet merged, we don't have CI enforcement that the vtable slots are always completely up-to-date. Several PRs adding new C API functions merged between the merge-base of gh-15761[^1] and `main` at its time of merge (and subsequently). This PR uses the machinery in gh-15778 to bring the slots completely up-to-date. [^1]: 888c877: Add C API vtable to `pyext`

mrossinek

Nice, I like the way you went about allowing duplicates (even though we still don't have a use case for it, per se) 👍

mrossinek · 2026-03-20T09:24:11Z

+    // SAFETY: per documentation, we are attached to a Python interpreter, `object` points to a
+    // valid Python object and `address` points to enough space to write a pointer.


Although valid changes, these seem unrelated to this PR

They're from Janani's review above

mrossinek · 2026-03-20T09:24:41Z

+    your compiler's search path using :func:`qiskit.capi.get_include`.
 build:
-  - Packages dependening on the Qiskit C API to build can now specify Qiskit as a Python-space build
+  - Packages depending on the Qiskit C API to build can now specify Qiskit as a Python-space build


Same as previous comment: relevant but unrelated

jan-an

Thanks for the changes Jake, LGTM 👍🏻

jan-an · 2026-03-20T13:34:20Z

Thanks for the changes Jake, LGTM 👍🏻

There seem to be a couple of failures in the CI jobs, I've re-run them just to be sure there is something needing correction.. if it fails again, maybe some minor change maybe needed to account for that

A risk of having the definitions of C API functions (`cext`) separate to the crate defining the static vtable structure (`cext-vtable`) is that these can more easily get out of sync. This extends the capabilities of `qiskit-bindgen-c` to add linting of those two crates against each other, providing a convenience wrapper (`tools/check_slots.bash`) to run a suitable set of checks, and enforcing this in CI. Co-authored-by: Janani Ananthanarayanan <101062130+jan-an@users.noreply.github.com>

jakelishman · 2026-04-28T10:26:46Z

I'd forgotten this wasn't merged yet - I just rebased it, so it should be good to review (and is unchanged since the last review).

coveralls · 2026-04-28T10:51:41Z

Coverage Report for CI Build 25081912411

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage decreased (-0.08%) to 87.484%

Details

Coverage decreased (-0.08%) from the base build.
Patch coverage: 120 uncovered changes across 5 files (0 of 120 lines covered, 0.0%).
61 coverage regressions across 8 files.

Uncovered Changes

File	Changed	%
crates/bindgen-c/src/lint.rs	65	0.0%
crates/bindgen-c/src/abi.rs	22	0.0%
crates/bindgen/src/lib.rs	17	0.0%
crates/bindgen-c/src/main.rs	9	0.0%
crates/cext-vtable/src/impl_.rs	7	0.0%

Coverage Regressions

61 previously-covered lines in 8 files lost coverage.

File	Lines Losing Coverage	Coverage
crates/transpiler/src/passes/consolidate_blocks.rs	38	87.75%
crates/cext/src/lib.rs	7	20.0%
crates/qasm2/src/lex.rs	6	91.77%
crates/qasm2/src/parse.rs	6	96.68%
crates/bindgen-c/src/main.rs	1	0.0%
crates/circuit/src/parameter/parameter_expression.rs	1	90.53%
crates/circuit/src/parameter/symbol_expr.rs	1	74.25%
crates/qasm2/src/expr.rs	1	93.82%

Coverage Stats


Relevant Lines:	120034
Covered Lines:	105010
Line Coverage:	87.48%
Coverage Strength:	977426.49 hits per line

💛 - Coveralls

mtreinish

Overall I think this looks ready to merge, just a few small inline questions.

mtreinish · 2026-04-28T21:27:09Z

@@ -5,14 +5,25 @@ All the crates in here are called `qiskit-*`, and are stored in directories that
 This crate structure currently serves the purpose of building only a single Python extension module, which still separates out some of the Rust code into separate logical chunks.
 The intention is that (much) longer term, we might be wanting to expose more of Qiskit functionality directly for other languages to interact with without going through Python.


We should probably update this, but we can do it in a follow up.

mtreinish · 2026-04-28T21:28:52Z

 ///
 /// The safety requirements of `qk_transpiler_pass_standalone_vf2_layout_average` must be respected
 /// for `circuit` and `target`.
+/// cbindgen:qk-vtable-rules=[no-export]


This raises a question for me what is the vtable removal policy longer term. Right now it doesn't matter too much, but if we look to a future with abi compatibility are we going to replace the slot of a removed function with a null pointer? I guess the assumption is that inside a major version we won't remove a function from the C API so this is not really a concern. But I hadn't thought of it until I saw this being excluded.

Yeah, and the vtable crate already allows for this - you can replace an export_fn! call in the table defintion with just a raw None (which is also what happens when you use the feature = <whatever> argument with the feature disabled, or to fill in the gaps between sections of the table).

I've got half-baked logic on my personal laptop that adds a semver lint for the slots too, which knows to allow deletions in major releases.

mtreinish · 2026-04-28T21:39:15Z

+use qiskit_cext_vtable::ExportedFunction;
+
+pub struct SlotsLists {
+    pub slots: IndexMap<String, SlotsList>,


I get performance is not critical for this, especially since we're running it in debug mode. But I just have to point out we've standardized on use ahash (or foldhash once #15920 merges) for IndexMap.

I can change this if you prefer - it just feels a bit awkward to me to have to go in and specifically set the hasher in simple uses that aren't on the hot path (or even in release software, in this case).

Like I said performance doesn't matter here so in this particular case don't bother. But it's more it's a common enough of an oversight in places it does matter that I always notice it. I feel like I need to write a lint rule to just auto enforce it.

Now we have a utils crate, perhaps there's a pattern where we define

type IndexMap<K, V> = indexmap::IndexMap<K, V, foldhash::RandomState>; type HashMap<K, V> = hashbrown::HashMap<K, V, foldhash::RandomState>; type HashSet<T> = hashbrown::HashSet<T, foldhash::RandomState>;

and use that throughout the main-crate workspace? That'd also make it easier to switch preferred implementations in the future should we choose.

It wouldn't (need to) apply in this sort of build script / extra repo tooling, but maybe it'd work for the parts you care about?

I like this idea and having a centralized location for this kind of thing makes it a lot easier to work with. It would probably also move our hasher dependency into a single place. I will probably push a PR on top of #15920 to implement this.

We would only need to do it for IndexMap and IndexSet we already have a clippy rule that only disallows std::collections::HashMap and std::collections::HashSet so we are always using hashbrown everywhere and we just defer to their default hasher selection since it tends to be performance optimized.

jakelishman added this to the 2.4.0 milestone Mar 7, 2026

jakelishman requested a review from a team as a code owner March 7, 2026 00:35

github-project-automation Bot added this to Qiskit 2.4 Mar 7, 2026

jakelishman added Changelog: None Do not include in the GitHub Release changelog. C API Related to the C API labels Mar 7, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.4 Mar 7, 2026

jakelishman mentioned this pull request Mar 7, 2026

Provide a safe way for Python extension modules to use the C API #15572

Closed

jakelishman force-pushed the c/slots-lint branch from 4c429bd to c1f9165 Compare March 7, 2026 00:44

jakelishman added the type: qa Issues and PRs that relate to testing and code quality label Mar 7, 2026

alexanderivrii assigned jan-an Mar 10, 2026

jakelishman mentioned this pull request Mar 17, 2026

Add C API vtable to pyext #15761

Merged

3 tasks

jan-an reviewed Mar 17, 2026

View reviewed changes

jan-an reviewed Mar 18, 2026

View reviewed changes

mrossinek reviewed Mar 19, 2026

View reviewed changes

jakelishman mentioned this pull request Mar 19, 2026

Add cext slots added since last update #15838

Merged

jakelishman force-pushed the c/slots-lint branch from c1f9165 to c8033da Compare March 19, 2026 18:32

mrossinek reviewed Mar 20, 2026

View reviewed changes

jan-an reviewed Mar 20, 2026

View reviewed changes

ShellyGarion removed this from Qiskit 2.4 Apr 15, 2026

ShellyGarion added this to Qiskit 2.5 Apr 15, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 Apr 15, 2026

ShellyGarion modified the milestones: 2.4.0, 2.5.0 Apr 15, 2026

jakelishman force-pushed the c/slots-lint branch from d73bd20 to 8817100 Compare April 28, 2026 10:19

jakelishman mentioned this pull request Apr 28, 2026

Rename bindgen-c to bindgen-cli #16096

Merged

3 tasks

mtreinish reviewed Apr 28, 2026

View reviewed changes

Simplify slots-check script

0f4be66

mtreinish approved these changes Apr 28, 2026

View reviewed changes

mtreinish enabled auto-merge April 28, 2026 23:17

mtreinish added this pull request to the merge queue Apr 28, 2026

Merged via the queue into Qiskit:main with commit 0a9bb6f Apr 29, 2026
45 of 47 checks passed

github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Apr 29, 2026

jakelishman deleted the c/slots-lint branch April 29, 2026 06:52

		// SAFETY: per documentation, we are attached to a Python interpreter, `object` points to a
		// valid Python object and `address` points to enough space to write a pointer.

		@@ -5,14 +5,25 @@ All the crates in here are called `qiskit-*`, and are stored in directories that
		This crate structure currently serves the purpose of building only a single Python extension module, which still separates out some of the Rust code into separate logical chunks.
		The intention is that (much) longer term, we might be wanting to expose more of Qiskit functionality directly for other languages to interact with without going through Python.

Uh oh!

Conversation

jakelishman commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiskit-bot commented Mar 7, 2026

Uh oh!

jan-an left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jan-an left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrossinek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrossinek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jan-an left a comment

Choose a reason for hiding this comment

Uh oh!

jan-an commented Mar 20, 2026

Uh oh!

jakelishman commented Apr 28, 2026

jakelishman commented Mar 7, 2026 •

edited

Loading

coveralls commented Apr 28, 2026 •

edited

Loading

mtreinish Apr 28, 2026 •

edited

Loading