Skip to content

Commit b3b8e67

Browse files
committed
feat(timers,xml-linq,text-json): three approval-gated tickets land, plus the two cna repairs (#2155, #2199, #1896, #2366, #2377)
#2155 -- Timers::Timer derives from System::Object so Elapsed reports the raising timer, as .NET does (Timer.cs:313). It reported nullptr for a STRUCTURAL reason: EventHandler<T>::Raise types its sender as Object* and Timer had no such base, so nullptr was the only value that compiled. The obvious alternative does not exist here -- .NET derives from Component and this port has no ComponentModel Component at all -- so the divergence is in the BASE, not the sender. sizeof 104 -> 112 plus a new vtable; two shipped pins inverted, and BOTH FAILED THE BUILD, each having said "#2155 landed without updating its pin". Four mutations, all caught. #2199 -- XObject's Changed/Changing notification, inert since it was ported. sizeof(XObject) 16 -> 24 and every derived type by exactly one pointer, allocated only on first registration. Removal is a REGISTRATION TOKEN, because std::function has no operator== and a handler cannot name its own registration -- not a cost question but an impossibility. Semantics transcribed from the reference, including that `notify` means "any object on the chain carries registrations", not "a changing handler ran", which is what keeps Changed-only subscriptions alive. Nine mutations, all caught, FOUR only after test repairs; three share one root cause (only the Changed half's kinds and senders were recorded, so a mutation corrupting the Changing half alone passed) and the fourth was a vacuous assertion. #1896 -- deep tree construction goes quadratic to linear: 100,000 levels 42.583s -> 0.045s (JSON) and 120.028s -> 0.051s (XML). THE APPROVED LAYOUT GROWTH WAS NOT NEEDED AND WAS NOT TAKEN: the node being attached is parentless, so it can only be an ancestor if it HAS a subtree. There were TWO quadratic sources and only one was in the ticket -- the second is #2199's own ancestor walk, which .NET shares, fixed by an exact process-wide registration count. Eight mutations, all caught, but M7 and M8 were first reported NOT CAUGHT honestly: a leaked count corrupts nothing and only makes the suite slower, and a mutation caught as a timeout is not caught by name. Both cases now time the build. #2366 and #2377 -- the two cna repairs, in that repository's WORKING TREE under a per-action instruction, with no commit authorised. Both carry a premise correction: cna builds against the sibling checkout on develop, which lacks #2313, so `std::nullopt` does not compile there and the repair uses `{}`, which means remove under both versions; and #2377 named five types where there are seven. CnaTests 68/68. Gate: 17,558 run, 17,558 passed, 0 failed, 0 skipped across 38 executables (+15). Module graph 41/93. Negative fixtures 45/231. Build directory: build/ only, --parallel 2 throughout.
1 parent 94fefdc commit b3b8e67

26 files changed

Lines changed: 1894 additions & 286 deletions

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
# Migration — deep tree construction is linear (#1896)
3+
4+
Ticket **#1896** (CCF-019, probes J19d / X27d), landed 2026-08-19 on a per-action approval
5+
(`docs/StandingApprovals.md` SA-13), which **withdrew** the refusal recorded on 2026-07-31.
6+
7+
**No public signature, layout, vtable or behaviour changed.** Every input accepted before is
8+
accepted now, every input rejected before is rejected now, with the same exception type and text.
9+
The only difference is how long it takes.
10+
11+
## The measurement
12+
13+
Same binary, same flags (`-O2`), before and after
14+
(`build-probe/1896_probe1_before.log`, `..._after.log`):
15+
16+
| depth | JSON before | JSON after | XML before | XML after |
17+
|---|---|---|---|---|
18+
| 20,000 | 0.994 s | **0.009 s** | 4.668 s | **0.015 s** |
19+
| 50,000 | 7.884 s | **0.023 s** | 34.547 s | **0.029 s** |
20+
| 100,000 | 42.583 s | **0.045 s** | 120.028 s | **0.051 s** |
21+
22+
Quadratic → linear. **946×** at 100,000 levels for JSON, **2,353×** for XML.
23+
24+
## The approved layout growth was NOT taken, and that is the ticket's own premise corrected
25+
26+
#1896 was blocked on an object-layout approval — the plan was for `JsonNode` and `XContainer` to
27+
cache a root or a depth. **The approval was granted and then turned out not to be needed.**
28+
`sizeof` is unchanged on every type this ticket touches, no member was added, and no virtual was
29+
added either (a virtual would have been a vtable change, a heavier approval than this needs).
30+
31+
The guards ask *"is `this` an ancestor-or-self of `parent`?"*, and two facts answer that in O(1)
32+
whenever the answer is no:
33+
34+
* the node being attached is **parentless** — the existing already-has-a-parent check has thrown
35+
otherwise — so it is the root of its own subtree;
36+
* it can therefore only be an ancestor of `parent` if it **has** a subtree.
37+
38+
So a direct `parent == this` comparison catches self-attachment, and a **childless** node cannot
39+
contain anything, so the walk is skipped. Both container counts are O(1) vector sizes, and both
40+
container headers were already included at both sites.
41+
42+
**The guard is faster, not weaker.** It rejects exactly what it rejected before — the `shared_ptr`
43+
reference cycle it exists to prevent stays prevented — and every rejection path is asserted:
44+
already-parented, self-attach on an *empty* container, self-attach on a non-empty one, an ancestor
45+
under its own descendant, the same 64 levels deep, and (for JSON) an *object* ancestor rather than
46+
an array.
47+
48+
**The trap, and it is the one mutation most likely to be written by accident:** an empty container
49+
short-circuits the walk, so `n == this` must be tested **outside** the emptiness guard. Moving it
50+
inside makes self-attachment legal again. That is mutations M1 and M4, and both are caught.
51+
52+
## There were TWO quadratic sources, and only one was in the ticket
53+
54+
Fixing the guard took XML from 120.0 s to **89.3 s** — a real improvement and still quadratic. A
55+
test asserting only "faster" would have called that a success.
56+
57+
The second source is **#2199's own ancestor walk**, added earlier the same day: `NotifyChanging`
58+
visits every ancestor on every mutation *even when nothing is subscribed*. **.NET has exactly that
59+
shape**`XObject.cs:424-427` skips annotation-less ancestors cheaply but still visits each one —
60+
so .NET's XLinq is quadratic here too.
61+
62+
The fix is a **process-wide count of live registrations**. If it is zero, no walk can find anything,
63+
so `NotifyChanging` returns `false` immediately. This is **exact, not approximate**: no handler is
64+
ever skipped, because none exists. When registrations do exist the walk runs in full and behaviour
65+
is identical to .NET's; only the nobody-is-listening case is faster.
66+
67+
The count is decremented by `remove_*` **and by the registration block's destructor**. Without the
68+
latter, destroying an observed object would leave the count permanently non-zero and every tree in
69+
the process would pay the full walk for ever — a silent, permanent regression of the very defect
70+
this ticket fixes. That is mutation M7, and it is caught by a case built for it.
71+
72+
## Tests
73+
74+
`Pin2119_TheProgrammaticHalfOfTheResidualSURVIVESAndHasAnOwner` is **inverted**, as it said it must
75+
be: it recorded the last surviving row of `OwnedTreeLifetimeContractPlan.md`'s deep-nesting group
76+
and said *"#1896 landing is a visible change here rather than a silent one."*
77+
78+
Five cases added across the two modules. The timing cases use 40,000 levels with a generous bound
79+
rather than a benchmark threshold — a quadratic implementation misses it by three orders of
80+
magnitude, so it discriminates without flaking under gate load.
81+
82+
## Mutation testing
83+
84+
Eight mutations, **all caught — but two only after the tests that were supposed to catch them were
85+
strengthened, and the reason is the most useful thing in this ticket.**
86+
87+
| # | Mutation | Caught by |
88+
|---|---|---|
89+
| M1 | JSON: move the `parent == this` check *inside* the emptiness guard | the cycle-guard case + a pre-existing `JsonArrayTests.Add_Self_Throws` |
90+
| M2 | JSON: `hasChildren()` always false | the cycle-guard case + `JsonNodeMutationConsistencyTests` |
91+
| M3 | JSON: `hasChildren()` ignores objects | the cycle-guard case, whose fifth arm exists for it |
92+
| M4 | XML: move the self-check inside the guard | the self-insertion case + `XLinqMutationConsistencyTests` |
93+
| M5 | XML: invert the guard | the self-insertion case + `XLinqLifetimeTests.SelfInsert…` |
94+
| M6 | the notification short-circuit always skips | ~20 `Fix2199_*` cases |
95+
| M7 | the registration count is never decremented on destroy | the destroy case, **after strengthening** |
96+
| M8 | the count is not decremented on `remove_*` | the same, **after strengthening** |
97+
98+
**M7 and M8 were first reported NOT CAUGHT, and that was the honest result.** A leaked count does
99+
not corrupt anything — the tree still builds correctly and every assertion still passes. It only
100+
makes every tree in the process pay the full ancestor walk **for ever**, which is a silent,
101+
permanent regression of the exact defect this ticket fixes. The suite merely got slower, and one run
102+
hit the 240-second mutation timeout — and *a mutation caught only as a timeout is not caught by
103+
name*.
104+
105+
The fix was to make the two cases **time** the build rather than merely run it. The bound is
106+
2 seconds, and both margins are measured rather than guessed: 40,000 levels costs **~40 ms** linear
107+
(50× of headroom under the bound) and **13,870 ms** under M7 (7× over it). So it discriminates
108+
without being a benchmark that flakes under gate load — the same reasoning #2326 applied when
109+
rescaling its resolution tests.
110+
111+
One mutation (M2, first spelling) was **invalid as written** — it left an orphaned function and
112+
failed to compile — and was reformulated rather than counted.
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
# Migration — `System::Timers::Timer` derives from `System::Object` (#2155)
3+
4+
Ticket **#2155** (SR-AUD-239, cause TM-B), landed 2026-08-19 on an explicit per-action approval
5+
(`docs/StandingApprovals.md` SA-13). **Decided against the recommendation on the record**; the
6+
reasoning is preserved below so the trade stays visible rather than relitigated.
7+
8+
## What changed
9+
10+
```cpp
11+
class Timer : public System::Object { ... }; // was: class Timer { ... };
12+
Elapsed.Raise(this, args); // was: Elapsed.Raise(nullptr, args);
13+
```
14+
15+
`Elapsed` now reports **the raising timer** as its sender, matching .NET's
16+
`intervalElapsed(this, elapsedEventArgs)` (`Timer.cs:313`).
17+
18+
## Why it was `nullptr`, and why the fix had to be a base class
19+
20+
Nothing was wrong with the *intent*. `EventHandler<T>::Raise` types its sender as
21+
`System::Object*`, `Timer` had no such base, and `std::is_convertible_v<Timer*, Object*>` was `0` —
22+
so **`nullptr` was the only value that compiled**. The defect was structural, not a mistake at the
23+
call site.
24+
25+
**The obvious alternative does not exist here.** .NET's `Timer` derives from `Component`
26+
(`Timer.cs:15`), **not** from `Object` directly. This port has **no `ComponentModel` `Component`
27+
class at all** — measured: no `Component.hpp` anywhere under `modules/`. So the `Object` base is the
28+
only available route. The divergence from .NET is therefore in the **base**, not in the observable
29+
sender, which now matches exactly. Pinned by `Decl2155_TheDivergenceIsInTheBaseNotTheSender`, so a
30+
future `Component` would be a deliberate re-basing rather than a silent one.
31+
32+
## The cost — a silent binary break
33+
34+
| | before | after |
35+
|---|---|---|
36+
| `sizeof(Timer)` | 104 | **112** |
37+
| `alignof(Timer)` | 8 | 8 |
38+
| polymorphic | no | **yes** — new vtable |
39+
40+
Nothing fails to compile. **Every consumer must rebuild completely.** `docs/StandingApprovals.md`
41+
SA-3 authorises private data members and **explicitly excludes** vtable and base-class changes,
42+
which still ask per action; SA-8 does not reach it either. The approval was granted on this
43+
measurement:
44+
45+
* **zero** `System::Timers` sites in `cna`;
46+
* **zero** in `mobile-eggbert`;
47+
* one first-party site outside `modules/timers` itself.
48+
49+
The recommendation was to decline — a new vtable on a public type is the most expensive break
50+
available, and `Timer` is not polymorphic for any other reason. It was granted anyway. Recorded, not
51+
reopened.
52+
53+
## Consequences a subscriber and a subclasser should know
54+
55+
* `Timer` now inherits `ToString()`, `Equals(const Object*)` and `GetHashCode()` from
56+
`System::Object`, and implements the base's pure virtual `GetTypeName()` as
57+
`"System.Timers.Timer"`.
58+
* `Timer` is **still non-copyable** — asserted by a pin written *before* this change precisely
59+
because a base-class edit is the kind that quietly reintroduces a copy constructor.
60+
* The `Object` subobject is at **offset 0**, so an `Object*` obtained from a `Timer*` compares equal
61+
to it. Pinned, because that is the half `sizeof` cannot express.
62+
63+
## Tests
64+
65+
Two shipped pins were **inverted**, and both had said in terms *"#2155 landed without updating its
66+
pin"* — **both failed the build**, at compile time, which is the evidence they were load-bearing
67+
rather than decorative:
68+
69+
| Pin | Now |
70+
|---|---|
71+
| `ElapsedStillReportsANullSender_SeeBlockedTicket2155` | `Fix2155_ElapsedReportsTheRaisingTimerAsItsSender` |
72+
| `TimerRemainsANonPolymorphicNonObjectType_SeeBlockedTicket2155` | `Fix2155_TimerIsNowAPolymorphicObjectType` |
73+
74+
Two pins added: `Fix2155_TheLayoutCostIsExactlyOneVptr` (the price, asserted as a relationship as
75+
well as a literal, plus the offset-0 check) and `Decl2155_TheDivergenceIsInTheBaseNotTheSender`.
76+
77+
The sender assertion is `EXPECT_EQ(seen, static_cast<Object*>(&timer))`, **not** merely non-null: a
78+
mutation passing some other `Object*` satisfies a null check and fails this one. That is M2.
79+
80+
## Mutation testing
81+
82+
Four mutations, **all caught**:
83+
84+
| # | Mutation | Caught by |
85+
|---|---|---|
86+
| M1 | revert the sender to `nullptr` | `Fix2155_ElapsedReportsTheRaisingTimerAsItsSender` |
87+
| M2 | pass some other `Object*` | the same case — which is why it asserts identity, not non-nullness |
88+
| M3 | `GetTypeName()` returns `"System.Object"` | `Fix2155_TheLayoutCostIsExactlyOneVptr` |
89+
| M4 | drop the base | compile error |
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
# Migration — `XObject` Changed/Changing notification is implemented (#2199)
3+
4+
Ticket **#2199** (SR-AUD-336), landed 2026-08-19. Both of its gates were opened the same day
5+
(`docs/StandingApprovals.md` SA-13).
6+
7+
## What changed
8+
9+
Until now the four accessors **accepted a handler and discarded it**, and no mutation anywhere in
10+
the hierarchy raised anything. `XLinqChangeNotificationTests.cpp` pinned that inert surface at
11+
every door, deliberately, so a partial implementation could not land silently and so the eventual
12+
repair would have failing tests to turn green. This is that repair.
13+
14+
```cpp
15+
// before // after
16+
void add_Changed(const Handler&); [[nodiscard]] XObjectChangeRegistration add_Changed(const Handler&);
17+
void remove_Changed(const Handler&); bool remove_Changed(const XObjectChangeRegistration&) noexcept;
18+
void add_Changing(const Handler&); [[nodiscard]] XObjectChangeRegistration add_Changing(const Handler&);
19+
void remove_Changing(const Handler&); bool remove_Changing(const XObjectChangeRegistration&) noexcept;
20+
```
21+
22+
## Gate 1 — the layout growth
23+
24+
`sizeof(XObject)` **16 → 24**, and every derived type with it. One pointer: a `unique_ptr` to the
25+
registration block, allocated **only on first registration**, so an unobserved tree pays a null
26+
pointer and no allocation — the closest analogue of .NET's annotation slot, which is likewise
27+
absent until something is annotated.
28+
29+
| type | before | after |
30+
|---|---|---|
31+
| `XObject`, `XNode` | 16 | 24 |
32+
| `XContainer` | 40 | 48 |
33+
| `XElement` | 128 | 136 |
34+
| `XAttribute` | 120 | 128 |
35+
| `XText`, `XCData`, `XComment` | 48 | 56 |
36+
| `XProcessingInstruction` | 80 | 88 |
37+
| `XDocument` | 56 | 64 |
38+
39+
Silent binary break; **every consumer must rebuild**. Measured: **zero** `Xml::Linq` sites in `cna`
40+
and **zero** in `mobile-eggbert`. **Three** shipped layout pins had to be updated — two `sizeof`
41+
pins and one `static_assert` block from #1890 — and all three failed the build, which is the
42+
evidence they were load-bearing.
43+
44+
## Gate 2 — the removal design, and the one deliberate divergence
45+
46+
.NET removes a registration by passing the **delegate** back, because a C# delegate is
47+
equality-comparable. `XObjectChangeEventHandler` is a `std::function`, and `std::function` has **no
48+
`operator==` against another `std::function`** — proved at compile time, not assumed. A
49+
handler-taking `remove_*` therefore cannot identify which registration a caller means. **This was
50+
never a cost question**; it is not implementable as declared, at any layout cost.
51+
52+
The decision was a **registration token**, chosen so the divergence is visible *in the type* rather
53+
than hidden in the behaviour. Two alternatives were offered and declined:
54+
55+
* keep .NET's signature and remove **all** registrations — silently drops a third party's subscription;
56+
* keep it and **throw** from `remove_*` — a subscriber could then never unsubscribe.
57+
58+
```cpp
59+
auto token = element.add_Changed(handler); // was: element.add_Changed(handler);
60+
element.remove_Changed(token); // was: element.remove_Changed(handler);
61+
```
62+
63+
`add_*` is `[[nodiscard]]` deliberately: a discarded token is a registration that can never be
64+
removed. Ids come from a **process-wide** atomic counter, so a token issued by one `XObject` can
65+
never match a registration on another; a foreign or default-constructed token removes nothing and
66+
returns `false`.
67+
68+
## Semantics, all transcribed from the reference
69+
70+
* **Notifications bubble.** Every object from the changed one up to the root is notified,
71+
**innermost first** (`XObject.cs:418-460`).
72+
* **The sender is the object *changed*, not the object *observed*.** For `Add` and `Remove` .NET
73+
walks the **parent's** chain with the **child** as sender (`XLinq.cs:156,177`) — an asymmetry
74+
reproduced rather than tidied.
75+
* **`Changing` runs before the mutation, `Changed` after**, and `Changed` is guarded on what
76+
`Changing` returned, as every .NET call site does.
77+
* **`notify` means "any object on the chain carries registrations at all", not "a changing handler
78+
ran".** .NET tests `Annotation<XObjectChangeAnnotation>() != null` and only then invokes the
79+
possibly-null delegate. Reading it the other way would **silently disable every `Changed`-only
80+
subscription** — the case `Fix2199_AChangedOnlySubscriptionStillReceivesChanged` exists for that.
81+
* **Handlers are invoked from a snapshot**, so a handler may register or unregister during a
82+
notification; the change takes effect on the next one.
83+
84+
### Two places where raising nothing is the correct answer
85+
86+
* **`XElement::setValueProperty` raises no `Value`.** .NET's setter is `RemoveNodes(); Add(value);`,
87+
so a subscriber sees a `Remove` pair per existing child then an `Add` pair. Raising `Value` would
88+
invent an event .NET does not raise.
89+
* **`Add(std::string)` raises `Add` or `Value` depending on merging**, because it merges into a
90+
trailing `XText` rather than creating a sibling; an empty string is a genuine no-op and raises
91+
nothing.
92+
93+
## Mutation testing
94+
95+
Nine mutations, **all caught — but four only after a test was repaired, and the repairs are the
96+
interesting part.**
97+
98+
| # | Mutation | Caught by |
99+
|---|---|---|
100+
| M1 | `notify` means "a changing handler ran" | two ancestor-walk cases |
101+
| M2 | the walk stops at the first registration | two bubbling cases |
102+
| M3 | iterate the live vector instead of a snapshot | the reentrancy case, **after repair** |
103+
| M4 | ids `thread_local` instead of process-wide | the threaded token case, **added for it** |
104+
| M4b | ids per-object | the foreign-token case, **after repair** |
105+
| M5 | `Add`'s *Changing* sender is the parent | the add case, **after repair** |
106+
| M6 | `RemoveNodes` raises once, not per child | the per-child case |
107+
| M7 | `Changed` raised before the mutation | the before/after state case |
108+
| M8 | attribute `Add` raises nothing | the attribute add case |
109+
| M9 | `RemoveAttribute`'s *Changing* kind is `Value` | the attribute remove case, **after repair** |
110+
111+
**Three of the four repairs share one root cause and it is worth stating plainly.** The recorder
112+
captured only the **`Changed`** half's kinds and senders, so any mutation that corrupted the
113+
**`Changing`** half alone (M5, M9) passed. Half a pair is still a wrong notification, so the
114+
recorder now records both and every case asserts they agree.
115+
116+
The fourth (M4b) was a **vacuous assertion**: the foreign-token case removed A's registration
117+
*before* trying B's token, leaving A's list empty — so it returned `false` for the wrong reason and
118+
a per-object counter went undetected. The foreign attempt now comes first, while A's registration
119+
is still present.
120+
121+
M3's repair is the same shape: the reentrancy case removed the running handler as well as adding
122+
one, which ended the live-vector iteration early and hid the difference. It now only adds, and adds
123+
enough to force a reallocation.
124+
125+
## SA-2 conditions
126+
127+
1. This note. ✔
128+
2. `test/consumer/xml_linq_change_registration_token_negative.cpp`**5 sites**, the third being
129+
the spelling most likely to survive a careless migration: keeping the old `add_Changed(h);` line
130+
and discarding the token. Fixture set grows to **45 fixtures / 231 sites**. ✔
131+
3. Downstream ticket: **#2394**. ✔
132+
4. Full gate. ✔
133+
5. Measured consumer impact: **zero sites** in both. ✔

modules/text-json/include/System/Text/Json/Nodes/JsonNode.hpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,14 @@ namespace System::Text::Json::Nodes {
6464
*/
6565
void AssignParent(JsonNode* parent);
6666

67+
private:
68+
/** @return true if this node currently contains other nodes. #1896's cycle-guard
69+
* short-circuit; see AssignParent. Non-virtual by design -- a virtual would be a vtable
70+
* change this repair does not need. */
71+
[[nodiscard]] bool hasChildren() const;
72+
73+
public:
74+
6775
/** @brief Internal: clears the parent container pointer. Not part of .NET's public surface (mirrors JsonNode.cs's internal DetachParent). */
6876
void DetachParent() { parent_ = nullptr; }
6977

0 commit comments

Comments
 (0)