Skip to content

Commit 90769ee

Browse files
fix(reactor): §8.2 — setters run inside echo-suppression scope
`ApplySetters` previously ran outside any echo-suppression scope, so a user-authored `.Set(c => c.X = ...)` write on a value-bearing control echoed back into the OnXChanged callback the engine had already wired. The headline reproducer (M13 in spec 047 Phase 0): `ToggleSwitch(false, onIsOnChanged: …).Set(ts => ts.IsOn = true)` fires the callback once at mount, which then writes the engine-driven value back into the owning component's state — the same shape as the spec-030 cross-row swap, just triggered by the setter chain instead of an Update path. This carve-out plumbs a scope-based suppression mode on the control's `ReactorState` for the duration of `ApplySetters`. A depth counter on `ReactorState` (`EchoSuppressScopeDepth`) sits alongside the existing paired counter; `ShouldSuppress` drops events when the scope depth is nonzero without consuming a token, so the existing paired BeginSuppress / change-event flow keeps working for declarative property writes. Scope is only entered when `ReactorState` already exists, so bare leaves (TextBlock / RichTextBlock) don't pay an extra allocation. Validation: - M13 `OnIsOnChangedFireCount` flips from 1 → 0 across all 5 reps on both ReactorToday and ReactorV2 (was the §8.2 baseline witness in PR #411). Phase-0 JSONL is left intact as the pre-fix witness; the follow-up note in `baseline-results/summary.md` records the flip. - Reactor.Tests: 9036 passed, 0 failed. - Reactor.SelfTests: 827 passed; three new SettersScope_* fixtures (ToggleSwitch / Slider / NumberBox) lock the behavior with a mount-no-echo + user-edit-still-fires pair so a future regression on any of the three shapes surfaces immediately. Spec docs updated: §8.2 marked Resolved, factoring-recommendation.md carve-out marked landed, task file 0.7 + M13 lines note the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 3807774 commit 90769ee

8 files changed

Lines changed: 143 additions & 5 deletions

File tree

docs/specs/047-extensible-control-model.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,8 @@ This is the version of the design where the framework gets a *fundamentally diff
458458

459459
A latent issue worth fixing as part of this work, called out in §13 Q8 below: `ApplySetters` runs after declarative property writes today and bypasses any echo suppression scope. A user writing `Set(ts => ts.IsOn = true)` on a `ToggleSwitch` whose `el.IsOn = false` produces an unmasked write that *will* fire `Toggled` and feed back into state on the next event-loop tick. The descriptor model should require setters to either run inside the same suppression / round-trip scope as declared props, or be opted into an explicit raw-write mode (`Set.Raw(...)`) that the author has audited and accepted responsibility for. Default behavior should match declared props.
460460

461+
**Resolved** as a carve-out ahead of Phase 1 (per [`047/factoring-recommendation.md`](047/factoring-recommendation.md)). `ApplySetters` now enters a scope-based suppression mode on the control's `ReactorState` (a depth counter alongside the existing paired `EchoSuppressCount`) for the duration of the setter chain, so any change event raised by a setter-driven write is dropped without consuming a paired token. The M13 perf-bench check flips from `OnIsOnChangedFireCount = 1` to `0` on both `ReactorToday` and `ReactorV2`. Default behavior now matches declared props as called for above; an explicit raw-write opt-out (`Set.Raw(...)`) is still a future refinement should it become needed.
462+
461463
---
462464

463465
## §9 Simplification direction: per-control trampoline tables

docs/specs/047/baseline-results/summary.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,12 @@ total — divide by iterations for per-op alloc.
9898
**Confirms the §8.2 bug exists in the baseline.** Phase 1's fix
9999
(the §8.2 standalone setter-suppression PR per
100100
[`factoring-recommendation.md`](../factoring-recommendation.md))
101-
flips this counter to 0.
101+
flips this counter to 0. **Follow-up:** carve-out PR landed —
102+
`ApplySetters` now enters a scope-based suppression scope on the
103+
control's `ReactorState`; re-running M13 against the post-fix build
104+
shows `OnIsOnChangedFireCount = 0` on every ReactorToday and ReactorV2
105+
row. The Phase-0 JSONL above is deliberately left intact as the
106+
witness to the failing baseline.
102107
- **M11 `ModifierEHS_Frequency`** — placeholder counter at Phase 0.
103108
Real EventSource counter wiring deferred to Phase 1.
104109
- **V2 vs Today columns** range from -16.7% to +16.7%. None are real

docs/specs/047/factoring-recommendation.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,13 @@ the §8.2 description in the PR.
7171
bug. M13's Phase-0 baseline flips from `fires=1` to `fires=0`. ~1
7272
PR, ~50 lines of change in `Reconciler.cs` to thread `BeginSuppress`
7373
around `ApplySetters` for value-bearing controls.
74+
**Status:** landed via the carve-out PR. `ApplySetters` now enters
75+
a scope-based suppression mode on the control's `ReactorState`
76+
(a depth counter alongside the existing paired `EchoSuppressCount`)
77+
for the duration of the setter chain. M13 re-run confirms
78+
`OnIsOnChangedFireCount = 0` on both ReactorToday and ReactorV2.
79+
Reactor.Tests + Reactor.SelfTests pass; three new SettersScope_*
80+
fixtures lock the behavior under the SelfTest batch.
7481

7582
That's the only one. Everything else stays in spec 047.
7683

docs/specs/tasks/047-extensible-control-model-implementation.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,8 @@ implementation is intentionally identical to `ReactorToday`.
146146
verify callback fires exactly once today (the bug per §8.2), zero times
147147
after Phase 1's fix. Phase 0 records the failing behavior as baseline
148148
(`OnIsOnChangedFireCount = 1`) so the Phase 1 fix has a target to flip.
149+
(Carve-out PR flipped this to 0; baseline JSONL preserved as the
150+
failing-state witness — see `baseline-results/summary.md` follow-up.)
149151
- [x] Each bench reports: mean ns + 95% CI, allocation bytes, Gen0/1/2 collections,
150152
managed heap delta. `BenchRunner` instruments `GC.GetAllocatedBytesForCurrentThread`
151153
+ `GC.CollectionCount` + `GC.GetTotalMemory` per rep; aggregator computes
@@ -316,4 +318,6 @@ proposal can move to greenlight (spec §14).
316318
factoring-recommendation.md.
317319
- [x] 0.7 Factoring recommendation committed and reviewed. Outcome: keep
318320
spec 047 unified; only the §8.2 setter-suppression fix is carved out
319-
as a standalone PR ahead of Phase 1.
321+
as a standalone PR ahead of Phase 1. (Carve-out landed: `ApplySetters`
322+
now runs inside a scope-based suppression scope on the control's
323+
`ReactorState`. M13 flipped from `OnIsOnChangedFireCount = 1` to `0`.)

src/Reactor/Core/ChangeEchoSuppressor.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,13 @@ internal static void BeginSuppress(UIElement control)
5656
internal static bool ShouldSuppress(UIElement control)
5757
{
5858
if (control is not FrameworkElement fe) return false;
59-
if (fe.GetValue(Reconciler.ReactorAttached.StateProperty) is Reconciler.ReactorState state
60-
&& state.EchoSuppressCount > 0)
59+
if (fe.GetValue(Reconciler.ReactorAttached.StateProperty) is not Reconciler.ReactorState state)
60+
return false;
61+
// §8.2 — setter-suppression scope: drop the echo without consuming a
62+
// counter token. The scope wraps ApplySetters, where the engine can't
63+
// predict which value-bearing DPs the user's `.Set(...)` will write.
64+
if (state.EchoSuppressScopeDepth > 0) return true;
65+
if (state.EchoSuppressCount > 0)
6166
{
6267
state.EchoSuppressCount--;
6368
return true;

src/Reactor/Core/Reconciler.cs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,10 @@ internal sealed class ReactorState
300300
// wrapper and ShouldSuppress on a different wrapper for the same native
301301
// DependencyObject see the same counter. See ChangeEchoSuppressor.cs.
302302
public int EchoSuppressCount;
303+
// §8.2 — depth of the active setter-suppression scope. While > 0, any
304+
// change event on this control is dropped without consuming a token
305+
// from EchoSuppressCount. See ApplySetters / ChangeEchoSuppressor.
306+
public int EchoSuppressScopeDepth;
303307
// Spec 042 Phase 1 — keyed-list reconciliation state. Set when the
304308
// host element is a templated items control (ListView/GridView/
305309
// ItemsRepeater). The internal ObservableCollection<ReactorRow> in
@@ -405,6 +409,7 @@ internal static void ClearCurrentEventHandlers(FrameworkElement fe)
405409
{
406410
state.Events?.ClearCurrentHandlers();
407411
state.EchoSuppressCount = 0;
412+
state.EchoSuppressScopeDepth = 0;
408413
}
409414
}
410415

@@ -425,6 +430,7 @@ internal static void DetachReactorState(FrameworkElement fe)
425430
state.Events?.ClearCurrentHandlers();
426431
state.Events = null;
427432
state.EchoSuppressCount = 0;
433+
state.EchoSuppressScopeDepth = 0;
428434
}
429435

430436
// ════════════════════════════════════════════════════════════════════
@@ -1435,7 +1441,27 @@ private static bool DepsEqual(object?[] prev, object?[] next)
14351441

14361442
internal static void ApplySetters<T>(Action<T>[] setters, T control) where T : class
14371443
{
1438-
foreach (var setter in setters) setter(control);
1444+
if (setters.Length == 0) return;
1445+
// §8.2 — run setters inside the echo-suppression scope so that a
1446+
// user-authored `.Set(c => c.IsOn = true)` write does not echo back
1447+
// through the OnXChanged callback the engine already wired. Only
1448+
// enters scope when ReactorState already exists; value-bearing mounts
1449+
// call SetElementTag before ApplySetters, so the state is present for
1450+
// every control that can fire a suppressed change event. Bare leaves
1451+
// (TextBlock, RichTextBlock) skip the scope and pay no allocation.
1452+
ReactorState? state =
1453+
control is FrameworkElement fe
1454+
&& fe.GetValue(ReactorAttached.StateProperty) is ReactorState rs
1455+
? rs : null;
1456+
if (state is not null) state.EchoSuppressScopeDepth++;
1457+
try
1458+
{
1459+
foreach (var setter in setters) setter(control);
1460+
}
1461+
finally
1462+
{
1463+
if (state is not null) state.EchoSuppressScopeDepth--;
1464+
}
14391465
}
14401466

14411467
// WPF/WinUI auto-populate UIA Name from a control's string Content through the

tests/Reactor.AppTests.Host/SelfTest/Fixtures/EchoSuppressionFixtures.cs

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,88 @@ public override async Task RunAsync()
503503
}
504504
}
505505

506+
// ── §8.2 setter-suppression scope ─────────────────────────────────
507+
//
508+
// Until the §8.2 carve-out, `ApplySetters` ran outside the echo-suppression
509+
// scope so a user-authored `.Set(c => c.Value = ...)` write fed back into
510+
// the OnXChanged callback the engine had already wired. The fixtures below
511+
// pin three properties:
512+
// 1. The mount + setter combo does not echo into the callback even when
513+
// the setter writes a value-bearing DP whose change event the engine
514+
// subscribed to (the headline §8.2 case for ToggleSwitch).
515+
// 2. After mount completes, a real user-style write to the same DP still
516+
// reaches the callback. This rules out a regression where the scope
517+
// leaks past ApplySetters and silences genuine input.
518+
// 3. The behavior covers a representative spread of value-bearing
519+
// controls (ToggleSwitch / Slider / NumberBox) so a future regression
520+
// that re-introduces the bug on one shape is caught.
521+
522+
internal class SettersScope_ToggleSwitch_NoEcho(Harness h) : SelfTestFixtureBase(h)
523+
{
524+
public override async Task RunAsync()
525+
{
526+
var calls = new List<bool>();
527+
var host = H.CreateHost();
528+
host.Mount(_ =>
529+
ToggleSwitch(isOn: false, onIsOnChanged: v => calls.Add(v))
530+
.Set(ts => ts.IsOn = true));
531+
await Harness.Render();
532+
H.Check("SettersScope_ToggleSwitch_MountNoFire", calls.Count == 0);
533+
534+
var ts = H.FindControl<ToggleSwitch>(_ => true);
535+
H.Check("SettersScope_ToggleSwitch_SetterAppliedValue", ts?.IsOn == true);
536+
537+
if (ts is not null) ts.IsOn = false;
538+
await Harness.Render();
539+
H.Check("SettersScope_ToggleSwitch_UserEditStillFires",
540+
calls.Count >= 1 && calls[^1] == false);
541+
}
542+
}
543+
544+
internal class SettersScope_Slider_NoEcho(Harness h) : SelfTestFixtureBase(h)
545+
{
546+
public override async Task RunAsync()
547+
{
548+
var calls = new List<double>();
549+
var host = H.CreateHost();
550+
host.Mount(_ =>
551+
Slider(value: 25.0, min: 0.0, max: 100.0, onValueChanged: v => calls.Add(v))
552+
.Set(s => s.Value = 75.0));
553+
await Harness.Render();
554+
H.Check("SettersScope_Slider_MountNoFire", calls.Count == 0);
555+
556+
var sl = H.FindControl<Slider>(_ => true);
557+
H.Check("SettersScope_Slider_SetterAppliedValue", sl?.Value == 75.0);
558+
559+
if (sl is not null) sl.Value = 50.0;
560+
await Harness.Render();
561+
H.Check("SettersScope_Slider_UserEditStillFires",
562+
calls.Count >= 1 && calls[^1] == 50.0);
563+
}
564+
}
565+
566+
internal class SettersScope_NumberBox_NoEcho(Harness h) : SelfTestFixtureBase(h)
567+
{
568+
public override async Task RunAsync()
569+
{
570+
var calls = new List<double>();
571+
var host = H.CreateHost();
572+
host.Mount(_ =>
573+
NumberBox(value: 10.0, onValueChanged: v => calls.Add(v))
574+
.Set(nb => nb.Value = 42.0));
575+
await Harness.Render();
576+
H.Check("SettersScope_NumberBox_MountNoFire", calls.Count == 0);
577+
578+
var nb = H.FindControl<NumberBox>(_ => true);
579+
H.Check("SettersScope_NumberBox_SetterAppliedValue", nb?.Value == 42.0);
580+
581+
if (nb is not null) nb.Value = 77.0;
582+
await Harness.Render();
583+
H.Check("SettersScope_NumberBox_UserEditStillFires",
584+
calls.Count >= 1 && calls[^1] == 77.0);
585+
}
586+
}
587+
506588
// ── ToggleSplitButton ─────────────────────────────────────────────
507589

508590
internal class ToggleSplitButtonNoEcho(Harness h) : SelfTestFixtureBase(h)

tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,10 @@ internal static class SelfTestFixtureRegistry
554554
"EchoSuppress_PasswordBox",
555555
"EchoSuppress_TextBox",
556556
"EchoSuppress_ToggleSplitButton",
557+
// §8.2 setter-suppression scope — Set(c => c.X = ...) must not echo.
558+
"SettersScope_ToggleSwitch",
559+
"SettersScope_Slider",
560+
"SettersScope_NumberBox",
557561
// TextBox mount property-ordering: AcceptsReturn must precede Text.
558562
"TB_Mount_MultiLinePreserved",
559563
"TB_Mount_SingleLineCorrect",
@@ -1544,6 +1548,9 @@ internal static class SelfTestFixtureRegistry
15441548
"EchoSuppress_PasswordBox" => new EchoSuppressionFixtures.PasswordBoxNoEcho(harness),
15451549
"EchoSuppress_TextBox" => new EchoSuppressionFixtures.TextBoxNoEcho(harness),
15461550
"EchoSuppress_ToggleSplitButton" => new EchoSuppressionFixtures.ToggleSplitButtonNoEcho(harness),
1551+
"SettersScope_ToggleSwitch" => new EchoSuppressionFixtures.SettersScope_ToggleSwitch_NoEcho(harness),
1552+
"SettersScope_Slider" => new EchoSuppressionFixtures.SettersScope_Slider_NoEcho(harness),
1553+
"SettersScope_NumberBox" => new EchoSuppressionFixtures.SettersScope_NumberBox_NoEcho(harness),
15471554
"TB_Mount_MultiLinePreserved" => new TextBoxMountFixtures.MultiLineTextPreserved(harness),
15481555
"TB_Mount_SingleLineCorrect" => new TextBoxMountFixtures.SingleLineMountCorrect(harness),
15491556
"TB_Mount_MultiLineUpdatePreserved" => new TextBoxMountFixtures.MultiLineUpdatePreserved(harness),

0 commit comments

Comments
 (0)