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

CHANGELOG.md

@@ -157,6 +157,36 @@ API surface).
 
 ### Fixed
 
+- **A locale-formatted entry could submit ten times what the user typed.**
+  `morph::render::normalizeLocaleNumber` dropped every occurrence of the group
+  separator unconditionally, with no check on placement, so a de-DE user typing
+  the US form `"1.5"` into a price field submitted `15` — a perfectly valid
+  number that nothing downstream could recognise as wrong. `"1.50"` gave `150`,
+  `"1.2.3.4"` gave `1234`, the en-US mirror image `"1,5"` gave `15`, and two
+  equal separators silently ate the decimal. A group separator is now dropped
+  only where one can legally be — preceded by one to three digits, followed by
+  exactly three, never after the decimal separator — and everything else is
+  reported as malformed; equal separators are rejected outright. Every
+  well-formed entry (`"1.050,25"`, `"1.000.000,25"`, an ungrouped `"1050,25"`,
+  fr-FR's U+202F grouping) normalises exactly as before. The JavaScript mirror
+  in `src/qt/forms/qml/DynamicForm.qml` produced byte-identical wrong answers
+  and carries the identical validation now. See `docs/spec/forms/forms.md`,
+  "Locale data formatting"; morph#574.
+- **A deep `Quantity` derivation overflowed the stack.** A running total
+  (`total = total + one` in a loop) records one `ASTNode` per iteration chained
+  through `left`, and every walk over that chain was recursive: destroying a
+  21,000-node chain segfaulted at `-O0` while surviving 200,000 at `-O2`, where
+  clang rewrites the release into a loop, and `equation()` segfaulted at 25,000
+  nodes at every optimisation level. `~ASTNode` now releases the chain through
+  a local worklist and all four `equation()` traversals run over an explicit
+  stack, with the symbolic and substituted renderings unified into one stack
+  machine; `equation()` output is unchanged. `MORPH_QUANTITY_PROVENANCE` keeps
+  its default of `1` — the measured cost is real (54,056 KB and 0.034 s against
+  12,236 KB and 0.006 s for a 200,000-iteration total) but the toggle changes
+  observable behaviour, not just cost, so a bulk path that never calls
+  `equation()` should set it to `0` rather than have it flipped underneath every
+  build that did not. See `docs/spec/util/quantity_type.md`, *Provenance* and
+  *Limitations*; morph#574.
 - **Both of the cross-field rule vocabulary's safety checks were bypassed by
   wrapping a rule in one combinator.** Unsatisfiability detection stopped at
   the first compound node, because it skipped any node without a `fields` key

CMakeLists.txt

@@ -238,15 +238,21 @@ target_sources(morph
 )
 
 # The detail/ headers that public headers include. They are not public surface
-# and are deliberately outside the verified set -- quantity_equation.hpp is
-# included from partway down quantity.hpp and is not self-contained, so
-# VERIFY_INTERFACE_HEADER_SETS would fail on it -- but they must still ship,
-# or an installed morph/util/quantity.hpp cannot find fixed_string.hpp and the
-# install compiles nowhere (morph#232). A second FILE_SET gets them installed
-# without adding them to what is standalone-compiled;
+# and are deliberately outside the verified set -- standalone compilation is a
+# promise made about morph's public headers, and a detail/ header is free to be
+# a fragment included partway down the one that owns it -- but they must still
+# ship, or an installed morph/util/quantity.hpp cannot find fixed_string.hpp
+# and the install compiles nowhere (morph#232). A second FILE_SET gets them
+# installed without adding them to what is standalone-compiled;
 # INTERFACE_HEADER_SETS_TO_VERIFY, which otherwise defaults to *every*
 # interface header set, keeps the verification list exactly as it was.
 #
+# As it happens every detail/ header compiles standalone today (morph#574 made
+# the last one that did not, quantity_equation.hpp, self-contained). That is a
+# convenience, not the rule this exclusion encodes, which is why
+# scripts/test_check_install_export.sh plants its own broken header rather than
+# leaning on one of these staying broken.
+#
 # Adding a detail/ header here is easy to forget, and forgetting is silent:
 # `cmake --install` still exits 0, and the drift guard above skips detail/ by
 # design. That is how morph#540 shipped -- `forms/detail/session_common.hpp`

docs/spec/forms/forms.md

@@ -1333,6 +1333,33 @@ Display formatting is the renderer's duty; the wire stays canonical:
   single byte that never matched, so a perfectly valid `"1 050,25"` typed by a
   French user normalised to `std::nullopt` and the control reported it
   malformed. An empty view means "this locale has no such separator".
+
+  **Grouping is validated, never merely stripped.** A group separator is
+  dropped only where a group separator can legally be: preceded by one to three
+  digits, followed by exactly three more, and never after the decimal
+  separator. `"1.050,25"`, `"1.000.000,25"` and an ungrouped `"1050,25"` all
+  normalise; `"1.5"`, `"1.50"`, `"1.05"` and `"1.2.3.4"` in a de-DE locale are
+  malformed, and so is the en-US mirror image `"1,5"`. This is not
+  strictness for its own sake: dropping every occurrence unconditionally, as
+  both control edges used to, turns a de-DE user's US-style `"1.5"` into `15` —
+  a perfectly valid number, ten times too large, that no downstream check can
+  recognise as wrong, so the user is charged ten times with no diagnostic
+  anywhere (morph#574). The field's job at this edge is to report a fact to the
+  layer that owns the policy, not to produce a number at any price.
+
+  **The two separators must differ.** A non-empty `groupSeparator` equal to
+  `decimalSeparator` is rejected like any other malformed entry — with one
+  string in both roles there is no reading of `"1.5"` the function could
+  defend, and the old code silently ate the decimal. It is reported through the
+  return value rather than an assertion, deliberately: an assertion would make
+  a control edge behave differently in Debug and Release, and would be
+  untestable in the configuration where it fires.
+
+  **Both edges, or neither.** `src/qt/forms/qml/DynamicForm.qml` carries a
+  JavaScript mirror of this function, and a divergence between them is a
+  divergence in what the product accepts. The mirror produced byte-identical
+  wrong answers on all of the cases above and carries byte-identical
+  validation now; changing one without the other is the defect, not the fix.
 - **Timestamps.** The wire value is strict UTC ISO-8601
   ([datetime.md](../util/datetime.md)); a renderer displays and edits in the
   user's zone by shifting a `morph::time::DateTime` with its existing

docs/spec/util/quantity_type.md

@@ -424,10 +424,11 @@ types:
 - **`ASTNode`** — a node in the DAG: its own `ASTUnit current` step, an optional
   symbol `name` (set by `named()` / `NamedQuantity`, which makes the node opaque
   and stops `equation()` expanding it), and `shared_ptr<ASTNode> left` / `right`
-  handles onto the nodes that fed this one. The struct is a plain aggregate, but
-  nodes are **never copied** in practice: sharing is done through the `shared_ptr`,
-  never by duplicating a node — every operation allocates a fresh `ASTNode` and
-  links the (shared) prior ones.
+  handles onto the nodes that fed this one. Copy and move are the defaulted
+  ones, but nodes are **never copied** in practice: sharing is done through the
+  `shared_ptr`, never by duplicating a node — every operation allocates a fresh
+  `ASTNode` and links the (shared) prior ones. The destructor is the one member
+  with a body, and its job is depth (see below), not ownership.
 - **`Context`** — the per-`Quantity` handle: a single `shared_ptr<ASTNode> node`
   pointing at the root of that value's derivation. Copying a `Quantity` copies its
   `Context`, which just bumps the node's refcount — the tree itself is never
@@ -443,6 +444,45 @@ can be shared across differently typed quantities. It is *not* precision-erased:
 every value stored in a node is the exact `Rational` as computed, carrying its
 own runtime `DecimalPlaces` tag.
 
+**Depth is unbounded, so every walk of the DAG is iterative.** The ordinary
+running-total pattern — `total = total + one` in a loop — records one node per
+iteration, chained through `left`, and nothing collapses that chain: a loop over
+*n* wire rows leaves an *n*-deep derivation. A recursive walk of it overflows
+the stack, so none of the walks is recursive:
+
+- **Destruction.** `~ASTNode` detaches its children into a local worklist and
+  releases them one at a time, unlinking a node's own children only when that
+  pop holds its last reference. Every `~ASTNode` the loop reaches therefore runs
+  with both children already null and cannot recurse. Without this the
+  compiler-generated destructor releases the chain through
+  `~shared_ptr` → `~ASTNode` → `~shared_ptr` → …, one frame per node.
+- **`equation()`.** All four traversals — the reference count, the placeholder
+  labelling, and the symbolic and substituted renderings — run over an explicit
+  stack. The two renderings share one stack machine (`EquationRenderer::render`,
+  selected by `RenderMode`) whose frames resume at the same three points a
+  recursive call would: render-or-descend-left, take-left-descend-right,
+  combine.
+
+Measured against the recursive code (morph#574, 8 MiB stack, clang 22.1.8 and
+gcc 16.2.1):
+
+| 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 `-O2` | — | 40,000 |
+
+Two things follow, and both are why the flattening is a specified property of
+the type rather than something left to the optimiser. Destruction *had no
+failing depth at all* under `-O2`, because clang rewrites that particular
+`shared_ptr` chain into a loop — so the defect crashed in Debug and survived in
+Release, the worst signature a defect can have. `equation()`, whose frames hold
+live `Rendered` strings across the call, could not be rewritten that way:
+optimisation only moved its limit, and gcc's limit was lower than clang's
+unoptimised one.
+
 **Nodes are immutable once built.** No operation ever mutates an existing
 `ASTNode` — arithmetic, conversion, and `named()` each allocate a *new* node
 that points at the (unchanged) prior ones. Immutability is what makes sharing
@@ -1219,6 +1259,39 @@ deliberately not attempted):
   never crosses the wire and has a **single consumer**, `equation()`. For hot
   paths that never call `equation()`, build with `MORPH_QUANTITY_PROVENANCE=0`:
   the API stays callable and no nodes are allocated.
+
+  The cost is measured, not estimated. A 200,000-iteration running total
+  (morph#574, clang 22, `-O2`, Linux): **54,056 KB** max RSS and 0.034 s with
+  the default, against **12,236 KB** and 0.006 s with `MORPH_QUANTITY_PROVENANCE=0`
+  — 4.4x the memory and 5.7x the time, for a loop that adds integers. The
+  retained chain is proportional to the loop bound, so a loop whose bound comes
+  from wire input (a ledger replay, a batch of rows) allocates in proportion to
+  that input. **An application that puts `Quantity` on a bulk path, and does not
+  need `equation()` on it, should build with the macro set to `0`.**
+
+  morph#574 proposed flipping the default to `0` on those numbers. It stays at
+  `1`, and the reasoning is recorded here rather than left implicit, because the
+  two readings are both defensible and the disagreement is the interesting part:
+
+  - *For flipping.* Nobody opts into a cost they do not know about, and the
+    price of the default is paid by every build that never calls `equation()`.
+  - *Against, and this is the decision.* The toggle **changes observable
+    behaviour, not just performance** — see the limitation two entries below:
+    with tracing off `equation()` collapses to the bare value and `named()`
+    discards the name. Flipping the default would silently empty the output of
+    both for every existing build that did not set the macro. And provenance is
+    not an incidental extra: this document opens by naming it as the third of
+    the three things a `Quantity` knows, and as the reason a domain application
+    reaches for this type instead of an exact number. A default that turns the
+    distinguishing feature off, silently, to buy speed on paths that can already
+    opt out of it with one flag, trades the wrong way round.
+
+  The crash that report also found is a separate matter and was **not** left to
+  the toggle: a deep chain used to overflow the stack in *either* setting, which
+  is not an acceptable failure mode for a default, and both the destructor and
+  every `equation()` traversal are now iterative (see *Provenance*, "Depth is
+  unbounded"). If the default is ever revisited, it should be revisited on the
+  behaviour argument above, not on the crash — that is fixed.
 - **`int64` ratio overflow for wide-range unit systems.** Conversion ratios are
   exact `Rational`s of 64-bit integers. A unit system spanning many orders of
   magnitude (pico- to tera-, say) risks overflowing a composed chained ratio —

examples/ledger/README.md

@@ -388,11 +388,13 @@ data; the submit→poll job idiom.
   unit at `dp=0` and the type system carries it natively. Named test: a
   JPY leg stores and displays as a true integer, with no `x-rules` gate
   required.
-- **Locale entry**: in de-DE the group separator is "." and the shipped
-  normalizer strips it anywhere — typing `1.5` submits **15**, a silent 10×
-  money error. Pin the behavior, fix (positional grouping validation or
-  reject), and mirror the vectors through `normalizeLocaleNumber` (D5).
-  Related: result *display* in the shipped forms renderer goes through
+- **Locale entry** — *fixed, morph#574*: in de-DE the group separator is "."
+  and the shipped normalizer stripped it anywhere, so typing `1.5` submitted
+  **15**, a silent 10× money error. `normalizeLocaleNumber` and its QML mirror
+  now validate group placement (one to three digits before, exactly three
+  after, never past the decimal separator) and report a malformed entry
+  instead; `"1.050,25"` still normalises. Remaining from this item:
+  result *display* in the shipped forms renderer goes through
   `double` division — balances beyond 2^53 drift on readback while the
   payload is exact. This rung's own views do not: every money label binds
   text the bridge pre-rendered through `ledger::formatMoney`, which is exact