Skip to content

Resolve PHIR JSON qubit arguments through declaration-order register ranges - #887

Merged
ciaranra merged 3 commits into
devfrom
phir-json-register-addressing
Sep 28, 2026
Merged

ciaranra merged 3 commits into
devfrom
phir-json-register-addressing

Conversation

@ciaranra

Copy link
Copy Markdown
Member

Closes #819.

Problem

The Rust PHIR JSON layers resolved a qubit argument [register, index] to the bare index and discarded the register name, so in any program with more than one qubit register every gate after the first register targeted the wrong qubit. Verified at the IR layer on dev before the fix, for a program declaring a[2] then b[2]:

X on b[0]  ->  Quantum(X) operands=[SSAValue { id: 0 }]
X on a[1]  ->  Quantum(X) operands=[SSAValue { id: 1 }]

b[0] and a[0] resolved to the same SSA value, so the second register aliased the first. a[1] was right only because a is declared first.

Change

Environment now owns declaration-order global qubit ranges (add_quantum_register, resolve_qubit with an explicit out-of-bounds error), and every qubit-argument resolution goes through it:

  • the converter's operands and indexed quantum returns (phir_converter.rs),
  • the shared quantum-argument collector, barrier validation and the machine-operation builders (operations.rs),
  • the classical interpreter, which loses its private qvar_meta duplicate of the same bookkeeping (classical_interpreter.rs).

The numbering matches the authoritative Python path, which is correct today: each qvar_define takes the next contiguous block in declaration order (pyphir.py ~306) and an argument resolves as qvar_meta[qsym].qubit_ids[qid] (~248). The converter reserves SSA ids 0..num_qubits for qubits and allocates every later id through one checked allocator that errors on exhaustion.

Validation tightened where the converter previously accepted malformed input: a qvar_define with no size or a non-qubit data_type is rejected (the spec requires size; the Python reference raises on the type), and a duplicate declaration is refused. That last one is a deliberate divergence — Python allocates a second block and overwrites the name — and is commented as such at the rejection.

Found along the way, filed separately

Tests

Eight-plus regressions in tests/qubit_register_resolution.rs, all at the IR or command layer:

  • the reproduction above, asserting the declaration-order ids;
  • an equivalence oracle independent of the implementation: the module for two registers a[2] b[2] equals the module for one flat q[4] with hand-computed indices, repeated for uneven sizes x[1] y[3] z[2] and for z declared before a so declaration order cannot silently become alphabetical order;
  • the same oracle at the collector/command layer;
  • rejection of an unknown register name and of an index equal to the register size;
  • SSA exhaustion at declaration, instruction and measurement-combining emission, each asserting an error rather than a panic.

Mutation-checked: resolving to the bare index fails 7 of 8 addressing tests including both oracles; making the SSA allocator wrap instead of checking fails all three exhaustion tests.

Verification

cargo test -p pecos-phir-json -p pecos-phir -p pecos-qasm --no-fail-fast (966 passed, 0 failed), cargo clippy --locked on those three crates with --all-targets -- -D warnings run cold after touching the changed files, and cargo fmt --check.

How this was produced

Implemented by OpenAI gpt-6-astra (Codex CLI 0.154.0) from a task packet written by Claude Fable 5.1 in Claude Code; Claude reproduced the defect itself first, reviewed every hunk, re-ran all verification and both mutation checks, and sent findings back for a fix round. An independent Codex review arm, blind to the implementation session, found the SSA-overflow defect that the fix round then closed; every finding was reproduced before being acted on. Posted at the maintainer's request.

@ciaranra
ciaranra force-pushed the phir-json-register-addressing branch from 645d057 to b0c6938 Compare September 27, 2026 20:53
@ciaranra

Copy link
Copy Markdown
Member Author

Updated to b0c6938dc (rebased onto 96ff390c6) after a second, narrowly scoped independent review of the hardening slice — the checked SSA allocator and the new declaration validation, which the first review arm had not seen.

It found one regression this PR had introduced: a qvar_define with data_type omitted was rejected, although the v0.1 specification marks that field optional (specification/v0.1/spec.md:141, "data_type": "qubits", // Optional) and a documented example in exp/zlup/src/codegen/phir.rs omits it. An absent field now defaults to "qubits"; only a value that is present and not "qubits" is rejected, null included. The emitted VarDefine type gets the same default so nothing downstream sees an empty type. Two tests added (omitted type resolves to the right declaration-order ids; "u32" still rejected), and mutation-checked: restoring the old comparison fails them.

That review also settled, by building and running rather than reading, the question the first arm could not: the wrap hazard is genuinely closed in release. It swept allocation budgets 0–14 through declarations, measurement returns, combining and export (0–10 error, 11–14 succeed with ids outside the reserved range) and confirmed black_box(u32::MAX) + black_box(1) wraps in that same build, so the protection comes from the checked allocator and not from debug overflow trapping. It also inventoried all 15 SSAValue construction sites, traced each exhaustion test to its exact allocating line, and extracted and converted 101 PHIR JSON programs from 54 files to check for other newly rejected inputs — the omitted-data_type case was the only one.

Verification on the rebased tree: cargo test -p pecos-phir-json -p pecos-phir -p pecos-qasm --no-fail-fast 969 passed, cargo test --release -p pecos-phir-json --test qubit_register_resolution 15 passed, cold clippy -D warnings and cargo fmt --check clean.

@ciaranra

Copy link
Copy Markdown
Member Author

Fixed in 21d9c59e5. The red CI was a semantic merge conflict from the dev merge, textually clean but non-compiling.

#851 added the RXYXY2Q batch split in phir_converter.rs, whose per-pair result allocation calls self.new_ssa_id() — infallible when #851 was written. This PR makes new_ssa_id() fallible (checked allocation, so exhaustion cannot wrap into the reserved qubit id range). Git merged the two on different lines without complaint and produced a call passing Result<u32, PecosError> where u32 is expected, so the library did not build; rust-lint, rust-lint-no-llvm, pr-core-rust and pr-core-python (qec-guppy) all failed off that one error.

The site now collects into Result<Vec<_>, PecosError> and propagates with ?, which is the only correct shape inside that closure. It was the sole new_ssa_id() call site missing propagation.

My earlier verification missed it because I ran cargo clippy -p on three crates rather than the lane CI actually runs. Re-verified with the recipes by name on the merged tree:

  • cargo clippy --locked --workspace --all-targets --all-features -- -D warnings — clean
  • pecos rust test (release CLI, same runtime hardware detection as CI) — 13400 passed, 0 failed
  • just python-ci-build-test then just pytest-ci-core-shard qec-guppy — 1766 passed, 1 skipped, 8 xfailed
  • just python-ci-lint — clean

Branch is level with dev at 78f8079e6.

@ciaranra
ciaranra merged commit db39539 into dev Sep 28, 2026
60 checks passed
@ciaranra
ciaranra deleted the phir-json-register-addressing branch September 28, 2026 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rust PHIR JSON engine discards the register name when resolving qubit args, mis-addressing every gate in multi-register programs

1 participant