Fix ElementPool.CleanElement never releasing Control/Panel-level modifier properties (#985) - #1015
Conversation
There was a problem hiding this comment.
🟢 Ready to approve
The fix is correctly placed in the FE-common scanned region, updates analyzer metadata/tests consistently, and adds targeted selftests that non-vacuously validate pool round-trip local-value release across all intended receivers.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a control-pooling correctness bug where ElementPool.CleanElement did not clear several modifier-backed dependency properties (notably Panel.Background plus multiple Control/Border-level props), allowing local values to leak across pool reuse and override styles due to WinUI DP precedence. It aligns the pool reset behavior with Reconciler.ApplyModifiers receiver coverage, updates analyzer classification so .Set(...) writes to these properties correctly report as pool-lost, and adds selftest + analyzer-test pinning to prevent regressions.
Changes:
- Clear six common modifier-backed DPs (
Padding,CornerRadius,BorderThickness,BorderBrush,Background,IsEnabled) inElementPool.CleanElement’s FE-common block via a receiver-mirroring chain. - Update analyzer metadata/tests to reflect
poolReset: truefor those properties (diagnostic escalationsREACTOR_MOD_002→REACTOR_POOL_001where applicable) and harden source scanners against comment-induced truncation. - Add new selftest fixtures to validate pool round-trips release local values using
ReadLocalValue(...) == DependencyProperty.UnsetValue, and update docs + changelog to communicate the new behavior.
File summaries
| File | Description |
|---|---|
src/Reactor/Core/ElementPool.cs |
Adds FE-common receiver chain to ClearValue(...) the missing Control/Panel/TextBlock/Border modifier-backed DPs; removes redundant type-arm clears. |
src/Reactor.Analyzers/ModifierTable.cs |
Marks the six properties as poolReset: true (with gates preserved where needed) so diagnostics correctly report pool-lost .Set(...) writes. |
tests/Reactor.Tests/AnalyzerTests/PoolResetSetConsistencyTests.cs |
Updates instance/attached-property classification to include new owners; makes the FE-common-block boundary regex line-anchored; updates stubs to satisfy Control-gated matching. |
tests/Reactor.Tests/AnalyzerTests/ModifierUnsetClearValueTests.cs |
Pins the new required clears and hardens FE-common-block boundary detection against comment matches. |
tests/Reactor.Tests/AnalyzerTests/ModifierAvailableAnalyzerTests.cs |
Updates expected IDs to REACTOR_POOL_001 for newly pool-reset properties and keeps a mixed-id code-fix case anchored on content-alignment. |
tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs |
Registers the new selftest fixtures in both the name list and the Create() switch. |
tests/Reactor.AppTests.Host/SelfTest/Fixtures/ModifierEventFixtures.cs |
Adds selftests covering Control/Panel/StackPanel/TextBlock pool-release behavior with non-vacuous guards (Returned, ReferenceEquals, ReadLocalValue). |
plugins/reactor/skills/reactor-build-and-check/SKILL.md |
Updates shipped agent-kit documentation to mention instance-property writes now covered by REACTOR_POOL_001. |
docs/_pipeline/templates/analyzer-architecture.md.dt |
Updates the template wording to reflect the instance-vs-attached pool-reset balance post-fix. |
docs/guide/analyzer-architecture.md |
Regenerated compiled guide page for the updated paragraph. |
CHANGELOG.md |
Documents the pooling fix and the user-visible diagnostic severity escalation. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
📦 Build metricsArtifact sizes for Packages (compressed .nupkg)
Assemblies in Microsoft.UI.Reactor
Assemblies in Microsoft.UI.Reactor.Advanced
Assemblies in Microsoft.UI.Reactor.Devtools
✅ smaller / |
🧪 Merged coverageCoverage for
No coverage change beyond the noise floor. ✅ ✅ higher / |
bdf480f to
95338b4
Compare
|
Amended head to The previous note claimed The sentence stays true while the guarantee it implies quietly shrinks, which is worse than a plainly wrong comment: it keeps parsing as correct and gets defended. Reworded to separate the structural claim from the coverage claim and to name the maintenance obligation, with a pointer to #1017 for why the consistency invariant won't catch a widened gate. No behaviour change. Full headless unit suite: 13041 passed / 0 failed. |
There was a problem hiding this comment.
🟢 Ready to approve
The pooling resets, analyzer/table updates, and selftest+source-scanning coverage appear consistent and correctly scoped to the FE-common scan boundary that enforces pool⇄analyzer parity.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…985) Reconciler.ApplyModifiers writes Padding, CornerRadius, BorderThickness, BorderBrush, Background and IsEnabled onto Control, Border, Panel and StackPanel receivers, but CleanElement only ever reset the Border arm. On mount there is no previous element, so no unset arm runs — a recycled Button, ScrollViewer, Grid or VStack started life carrying the previous renter's *local* values, and a local value outranks every Style setter in WinUI's dependency-property precedence order. Panel.Background was the widest hole, since VStack/HStack/Grid are all poolable. CleanElement now clears all six through a Control | Border | Panel/ StackPanel chain that mirrors ApplyModifiers' receivers exactly. The chain lives in the FrameworkElement-common region, not in the type-dispatch arms: PoolResetSetConsistencyTests stops scanning at the switch, so a clear placed after it is invisible to every pool/analyzer invariant. That placement is the fix, not an implementation detail: on a trial merge with #1003, no-op'ing three clears sitting below the dispatch left the entire headless unit suite green and only a live-WinUI selftest caught it. These six properties were carried briefly by #984 and backed out in 14c8aaf as a missing-reset fix rather than the wrong-reset-shape fix that PR was about, and explicitly deferred ("Filed as a follow-up instead"). #985 is that follow-up, and accepts the REACTOR_POOL_001 escalation as correct rather than avoiding it. The Border case arm keeps only `border.Child = null`; the redundant IsEnabled clears in the Button and ToggleSwitch arms are removed; TextBlock.Padding stays in its own arm (TextBlock is neither Control, Border nor Panel) with a corrected comment. BREAKING (analyzer severity): the six properties are now poolReset: true in ModifierTable, so `.Set(c => c.Padding = ...)` and friends report REACTOR_POOL_001 (Warning) where they previously reported REACTOR_MOD_002 (Info). Control gates are unchanged, the suggested fix is unchanged, and the code fix still applies it automatically — but downstream projects building with TreatWarningsAsErrors may need to convert those call sites. The repo's own Release build of Reactor.slnx is clean. Tests: - PoolResetSetConsistencyTests: InstancePropertyOwners gains Border/Panel/ StackPanel (otherwise Border.Padding is misclassified as attached); the instance ClearValue regex is now derived from that set and accepts the alias-qualified `WinUI.Border.PaddingProperty` spelling; FakeElement derives from a stub Control so the newly gated rows still pass PassesControlGate. - Both copies of ReadCleanElementCommonBlock anchor the `switch (fe)` boundary to the start of a line: an unanchored match let a *comment* mentioning the dispatch truncate the scanned region, silently shrinking every invariant built on it. - ModifierUnsetClearValueTests pins all 13 receiver/property pairs so an outright deletion fails positively. - ModifierAvailableAnalyzerTests: ~25 markers flip to REACTOR_POOL_001; CodeFix_Chains_Across_Both_Rule_Ids_In_One_Body swaps IsEnabled for HorizontalContentAlignment to keep its cross-id premise. - Two new selftest fixtures (ModifierEvent_PoolClearValueControlPanel, ModifierEvent_PoolClearValueStackPadding) assert ReadLocalValue == UnsetValue after a real rent/return cycle, guarded by a ReferenceEquals instance-reuse check. ScrollViewer/Grid carry the values because their elements declare none of the six — a Button would be vacuous for IsEnabled. Each of the 9 ClearValue lines was mutation-tested individually. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
95338b4 to
20e44ca
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The pooling reset logic is now aligned with ApplyModifiers receivers, and the change is backed by targeted selftests plus updated analyzer/table-driven unit tests that should catch regressions.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Resolves the #1003 x #1015 union. #1003 widened the Padding/CornerRadius control gates to the concrete panel types; #1015 makes the same six properties pool-reset. The two interact in seven shared files, only three of which conflict. Conflicts (3): - ModifierTable.cs: #1003 moved the gated rows, so the conflict renders as empty-vs-five. Dropped the incoming copies and widened the surviving poolReset rows in place, keeping one row per property. Also retired the ControlBorderStackText array reference, which #1003 removed - the raw auto-merge did not compile. - ModifierAvailableAnalyzerTests.cs: took #1003's wider statement set and #1003's BorderThickness example text, with #1015's POOL_001 ids. - ModifierUnsetClearValueTests.cs: additive union, 34 receiver pins. Silent auto-merges that still needed work (4): - ElementPool.cs: both branches added a receiver chain. Both compiled and both ran, so nothing failed - folded #1003's concrete-panel clears into the FE-common Panel arm and removed the duplicate chain, which also removes the second resetStack declaration. - SKILL.md: merged to four diagnostic rows with no marker. Kept #1003's corrected MOD_003 and #1015's POOL_001, in that order, dropping both stale rows. - Fires_For_Padding_On_Grid_And_RelativePanel: a test #1003 added outside any conflict, still marked MOD_002. Padding is pool-reset here, so the analyzer now reports POOL_001. - DeliberatelyExcludedAttached: dropped StackPanel.CornerRadius, whose owner is an instance-property owner in #1015; both Grid entries stay. Prose corrected where the widening falsified it: ModifierTable's allow-list comment, and the TextBlock.Padding pin comment's receiver ordinal. Verified: 13043 passed / 0 failed / 64 skipped; Tooling 117/117; union acceptance checker 15/15. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟡 Not ready to approve
The analyzer’s poolReset escalation applies to RelativePanel receivers even though RelativePanel isn’t pooled, making REACTOR_POOL_001 semantics inaccurate for that case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/Reactor/Core/ElementPool.cs:275
- The comment claims this clear chain "mirrors ApplyModifiers' receiver types exactly", but ApplyModifiers also writes Padding to Grid (and RelativePanel). The implementation does clear Grid under the Panel arm, so the comment should be updated to match the actual receiver set (and/or call out non-poolable receivers).
// The chain mirrors ApplyModifiers' receiver types exactly (Reconciler.cs):
// Padding → Control | Border | StackPanel | TextBlock, IsEnabled → Control,
// CornerRadius/BorderBrush/BorderThickness → Control | Border,
// Background → Panel | Control | Border.
src/Reactor.Analyzers/ModifierTable.cs:270
poolReset: truefor Padding/CornerRadius combined with a controlGate that includesRelativePanelmeansREACTOR_POOL_001will fire for.Set(x => x.Padding = …)onRelativePanelElement, butElementPooldoes not poolRelativePanel(not inPoolableTypes), so that write is not actually “lost on pool return”. Consider making POOL vs MOD selection receiver/poolable-aware (or aligning poolability + CleanElement resets) so the diagnostic semantics remain accurate.
// Pool-reset but only on some receivers (issue #985). CleanElement clears these
// through a Control | Border | Panel/StackPanel chain that mirrors the receivers
// ApplyModifiers writes them to, so the control gates below still apply: the gate
// decides whether the modifier reaches the control at all, poolReset decides which
// rule id reports it. A .Set write to any of them is now unwound on pool return.
//
// WinUI declares most of these on Panel subclasses too, and the allow-lists
// genuinely differ: Panel itself declares only Background; Grid, StackPanel, and
// RelativePanel each declare their own border-box properties, and TextBlock takes
// Padding but not CornerRadius. IsEnabled needs no gate — WinUI declares it on
// Control, so if the .Set lambda compiles the receiver already qualifies.
{ "IsEnabled", new ModifierInfo("IsEnabled", poolReset: true) },
{ "Padding", new ModifierInfo("Padding", poolReset: true, controlGate: ControlBorderGridStackRelativeText) },
{ "CornerRadius", new ModifierInfo("CornerRadius", poolReset: true, controlGate: ControlBorderGridStackRelative) },
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Copilot review at fca7354 flagged that ElementPool's chain comment still carried the pre-widening receiver lists (Padding -> Control | Border | StackPanel | TextBlock), which #1003's widening falsified in the union merge. The comment now states the post-union lists, records why RelativePanel gets no clear (not in PoolableTypes, so it is never returned to the pool), and notes the Canvas counter-case (poolable, admitted only by the Background gate, which the Panel arm clears). It also names #1003 as the realization of the block's own "widening a gate obliges you to widen the arm" prediction. A generalized sweep for the same staleness found two more carriers that describe the chain with pipe separators - ModifierTable.cs and PoolResetSetConsistencyTests.cs - both corrected to Control | Border | Panel/Grid/StackPanel | TextBlock. The original extinction check only matched slash-joined gate strings, so a pipe-joined list escaped it; that separator blind spot is the reason all three survived the union verification. Comments only - no behavior change. Verified: build 0 warnings / 0 errors, Reactor.Tests 13043 passed / 0 failed / 64 skipped, union checker 15/15 (ElementPool comments sit inside the scanner's FE-common region, so re-verification is not optional). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟢 Ready to approve
The resets now match the reconciler’s receiver writes, analyzer expectations are updated accordingly, and the change is backed by non-vacuous selftests plus strengthened source-scanning guards.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Heads-up: this PR silently disarms an assertion in a fixture it doesn't touch (
|
stale sp reads |
assertion | |
|---|---|---|
main today — StackPanel padding not pool-cleaned |
retains Thickness(22) |
fails loudly — safe |
with #1015 — ElementPool.cs:326 resetStack.ClearValue(WinUI.StackPanel.PaddingProperty) |
UnsetValue |
passes for the wrong reason |
So the assertion goes from fail-closed to fail-open. It keeps passing, and stops testing. Nothing in your diff contains the defect, and nothing in the fixture changes — which is exactly why it needs flagging rather than catching.
Your Control.PaddingProperty clear at :296 has the same potential, but Issue950_Unset_ControlLocalValueCleared is already protected by an identity check at :268, so it's unaffected. The protection works; three assertions just don't have it.
Two of the three are already broken on main — my bug, from #970
Of 11 absence-shaped assertions in that fixture, 8 carry an identity positive control and 3 don't:
| assertion | object | status |
|---|---|---|
_Unset_InlineOnlyLocalValueCleared :282 |
TextBlock | already fail-open on main (#970 made the pool clear TextBlock padding) |
_AllTypes_InlineEndCleared :429 |
TextBlock | already fail-open on main |
_AllTypes_StackCleared :422 |
StackPanel | fail-closed today, flips with this PR |
The fix — three identity checks, mirroring the eight already there
Insert after each await Harness.Render() that follows a toggle, before the *_Cleared assertions. Predicates copied verbatim from the mount-time lookups:
// ~:270, alongside Issue950_Unset_ControlSameInstance
H.Check("Issue950_Unset_InlineSameInstance",
ReferenceEquals(inlineTb, H.FindText("Issue950_inline_only")));
// ~:417, alongside Issue950_AllTypes_BorderSameInstance
H.Check("Issue950_AllTypes_StackSameInstance",
ReferenceEquals(sp, H.FindControl<WinUI.StackPanel>(s =>
s.Children.Count == 1 && s.Children[0] is WinUI.TextBlock c
&& c.Text == "Issue950_AllTypes_StackChild")));
H.Check("Issue950_AllTypes_TextSameInstance",
ReferenceEquals(tb, H.FindText("Issue950_AllTypes_InlineEnd")));How you'd like to handle it
Happy either way — fold these three into this PR (they're additive, test-only, and touch nothing you're changing), or say the word and I'll land them separately. If separate, they need to merge before this one, otherwise there's a window where the assertion is live and hollow.
There was a problem hiding this comment.
🟡 Human review recommended
It changes core pooling cleanup semantics and analyzer severity selection across multiple subsystems, so it should receive final human validation (including running the full CI-equivalent test matrix) before approval.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…l-panel-resets Clean merge, no resolution required. Merged the pinned SHA 4095674 rather than the ref name: sibling worktrees share one .git ref store, so origin/main can move between a verify and a merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟡 Human review recommended
It changes core pooling reset behavior and analyzer severity selection across multiple subsystems (framework + analyzers + tests + docs), so a final human review is warranted despite strong test coverage.
Review details
Suppressed comments (1)
src/Reactor/Core/ElementPool.cs:318
- This comment says the consistency invariants “will not remind you”, but this PR adds derived tests (e.g. ModifierUnsetClearValueTests.Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement) that do fail if a pooled gated receiver isn’t released. Leaving this as-is is misleading for future maintainers deciding whether gate widening is guarded.
// The consistency invariant will not remind you: it checks reset ⇒ marked, never
// marked ⇒ reset on every gated receiver (see #1017).
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The comment above the FE-common reset chain said "The consistency invariant will not remind you: it checks reset => marked, never marked => reset on every gated receiver (see #1017)." That was true when it was written in 20e44ca, and this PR falsified it two commits later without the prose being revisited. Measured rather than reasoned - both legs rebuilt, baseline green as a precondition, mutation site counts asserted before writing, restore proved byte-identical: widen poolResetGate only (Canvas into ControlBorderGridStackText) -> Every_Pool_Reset_Gate_Matches_The_Poolable_Intersection 1 of 8 widen controlGate AND poolResetGate, as a real gate-widening PR would -> Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement 1 of 8 The second leg is the faithful reproduction of the scenario the comment itself cites (#1003 widening Padding/CornerRadius to the concrete panels), and it is guarded. Each detector fires alone, so the comment now names both and says what each covers; a reader deciding whether widening a gate is safe needs to know which test speaks, not merely that something does. #1017 stays referenced but scoped honestly: both tests range over gate INTERSECT PoolableTypes, so the non-poolable receivers a gate may also admit remain uncovered and the issue remains open. Comment-only: 8 comment lines added, 2 removed, 0 code lines, verified by classifying every +/- line rather than reading --stat. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
|
Suppressed finding at The finding is correct, and it caught a stale claim I introduced myself. The comment was written in Measured rather than reasonedI did not want to rewrite one unverified claim into another, so I mutated the exact scenario the comment cites. Baseline-green enforced as a precondition, mutation site counts asserted before writing (
LEGB is the faithful reproduction of the scenario the comment itself cites — #1003 widening Why the comment now names both tests rather than saying "this is guarded"The two legs redden different tests, so a single failing test could not have told me which guarantee holds. A reader deciding whether widening a gate is safe needs to know which one speaks: LEGA's detector stops you claiming the pool resets a receiver it does not, LEGB's stops the arm from lagging the gate. Naming only one would also invite a later reader to treat the other as decorative and simplify it away — a hazard I hit earlier in this PR and corrected in Scope kept honestBoth tests range over Verification
|
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently align pool cleanup, analyzer behavior, tests, and documentation around receiver-aware pooling semantics with no verified regressions in the reviewed diffs.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…antee
The FE-common region of ElementPool.CleanElement is read by two mirrored helpers, and
three comments describe the guarantees around it more strongly, or more narrowly, than
the code delivers. All three are corrected here; no code changes.
1. The anchor claim was too strong (both helpers).
Both ReadCleanElementCommonBlock helpers say the line-anchored `^\s*switch\s*\(fe\)`
boundary means "a comment mentioning the type dispatch cannot masquerade as the
boundary". Measured, that holds for a `//` comment and not in general:
// comment mentioning the dispatch region 12922 -> 13011 (grew; no truncation)
/* inner line BEGINNING with it */ region 12922 -> 7335 (truncated)
A block comment's inner line still satisfies `^\s*switch`. The comments now say which
case the anchor closes and which it does not.
2. Truncation is invisible to exactly the assertions that look like they would catch it.
Every absence-shaped assertion over this region -- "no offender is present" -- passes
MORE readily on a smaller region, so a truncation makes it vacuous rather than red.
CleanElement_Resets_Through_ClearValue_Not_Default_Assignment stays green on the
mutation above. What actually reddens is the presence-shaped half, and it is four
detectors across two files:
CleanElement_Releases_Every_Modifier_Backed_Dependency_Property
Every_TrappedProperty_Is_Reset_In_CleanElement
Every_TrappedAttachedProperty_Is_Reset_In_CleanElement
Attached_Reset_Scan_Sees_Every_Owner_The_Table_Names (54/0 -> 54/3)
Named in both helpers so nobody deletes one believing the anchor makes it redundant.
3. The pin set's stated rationale covered one of its three roles.
CleanElement_Releases_Every_Modifier_Backed_Dependency_Property was documented only as
catching an outright deleted ClearValue. That is real but it is the role that looks
dispensable next to the pool analyzer, and it is not why the pair is load-bearing.
Measured, the two halves are duals, each blind exactly where the other is loud:
region 12922 -> 7335 scan PASS (vacuous) pin set FAIL (catches)
fe.AllowDrop = false; scan FAIL (names it) pin set PASS (blind)
The pin set guards the scan's SCOPE; the scan guards the pin set's CLOSED WORLD -- a
hardcoded list defends only the names someone wrote down, and the scan catches the
35th. The doc comment now states all three roles, so a reviewer reading the weakest
one cannot collapse the pair and remove two protections nobody wrote down.
Mutations verified landed by an independent PowerShell re-implementation of the scanner
(region length before/after), each restored byte-identical with the tree at porcelain=0.
AllowDrop chosen because it is neither pinned nor in CleanElementAssignmentExceptions;
Opacity, the obvious vehicle, is pinned here.
Comment-only: 50 lines added, 7 removed, 0 code lines. ElementPool.cs untouched (region
still 12922, pin count still 34). Release build 0/0; Reactor.Tests 13140/0/64; Tooling
gate run separately 203/0/0; both affected classes 62/0.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟡 Not ready to approve
IsPooledReceiver currently keys off the lambda parameter’s declared type, which can misclassify contravariant .Set lambdas and incorrectly downgrade true pooled-receiver cases to REACTOR_MOD_002.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/Reactor.Analyzers/PoolResetSetAnalyzer.cs:509
- IsPooledReceiver uses the lambda parameter’s declared type (GetDeclaredSymbol(lambdaParam).Type). Because Action is contravariant, callers can legally pass a base-typed lambda (e.g. Action) to a Set(Action) overload; in that case this logic treats the receiver as unpooled ("Control"), and REACTOR_POOL_001 incorrectly downgrades to MOD_002 even though ElementPool will recycle the actual control instance.
var controlType = context.SemanticModel
.GetDeclaredSymbol(lambdaParam, context.CancellationToken)?.Type;
return controlType is not null
&& ModifierTable.IsPoolableTypeName(controlType.Name)
src/Reactor/Core/ElementPool.cs:55
- The XML doc references
ModifierTable.PoolableTypeNameSet, but the public surface isPoolableTypeNames/IsPoolableTypeName(...)(the set itself is private). Updating this reference will keep the comment accurate for readers outside Reactor.Analyzers.
/// This list is mirrored by name in <c>ModifierTable.PoolableTypeNameSet</c>
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
A review of #1015 raised REACTOR_POOL_001 misclassifying a contravariant `.Set` lambda: since `Action<T>` is contravariant, a caller could ostensibly pass an `Action<Control>` lambda to a `Set(Action<Button>)` overload, `IsPooledReceiver` would read `Control` off the lambda parameter, and a genuinely pooled receiver would be downgraded to REACTOR_MOD_002. The premise does not hold. Variance governs delegate conversions, not anonymous-function conversions: C# requires an explicitly typed lambda's parameter types to match the delegate's exactly. Measured rather than argued -- `el.Set((object fe) => { })` is rejected with CS1678 ("declared as type 'object' but should be 'Button'") and CS1661 ("the parameter types do not match the delegate parameter types"), while `Action<object> h = ...; el.Set(h)` in the same compilation converts cleanly. `object` is the strongest available vehicle for the claim, being a base of every receiver type. So the claimed call site cannot be written, and the one argument shape that does convert contravariantly carries no lambda syntax at all: GetSingleLambdaParameter returns null and the analyzer returns before IsPooledReceiver, so neither POOL_001 nor the MOD_002 downgrade is reached. Both facts are pinned as regression tests rather than left as prose. The delegate-value test's silence is absence-shaped and so would be vacuous alone; it is paired with the identical write in lambda form in the SAME compilation. Mutating IsPooledReceiver's namespace comparison reddens exactly that test and leaves the compiler-diagnostic test green, so the marker is live and the two are independently sensitive. Also corrects the ElementPool doc block, which pointed readers at `ModifierTable.PoolableTypeNameSet` -- a private field. The public surface is `PoolableTypeNames` (the list) and `IsPoolableTypeName` (the membership test the analyzer actually calls); the mirror-parity test already consumes the former. That change is comment-only: 7 changed lines, 0 of them code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
|
Both findings from the review at 1.
|
| test | pins |
|---|---|
Contravariance_Does_Not_Permit_A_Base_Typed_Set_Lambda |
CS1678 + CS1661 on the claimed call site, plus the Action<object> control proving the conversion is otherwise legal |
Set_With_A_Delegate_Value_Is_Not_Classified_At_All |
the delegate-value argument is not classified at all |
The second test's assertion is absence-shaped, and absence alone is vacuous — so it pairs the silent .Set with the identical write in lambda form in the same compilation, which must still fire. Mutation-checked rather than asserted:
baseline Failed=0 Passed=2 Total=2
mutant (IsPooledReceiver ns broken) Failed=1 Passed=1 Total=2
failed: Set_With_A_Delegate_Value_Is_Not_Classified_At_All
restore SHA256 identical
post-restore regression Failed=0 Passed=2 Total=2
Exactly the delegate-value test reddens; the compiler-diagnostic test stays green, since it tests the language rule rather than the analyzer's classification. The two are independently sensitive, and the first test would fail loudly — expected diagnostics absent — if a future C# ever relaxed the parameter match.
2. ElementPool.cs:55 — stale doc reference → valid, fixed
Confirmed by inspection: PoolableTypeNameSet is private static readonly at ModifierTable.cs:314; the public surface is PoolableTypeNames (:323) and IsPoolableTypeName(...) (:326). The doc now names both, with the role each plays — the list a reader can inspect, and the membership test the analyzer actually calls. ModifierTable.cs:122 and the mirror-parity test at ModifierUnsetClearValueTests.cs:471 already used the public name, so this was the only site out of step; a tree-wide sweep confirms PoolableTypeNameSet now appears only in its own declaration and its two public accessors.
That change is comment-only — 7 changed lines, 0 of them code.
State at 89f2bdd2: Reactor.Tests Release 13142 / 0 / 64 (+2 = exactly the two new tests) · Tooling gate run separately 203 / 0 / 0 · build 0 warnings / 0 errors · behind_by = 0 · porcelain clean.
Not merging — the user merges manually.
There was a problem hiding this comment.
🟢 Ready to approve
The pooling reset gaps are fixed at the correct scan-visible location, and the analyzer/tests/selftests/docs are updated consistently to match the new receiver-aware semantics.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
… checks Issue950_Unset_InlineOnlyLocalValueCleared and Issue950_AllTypes_InlineEndCleared assert ReferenceEquals(UnsetValue, x.ReadLocalValue(TextBlock.PaddingProperty)) on a captured reference. ElementPool.CleanElement already releases TextBlock.Padding, so a recycled control makes a stale reference read UnsetValue for the pool's reason rather than the unset arm's - the assertion passes without testing what it exists to test. Mirrors the six identity controls the fixture already carries, and the StackPanel one this branch added when it made StackPanel.Padding pool-released. Mutation-checked: pointing either finder at a name that does not resolve turns the identity check red while the absence check it guards stays green - which is the fail-open itself, reproduced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
Constraint addressed at
|
| identity controls | |
|---|---|
origin/main |
6 |
| this PR (before this commit) | 7 — adds Issue950_AllTypes_StackSameInstance |
The fail-open the constraint describes was closed in the same change that created it.
2. The other two from the #965 audit are now closed as well
Issue950_Unset_InlineOnlyLocalValueCleared and Issue950_AllTypes_InlineEndCleared are fail-open on main today — ElementPool.CleanElement has released TextBlock.Padding since #970, so both predate this PR and neither is caused by it. Same fixture, same mechanism, two lines each; closing them here rather than leaving a known-inert guard for someone to re-derive.
Identity controls: 6 → 9. No new fixtures, so no registry change.
3. ⚠️ Both string literals in the written-out fix do not resolve
The sketch at #965 comment 5148751123 asks for:
H.FindText("Issue950_InlineOnlyTarget") // actual: "Issue950_inline_only"
H.FindText("Issue950_AllTypes_InlineEndTarget") // actual: "Issue950_AllTypes_InlineEnd"Neither name exists in the fixture. FindText returns null, ReferenceEquals(x, null) is false, and both checks land red. The guidance to "read the comment rather than re-deriving it — it is the cheaper path by a wide margin" is sound about the diagnosis and unsafe about the literals; the sketch marks its third argument /* same predicate as mount */, which is the tell that it was written as a description and not executed. Correct predicates taken from the mount sites (:237/:253, :395/:410).
4. Mutation evidence — the guard fires while the check it guards stays green
Pointing either finder at a name that does not resolve:
not ok Issue950_Unset_InlineSameInstance <- new guard fires
ok Issue950_Unset_InlineOnlyLocalValueCleared <- absence check STILL PASSES
not ok Issue950_AllTypes_TextSameInstance <- new guard fires
ok Issue950_AllTypes_InlineEndCleared <- absence check STILL PASSES
That is the fail-open reproduced rather than argued: the absence assertions cannot distinguish the two producers on their own, and now something does.
Verification
Issue950 selftests 77 ok / 0 not ok
full selftest 6409 ok / 2 not ok
Reactor.Tests 13142 passed / 0 failed / 64 skipped
The 2 selftest failures are Issue487_SV2ParkedAtBottom and Issue487_SV2ClampObserved in Issue487ScrollAnchorFixtures.cs. Measured, not assumed: they reproduce identically on pristine HEAD with this change reverted and porcelain = 0. Untouched by this PR.
There was a problem hiding this comment.
🟡 Not ready to approve
A couple of updated docs/comments still describe the MOD_002 vs POOL_001 behavior in a misleading way and should be corrected to match the new receiver-aware semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/Reactor.Analyzers/ModifierTable.cs:124
- The attached-modifier docs here say an attached setter on a non-pooled receiver is “dropped by the next render”, but for non-pooled receivers the pool-return claim is what becomes false; the broader MOD_002 issue is that
.Setwrites aren’t unwound and force the update path. This wording is misleading for why MOD_002 applies.
/// <c>PoolResetSetAnalyzer</c> also requires the <c>.Set</c> lambda parameter's exact type
/// to be in <see cref="ModifierTable.PoolableTypeNames"/>; on anything else the same write reports
/// <c>REACTOR_MOD_002</c>, because the modifier still exists and the write is still dropped
/// by the next render — it just is not the pool that drops it. The instance and attached
plugins/reactor/skills/reactor-build-and-check/SKILL.md:116
REACTOR_MOD_002can now also be reported for attached-setter writes (Owner.SetProp(c, v)) when the receiver isn’t pooled (since attached writes are no longer always POOL_001). This row currently only mentions the instance-assignment shape, which will mislead users/agents reading the kit.
| `REACTOR_MOD_002` | info | `.Set(c => c.Prop = v)` where a first-class `.Prop(v)` modifier exists | Use the modifier. `.Set` setters re-run every render, are never unwound when a later render drops them, and `Element.SettersEqual` pins the element to the reconciler's update path. The fix rewrites the whole `.Set` — including multi-statement bodies — into a modifier chain. |
| `REACTOR_MOD_003` | warning | A common modifier that `ApplyModifiers` never writes to this element's control (`.Background(...)` on a `Rectangle`, `.Padding(...)` on a `Canvas`, `.CornerRadius(...)` on an `Image`) | The call compiles and is silently discarded. On a shape use the paint modifier (`.Fill(...)` / `.Stroke(...)` / `.StrokeThickness(...)` — the fix does the rewrite); otherwise host the element in a control that supports it. Gates: `Background` → Panel/Control/Border; `Foreground` and fonts → Control/TextBlock; `Padding` → Control/Border/Grid/StackPanel/RelativePanel/TextBlock; `CornerRadius` → Control/Border/Grid/StackPanel/RelativePanel; `BorderBrush`/`BorderThickness` → Control/Border. |
| `REACTOR_POOL_001` | warning | `.Set` writes a property `ElementPool.CleanElement` clears on pool return — either an instance write `c.Prop = v` (layout/appearance: `Margin`, `Width`/`Height`, `Min`/`Max` sizes, `HorizontalAlignment`/`VerticalAlignment`, `Opacity`, `AccessKey`, `IsTabStop`, plus `Padding`, `CornerRadius`, `BorderThickness`, `BorderBrush`, `Background` and `IsEnabled` on their supported receivers) or an attached setter `Owner.SetProp(c, v)` (`AutomationProperties.*`, `ToolTipService.*`, `TitleBar.IsDragRegion`, `FlexPanel.*`). **Receiver-aware:** fires only when the `.Set` lambda parameter's *exact* type is one `ElementPool` recycles — `TextBlock`, `RichTextBlock`, `StackPanel`, `Grid`, `Border`, `ScrollViewer`, `Canvas`, `Viewbox`, `ProgressBar`, `ProgressRing`, `Image`, `InfoBadge`, `Button`, `TextBox`, `ToggleSwitch`. The identical write on a receiver the pool never holds (`CheckBox`, `RelativePanel`, a user subclass of a pooled type) reports `REACTOR_MOD_002` instead — same modifier advice, minus the pool-return claim: the write lands and is never unwound, so what it costs is the element's structural skip (`Element.SettersEqual`), not the value. | Use the surviving modifier — the value is stored on `Element.Modifiers` and re-applied every render, whereas the `.Set` write is silently discarded when the control is reused. Most map 1:1 and ship a fix (`.Padding(...)`, `.Background(...)`, `AutomationProperties.SetName` → `.AutomationName(...)`, `ToolTipService.SetToolTip` → `.ToolTip(...)`); the rest are diagnostic-only because the modifier's shape differs — `.PositionInSet(position, size)`, `.Required()`, `.Flex(grow: ...)`. |
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…e in the kit Two suppressed findings from the Copilot review at 5e9f6c4. ModifierTable.cs said an unpooled receiver's write "is still dropped by the next render - it just is not the pool that drops it". That is the one claim MOD_002 does not make: the write lands and is never unwound, which is why the diagnostic sells the structural skip (Element.SettersEqual) rather than the value. Six other sites already word it that way - PoolResetSetAnalyzer.cs:55 and :181, ModifierTable.cs:83, :259, :301, and the generated analyzer-architecture.md row - so this was the lone outlier, and it contradicted the message the analyzer actually emits. The agent-kit MOD_002 row named only the instance-assignment shape. Since attached writes became receiver-aware they can report MOD_002 too, which POOL_001's row already stated - so the table disagreed with itself and the kit under-described the id an agent would see. Both wordings match Attached_Setter_Reports_ModifierAvailable_On_An_Unpooled_Receiver, whose own comment calls the pool-return sentence "the false claim". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
Both suppressed comments addressed at
|
| site | wording |
|---|---|
PoolResetSetAnalyzer.cs:181 (the emitted message) |
"re-runs every render, is never unwound, and keeps the element on the reconciler's update path" |
PoolResetSetAnalyzer.cs:55 |
"…and is never unwound when a later…" |
ModifierTable.cs:83 |
"structural skip (Element.SettersEqual), not the value" |
ModifierTable.cs:259 |
"the value is never unwound when a later…" |
ModifierTable.cs:301 |
"structural skip (Element.SettersEqual) rather than the value" |
docs/guide/analyzer-architecture.md:191 (generated) |
"re-runs every render, is never unwound, and pins the element to the update path" |
Six sites agree; :123-124 was the only dissenter, and it contradicted the string the analyzer actually emits. Rewritten to match :83/:301 verbatim in structure.
2. plugins/reactor/skills/reactor-build-and-check/SKILL.md:114 — correct, and the table disagreed with itself
The MOD_002 row named only .Set(c => c.Prop = v). Since attached writes became receiver-aware they report MOD_002 on an unpooled receiver — which the POOL_001 row two lines below already said. So the kit contradicted itself in adjacent rows, and an agent reading the MOD_002 row would not recognise the id it was about to see. Row now names both shapes. Table structure unchanged: 156 lines, uniform 6 pipe-fields across the block.
Both wordings are pinned by an existing test
Attached_Setter_Reports_ModifierAvailable_On_An_Unpooled_Receiver (PoolResetSetAnalyzerTests.cs:642) asserts POOL_001 on FakeElement and MOD_002 on UnpooledElement in one body, and its own comment calls the pool-return sentence "the false claim" — the same phrase the review reached for independently.
Verification
Reactor.Tests 13142 passed / 0 failed / 64 skipped
FullyQualifiedName~Tooling 203 passed / 0 failed
Issue950 selftests 77 ok / 0 not ok
No public API surface changed, so no reactor.api.txt regeneration; no gallery snippet touched; analyzer-architecture.md already carried the correct wording and needed no template edit.
There was a problem hiding this comment.
🟢 Ready to approve
The pooling reset behavior, analyzer rule selection, and the pool/analyzer parity invariants are all updated coherently and are backed by targeted unit + live selftest coverage in the changed areas.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The code-quality thread on this loop asked for an explicit filter instead of a `continue`, and the literal rewrite -- `Where(closure.ContainsKey)` followed by `closure[gateName]` -- hashes the same key twice, which is itself a flagged pattern. Project each gate through the dictionary once with GetValueOrDefault and drop the misses with OfType, so the exclusion still reads at the loop header and no key is looked up twice. ReadPoolableReceiverClosure never stores a null value (the walk is guarded by `type is not null`), so GetValueOrDefault + OfType<Type> is exactly equivalent to the ContainsKey + indexer pair it replaces. The sibling site that filters gate *names* without indexing is left alone -- it is a single lookup already. Mutation-checked: forcing the projection empty reddens the vacuity-defence assert at line 302, so the derivation is load-bearing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟢 Ready to approve
The pooling reset logic, analyzer receiver classification, and new/updated tests appear internally consistent and specifically validate the previously-missed leak paths without introducing obvious regressions.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The identity pin added earlier claimed more than it delivered. Its comment said pinning identity "is what keeps the next assertion a statement about ApplyModifiers", but ElementPool.Return pushes onto a per-type stack and TryRent pops it, so an unmount immediately followed by a remount of the same type hands back that exact instance. ReferenceEquals stays true across a round-trip that ran CleanElement, which means identity alone protects against the unlikely branch (a different instance) and fails open in the likely one. Write a Background sentinel on the StackPanel natively before the toggle and assert it survives. CleanElement releases Panel.Background (#985) and this element never declares .Background(...), so ApplyModifiers has no unset arm to run against it: the brush can only disappear via a pool round-trip. The clear added for #985 thus becomes the instrument that proves the pool did not run. Measured rather than argued: - no-op the StackPanel unset arm in Reconciler.ApplyModifiers -> Issue950_AllTypes_StackCleared goes RED (assertion is fail-closed today) - remove the sentinel write -> Issue950_AllTypes_StackNotPooled goes RED (the new check is non-vacuous) - StackSameInstance stays green under both, confirming the reconciler updates in place here and the pool never touches this element at present. The new check is therefore a tripwire: the day that changes, it reddens instead of StackCleared silently passing for the wrong reason. Full selftest suite 6410 ok / 2 not ok; both failures are Issue487_SV2ParkedAtBottom and Issue487_SV2ClampObserved, confirmed failing at pristine HEAD with this change absent. Refs #985, #965 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟢 Ready to approve
The pooling reset logic, analyzer rule selection, and supporting tests/docs are updated cohesively, and the changes appear internally consistent with the receiver/poolability contracts described in the PR.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…l-panel-resets CHANGELOG.md conflicted: both sides appended a bullet under the same `### Fixed` heading. Union resolution, no content from either side dropped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟢 Ready to approve
The pooling fix is implemented in the correct scanned region, analyzer semantics are aligned to exact-type poolability, and the change is backed by targeted unit + selftest coverage plus updated shipped guidance/docs.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…ol-control-panel-resets Three files conflicted. CHANGELOG.md and ModifierEventFixtures.cs are unions: both sides appended bullets under the same `### Fixed` heading, and both added `using` aliases. Nothing dropped from either side. ModifierUnsetClearValueTests.cs is structural. #1018 renamed three members and generalised `ReadUnsetArms` from a single-method 2-tuple to a multi-method 3-tuple driven by `ScannedMethods`; my side added seven members for the pool half. The two addition sets are disjoint -- zero name collisions -- and none of mine calls `ReadUnsetArms`, so theirs supersedes mine outright rather than needing a pick. `ReadCleanElementCommonBlock` takes #1018's Roslyn form. It is not a preference: the auto-merged region above the conflict had already replaced my regex method-location with their syntax walk, leaving my hunk referencing a `braceStart` that no longer exists. Their `SwitchStatementSyntax` lookup also removes outright the residual my comment documented at length -- a block comment whose inner line began with the dispatch keyword could satisfy `^\s*switch` and silently truncate the region. A syntax node cannot match a comment, so the hazard is gone rather than described. The comment is replaced with a shorter note recording why, and the four presence-shaped detectors it warned against deleting are still named in the remarks above. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
There was a problem hiding this comment.
🟡 Human review recommended
It changes core pooling reset semantics and analyzer severity selection with broad user-facing impact (warnings-as-errors), so a final human review is warranted despite strong test coverage.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently align pooling behavior, analyzer classification, and non-vacuous test coverage across all affected receivers without introducing evident correctness or API regressions.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Fixes #985.
The bug
Reconciler.ApplyModifierswrites six common modifiers onto receivers thatElementPool.CleanElementnever reset:ApplyModifierswrites toCleanElementreset (before)PaddingControl|Border|StackPanel|TextBlockBorder,TextBlockIsEnabledControlButton/ToggleSwitchcase arms onlyCornerRadiusControl|BorderBorderBorderBrushControl|BorderBorderBorderThicknessControl|BorderBorderBackgroundPanel|Control|BorderBorderOn mount
oldM is null, soApplyModifiersruns no unset arm. A recycledButton,ScrollViewer,GridorVStacktherefore started life carrying the previous renter's local value — and a local value outranks everyStylesetter in WinUI's dependency-property precedence order, so it could not be corrected by a style either.Panel.Backgroundwas the widest hole, sinceVStack/HStack/Gridare all poolable and.Background(...)is one of the most-used modifiers.This is the pooling half of the #952 family. #952 fixed the wrong shape of a reset (assigning a default instead of
ClearValue); #985 is the missing reset.The fix
One mutually-exclusive receiver chain in
CleanElement, mirroringApplyModifiers' receiver types exactly:Control,Border,PanelandTextBlockare pairwise disjoint, so the chain is exhaustive over those receivers. The fiveBorderclears moved out ofcase WinUI.Border:(which keeps onlyborder.Child = null), and the now-redundantIsEnabledclears were removed from theButtonandToggleSwitcharms.Placement is the fix, not an implementation detail
PR #984 (merged) carried these six clears briefly and backed them out in
14c8aaf4, as a missing-reset fix rather than the wrong-reset-shape fix that PR was scoped to — and because the escalation churn was out of scope. Its own message defers them explicitly: "Filed as a follow-up instead." #985 is that follow-up. (It was not a revert of #984, and not a rejection of this approach — the approach was deferred, never turned down.)The placement argument stands on its own measurement, not on that history:
PoolResetSetConsistencyTests.ReadCleanElementCommonBlockscans fromCleanElement's opening brace to the firstswitch (<param>), so anything past that point is invisible to every pool ⇄ analyzer invariant. Measured on a trial merge with #1003: no-op'ing three clears placed below the dispatch left the entire headless unit suite green, and only a live-WinUI selftest caught it — 25 of 26 checks stayedok, exactly one reddened.Here the clears sit inside the
FrameworkElement-common region, next to theIsTabStopprecedent from #162. That deliberately tripsEvery_Reset_Property_With_Matching_Modifier_Is_Tracked, which forces theModifierTableflip — the churn #984 tried to avoid and that #985 explicitly accepts as correct.TextBlock.Paddingcame along for the same reason: it has been reset since #950, but from thecase WinUI.TextBlock:arm, i.e. past the point every scanner stops reading.ModifierTable's claim thatPaddingis pool-reset onTextBlockwas the one receiver in that gate nothing verified — deleting the line used to break no test. It now lives in the same chain and is pinned.Because the six are now
poolReset: true,.Set(c => c.Padding = …)and friends reportREACTOR_POOL_001(Warning) where they previously reportedREACTOR_MOD_002(Info)..Padding(…),.Background(…), …).TreatWarningsAsErrorsmay need to convert those call sites (or suppress the id) when upgrading. Called out inCHANGELOG.md.AnalyzerReleases.Unshipped.mdrow required (RS2008does not apply).Deliberately out of scope
HorizontalContentAlignment/VerticalContentAlignmentleak by the identical mechanism (Reconciler.cs:3775-3786writes them to anyControl;ElementPool.cshas zeroContentAlignmentmatches). Not fixed here — #985 enumerates its scope and asks for adjacent finds to be split out, and flipping the pair would destroy the last ungatedREACTOR_MOD_002anchor that twoModifierAvailableAnalyzerTestscases depend on. Tracked as #1013, with the re-anchoring work and the fixture non-vacuity constraints written up.Tests
New selftest fixtures (registered in both
AllFixturesand theCreate()switch):ModifierPoolClearValueControl—Bordercarrier; addsBorderBrush/Backgroundcoverage that no live fixture had.ModifierPoolClearValueControlPanel— aScrollViewer(Control) +Grid(Panel) pair.ModifierPoolClearValueStackPadding—StackPanel, the Panel-but-not-Grid subset.ModifierPoolClearValueTextPadding—TextBlock, the fourthPaddingreceiver.ScrollViewerrather thanButtoncarries the Control case on purpose:ButtonElement's descriptor writesIsEnabledon every mount, which would make anIsEnabled-cleared oracle vacuous.ScrollViewerElementdeclares none of the six.Every fixture keeps three load-bearing guards, per
AGENTS.md§Checks that actually prove something:ApplyModifiers' unset arm rather thanCleanElement;ReferenceEqualsinstance-reuse guard — without it every "cleared" assertion passes trivially on a fresh control;ReadLocalValue(dp) == DependencyProperty.UnsetValue, never "equals the default value" — assigning the default is exactly the [Bug] Unsetting a common modifier writes the DP default as a local value instead of ClearValue, permanently overriding Style-provided values #952 bug and would sail past a default-value oracle.Source-scanning pins were extended too:
CleanElementRequiredClearsgained 14 receiver/property entries so an outright deletion fails positively rather than merely un-tripping an invariant.Both
ReadCleanElementCommonBlockimplementations are now line-anchored (RegexOptions.Multiline). Previously an unanchored regex let a comment containing the literalswitch (fe)truncate the scanned block and produce 32 misleading failures.Validation
dotnet test tests/Reactor.Tests(full headless)--filter "FullyQualifiedName~AnalyzerTests"--filter "FullyQualifiedName~Tooling"--self-test(full tier, 1404 checks)dotnet build Reactor.slnx -c Release -p:Platform=x64¹
Issue487_SV2ParkedAtBottom/Issue487_SV2ClampObserved. Verified by stashing all work, checking out the merge base (3f85ca49), rebuilding and re-running: identical 2 failures on the clean base.² This is the meaningful gate for the severity escalation —
samples/Directory.Build.propswiresReactor.Analyzersas anOutputItemType="Analyzer"andTreatWarningsAsErrors=trueis set forConfiguration=Release, so a newly-Warning diagnostic in any sample would be a hard break. (ParallaxViewPage's.Set(lv => lv.Background = null)stays silent becausePoolResetSetAnalyzer.IsNullOrDefaultskips null/default RHS.)Mutation-checked
Every new
ClearValueline was removed one at a time and the run re-executed; each produced exactly one matchingnot okand no others. TheTextBlock.Paddingline was additionally deleted rather than commented out — to defeat any comment-scanning artifact — andCleanElement_Releases_Every_Modifier_Backed_Dependency_Propertyfailed as expected (Failed: 1, Passed: 2747).Docs
plugins/reactor/skills/reactor-build-and-check/SKILL.md(shipped agent kit) — theREACTOR_POOL_001row named only attached setters; it now names the instance properties too, so an agent reading the kit doesn't mis-explain the new warnings.docs/_pipeline/templates/analyzer-architecture.md.dt— the claim thatCleanElement"clears far more attached properties than instance ones" was true before this change and is not now. Measured with a script mirroring the tests' own parsing logic: 35 instance sites (28 unique names) vs 32 attached (32 unique). Reworded in the template;docs/guide/analyzer-architecture.mdregenerated viamur docs compile --topic analyzer-architecture, diff confined to that one paragraph.Review
Ran the repo's
.github/skills/pr-review/skill across all 8 dimensions. security / api-ergonomics / packaging / alternative-solution-beyond-#1013 came back clean; the correctness, test-coverage and docs findings are all addressed above. The multi-model cross-check (different model family, high reasoning) returned 0 critical/high findings on the final commit.