Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #892, which merged before this fix round landed. #892 introduced a live regression on
devand this closes it, plus two gaps its independent review found.1. Regression: PECOS's own empty circuit no longer round-trips
#892 made a quantum declaration's
sizerequired and strictly positive, matchingspecification/v0.1/spec.md:141and upstreamphir.model.QVarDefine(size: intwithGt(gt=0)). Correct rule — but three PECOS producers emitsize: 0for an empty register, so PECOS generates documents its own readers now reject.On
devatc0d311267,to_phir_dict(QuantumCircuit())emits:[{"data": "qvar_define", "data_type": "qubits", "variable": "q", "size": 0}]and the converter, AST deserialization, the Rust interpreter and the engine all reject it. With this change the same call emits
[]and round-trips through every entry point.The rule is not relaxed. The producers are fixed:
python/quantum-pecos/src/pecos/circuits/qc2phir.py— skips empty quantum and classical declarations, and no longer emits the implicitqvar_specregister when the circuit has no qubitscrates/pecos-qasm/src/qasm_to_phir_json.rs— skips empty quantum and classical registers, and drops them from automatic exports so an export cannot name an undeclared registerexp/zlup/src/codegen/phir.rs— skips zero-capacity allocatorsSwept for others: the five
examples/phirprograms, the PHIR and v0.1 READMEs, the specification examples and all thirteen example notebooks contain no zero-size declarations.2. The direct Python reader did not enforce the rule
#892's agreement table claimed Python matched. It did not —
PyPHIR.from_phirvalidated only the type. Measured on that commit: size absent raised a bareKeyError, size0was accepted with count 0, size-1accepted with count −1, and sizetrueaccepted as 1 (becauseboolis anintsubclass). It now requires a positive integer and rejectsboolexplicitly, with the register named in the message. Python's duplicate-overwrite behaviour is untouched.3. Test coverage the earlier suite missed
Three independent mutations passed all six of #892's new Rust tests: removing
from_jsonheader registration, the processor silently coercing size zero to one, and skipping engine runtime declarations. The table test never reached the block executor or engine command execution, and one test discarded the engine without asserting its qubit count.Added: a direct processor zero-size rejection test, engine assertions on qubit count, resolved ids and emitted command ids, and execution of declarations through both the block executor and engine commands. All three mutations now fail, along with the three from #892 (inferring a missing size, accepting a wrong type, rejecting an identical redeclaration).
Python and Rust share one checked-in fixture,
crates/pecos-phir-json/tests/fixtures/empty_quantum_circuit.phir.json: Python asserts the generator's exact output against it, Rust consumes that same document through all four entry points. No cross-language dependency is added in either direction.Verification
cargo clippy --locked --workspace --all-targets --all-features -- -D warnings,cargo test -p pecos-phir-json -p pecos-qasm -p zlup --no-fail-fast(1689 passed),cargo fmt --check,just python-ci-build-test,just pytest-ci-core-shard rest(6752 passed),just python-ci-lint. Branch is level withdev.How this was produced
Implemented by OpenAI
gpt-6-astra(Codex CLI 0.154.0) from findings I wrote after an independent Codex review arm, blind to the implementation session, attacked #892 with build access. That arm found all three items here; I reproduced the regression myself before acting on it, and it also confirmed that no existing validation was lost at any entry point and that #892's removal of the engine size scan is safe. The producer sweep beyondqc2phir.py— the QASM emitter and ZLUP — came from asking whether the same defect existed elsewhere rather than fixing only the reported instance. Claude reviewed every hunk, re-ran the named lanes, and mutation-checked the producer fix independently. Posted at the maintainer's request.