diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d9b8dba4..4d1329568 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CMakeLists.txt b/CMakeLists.txt index 82a05bf2a..7ebc6a13c 100644 --- a/CMakeLists.txt +++ b/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` diff --git a/docs/spec/forms/forms.md b/docs/spec/forms/forms.md index 89a005c55..8da1e5dbc 100644 --- a/docs/spec/forms/forms.md +++ b/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 diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index 38ede6139..d0db6bae7 100644 --- a/docs/spec/util/quantity_type.md +++ b/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 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 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 — diff --git a/examples/ledger/README.md b/examples/ledger/README.md index 4f0a1a3af..30f65077e 100644 --- a/examples/ledger/README.md +++ b/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 diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index fece47402..7d6445bda 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -10,14 +10,24 @@ /// lines: formula, substitution, result, and a `where` legend for reused /// values. See `docs/spec/util/quantity_type.md` for the output contract. +#include #include #include #include #include #include +#include #include #include "../attributes.hpp" +// Included back deliberately, and not circular: `util/quantity.hpp` includes +// this file at the very end, `#pragma once` makes the inner visit a no-op, and +// nothing in that header past the include point needs anything defined here. +// It is what makes this file self-contained, which matters because a tool that +// opens a header on its own -- clang-tidy analysing a changed header, an IDE, +// include-what-you-use -- otherwise sees `unknown type name 'ASTNode'` on +// every line and reports a cascade of findings about code that compiles fine. +#include "../util/quantity.hpp" #include "../util/rational.hpp" namespace morph::units::detail { @@ -63,6 +73,42 @@ struct Rendered { int precedence{100}; }; +/// @brief Which of the two renderings `EquationRenderer::render` produces. +enum class RenderMode : std::uint8_t { + /// @brief Symbolic: a named node prints as its name and a reused node as + /// its `cK` placeholder, neither expanded. + symbolic, + + /// @brief Substituted: a named node and a reused node both print as their + /// value. + substituted, +}; + +/// @brief One suspended visit in the iterative renderer — what a recursive +/// `renderSymbolic` call would have kept in its stack frame. +struct RenderFrame { + /// @brief The node being rendered. + const ASTNode* node{nullptr}; + + /// @brief Whether to expand @ref node itself rather than label it. + bool expandThis{false}; + + /// @brief 0 = not started, 1 = left operand rendered, 2 = both rendered. + int stage{0}; + + /// @brief The left operand's rendering, once stage 1 is reached. + Rendered left; +}; + +/// @brief One suspended visit in the iterative `assignLabels` walk. +struct LabelFrame { + /// @brief The node to label or descend through. + const ASTNode* node{nullptr}; + + /// @brief Whether to expand @ref node itself rather than label it. + bool expandThis{false}; +}; + /// @brief Stateful renderer for one `equation()` call. struct EquationRenderer { /// @brief Placeholder number per reused, unnamed node (1-based). @@ -85,53 +131,70 @@ struct EquationRenderer { [[nodiscard]] bool isPlaceholder(const ASTNode* node) const { return refCount.at(node) >= 2; } /// @brief Counts in-edges through displayed paths (stops at opaque nodes). - /// Recurses into children unconditionally; the null guard handles the - /// absent operands of leaf/unary/scalar steps. - /// @param node The current node (may be null). - void countRefs(const ASTNode* node) { - if (node == nullptr) { - return; - } - if (seen.contains(node)) { - return; - } - seen.insert(node); - if (node->name.has_value() || isAtomNode(*node)) { - return; - } - if (node->left) { - ++refCount[node->left.get()]; - } - if (node->right) { - ++refCount[node->right.get()]; + /// + /// Iterative, over an explicit worklist, for the reason spelled out on + /// `render` below: the derivation of a running total is a linear chain and + /// a recursive walk of it overflows the stack (morph#574). Each node is + /// counted exactly once (the `seen` set), so the worklist order does not + /// affect the counts. + /// @param root The node to start from (may be null). + void countRefs(const ASTNode* root) { + std::vector pending; + pending.push_back(root); + while (!pending.empty()) { + const ASTNode* node = pending.back(); + pending.pop_back(); + // Children are pushed unconditionally and the null case handled + // here, exactly as the recursive form handled it on entry: the + // absent operand of a leaf, unary or scalar step is a null child. + if (node == nullptr || seen.contains(node)) { + continue; + } + seen.insert(node); + if (node->name.has_value() || isAtomNode(*node)) { + continue; + } + if (node->left) { + ++refCount[node->left.get()]; + } + if (node->right) { + ++refCount[node->right.get()]; + } + pending.push_back(node->left.get()); + pending.push_back(node->right.get()); } - countRefs(node->left.get()); - countRefs(node->right.get()); } /// @brief Assigns placeholder labels in first-appearance order. - /// @param node The current node. - /// @param expandThis Whether to expand @p node itself (rather than label it). - void assignLabels(const ASTNode* node, bool expandThis) { - if (node == nullptr || node->name.has_value()) { - return; - } - if (!expandThis && isPlaceholder(node)) { - if (!labelIndex.contains(node)) { + /// + /// Iterative for the same reason as `countRefs`. First appearance is a + /// left-before-right pre-order, so the right child is pushed first and the + /// left one popped first — the order a recursive walk would have visited + /// them in. + /// @param root The node to start from. + /// @param expandRoot Whether to expand @p root itself (rather than label it). + void assignLabels(const ASTNode* root, bool expandRoot) { + std::vector pending; + pending.push_back(LabelFrame{.node = root, .expandThis = expandRoot}); + while (!pending.empty()) { + LabelFrame const frame = pending.back(); + pending.pop_back(); + const ASTNode* node = frame.node; + if (node == nullptr || node->name.has_value()) { + continue; + } + if (!frame.expandThis && isPlaceholder(node) && !labelIndex.contains(node)) { labelIndex.emplace(node, placeholderOrder.size() + 1); placeholderOrder.push_back(node); } - if (!isAtomNode(*node)) { - assignLabels(node->left.get(), false); - assignLabels(node->right.get(), false); + // Both the labelled and the unlabelled arm descend exactly when the + // node is not an atom, so the two cases share one exit. + if (isAtomNode(*node)) { + continue; } - return; - } - if (isAtomNode(*node)) { - return; + pending.push_back(LabelFrame{.node = node->right.get(), .expandThis = false}); + pending.push_back(LabelFrame{.node = node->left.get(), .expandThis = false}); } - assignLabels(node->left.get(), false); - assignLabels(node->right.get(), false); } /// @brief Parenthesises and joins a binary subexpression. @@ -145,7 +208,7 @@ struct EquationRenderer { bool const rightNeedsParens = (right.precedence < precedence) || (right.precedence == precedence && (op == "-" || op == "/")); std::string const rightText = rightNeedsParens ? "(" + right.text + ")" : right.text; - return Rendered{leftText + " " + op + " " + rightText, precedence}; + return Rendered{.text = leftText + " " + op + " " + rightText, .precedence = precedence}; } /// @brief Renders a unary-negation subexpression. @@ -153,66 +216,124 @@ struct EquationRenderer { /// @return The combined rendering. [[nodiscard]] static Rendered combineUnary(const Rendered& operand) { std::string const text = (operand.precedence <= 1) ? "-(" + operand.text + ")" : "-" + operand.text; - return Rendered{text, 3}; + return Rendered{.text = text, .precedence = 3}; } - /// @brief Renders a node symbolically (names, placeholders, inlined ops). + /// @brief The rendering of a node that stops the walk — a name, a `cK` + /// placeholder, a leaf value or a conversion result. /// @param node The node. - /// @param expandThis Whether to expand @p node even if it is a placeholder. - /// @return The symbolic rendering. - [[nodiscard]] Rendered renderSymbolic(const ASTNode* node, bool expandThis) { - if (node->name.has_value()) { - return Rendered{"\"" + *node->name + "\"", 100}; - } - if (!expandThis && isPlaceholder(node)) { - return Rendered{"c" + std::to_string(labelIndex.at(node)), 100}; + /// @param expandThis Whether to expand @p node even if named/placeholder. + /// @param mode Symbolic or substituted. + /// @return The atom's rendering, or `std::nullopt` when @p node has + /// operands that must be rendered first. + [[nodiscard]] std::optional atomRendering(const ASTNode* node, bool expandThis, RenderMode mode) const { + if (mode == RenderMode::symbolic) { + if (node->name.has_value()) { + return Rendered{.text = "\"" + *node->name + "\"", .precedence = 100}; + } + if (!expandThis && isPlaceholder(node)) { + return Rendered{.text = "c" + std::to_string(labelIndex.at(node)), .precedence = 100}; + } + } else if (!expandThis && (node->name.has_value() || isPlaceholder(node))) { + return Rendered{.text = formatOptional(nodeValue(*node)), .precedence = 100}; } if (isLeafNode(*node)) { - return Rendered{formatOptional(node->current.lhs), 100}; + return Rendered{.text = formatOptional(node->current.lhs), .precedence = 100}; } if (isConversionNode(*node)) { - return Rendered{formatOptional(node->current.result), 100}; + return Rendered{.text = formatOptional(node->current.result), .precedence = 100}; } - Rendered const left = - node->left ? renderSymbolic(node->left.get(), false) : Rendered{formatOptional(node->current.lhs), 100}; - bool const hasRight = node->right || node->current.rhs.has_value(); - if (!hasRight) { - return combineUnary(left); + return std::nullopt; + } + + /// @brief Renders a node, iteratively. + /// + /// The walk is an explicit stack rather than recursion because the + /// derivation of a running total (`total = total + x` in a loop) is a + /// linear chain one node deep per iteration, and a recursive walk of it + /// runs the stack out. Measured on the recursive code (morph#574, 8 MiB + /// stack): `equation()` returned at 24,000 nodes and segfaulted inside + /// `renderSymbolic` at 25,000 under clang `-O0`, returned at 50,000 and + /// segfaulted at 60,000 under clang `-O2`, and segfaulted already at + /// 40,000 under gcc `-O2`. Optimisation only moved the limit: unlike the + /// operand destructor chain, this recursion cannot be rewritten into a + /// loop, because the frames hold live `Rendered` strings across the call. + /// + /// Each frame resumes where its recursive twin would have: stage 0 renders + /// the node or descends left, stage 1 takes the left result and descends + /// right, stage 2 combines. `finished` carries the rendering of the frame + /// that just popped, in place of a return value. + /// @param root The node to render. + /// @param expandRoot Whether to expand @p root even if named/placeholder. + /// @param mode Symbolic or substituted. + /// @return The rendering of @p root. + [[nodiscard]] Rendered render(const ASTNode* root, bool expandRoot, RenderMode mode) const { + std::vector stack; + stack.push_back(RenderFrame{.node = root, .expandThis = expandRoot}); + Rendered finished; + while (!stack.empty()) { + RenderFrame& top = stack.back(); + if (top.stage == 0) { + if (std::optional atom = atomRendering(top.node, top.expandThis, mode)) { + finished = *std::move(atom); + stack.pop_back(); + continue; + } + top.stage = 1; + if (top.node->left) { + const ASTNode* child = top.node->left.get(); + stack.push_back(RenderFrame{.node = child, .expandThis = false}); + continue; + } + finished = Rendered{.text = formatOptional(top.node->current.lhs), .precedence = 100}; + continue; + } + if (top.stage == 1) { + top.left = std::move(finished); + // Put the slot back into a known state rather than leaving it + // moved-from: it is read again below, on the pop that follows. + finished = Rendered{}; + bool const hasRight = top.node->right || top.node->current.rhs.has_value(); + if (!hasRight) { + finished = combineUnary(top.left); + stack.pop_back(); + continue; + } + top.stage = 2; + if (top.node->right) { + const ASTNode* child = top.node->right.get(); + stack.push_back(RenderFrame{.node = child, .expandThis = false}); + continue; + } + finished = Rendered{.text = formatOptional(top.node->current.rhs), .precedence = 100}; + continue; + } + finished = combine(top.node->current.operation, top.left, finished); + stack.pop_back(); } - Rendered const right = - node->right ? renderSymbolic(node->right.get(), false) : Rendered{formatOptional(node->current.rhs), 100}; - return combine(node->current.operation, left, right); + return finished; + } + + /// @brief Renders a node symbolically (names, placeholders, inlined ops). + /// @param node The node. + /// @param expandThis Whether to expand @p node even if it is a placeholder. + /// @return The symbolic rendering. + [[nodiscard]] Rendered renderSymbolic(const ASTNode* node, bool expandThis) const { + return render(node, expandThis, RenderMode::symbolic); } /// @brief Renders a node with values substituted for symbols/placeholders. /// @param node The node. /// @param expandThis Whether to expand @p node even if named/placeholder. /// @return The substituted rendering. - [[nodiscard]] Rendered renderSubstituted(const ASTNode* node, bool expandThis) { - if (!expandThis && (node->name.has_value() || isPlaceholder(node))) { - return Rendered{formatOptional(nodeValue(*node)), 100}; - } - if (isLeafNode(*node)) { - return Rendered{formatOptional(node->current.lhs), 100}; - } - if (isConversionNode(*node)) { - return Rendered{formatOptional(node->current.result), 100}; - } - Rendered const left = - node->left ? renderSubstituted(node->left.get(), false) : Rendered{formatOptional(node->current.lhs), 100}; - bool const hasRight = node->right || node->current.rhs.has_value(); - if (!hasRight) { - return combineUnary(left); - } - Rendered const right = node->right ? renderSubstituted(node->right.get(), false) - : Rendered{formatOptional(node->current.rhs), 100}; - return combine(node->current.operation, left, right); + [[nodiscard]] Rendered renderSubstituted(const ASTNode* node, bool expandThis) const { + return render(node, expandThis, RenderMode::substituted); } /// @brief Builds one `where`-legend line for a placeholder. /// @param node The placeholder node. /// @return The legend body (`cK = ...`). - [[nodiscard]] std::string legendLine(const ASTNode* node) { + [[nodiscard]] std::string legendLine(const ASTNode* node) const { std::string const label = "c" + std::to_string(labelIndex.at(node)); if (isAtomNode(*node)) { return label + " = " + formatOptional(nodeValue(*node)); diff --git a/include/morph/render/locale_format.hpp b/include/morph/render/locale_format.hpp index 3e4a12332..a23fb37ea 100644 --- a/include/morph/render/locale_format.hpp +++ b/include/morph/render/locale_format.hpp @@ -30,10 +30,80 @@ namespace morph::render { +namespace detail { + +/// @brief Whether every group separator in @p text sits where a group +/// separator can legally sit. +/// +/// The rule, in one place so it can be read on its own: a group separator is +/// preceded by one to three digits (the first group), or by exactly three +/// (every later one); it is followed by exactly three more digits, at each +/// group and at the end of the integer part; and it never appears after the +/// decimal separator. A locale with no grouping (@p groupSeparator empty) has +/// nothing to place, so it trivially passes. +/// +/// This is a pass of its own rather than extra state inside the normalising +/// scan below: "the grouping is well placed" and "the digits convert" are two +/// separate statements about the entry, and reading them as one made neither +/// clear. +/// +/// Characters this function does not recognise are simply not digits — the +/// normalising scan is what rejects them, and it rejects them whatever this +/// pass concludes. +/// @param text The locale-formatted entry. +/// @param decimalSeparator The locale's decimal-point string; may be empty. +/// @param groupSeparator The locale's digit-grouping string; empty means the +/// locale does not group. +/// @return `true` when the grouping is well placed (or absent). +[[nodiscard]] inline bool groupingIsWellPlaced(std::string_view text, std::string_view decimalSeparator, + std::string_view groupSeparator) { + if (groupSeparator.empty()) { + return true; + } + constexpr std::size_t kGroupSize = 3; + std::size_t digits = 0; + bool sawGroup = false; + bool sawDecimal = false; + + for (std::size_t i = 0; i < text.size();) { + const std::string_view rest = text.substr(i); + if (rest.starts_with(groupSeparator)) { + bool const opensAGroup = sawGroup ? digits == kGroupSize : (digits >= 1 && digits <= kGroupSize); + if (sawDecimal || !opensAGroup) { + return false; + } + sawGroup = true; + digits = 0; + i += groupSeparator.size(); + continue; + } + if (!decimalSeparator.empty() && rest.starts_with(decimalSeparator)) { + if (sawGroup && digits != kGroupSize) { + return false; // the last group of the integer part is short + } + sawDecimal = true; + digits = 0; + i += decimalSeparator.size(); + continue; + } + // i is bounded by the loop condition. + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) + char const chr = text[i]; + digits = (chr >= '0' && chr <= '9') ? digits + 1 : 0; + ++i; + } + + // An ungrouped fractional part ends the number, so the trailing check only + // applies when the integer part was the last thing scanned. + return !sawGroup || sawDecimal || digits == kGroupSize; +} + +} // namespace detail + /// @brief Converts a locale-formatted numeric string to canonical /// (`-?[0-9]+(\.[0-9]+)?`) text. /// -/// Strips every occurrence of @p groupSeparator, then replaces every +/// Drops @p groupSeparator where it is correctly placed, and replaces every /// occurrence of @p decimalSeparator with `.`. Passing `decimalSeparator == /// "."` and an empty @p groupSeparator is the identity transform (the /// locale-free behavior). Malformed input (a second decimal separator, a @@ -43,6 +113,25 @@ namespace morph::render { /// ("`,-5`" in a de-DE locale) is rejected -- matching the QML mirror in /// `src/qt/forms/qml/DynamicForm.qml`, which has always rejected it (morph#497). /// +/// @par Grouping is validated, not stripped (morph#574) +/// A group separator is only dropped where a group separator can legally be: +/// preceded by one to three digits, followed by exactly three more, and never +/// after the decimal separator. Anything else is malformed and reported as +/// such. Stripping unconditionally instead is a wrong *value*, not a rejected +/// one: a de-DE user typing the US form `"1.5"` into a price field submitted +/// `15`, and nothing downstream could tell -- the result is a perfectly valid +/// number, ten times too large. `"1.50"` gave `150`, `"1.2.3.4"` gave `1234`, +/// and the en-US mirror image `"1,5"` gave `15`. +/// +/// @par The two separators must differ +/// When @p groupSeparator is non-empty and equal to @p decimalSeparator the +/// entry is rejected: with one string in both roles there is no reading of +/// `"1.5"` the function could defend. This is a caller (locale-configuration) +/// error rather than a user one, but it is reported through the return value +/// like any other malformed entry, deliberately not through an assertion -- +/// an assertion would make the two build configurations behave differently at +/// a control edge, and would be untestable in the one where it fires. +/// /// The result is `.`-decimal and digit-only, but is **not** narrowed to /// `-?[0-9]+(\.[0-9]+)?`: a bare "`.`", a leading "`.5`" and a trailing "`5.`" /// are passed through, exactly as that same QML mirror passes them. Tightening @@ -61,6 +150,13 @@ namespace morph::render { [[nodiscard]] inline std::optional normalizeLocaleNumber(std::string_view text, std::string_view decimalSeparator, std::string_view groupSeparator) { + if (!groupSeparator.empty() && groupSeparator == decimalSeparator) { + return std::nullopt; // one string cannot play both roles: see above + } + if (!detail::groupingIsWellPlaced(text, decimalSeparator, groupSeparator)) { + return std::nullopt; // a separator off a group boundary: see above + } + std::string canonical; canonical.reserve(text.size()); bool sawDecimal = false; @@ -69,8 +165,10 @@ namespace morph::render { for (std::size_t i = 0; i < text.size();) { const std::string_view rest = text.substr(i); if (!groupSeparator.empty() && rest.starts_with(groupSeparator)) { + // Placement was settled above, so by here the separator is display + // only and is never carried into the output. i += groupSeparator.size(); - continue; // grouping is display-only; never accepted back on entry + continue; } if (!decimalSeparator.empty() && rest.starts_with(decimalSeparator)) { if (sawDecimal) { diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index ed4feffaa..a4585a877 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -35,6 +35,15 @@ /// @brief Build-wide provenance toggle. Default on; define to `0` to compile /// the derivation DAG out (no `ASTNode` allocations). +/// +/// The default is `1` deliberately: the toggle changes observable behaviour, +/// not just cost — with it `0`, `equation()` collapses to the bare value and +/// `named()` discards the name — so a build that never set it must not have +/// that output emptied underneath it. The cost is real and measured, though: +/// a 200,000-iteration running total took 54,056 KB and 0.034 s with +/// provenance against 12,236 KB and 0.006 s without (morph#574, clang 22, +/// `-O2`). **A bulk path that never calls `equation()` should set this to `0`.** +/// See `docs/spec/util/quantity_type.md`, *Limitations*. #ifndef MORPH_QUANTITY_PROVENANCE #define MORPH_QUANTITY_PROVENANCE 1 #endif @@ -531,6 +540,66 @@ struct ASTNode { /// @brief Right operand's derivation (shared); null for unary/scalar steps. std::shared_ptr right; + + /// @brief Default-constructs an empty node. + ASTNode() = default; + + /// @brief Copies a node (the children stay shared). + ASTNode(const ASTNode&) = default; + + /// @brief Copy-assigns a node (the children stay shared). + /// @return `*this`. + ASTNode& operator=(const ASTNode&) = default; + + /// @brief Moves a node, leaving the source's children null. + ASTNode(ASTNode&&) = default; + + /// @brief Move-assigns a node, leaving the source's children null. + /// @return `*this`. + ASTNode& operator=(ASTNode&&) = default; + + /// @brief Releases the sub-derivation **iteratively**, so chain depth + /// cannot overflow the stack. + /// + /// The running-total pattern (`total = total + x` in a loop) builds a + /// derivation that is a linear chain down `left`, one node per iteration. + /// The compiler-generated destructor releases that chain recursively — + /// `~shared_ptr` -> `~ASTNode` -> `~shared_ptr` -> ... — one stack frame + /// per node, and a long enough chain runs the stack out. Measured + /// (morph#574, clang 22, `-O0`, 8 MiB stack): a 21,000-node chain + /// segfaults on destruction, while an optimised build survives 200,000 + /// because clang turns the same chain into a loop. "Crashes in Debug, + /// survives in Release" is the worst signature a defect can have, so the + /// flattening is written down here rather than left to the optimiser. + /// + /// Children are detached into a local worklist and released one at a time. + /// A node is only unlinked when this pop holds its last reference; when it + /// does not, dropping the handle cannot destroy it and its children stay + /// where they are. Every `~ASTNode` reached from the loop therefore runs + /// with both children already null, so it cannot recurse. + ~ASTNode() { + std::vector> pending; + if (left) { + pending.push_back(std::move(left)); + } + if (right) { + pending.push_back(std::move(right)); + } + while (!pending.empty()) { + std::shared_ptr const node = std::move(pending.back()); + pending.pop_back(); + if (node.use_count() == 1) { + if (node->left) { + pending.push_back(std::move(node->left)); + } + if (node->right) { + pending.push_back(std::move(node->right)); + } + } + // `node` goes out of scope here: either it was not the last owner + // (nothing happens) or it was, and its children are already null. + } + } }; /// @brief The per-`Quantity` handle onto the root of its derivation. diff --git a/scripts/test_check_install_export.sh b/scripts/test_check_install_export.sh index 37ef80467..9ff216e4e 100755 --- a/scripts/test_check_install_export.sh +++ b/scripts/test_check_install_export.sh @@ -185,21 +185,45 @@ expect_caught "the detail/ header set dropped from the install" \ "detail/instance_directory.hpp" # Bug 2: INTERFACE_HEADER_SETS_TO_VERIFY defaults to *every* interface header -# set, and quantity_equation.hpp is included partway down quantity.hpp and is -# not self-contained. This is the one case the slow phase exists for, so it is -# the one case that runs it. +# set, including the detail/ one, which is deliberately not held to compiling +# standalone. This is the one case the slow phase exists for, so it is the one +# case that runs it. # -# `formatOptionalDecimal` is the identifier the header uses before quantity.hpp -# has declared it. Both compilers name it -- clang as "use of undeclared +# The mutation plants its own non-self-contained detail header rather than +# relying on a real one being broken. It used to point at +# morph/detail/quantity_equation.hpp, which was included partway down +# quantity.hpp and used `formatOptionalDecimal` before that header declared it. +# morph#574 made it self-contained -- a good change for clang-tidy and for any +# tool that opens a header on its own -- and this case went red, correctly: no +# detail/ header was left that VERIFY_INTERFACE_HEADER_SETS would reject, so +# deleting the property no longer broke anything and the mutation could not be +# caught. That is a gate resting on an accident of the tree. Borrowing a real +# header's brokenness meant this case silently depended on it staying broken, +# and the next person to fix one would have hit the same wall. A planted +# fixture cannot be repaired out from under the gate. +# +# The header goes into the detail/ FILE_SET (two paths on one line, which +# `FILES` accepts, because a portable `sed` replacement cannot insert a +# newline), so the unmutated property keeps it out of the verified set and +# deleting that property pulls it in. +# +# `morphSelfTestUndeclaredHelper` is the identifier the planted header calls +# without declaring. Both compilers name it -- clang as "use of undeclared # identifier", GCC as "was not declared in this scope" -- and it appears # nowhere else in the output, which the file name alone cannot promise (ninja # prints that on the progress line for a unit that compiled fine). expect_caught "the verified header sets widened back to include detail/" \ - "edit CMakeLists.txt -e '/INTERFACE_HEADER_SETS_TO_VERIFY HEADERS/d'" \ + "edit CMakeLists.txt \ + -e '/INTERFACE_HEADER_SETS_TO_VERIFY HEADERS/d' \ + -e 's@include/morph/detail/fixed_string.hpp@include/morph/detail/fixed_string.hpp include/morph/detail/selftest_not_standalone.hpp@' \ + && printf '%s\n' \ + '#pragma once' \ + 'inline int morphSelfTestNotStandalone() { return morphSelfTestUndeclaredHelper(); }' \ + > include/morph/detail/selftest_not_standalone.hpp" \ full \ "morph's interface header sets do not all compile standalone" \ - "morph/detail/quantity_equation.hpp" \ - "formatOptionalDecimal" + "morph/detail/selftest_not_standalone.hpp" \ + "morphSelfTestUndeclaredHelper" # Bug 3: install(EXPORT NAMESPACE morph::) prefixes the target name, so # morph_net is exported as morph::morph_net while every in-tree alias, the diff --git a/src/qt/forms/qml/DynamicForm.qml b/src/qt/forms/qml/DynamicForm.qml index 9a317719c..52ecab810 100644 --- a/src/qt/forms/qml/DynamicForm.qml +++ b/src/qt/forms/qml/DynamicForm.qml @@ -807,32 +807,68 @@ Frame { // The payload's exact digit routines below stay entirely locale-free — // this is the one control-edge conversion step, applied once per entry. + // Grouping is *validated*, not stripped (morph#574). A group separator is + // only dropped where one can legally be -- preceded by one to three digits, + // followed by exactly three more, never after the decimal separator. + // Stripping it unconditionally, which both this function and its C++ twin + // used to do, turns a de-DE user's US-style "1.5" into 15: a valid number, + // ten times too large, that nothing downstream can recognise as wrong. + // Verified against the C++ side on the same inputs before and after; the + // two edges agreed on every wrong answer and now agree on every rejection. function normalizeLocaleNumber(text, decimalSeparator, groupSeparator) { - let stripped = "" - for (let i = 0; i < text.length; ++i) { - if (groupSeparator !== "" && text[i] === groupSeparator) - continue - stripped += text[i] - } + // One string cannot play both roles: there is no reading of "1.5" this + // function could defend, so it reports rather than guesses. + if (groupSeparator !== "" && groupSeparator === decimalSeparator) + return null + + const groupSize = 3 let canonical = "" let sawDecimal = false - for (let i = 0; i < stripped.length; ++i) { - const ch = stripped[i] - if (ch === decimalSeparator) { + let sawAnyOutput = false + let digitsInGroup = 0 + let sawGroup = false + for (let i = 0; i < text.length; ++i) { + const ch = text[i] + if (groupSeparator !== "" && ch === groupSeparator) { if (sawDecimal) + return null // grouping belongs to the integer part only + // The first group is one to three digits; every later one is + // exactly three. + const wellPlaced = sawGroup ? digitsInGroup === groupSize + : (digitsInGroup >= 1 && digitsInGroup <= groupSize) + if (!wellPlaced) return null + sawGroup = true + digitsInGroup = 0 + continue + } + if (decimalSeparator !== "" && ch === decimalSeparator) { + if (sawDecimal) + return null + if (sawGroup && digitsInGroup !== groupSize) + return null // the last group is short: "1.5" in de-DE sawDecimal = true canonical += "." - } else if (ch === "-") { - if (i !== 0) + // The decimal point is output, so a sign straight after it is + // not leading (morph#497). + sawAnyOutput = true + continue + } + if (ch === "-") { + if (sawAnyOutput) return null canonical += ch } else if (ch >= "0" && ch <= "9") { canonical += ch + ++digitsInGroup } else { return null } + sawAnyOutput = true } + // A grouped integer part has to end on a group boundary too. + if (sawGroup && !sawDecimal && digitsInGroup !== groupSize) + return null if (canonical === "" || canonical === "-") return null return canonical diff --git a/src/qt/forms/tests/tst_i18n.qml b/src/qt/forms/tests/tst_i18n.qml index cdac769be..655be2e60 100644 --- a/src/qt/forms/tests/tst_i18n.qml +++ b/src/qt/forms/tests/tst_i18n.qml @@ -137,6 +137,32 @@ Item { compare(localeForm.previewLine, '{"mass":{"num":1050250,"den":1000,"dp":3}}') } + // morph#574. The de-DE locale groups with "." and this form's user + // typed the US decimal form. Stripping the group separator + // unconditionally -- which this mirror did, byte for byte in step with + // its C++ twin -- submitted 1.5 as 15: a valid payload, ten times too + // large, with nothing downstream able to tell. The field is now + // reported malformed, which is what the user can act on. + function test_foreignDecimalSeparatorIsRejectedNotAbsorbed() { + localeForm.setFieldValue("mass", "1.5") + verify(!localeForm.ready) + compare(localeForm.previewLine, "") + + localeForm.setFieldValue("mass", "1.50") + verify(!localeForm.ready) + + // A group separator off a group boundary is malformed wherever it + // sits, not only at the end. + localeForm.setFieldValue("mass", "1234.050,25") + verify(!localeForm.ready) + + // Control: the well-formed entry still goes through, so the + // rejection above is about placement and not about "." at all. + localeForm.setFieldValue("mass", "1.050,25") + verify(localeForm.ready) + compare(localeForm.previewLine, '{"mass":{"num":1050250,"den":1000,"dp":3}}') + } + function test_zonedTimestampRoundTripsToUtc() { zonedForm.setFieldValue("when", "2026-07-05T16:30:00") // 16:30 in UTC+2 verify(zonedForm.ready) diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index e84c52b55..932954810 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -670,3 +670,79 @@ TEST_CASE("formatRationalDecimal: an un-canonicalised INT64_MIN numerator render // survive the unsigned negation intact rather than wrapping. REQUIRE(morph::units::detail::formatRationalDecimal(value) == "-9223372036854775808"); } + +// ── morph#574: a deep derivation chain must not overflow the stack ── +// +// `total = total + one` in a loop records one ASTNode per iteration, chained +// through `left`, and nothing collapses the chain. Both walks over it used to +// be recursive -- the compiler-generated `~ASTNode` and every traversal in +// `equation()` -- so a long enough chain ran the stack out. +// +// **What makes each case evidence, and where each one is vacuous.** Both +// depths below were measured against unfixed code with an 8 MiB stack, not +// guessed: +// +// destruction, clang -O0 returns at 20,800 nodes, SIGSEGV at 21,000 +// destruction, clang -O2 returns at 200,000 -- clang rewrites the +// recursive release into a loop, so no depth fails +// equation(), clang -O0 returns at 24,000, SIGSEGV at 25,000 +// equation(), clang -O2 returns at 50,000, SIGSEGV at 60,000 +// equation(), gcc 16 -O2 SIGSEGV already at 40,000 +// +// So the destruction case is load-bearing in an unoptimised build only, and +// passes vacuously in a Release one -- "crashes in Debug, survives in Release" +// is the defect's own signature, and it is not reproducible any other way. CI +// runs it where it bites: gcc-debug, clang-debug, the three sanitizer legs and +// cl-debug are all -O0. The `equation()` case has no such escape: optimisation +// shrinks its frames but cannot eliminate the recursion, because they hold +// live `Rendered` strings across the call, so its depth is chosen to fail in +// every configuration rather than only the unoptimised ones. +namespace { +// Destruction is linear in the chain, so the depth the ticket asked for costs +// nothing to run. +constexpr int kDeepChainNodes = 100000; + +// equation() renders the whole chain into one string by repeated +// concatenation, which is quadratic in the depth, so this one is priced: 70,000 +// nodes cost 32 s under ASan where 100,000 cost 83 s. It stays above the +// highest measured survival depth (50,000, clang -O2) with margin, which is +// what keeps it from passing vacuously in an optimised build. +constexpr int kDeepEquationNodes = 70000; + +// Builds `0 + one + one + ...`, one retained ASTNode per term. +[[nodiscard]] Euro runningTotal(int terms) { + Euro total{Rational{Numerator{0}, Denominator{1}, DecimalPlaces{2}}}; + Euro const one{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; + for (int i = 0; i < terms; ++i) { + total = total + one; + } + return total; +} +} // namespace + +TEST_CASE("A 100000-node provenance chain is destroyed without overflowing the stack", + "[quantity][provenance][morph574]") { + { + Euro const total = runningTotal(kDeepChainNodes); + REQUIRE(total.hasValue()); + REQUIRE(total.value()->toDouble() == static_cast(kDeepChainNodes)); + // The chain is released here. On unfixed code this is a SIGSEGV at -O0, + // which takes the whole test binary with it rather than failing one + // assertion -- the crash *is* the failure signal. + } + SUCCEED("the chain was released without a stack overflow"); +} + +TEST_CASE("equation() walks a 70000-node provenance chain without overflowing the stack", + "[quantity][provenance][equation][morph574]") { + Euro const total = runningTotal(kDeepEquationNodes); + + auto const lines = total.equation(); + // Formula, substitution, result, and one `where` line: the single `one` + // leaf is referenced 70,000 times, so it earns exactly one placeholder. + REQUIRE(lines.size() == 4); + CHECK(lines[0].starts_with("0 + c1 + c1")); + CHECK(lines[1].starts_with(" = 0 + 1 + 1")); + CHECK(lines[2] == " = 70000"); + CHECK(lines[3] == "where c1 = 1"); +} diff --git a/tests/test_render_locale_format.cpp b/tests/test_render_locale_format.cpp index cdd4c3d6a..73b59f38d 100644 --- a/tests/test_render_locale_format.cpp +++ b/tests/test_render_locale_format.cpp @@ -178,3 +178,88 @@ TEST_CASE("normalizeLocaleNumber: the loose shapes stay accepted, in step with t REQUIRE(morph::render::normalizeLocaleNumber("5.", ".", ",") == "5."); REQUIRE(morph::render::normalizeLocaleNumber(".", ".", ",") == "."); } + +// ── morph#574: a group separator is validated, not stripped ────────────────── +// +// Before this, every occurrence of the group separator was dropped +// unconditionally, so a de-DE user typing the US form "1.5" into a price field +// submitted 15 -- a valid-looking number, ten times too large, with no +// diagnostic anywhere. The suite above has 18 cases and not one of them was +// cross-locale: every single-locale case passes with the stripping or with the +// validation, which is exactly the check that would still pass if the feature +// did nothing. + +TEST_CASE("normalizeLocaleNumber: the decimal separator of another locale is rejected, not absorbed", + "[render][locale][morph574]") { + // THE case. de-DE locale, US-style decimal typed: this returned "15". + CHECK(normalizeLocaleNumber("1.5", ",", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.50", ",", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.2.3.4", ",", ".") == std::nullopt); + + // The mirror image: en-US locale, EU-style decimal typed. Returned "15". + CHECK(normalizeLocaleNumber("1,5", ".", ",") == std::nullopt); + + // And with a multi-byte group separator, where the same mistake is a + // narrow no-break space away from a well-formed entry. + CHECK(normalizeLocaleNumber(std::string{"1"} + std::string{kNarrowNbsp} + "5", ",", kNarrowNbsp) == std::nullopt); +} + +TEST_CASE("normalizeLocaleNumber: equal decimal and group separators are rejected rather than guessed", + "[render][locale][morph574]") { + // One string in both roles has no defensible reading, and the old code + // silently ate the decimal: this returned "15". + CHECK(normalizeLocaleNumber("1.5", ".", ".") == std::nullopt); + // Not even the shapes that would be unambiguous if you squinted: the + // rejection is on the configuration, not on the text. + CHECK(normalizeLocaleNumber("1.050", ".", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1", ".", ".") == std::nullopt); + // Control: an empty group separator is "this locale does not group", which + // is a different statement and stays legal. + CHECK(normalizeLocaleNumber("1.5", ".", "") == "1.5"); +} + +TEST_CASE("normalizeLocaleNumber: a group separator must sit on a group boundary", "[render][locale][morph574]") { + // Preceded by one to three digits... + CHECK(normalizeLocaleNumber("1.050", "", ".") == "1050"); + CHECK(normalizeLocaleNumber("12.050", "", ".") == "12050"); + CHECK(normalizeLocaleNumber("123.050", "", ".") == "123050"); + CHECK(normalizeLocaleNumber("1234.050", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber(".050", "", ".") == std::nullopt); + + // ...followed by exactly three, at every group and at the end of the + // integer part. + CHECK(normalizeLocaleNumber("1.05", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.0500", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.050.", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.000.00", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.000.000", "", ".") == "1000000"); + + // ...and never after the decimal separator. + CHECK(normalizeLocaleNumber("1,050.25", ",", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.000,250.25", ",", ".") == std::nullopt); + + // A grouping locale with no separator in the entry at all: there is no + // placement to be wrong about, and the digits pass through. + CHECK(normalizeLocaleNumber("1050", ",", ".") == "1050"); + + // A non-digit inside a grouping locale restarts the digit run rather than + // being counted into it -- and is malformed for the ordinary reason. + CHECK(normalizeLocaleNumber("1.0x0", "", ".") == std::nullopt); +} + +TEST_CASE("normalizeLocaleNumber: every well-formed locale entry still normalises", "[render][locale][morph574]") { + // The validation must not cost a single legitimate entry -- this is the + // half of the change that the rejection cases cannot show. + CHECK(normalizeLocaleNumber("1.050,25", ",", ".") == "1050.25"); + CHECK(normalizeLocaleNumber("-1.050,25", ",", ".") == "-1050.25"); + CHECK(normalizeLocaleNumber("1.000.000,25", ",", ".") == "1000000.25"); + CHECK(normalizeLocaleNumber("1050,25", ",", ".") == "1050.25"); // ungrouped + CHECK(normalizeLocaleNumber("1,050.25", ".", ",") == "1050.25"); // en-US + CHECK(normalizeLocaleNumber(std::string{"1"} + std::string{kNarrowNbsp} + "050,25", ",", kNarrowNbsp) == + "1050.25"); // fr-FR + + // A grouped entry round-trips through the display direction unchanged. + auto const canonical = normalizeLocaleNumber("1.000.000,25", ",", "."); + REQUIRE(canonical.has_value()); + CHECK(formatCanonicalNumber(*canonical, ",", ".") == "1.000.000,25"); +}