Organize CP MiniZinc parts with their respective meanings#446
Merged
Conversation
After the upcoming source refactor splits prefix/variables/constraints
into their semantic attributes, several tests that depended on the old
prefix-bake stop holding.
Pytest changes
- Index-based reads of model_constraints[i] that targeted declarations
switched to membership checks against model_variables (impossible,
hybrid impossible, deterministic, wordwise builder tests).
- len(model_constraints)+len(model_variables) totals decrement by 2
per affected test - the two prefix items no longer appear in
model_constraints: 1764->1762, 1661->1659, 2442->2440, 438->436,
1361->1359.
- mzn_model_test.test_build_generic_cp_model_from_dictionary_xor_linear
rewritten to call _variables_list.extend(declarations) and
_model_constraints.extend(constraints) directly instead of the old
prefix-bake pattern.
- test_assemble_model_preserves_legacy_order renamed to
test_assemble_model_orders_variables_constraints_outputs and clears
_model_prefix so its narrow ordering check still holds; the
"legacy" framing no longer matched what the test actually verifies.
Doctest changes
- mzn_model.py model_constraints property: switched the
`constraints[29]` index check to a membership check, since
model_constraints no longer starts with the include + helpers block.
See CP_MINIZINC_TEST_CHANGES_EXPLANATION.md for the per-test rationale.
Builders now place each MiniZinc fragment in the attribute that matches
its semantic role:
_model_prefix → only includes + helper predicates
_variables_list → only variable declarations
_model_constraints → only constraints + solve directive
mzn_output_directives → only output directives
Previously declarations from `input_*` methods were stuffed into
_model_prefix and constraints from those methods landed in
_variables_list. Each builder then baked the prefix into
_model_constraints with `_model_constraints = _model_prefix +
_model_constraints` so the assembled model came out right by accident.
Changes
- MznModel.current_model_parts() now exposes
`prefix=list(self._model_prefix)` so assemble_model() picks it up
automatically; builders no longer need the prefix-bake.
- Every CP builder updated to put declarations in _variables_list and
constraints in _model_constraints directly (mzn_cipher_model,
mzn_xor_differential_model, mzn_xor_linear_model,
mzn_deterministic_truncated_xor_differential_model,
mzn_semi_deterministic_truncated_xor_differential_model,
mzn_differential_linear_model, mzn_impossible_xor_differential_model,
mzn_hybrid_impossible_xor_differential_model,
mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model).
- Custom solve assemblies (impossible, hybrid impossible, two-step
trail search) now include _model_prefix explicitly since they bypass
assemble_model().
- solve_for_ARX and write_minizinc_model_to_file pass
prefix=list(self._model_prefix) in their explicit MiniZincModelParts;
without it the helper block (modadd_linear, etc.) was missing once
the prefix-bake was removed.
- Trail-search override of input_xor_differential_constraints now
returns the table constraint wrapped in a list, matching its parent
contract. The override previously returned a bare string, which the
old caller append()-ed as one item; the new caller uses extend(),
which would otherwise iterate the string character by character.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
peacker
approved these changes
May 1, 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.



Organize CP MiniZinc parts with their respective meanings
The scope of this PR is "put the right content in the right attribute". In the next PR we will modify every model to use the transport class MiniZincModelParts introduced in #445. By attributes we mean
_model_prefix,_variables_list,_model_constraints,mzn_output_directives. For example, before we were mixing content that must be in _variable_list with_model_prefix.We needed to update
test_assemble_model_preserves_legacy_orderandtest_assemble_model_accepts_explicit_partsbecause the first one is not anymore a legacy and the second one because we needed to decouple it from a specific cipher (since the "MiniZinc model parts" should not be attached to a cipher )We needed to update more tests in order to fulfill "organize CP MiniZinc parts with their respective meanings" refactor. Here the explanation why we need to update each tests.
Why the tests had to change
Before this refactor, several CP MiniZinc model builders mixed parts:
_model_prefixcollected variable declarations on top of the include directives and helper predicates it was supposed to hold._variables_listreceived items returned as constraints byinput_*methods._model_constraintswithself._model_constraints = self._model_prefix + self._model_constraints.That layout made
model_constraintscarry items that were not constraints (the includes and helpers, plus the input declarations), andmodel_variablescarry items that were not declarations (thecp_constraintsreturned frominput_*methods).The refactor moves each fragment to its semantically correct attribute:
_model_prefixincludedirectives and helper predicates_variables_list_model_constraintsmzn_output_directivesoutput [...]directivesMznModel.current_model_parts()now exposesprefix=list(self._model_prefix)soassemble_model()produces the same final MiniZinc source without each builder having to pre-merge the prefix into_model_constraints.This invalidates two kinds of test assertions that lived in the test suite:
model_constraints[i]that returned a declaration. Those declarations now live inmodel_variables, notmodel_constraints.len(model_constraints) + len(model_variables). The two prefix items (include "globals.mzn";and theMINIZINC_USEFUL_FUNCTIONSblock) no longer appear inmodel_constraintsafterdeduplication, so the combined count drops by exactly 2.
The sections below document each updated test and why.
tests/unit/cipher_modules/models/cp/mzn_model_test.py
test_assemble_model_orders_variables_constraints_outputs(line 31, wastest_assemble_model_preserves_legacy_order)Changes:
test_assemble_model_preserves_legacy_order— the new name describes what the test actually verifies now that the prefix is a separate,explicitly-cleared part.
model._model_prefix = []immediately after constructing the model.Reason:
MznModel.__init__callsinitialise_model(), which seeds_model_prefixwith['include "globals.mzn";', MINIZINC_USEFUL_FUNCTIONS]. Thetest sets
_variables_list,_model_constraints, andmzn_output_directivesmanually and then asserts the assembled output is exactly:
Now that
current_model_parts()includesprefix=list(self._model_prefix), the assembled output also contains the seeded include + helper block. The test narrowly exercises the variables → constraints → outputs ordering, so weexplicitly clear
_model_prefixto keep the expected string as-is. The assertion itself is unchanged. The "legacy" framing in the old name no longer fit (the test no longer mirrors the pre-refactor_model_constraintsprefix-bake), so the test is renamed to describe what it now checks.test_build_generic_cp_model_from_dictionary_xor_linear(line ~340)Change: replaced the legacy "extend prefix with declarations and bake the prefix into constraints" pattern with the new direct placement:
Before:
After:
Reason: the test was reproducing the bug at the call site — declarations were sent to
_model_prefix, constraints were sent to_variables_list, and then the prefix was baked into_model_constraints. Withcurrent_model_parts()now returning the prefix, repeating that pattern would emit the prefix twice. The new code mirrors what the production builders now do and exercises the same end-to-endmodel.solve(...)path.test_minizinc_model_parts_lines_concatenates_in_order(wastest_assemble_model_accepts_explicit_parts)Change: rewrote the test to call
MiniZincModelParts.lines()directly, asserting on the returned list. Removed theSpeckBlockCipher/MznModel(speck)setup.Before:
After:
Reason: the assertion only verified that
prefix + variables + constraints + outputs + carries_outputsare returned in that order — pure-function behaviour of the dataclass. The cipher and theMznModelinstance were not used by the assembly path whenpartsis provided (assemble_model_lines(parts)returnsparts.lines()without touchingself). They were a coupling artefact left over fromMznModel.__init__requiring a cipher. Testing the dataclass directly removes the unnecessary fixture, makes the test faster, and pins the contract on the layer that actually owns it. TheMznModel.assemble_modeldelegation is still exercised bytest_assemble_model_orders_variables_constraints_outputsabove.tests/unit/cipher_modules/models/cp/mzn_models/mzn_impossible_xor_differential_model_test.py
test_build_impossible_xor_differential_trail_with_extensions_model(line 11)Change 1 — total count:
1764 → 1762.Change 2 — index-based assertions replaced with membership checks:
Before:
After:
Reason: the strings being asserted are MiniZinc array declarations (start with
array[...] of var ...:). Under the old buggy behavior they ended up in_model_constraintsbecauseinput_impossible_constraints_with_extensionsreturned(declarations, constraints)and the builder put the declarations in_model_prefix, which then got baked into_model_constraints. After the refactor those declarations live in_variables_list. The exact index they occupy depends on the order in whichdirect_variables,inverse_variables,key_schedule_variables,constants_variables, and thecp_declarationsfrominput_impossible_constraints_with_extensionsare appended, plus the deduplication pass at the bottom of the builder. Membership checks are robust to this ordering and still verify the declarations are produced.The total drops by 2 because the two prefix items (
include "globals.mzn";andMINIZINC_USEFUL_FUNCTIONS) used to be deduped into_model_constraintsalong withset_of_constraints. After the refactor the prefix lives only in_model_prefix(added at assembly time viacurrent_model_parts()), so it no longer contributes tomodel_constraints + model_variables.test_build_impossible_xor_differential_trail_model(line 30)Change 1 — total count:
1661 → 1659.Change 2 — index-based assertions replaced with membership checks (same
pattern as above).
Reason: identical reasoning. The other builder method (
build_impossible_xor_differential_trail_model) had the same prefix/variables mix-up and the same prefix-baking. The two prefix items disappear from the combined count, and the declarations move tomodel_variables.tests/unit/cipher_modules/models/cp/mzn_models/mzn_hybrid_impossible_xor_differential_model_test.py
test_build_impossible_xor_differential_trail_model(line 10)Change 1 — total count:
2442 → 2440.Change 2 — index-based assertions replaced with membership checks against
model_variables:Reason: all four strings are declarations (the
set of int:line introduces theext_domaintype alias used by every subsequentarray[...] of var ext_domain: ...declaration). They are produced byMznHybridImpossibleXorDifferentialModel.input_constraintsas part ofcp_declarations. Before the refactor they sat in_model_constraintsbecause the builder pushed them through_model_prefix. After the refactor they live in_variables_list. The combined count drops by 2 for the prefix items, same as in the impossible model tests.tests/unit/cipher_modules/models/cp/mzn_models/mzn_deterministic_truncated_xor_differential_model_test.py
test_build_deterministic_truncated_xor_differential_trail_model(line 10)Change 1 — total count:
438 → 436.Change 2 — index-based assertions replaced with membership checks against
model_variables:Reason: the three strings are declarations returned by
input_deterministic_truncated_xor_differential_constraints. Same story as theimpossible-model tests: they used to be in
_model_constraintsvia theprefix-bake, now they are in
_variables_list.tests/unit/cipher_modules/models/cp/mzn_models/mzn_wordwise_deterministic_truncated_xor_differential_model_test.py
test_find_one_wordwise_deterministic_truncated_xor_differential_trail(line 9)Change 1 — total count:
1361 → 1359.Change 2 — index-based assertions replaced with membership checks against
model_variables:Reason:
input_wordwise_deterministic_truncated_xor_differential_constraintsemits these wordwise declarations. They follow the exact same migration path — out of_model_constraints, into_variables_list— and the two prefix items disappear from the combined count.The commented-out twin test (lines 22-59) was left untouched because it is
inactive.
Source-code fixes uncovered while running the test suite
The test run also exposed three production bugs that the old prefix-baking
behaviour was hiding. Each one is included in the same PR because the test
suite cannot pass without them.
claasp/cipher_modules/models/cp/mzn_model.py—solve_for_ARXChange: added
prefix=list(self._model_prefix)to theMiniZincModelPartsthat
solve_for_ARXpasses toassemble_model.mzn_model_string = self.assemble_model( MiniZincModelParts( + prefix=list(self._model_prefix), variables=list(self._model_constraints), constraints=list(self._variables_list), ) )Why the test failed:
solve_for_ARXbuilds an explicitMiniZincModelPartsinstead of going throughcurrent_model_parts(). Before the refactor, the helper-defining block (MINIZINC_USEFUL_FUNCTIONS, whichdefines
modadd_linearand friends) was visible to it because every model baked_model_prefixinto_model_constraints. The refactor removes that bake, so_model_constraintsno longer carries the helpers and the explicit-parts call has to ask for_model_prefixdirectly. Without that linemzn_xor_linear_model_test.py::test_find_one_xor_linear_trail_with_fixed_weight_solve_with_apifails with
MiniZincError: type error: no function or predicate with name 'modadd_linear' found.claasp/cipher_modules/models/cp/mzn_model.py—write_minizinc_model_to_fileSame change for the same reason — the writer constructs an explicit
MiniZincModelPartsand was relying on the prefix being baked into_model_constraints. After the refactor the writer must request the prefixexplicitly.
claasp/cipher_modules/models/cp/mzn_models/mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model.py—input_xor_differential_constraintsoverrideChange: wrap the constraint string in a list before returning it.
Why the tests failed: the override returns a
(declarations, constraints)tuple just like its parent — except it stuffs a single string into the constraints slot instead of a list. The old caller didself._variables_list.append(constraints), which appends the string as one list element, so the bug was invisible. The refactor switched the caller toself._model_constraints.extend(constraints), andlist.extend("constraint …")iterates the string character-by-character, producing a model withc\no\nn\ns\nt\nr\na\ni\nn\nt …after the last real constraint. MiniZinc then rejects line 2027 withError: syntax error, unexpected identifier, no solutions come back, and everymzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.pytest fails downstream withKeyError: 'solution1'.Wrapping the string in a one-element list keeps the contract consistent with the parent (
input_xor_differential_constraintsreturns(list[str], list[str])) and letsextendadd one constraint, not 35characters.
The five tests that started passing again after this fix are:
Doctest changes
claasp/cipher_modules/models/cp/mzn_model.py—model_constraintsproperty docstringChange:
Reason: before the refactor,
_model_constraintsstarted with'include "globals.mzn";',MINIZINC_USEFUL_FUNCTIONS, and the input declarations frominput_xor_differential_constraints. The constraint'constraint rot_0_0[2] = plaintext[11];'therefore landed at index 29. After the refactor_model_constraintscontains only constraints, so the index of any specific constraint shifts. Switching to a membership check matches the property's documented contract ("only the constraints, excluding variable declarations") and is robust to future ordering changes.The
model_variablesdoctest at line 1181 (variables[0] == 'array[0..15] of var 0..1: pre_modadd_0_1_0;') was left as-is. Component declarations are still appended to_variables_listfirst bybuild_xor_differential_trail_model_template; the input-declaration lines frominput_xor_differential_constraintsare appended after them, so index 0 is unchanged.Net effect on the combined count
For every "build_*" test that asserts
len(model_constraints) + len(model_variables), the count drops by exactly 2because:
The prefix items move out of
model_constraintsand out of the combined count. Everything else is conserved (same total of declarations, same total of constraints). That is why every count assertion above goes down by exactly 2.