Fix two shipped value-type defects: provenance chains that overflow the stack, and locale group separators that are stripped instead of validated

[Yaraslaut]

Closes #574.

Both defects reproduced first, with the issue's own commands, then fixed, then re-run. The Rational INT64_MIN work (#537 / PR #561) is deliberately untouched.

Part A — the provenance chain

Reproduced

$ ./prov0 200000                      # clang 22.1.8, -O0, 8 MiB stack
total = 200000EUR (chain of 200000 provenance nodes retained)
destroying...
$ echo $?
139                                   # SIGSEGV in the destructor chain

walk	build	last depth that returned	first that segfaulted
destruction	clang -O0	20,800	21,000
destruction	clang -O2	200,000	none found
equation()	clang -O0	24,000	25,000
equation()	clang -O2	50,000	60,000
equation()	gcc 16.2.1 -O2	—	40,000

The memory and time figures reproduce too: 54,056 KB / 0.034 s with the default against 12,236 KB / 0.006 s with MORPH_QUANTITY_PROVENANCE=0, for a 200,000-iteration running total at -O2.

The issue flagged equation() as unverified. It overflows too — gdb puts the crash in EquationRenderer::renderSymbolic, and unlike the destructor it has no -O2 escape, only a higher limit. gcc's limit is lower than clang's unoptimised one.

After

$ ./prov0_fixed 200000; echo $?       # same binary, fixed headers
total = 200000EUR (chain of 200000 provenance nodes retained)
destroying...
0
$ ./prov0_fixed 2000000 >/dev/null; echo $?
0
$ ./eq0_fixed 100000
built chain of 100000 nodes; calling equation()...
equation() returned 4 line(s), first line 500001 chars

• Fix (2), iterative destruction. ~ASTNode detaches its children into a local worklist and unlinks a node's own children only when that pop holds the last reference, so every ~ASTNode the loop reaches has null children and cannot recurse.
• Fix (3), iterative equation(). All four traversals run over an explicit stack. The symbolic and substituted renderings differed only in how they stop, so they became one stack machine (render, selected by a RenderMode) with a three-stage frame — one traversal to get right instead of two. Output is byte-identical; the existing 62 [quantity]/[render]/[locale] cases pass unchanged.
• Fix (4), depth cap: not done. It would change equation()'s documented output contract, which is a design decision for the type's owner rather than a bug fix. Now that depth cannot crash, it is a readability question.

Fix (1) — the default: not flipped, and this is the finding

docs/spec/util/quantity_type.md promises default-on in four places, including a section heading ("Provenance — a build-time toggle, on by default") and the sentence "The default is on: every Quantity is traceable". The issue's own "what would change the verdict" says to close A(1) if provenance-by-default is a deliberate product decision. The spec says it is, so both readings are recorded in the spec rather than one being picked silently:

• For flipping: nobody opts into a cost they do not know about, and every build that never calls equation() pays it.
• Against, and this is the decision: the toggle changes observable behaviour, not just performance — the spec's own limitations section says so. With it 0, equation() collapses to the bare value and named() discards the name. Flipping the default would silently empty both for every existing build that did not set the macro. And provenance is not an incidental extra: the spec opens by naming it as the third of three things a Quantity knows and the reason to reach for the type at all.

The measurement is now in the spec and on the macro, with "a bulk path that never calls equation() should set this to 0" stated outright — so the cost is documented rather than discovered.

Per the issue, (2) and (3) landed regardless of (1): a stack overflow is not an acceptable failure mode for either setting, and it is now fixed for both.

Part B — locale group separators

Reproduced, C++ and the QML mirror

The issue flagged the QML mirror as unverified. I lifted normalizeLocaleNumber out of src/qt/forms/qml/DynamicForm.qml and ran it on the same inputs. It produced byte-identical wrong answers:

                                          C++        QML
normalize("1.5",     dec=",", grp=".")  = "15"       "15"    <-- de-DE, US decimal typed
normalize("1.50",    dec=",", grp=".")  = "150"      "150"
normalize("1.2.3.4", dec=",", grp=".")  = "1234"     "1234"
normalize("1,5",     dec=".", grp=",")  = "15"       "15"    <-- en-US mirror image
normalize("1.5",     dec=".", grp=".")  = "15"       "15"    <-- separators equal

Consistently wrong is still wrong, so both edges are fixed.

After

normalize("1.5",       dec=",", grp=".") = NULLOPT
normalize("1.50",      dec=",", grp=".") = NULLOPT
normalize("1.2.3.4",   dec=",", grp=".") = NULLOPT
normalize("1,5",       dec=".", grp=",") = NULLOPT
normalize("1.5",       dec=".", grp=".") = NULLOPT
normalize("1.050,25",  dec=",", grp=".") = "1050.25"      <-- unchanged
normalize("1.000.000,25", dec=",", grp=".") = "1000000.25"
normalize("1050,25",   dec=",", grp=".") = "1050.25"      <-- ungrouped, unchanged
normalize("1 050,25",  dec=",", grp=" ") = "1050.25"      <-- U+202F too
normalize("1234.5",    dec=".", grp="")  = "1234.5"       <-- identity transform

A group separator is dropped only where one can legally be: preceded by one to three digits, followed by exactly three, never after the decimal separator. A 47-case differential over both implementations confirms the two edges agree on every input, before and after.

Equal separators are rejected through the return value, not an assert — deliberately, and against the issue's suggestion. An assertion would make a control edge behave differently in Debug and Release, and would be untestable in the build where it fires; that is the same signature as the Part A defect in this very ticket. Reported like any other malformed entry instead.

The negativeSign parameter the issue mentions in passing is not in this PR: it is an API change to both edges and all their call sites, and it is not what makes the value wrong. Happy to file it separately if wanted.

The two traps, addressed

1. The -O0 destruction test. Confirmed to fail on unfixed code before claiming it covers anything — headers reverted to origin/master, suite rebuilt, run:

tests/test_quantity.cpp:704: FAILED:
  {Unknown expression after the reported line}
due to a fatal error condition:
  SIGSEGV - Segmentation violation signal

and the same for the equation() case at :729. Both pass on fixed headers. The test comment carries all five measured depths and says plainly that the destruction case is load-bearing in an unoptimised build only (vacuous at -O2, where no depth fails at all), naming the CI legs that are -O0: gcc-debug, clang-debug, the three sanitizer legs, cl-debug.

The equation() case is priced rather than copied from the ticket: at 100,000 nodes it costs 83 s under ASan against 32 s at 70,000, for the same evidence — equation() concatenates the whole chain into one string, so its cost is quadratic in depth. 70,000 sits above every measured survival depth (highest: 50,000 under clang -O2), so it is not vacuous in an optimised build. An earlier draft used 40,000 and would have been; that is in the history as its own commit, because it is exactly the failure mode AGENTS.md is about.

2. The 18 single-locale cases. All 18 passed with the stripping and with the validation alike. Four new cases: normalize("1.5", ",", "."), equal separators, group placement, and a control that every well-formed entry still normalises. Three of the four fail on unfixed code (16 assertions); the fourth is the control and passes either way by design. On the QML side, test_foreignDecimalSeparatorIsRejectedNotAbsorbed fails on the unfixed mirror and passes on the fixed one, verified by reverting DynamicForm.qml and rebuilding.

Specs and docs

The render spec-sync gate is new as of #576 and does apply here; scripts/check_spec_sync.sh on this branch's file list reports OK.

• docs/spec/util/quantity_type.md — the depth table, the iterative-walk requirement as a property of the type, the measured provenance cost, and both readings of the default.
• docs/spec/forms/forms.md, Locale data formatting — grouping validated not stripped, the equal-separator rule with the reason it is not an assertion, and "both edges or neither".
• examples/ledger/README.md — this was listed as an open ladder finding; it now records the fix and keeps the separate double-division display item.
• CHANGELOG.md — one entry each.

Review notes (done inline, no /code-review)

• ~ASTNode and thread safety. The use_count() == 1 check is the only race-sensitive point. If another owner exists the count is ≥ 2 and dropping the handle cannot destroy the node, so nothing is unlinked; if the count is 1 we are the sole owner. Either way the nested ~ASTNode runs with null children. Matches what the spec says about reading a completed derivation from several threads.
• Traversal equivalence. countRefs counts each node once under a seen set, so worklist order cannot change the counts. assignLabels is a left-before-right pre-order, so the right child is pushed first and the left popped first. atomRendering preserves the original short-circuit order exactly, including that a named node wins over a placeholder in symbolic mode and that isPlaceholder is never reached for a named node. The proof that this is right is that equation()'s existing output assertions did not move.
• The scan refactor is behaviour-preserving, not just believed to be: the 47-case corpus output is byte-identical before and after the split into groupingIsWellPlaced, and still identical to the QML mirror.
• detail/quantity_equation.hpp was not self-contained. clang-tidy on the file alone reported 20 unknown type name 'ASTNode' errors and a cascade of bogus findings ("method countRefs can be made static" — it touches three members), because the header is included from quantity.hpp after ASTNode exists. Changing lines in it would have lit up the clang-tidy-diff gate. It now includes util/quantity.hpp back, which #pragma once makes free; checked with clang++ -fsyntax-only -x c++-header on the file alone.

One gate this change broke, and why the repair is not a workaround

scripts/test_check_install_export.sh proves that INTERFACE_HEADER_SETS_TO_VERIFY HEADERS is load-bearing by deleting it and requiring the build to break. It broke because detail/quantity_equation.hpp was not self-contained. Making it self-contained left no broken detail/ header in the tree — every other one already compiled standalone, checked one by one — so the mutation stopped breaking anything and CI said so:

error: NOT caught: the verified header sets widened back to include detail/
       -- the gate passed a tree it should reject

That is the self-test doing its job. Borrowing a real header's brokenness made the case depend on it staying broken, so the next person to fix one would have hit the same wall. The mutation now plants its own detail/selftest_not_standalone.hpp and adds it to the detail/ FILE_SET, so the property still has something to keep out and deleting it still pulls something in. Same three needles, all still toolchain-independent. The CMakeLists comment justified the exclusion by that header being broken; it now states the rule it actually encodes — standalone compilation is a promise about public headers — and notes that every detail/ header compiling standalone today is a convenience, not the rule.

Verification run locally

• morph_tests: 1500 cases, 22,329 assertions, all pass (one expected failure, pre-existing).
• morph_forms_qml_tests (Qt 6.11.2, offscreen): 268 passed, 0 failed.
• clang-tidy-diff (clang-tidy 22, origin/master...HEAD): clean.
• clang-format --dry-run -Werror on every changed C++ file: clean.
• scripts/check_spec_sync.sh, check_spec_citations.sh, check_catch_test_names.sh, check_automoc_includes.sh: all OK.
• Doxygen -DMORPH_BUILD_DOCUMENTATION=ON target doc (FAIL_ON_WARNINGS): builds.
• ASan + UBSan probe over both new paths at 100,000 nodes: no findings.
• scripts/test_check_install_export.sh: all seven cases caught, clean tree passes.
• Branch coverage, the gate's own pipeline (llvm-cov export → aggregate_lcov_branches.py → the floors in check_branch_coverage.py), morph_tests alone: include/morph/render 100.00% (94/94, floor 97), include/morph/detail 100.00% (106/106, floor 97), include/morph/util 96.77% (360/372, floor 93), no partial lines in render or detail. Two of the new locale cases and the shape of the two iterative walks exist to keep those two at 100 — guarding the child pushes instead of handling null on pop left root != nullptr with an arm nothing could take.

Filed rather than folded in

Two parts of #574 are deliberately not in this PR, and since this PR closes #574 they are filed so they are not lost with it:

• util: equation() renders an unbounded derivation in full, quadratically — #574's deferred depth cap #582 — equation() renders an unbounded derivation in full, quadratically. util/render: two reproduced value-type defects that ship by default — Quantity provenance chains and unvalidated locale group-separator stripping #574's fix (4), the depth cap, plus the measurement it did not have: 83 s and a 500,001-character line at 100,000 nodes. It is no longer a crash, which is why it is a separate ticket rather than part of this fix.
• render/forms: normalizeLocaleNumber matches the minus sign as a literal byte, so a U+2212 locale cannot round-trip #583 — normalizeLocaleNumber matches the minus sign as a literal '-' byte, so a U+2212 locale cannot round-trip. util/render: two reproduced value-type defects that ship by default — Quantity provenance chains and unvalidated locale group-separator stripping #574's other "smaller fix while there". It rejects a valid entry rather than mis-converting one, which makes it a different and smaller problem than the group separator was, and an API change to both edges.

The one other adjacent defect I met (double division in the shipped forms renderer's money display) was already recorded in examples/ledger/README.md, and that record is kept.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!