Validate PHIR JSON quantum declarations through one shared rule at every entry point - #892
Merged
Merged
Conversation
This was referenced Sep 28, 2026
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.
Closes #886.
Problem
PECOS's PHIR JSON entry points disagreed about what a legal quantum declaration is, in opposite directions. Verified on
devatbb5932b06:phir_json_to_moduleserde_json::from_str::<PHIRProgram>{"data":"qvar_define","variable":"q","size":2}(nodata_type){"data":"qvar_define","data_type":"u32","variable":"q","size":2}The v0.1 specification marks
data_typeoptional andsizerequired (specification/v0.1/spec.md:141), and the upstreamphir.model.QVarDefineagrees independently:data_type: str | None,size: intwithGt(gt=0). So row 1 is a legal program one door rejected, and row 2 an illegal program the other accepted. The fix in #887 had only covered one of the two doors.Two more divergences, found by reading:
classical_interpreter.rsmatched"qvar_define" if data_type == "qubits", so a declaration with any other type fell through a catch-all and was silently ignored — the register never existed, and later arguments naming it failed or mis-resolved.infer_size, which scrapes digits out of the type name, so a quantum declaration with nosizesilently became size 0.Change
One
validate_quantum_declarationbesideinfer_sizeinast.rsstates the rules once: an absentdata_typemeans"qubits"; a present value that is not"qubits"errors naming the register and the value;sizeis required and must be positive; overflow errors. A siblingdeclaration_sizeroutes quantum declarations through it and leaves classical inference untouched, so a quantum size is never inferred.Every entry point calls it: the AST deserializer, the converter pre-scan and emission, the Rust interpreter, the processor's declaration handler, the engine's constructors and command execution, and the block executor.
pyphir.pytakes the minimal local fix — default an absent type, keep its existing wrong-type rejection — with a comment naming the Rust validator as the normative statement rather than delegating across the language boundary.Duplicate policy stays deliberately non-uniform, documented on the validator:
OperationProcessor::add_quantum_variabletolerates an identical redeclaration because the engine visits declarations both when loading the header and during execution, while rejecting a conflicting one; the converter and interpreter see each op once and reject any duplicate. Python's overwrite behaviour is left as it was.Also closed: an
Operationclassification path where a declaration carrying avariableskey was parsed as a data export, bypassing declaration validation entirely.Agreement after the change
"qubits", positive size"u32"on a quantum declarationTests
tests/quantum_declarations.rsplustests/pecos/unit/test_quantum_declarations.py: the verdict table above asserted entry point by entry point, declaration-order ids under an omitted type, the processor's duplicate policy both ways, converter and interpreter rejecting duplicates, constructed ASTs unable to bypass validation, and size overflow.Mutation-checked: accepting a present wrong type, and inferring a quantum size instead of erroring, each fail
declaration_verdictsandconstructed_ast_cannot_bypass_validation; rejecting an identical redeclaration fails the processor policy test.Verification
cargo clippy --locked --workspace --all-targets --all-features -- -D warnings(the lane CI runs),cargo test -p pecos-phir-json -p pecos-phir -p pecos-qasm --no-fail-fast(990 passed),cargo fmt --check,just python-ci-build-test,just pytest-ci-core-shard rest(6743 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 a task packet written by Claude Fable 5.1 in Claude Code. The first attempt reached the contract by filtering quantum declarations out of upstreamPhirModel.model_validate, on the premise that the external schema "incorrectly" required a positive size; I readQVarDefineand found the schema correct, so that was reverted along with a new pyo3 validation binding and an unrequested change to Python's duplicate handling, and the contract was corrected to reject zero instead. Claude reviewed every hunk, re-ran the named lanes, and repeated the mutation checks independently — including discarding one of its own mutations that failed to compile and therefore proved nothing. Posted at the maintainer's request.