Make conflicting observable label and Pauli definitions fail in every build - #385
Merged
Conversation
This was referenced Jul 29, 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.
Two definitions of the same observable that disagree about the label or the Pauli were resolved silently in release builds. Now they fail in every build, with a message naming both values.
Found during the review of #384.
The defect
DetectorErrorModel::add_observablemerges a second definition of an already-declared observable. Records combine by parity, which is correct -- they accumulate. But the label and the Pauli identify the observable rather than accumulate, so two definitions disagreeing about either describe different observables sharing an id.That was handled with
debug_assert_eq!. Debug builds panicked; release builds silently kept whichever definition arrived first and discarded the other, producing a quietly mislabelled DEM with no diagnostic. Reachable from user input -- metadata JSON and a circuit annotation contradicting each other is exactly the situation #377 was about.The change
Both checks are now
assert_eq!/assert!, so they fire in every build:Two refinements from review:
kindand records before checking the label and Pauli, so a caught panic left a half-merged output. Validation now happens first. (Notably the borrow checker enforces this shape: moving the mutation earlier fails to compile, because validation borrows fields the mutation moves.)DemOutput::with_paulistrips phase because it carries no DEM meaning, butpauliis a public field that can be set directly. Comparing raw would have rejected-Xagainst+X-- a spurious panic on input the library explicitly treats as equivalent. A newpauli_eq_ignoring_phasecompares phase-free.Deliberately not converted
The remaining
debug_assert!s in this module are not all internal invariants -- an earlier version of this description claimed they were, and that was wrong.FaultMechanism::from_sorted,from_sorted_with_tracked_paulisandMeasurementMechanism::from_sortedarepuband validate their sortedness precondition debug-only, whilexordepends on it.They are left as-is deliberately: the precondition is stated in the function name, these are per-mechanism constructors on the DEM construction path, and always-validating would add an O(n) scan to each call. That is the "trust the caller, state the contract" case rather than boundary validation. Converting them, or adding fallible variants, is a reasonable separate change if the API is revisited.
add_observableand its ~21 callers are infallible, so returningResultwould be a public-API redesign rather than a fix to the silent path. Panicking loudly is the smaller change that removes the actual defect.Also out of scope
Review flagged two wider instances of the same disease, filed for follow-up rather than bundled:
apply_pecos_metadata_jsonandParsedDemstill do last-writer-wins via*existing = targeton paths that already returnResultand could report properly; and duplicate tracked-Pauli / detector ids remain silently merged.Testing
The control is what makes this worth reading. On
dev, in release:Only one of the three tests exists -- both conflict tests were gated
#[cfg(debug_assertions)], so the release-mode silent path was untested by construction. That gate is removed.Each production change was verified to be killed by reverting it:
assert_eq!back todebug_assert_eq!(release)The
should_panicstrings now match the added detail (already labelled "first", relabelled "second"), not just the unchanged prefix, so they would fail if the improved messages regressed.New tests:
duplicate_observable_definitions_ignore_global_phase,conflicting_definitions_do_not_mutate_before_panicking.cargo test -p pecos-qecgreen in debug and release: 597 lib tests, 0 failed.cargo clippy -p pecos-qec --all-targetsclean, forced cold;cargo fmt --all --checkclean.