Widen panel Padding and CornerRadius modifier gates - #1003
Merged
Alexandre Zollinger Chohfi (azchohfi) merged 9 commits intoAug 1, 2026
Merged
Widen panel Padding and CornerRadius modifier gates#1003Alexandre Zollinger Chohfi (azchohfi) merged 9 commits into
Alexandre Zollinger Chohfi (azchohfi) merged 9 commits into
Conversation
Alexandre Zollinger Chohfi (azchohfi)
requested a review
from Chris Anderson (codemonkeychris)
as a code owner
July 31, 2026 19:43
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
July 31, 2026 19:44
View session
Apply Padding and CornerRadius to the concrete WinUI panels that declare them, clear local values on reset and pool return, and keep REACTOR_MOD_003 in parity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Keep REACTOR_MOD_002 tests in parity, restore logical padding edges during modifier transitions, and harden the rendered-pixel oracle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Cover Grid and RelativePanel logical-padding mounts and every REACTOR_MOD_002 pair without broadening the pre-existing logical-transition algorithm. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Contributor
There was a problem hiding this comment.
Pull request overview
This PR widens Reactor’s “border-box” modifier gates so that .Padding(...) and .CornerRadius(...) are applied to the specific WinUI panel types that actually declare those dependency properties (notably Grid, StackPanel/VStack/HStack, and RelativePanel). It also aligns analyzer guidance, docs, samples, and pooling cleanup with the new runtime behavior so modifier application is consistent and style/theme values are restored correctly on unset.
Changes:
- Extend
Reconciler.ApplyModifiersto apply/clearPaddingforGrid/RelativePanelandCornerRadiusforGrid/StackPanel/RelativePanel, usingClearValue(...)on unset to restore styled values. - Update modifier analyzers/tests (
REACTOR_MOD_002/REACTOR_MOD_003) and shipped skills/docs/samples to reflect the widened gates. - Add selftest fixtures + registry entries for panel border-box behavior and pool reuse, and clear newly-written panel properties on pool return.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/AnalyzerTests/NoOpModifierAnalyzerTests.cs | Updates REACTOR_MOD_003 test matrix and messages for new supported panel types (Grid/RelativePanel) and remaining unsupported ones (Canvas). |
| tests/Reactor.Tests/AnalyzerTests/ModifierAvailableAnalyzerTests.cs | Updates REACTOR_MOD_002 rewrite expectations now that Grid/RelativePanel support more modifiers; adjusts mixed-block guard test accordingly. |
| tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs | Registers new selftest fixtures in both the name list and constructor switch. |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/Issue950PanelBorderBoxFixture.cs | Adds live selftest coverage for mount/update/unset returning to style, pooling behavior, and visual pixel proof for Grid corner clipping. |
| src/Reactor/Elements/ElementExtensions.cs | Updates CornerRadius section comment to match expanded supported set. |
| src/Reactor/Core/Reconciler.cs | Extends ApplyModifiers gates for Padding/CornerRadius to concrete panels and switches CornerRadius unset to ClearValue(...). |
| src/Reactor/Core/ElementPool.cs | Clears new panel-local values on pool return (Grid padding/corner radius, StackPanel corner radius). |
| src/Reactor.Analyzers/ModifierTable.cs | Aligns analyzer control-gate tables and explanatory text with the widened runtime gates. |
| skills/figma.md | Updates guidance for generated code to use Grid/RelativePanel directly for padding/corner radius and to reserve Border for strokes. |
| skills/design.md | Updates design guidance + compatibility table and examples to include Grid/RelativePanel padding and panel corner radius support. |
| samples/ReactorGallery/reactor-search-index.json | Regenerates search index snippet reflecting updated Margin/Padding guidance. |
| samples/ReactorGallery/ControlPages/DesignGuidance/SpacingPage.cs | Updates gallery content and code sample to reflect new supported Padding targets. |
| samples/apps/regedit/Components/ValueList.cs | Replaces imperative Grid padding workaround with declarative .Padding(...) now that gate supports Grid. |
| samples/.editorconfig | Updates REACTOR_MOD_003 debt counts and scoped file list after gate widening reduces hits. |
| plugins/reactor/skills/reactor-design/SKILL.md | Mirrors the design skill updates shipped via agent-kit. |
| plugins/reactor/skills/reactor-build-and-check/SKILL.md | Updates REACTOR_MOD_003 explanation and gate listing to match new behavior. |
| CHANGELOG.md | Documents the behavioral change: modifiers now apply to concrete panels and unset restores styles via ClearValue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Alexandre Zollinger Chohfi (azchohfi)
force-pushed
the
azchohfi-widen-panel-modifier-gates
branch
from
July 31, 2026 19:59
4adc293 to
3bfeafe
Compare
Contributor
📦 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 / |
Call out the pre-existing StackPanel padding debt while documenting the newly supported panel pairs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
July 31, 2026 20:13
View session
Contributor
🧪 Merged coverageCoverage for
No coverage change beyond the noise floor. ✅ ✅ higher / |
This was referenced Jul 31, 2026
Collaborator
Author
|
Heads-up from #1015 (issue #985, Nothing here needs changing in this PR on its own. It only matters for whoever merges second. 1. Two analyzer tests fail on the merged tree#1015 flips
Measured on the merged tree:
2.
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
July 31, 2026 22:53
View session
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Jul 31, 2026
Every_Diff_Guarded_Modifier_Has_An_Unset_Arm asks a per-property question, so it is satisfied the moment a property has any reset at all. Several arms are type-dispatch chains (`if (fe is WinUI.X) ... else if (fe is WinUI.Y) ...`) that get widened one control type at a time -- #970 added TextBlock to Padding, #1003 adds Grid/RelativePanel to Padding and Grid/StackPanel/RelativePanel to CornerRadius. Widening only the write half leaves a control type that can have the property set and never released: the #986 bug one type down, invisible to a property-level scan because the property's reset still exists for its original types. That is also the standing merge hazard on this file, since the two halves sit in different hunks and can auto-merge apart. Verified by mutation, on this branch and on the #986+#1003 merge result: deleting a single clear branch while leaving its write branch in place reddens Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear and nothing else, naming the property and the orphaned type. The same mutation passed silently before. Review comments: - github-code-quality flagged `a` as dereferenced after a null check in ApplyAccessibilityModifiers. The compiler's flow analysis proves the accesses safe (`a?.X.HasValue == true` implies non-null, and the project builds with nullable warnings as errors), but CodeQL cannot, and the explicit form reads better. Twelve set arms now use `a is not null && a.X... ` directly. The unset arms are unchanged: `a?.X.HasValue != true` never dereferences. Behaviour is identical, re-confirmed by re-running the ItemStatus unset-arm mutation, which still flips all three of its checks. - Removed the write-only `scanned` list from Every_Diff_Guarded_Modifier_Has_An_Unset_Arm. The stale-exception check reads `missing`, so `scanned` was dead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Jul 31, 2026
…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
This was referenced Jul 31, 2026
Merged
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 1, 2026 02:18
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Reactor/Core/ElementPool.cs:305
- The new pool-cleanup comment overstates the scope: this block only clears Grid.Padding/Grid.CornerRadius and StackPanel.CornerRadius, while StackPanel.Padding is a known pre-existing pool leak (not reset here, see the #965 note later). Please reword the comment so readers don’t infer that all modifier-backed panel locals are covered by this block.
// These properties are declared by the concrete panel types, not by Panel.
// Keep their resets above the type dispatch so the pool/reset consistency
// tests can account for every modifier-backed local value.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 1, 2026 02:29
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Reactor/Core/ElementPool.cs:305
- The new pool-cleanup comment implies this block accounts for “every modifier-backed local value”, but it only clears the newly-supported Grid.Padding/Grid.CornerRadius and StackPanel.CornerRadius. StackPanel.Padding is still explicitly not cleared (see the #965 note later), so this wording is easy to misread as complete coverage for panel border-box properties.
// These properties are declared by the concrete panel types, not by Panel.
// Keep their resets above the type dispatch so the pool/reset consistency
// tests can account for every modifier-backed local value.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 1, 2026 02:59
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Reactor/Core/Reconciler.cs:3733
- The inline-padding base value is computed via a long nested conditional expression, which is hard to read and easy to get wrong as the gate expands. A switch expression is equivalent but much more maintainable (and reduces risk of accidentally changing precedence when adding a new supported type).
var basePad = resolvedPadding ?? (fe is WinUI.Control pc ? pc.Padding : fe is WinUI.Border pb ? pb.Padding : fe is WinUI.StackPanel psp ? psp.Padding : fe is WinUI.Grid pg ? pg.Padding : fe is WinUI.RelativePanel prp ? prp.Padding : fe is WinUI.TextBlock ptb ? ptb.Padding : new Thickness());
src/Reactor/Core/ElementPool.cs:317
- This cleanup block declares
resetPanelbut never uses it, and the extra nesting isn’t needed. Using direct type patterns keeps the intent the same while avoiding unused-variable warnings and reducing indentation.
if (fe is WinUI.Panel resetPanel)
{
if (resetPanel is WinUI.Grid resetGrid)
{
resetGrid.ClearValue(WinUI.Grid.PaddingProperty);
resetGrid.ClearValue(WinUI.Grid.CornerRadiusProperty);
}
else if (resetPanel is WinUI.StackPanel resetStack)
{
resetStack.ClearValue(WinUI.StackPanel.CornerRadiusProperty);
}
}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94e12a39-ff6b-4da5-bf93-1bd79a5e58a6
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 1, 2026 03:24
View session
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 1, 2026
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
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 1, 2026
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
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 1, 2026
The union with #1003 widened CleanElement's Panel arm to clear Grid.Padding, Grid.CornerRadius and StackPanel.CornerRadius, but neither selftest carrier wrote those properties: the Grid carrier set only .Background(...) and the StackPanel carrier only .Padding(...) + .Background(...). Those three clears were therefore attested by the raw ClearValue source scan alone, which cannot tell a live clear from a commented-out one -- exactly the FALSE-GREEN shape AGENTS.md warns about, and the same blind spot that let TextBlock.Padding ship uncovered for a whole release. Both carriers now write the properties they are supposed to lose, with the matching phase-0 "local value written" premise (so the cleared assertions can come out the other way) and phase-2 ReadLocalValue == UnsetValue oracles. Mutation-checked per line, replacing each clear with `_ = 0;` rather than commenting it out, and asserting the mutation landed before reading the verdict: Grid.Padding clear removed -> only PoolCp_Phase2_PanelPaddingCleared reddens Grid.CornerRadius clear removed -> only PoolCp_Phase2_PanelCornerRadiusCleared reddens StackPanel.CornerRadius removed -> only PoolStack_Phase2_CornerRadiusCleared reddens Also corrects the StackPadding fixture's header comment, which claimed ApplyModifiers "only writes Padding to the StackPanel subset". That stopped being true when #1003 widened the gate to Grid. What actually keeps the Grid fixture from reaching the StackPanel clears is that CleanElement's Grid and StackPanel arms are mutually exclusive `else if` branches under one Panel receiver -- the exclusivity, not the property set, is what splits the fixtures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 1, 2026
CleanElement's FE-common block clears WinUI.Grid.PaddingProperty and WinUI.Grid.CornerRadiusProperty, but "Grid" was absent from InstancePropertyOwners. PoolResetSetConsistencyTests builds both reset scans from that one list -- the attached scan collects every `RECEIVER.ClearValue(OWNER.PROPProperty)` whose OWNER is *not* in it, and the instance pattern is generated from it -- so both Grid clears were read as *attached* properties named Grid.Padding / Grid.CornerRadius, then silenced by adding them to ModifierTable.DeliberatelyExcludedAttached. The entries documented their own defect: both reasons read "Instance dependency property on Grid, not an attached property with a static setter." That is a classification error being recorded rather than fixed. DeliberatelyExcludedAttached exists for genuinely attached properties the `Owner.SetPROP(x, v)` rule cannot match -- the three AutomationProperties list-valued ones, which have no static setter at all. Parking a misclassification beside them means a genuinely attached Grid.* reset added later lands in the same bucket and reads as already-triaged. Neither branch alone had both halves, which is why it survived: #985 wrote InstancePropertyOwners (adding Border, Panel, StackPanel, TextBlock for exactly this reason), #1003 added the Grid clears, and the union inherited the clears without the owner. Note StackPanel.PaddingProperty is cleared four lines below Grid.PaddingProperty in an identically-shaped statement and was classified correctly -- the asymmetry had no rationale behind it. Grid joins InstancePropertyOwners and both exclusions are dropped. This is #1003's own resolution rule ("remove Owner.Prop from DeliberatelyExcludedAttached iff Owner is in the resolved InstancePropertyOwners") applied to the corrected list. The change is necessarily atomic, and the suite enforces that: doing only the first half fails Attached_Reset_Scan_Sees_Every_Owner_The_Table_Names with "found no ClearValue at all for these owners: [Grid]" -- verified by running it (1 failed / 53 passed) before completing the second half. There is no behavior change. DeliberatelyExcludedAttached is consulted only by the consistency tests; no diagnostic, code fix, or runtime path reads it, and CleanElement is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
Alexandre Zollinger Chohfi (azchohfi)
deleted the
azchohfi-widen-panel-modifier-gates
branch
August 1, 2026 17:56
Alexandre Zollinger Chohfi (azchohfi)
pushed a commit
that referenced
this pull request
Aug 1, 2026
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
Alexandre Zollinger Chohfi (azchohfi)
pushed a commit
that referenced
this pull request
Aug 1, 2026
Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear carried a non-vacuity floor of `pairs >= 10` and a comment claiming 14 pairs. The oracle actually reads 28 pairs over 11 properties, so the floor tolerated losing 18 of them. That slack covered the exact arms the test exists for: dropping the Padding (6) and CornerRadius (5) chains outright still reports 17 and passes, and those are the two properties #970, #986 and #1003 each widened. A resolution that lost both chains was certified green. Raise the floor to 24 -- the loss of either chain now trips it, with four arms of slack for a legitimate narrowing, matching the ratio the per-method MinArms floors already use. Boundary-verified against the oracle's own count rather than a re-derivation: removing 4 CornerRadius arms leaves 24 and passes, removing 5 leaves 23 and fails. A line-scoped text scan reports 22 here because it misses writes nested inside a gate's block, so the comment now says to read the number out of the assertion message. Comment and constant only; no product code and no Reconciler.cs change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 4, 2026
* Add missing unset arms for modifiers that never released (#986) `ApplyModifiers` writes a modifier only when it is present in the new bag. Five common modifiers had no `else` arm at all, so dropping one from a chain left the previous render's write pinned on the control forever: phase == 0 ? Button("Go", Go).IsTabStop(false) : Button("Go", Go) // still not a tab stop `ApplyAccessibilityModifiers` was worse. It opened with `if (a is null) return;` while the caller invokes it precisely when `a11y is not null || oldA11y is not null` — so dropping the *whole* accessibility sub-record reached the method and returned immediately, releasing nothing. Ten of its eleven properties also had no per-property unset arm even when the bag survived. This is the mirror image of #952: there the reset ran but used the wrong shape; here it never ran. Changes - `ApplyModifiers`: unset arms for `ElementSoundMode`, `HeadingLevel`, `IsTabStop`, `TabIndex`, `XYFocusKeyboardNavigation`. The two Control-gated arms rebind their own pattern variable (the existing `enCtrl`/`enCtrl2` shape) — reaching the `else` only proves the whole set condition was false, which a non-Control satisfies while still having the modifier set. - `ApplyAccessibilityModifiers`: drop the early return, read every property through `a?.X`, and add `ClearValue` arms for the ten properties that lacked one. `LabeledBy` already had one; the `*Refs` edges unwind through `ApplyModifierReferenceEdges`. Every reset uses `ClearValue(<DP>Property)`, never a default assignment: a local value outranks every `Style` setter in WinUI's precedence order (#952). Tests - `ModifierUnsetClearValueTests` gains the positive scan the existing shape tests admitted they lacked — every diff-guarded set arm in both reconciler methods must have a matching unset arm. 49 modifiers are scanned today against a non-vacuity floor of 30, with a documented exception dictionary and a stale-exception assertion so entries cannot rot. - Selftests pin the behaviour, since only `ReadLocalValue(dp) == DependencyProperty.UnsetValue` distinguishes "released" from "assigned the default", and headless xUnit cannot construct a WinUI object. `ModifierClearResets` gains five release checks; a new `AccessibilityModifierClearResets` fixture covers apply / partial drop / whole-bag drop. - Each new arm was mutation-checked: removing the five Tier-1 `ClearValue` calls flips exactly the five new checks; restoring the early return flips only the whole-bag-drop phase; removing the eleven a11y arms flips the per-property phases. The transform family (`Scale`/`Rotation`/`Translation`/`CenterPoint`) is deliberately excluded and tracked in #1001: those are facade DPs whose live value sits on the composition visual, so `ClearValue` alone cannot undo an animated write. They are recorded in the new exception dictionary rather than silently tolerated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * fix(reconciler): guard deferred LabeledBy against stale re-application Follow-up to the multi-model pr-review pass on the issue #986 unset-arm work. Two high-severity findings: 1. The deferred LabeledBy path parks a `Loaded` handler when the target AutomationId is not yet in the tree. If the modifier is dropped before that handler runs, the reset arm's ClearValue is immediately undone by the stale handler, re-pinning a label the caller no longer asks for. Publish the in-flight request as ReactorState.PendingLabeledBy before parking, and have the handler no-op unless it still matches. Reset it alongside PendingEchoMatch in ClearCurrentEventHandlers, DetachReactorState, and the pool-return path. 2. The ModifierClear_Tier1Applied phase-0 assertions were a single 10-clause conjunction, so a broken *set* arm could be masked. Split into five per-property checks that each assert a local value exists AND the intended non-default effective value; release checks now compare against the phase-0 value rather than a guessed WinUI default. Structural gate: per-method non-vacuity floors replace the single global arm count, and a new Roslyn test pins the handler's pending-request re-check (the harness cannot hold `Loaded` open across a re-render, so the behavioural fixture takes the synchronous path on a warm machine). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * Close the type-branch blind spot and address code-quality review Every_Diff_Guarded_Modifier_Has_An_Unset_Arm asks a per-property question, so it is satisfied the moment a property has any reset at all. Several arms are type-dispatch chains (`if (fe is WinUI.X) ... else if (fe is WinUI.Y) ...`) that get widened one control type at a time -- #970 added TextBlock to Padding, #1003 adds Grid/RelativePanel to Padding and Grid/StackPanel/RelativePanel to CornerRadius. Widening only the write half leaves a control type that can have the property set and never released: the #986 bug one type down, invisible to a property-level scan because the property's reset still exists for its original types. That is also the standing merge hazard on this file, since the two halves sit in different hunks and can auto-merge apart. Verified by mutation, on this branch and on the #986+#1003 merge result: deleting a single clear branch while leaving its write branch in place reddens Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear and nothing else, naming the property and the orphaned type. The same mutation passed silently before. Review comments: - github-code-quality flagged `a` as dereferenced after a null check in ApplyAccessibilityModifiers. The compiler's flow analysis proves the accesses safe (`a?.X.HasValue == true` implies non-null, and the project builds with nullable warnings as errors), but CodeQL cannot, and the explicit form reads better. Twelve set arms now use `a is not null && a.X... ` directly. The unset arms are unchanged: `a?.X.HasValue != true` never dereferences. Behaviour is identical, re-confirmed by re-running the ItemStatus unset-arm mutation, which still flips all three of its checks. - Removed the write-only `scanned` list from Every_Diff_Guarded_Modifier_Has_An_Unset_Arm. The stale-exception check reads `missing`, so `scanned` was dead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: address code-quality review round 2 on the #986 modifier gates Five `github-code-quality` findings on PR #1018. LINQ shape (3 findings) - the three scanner loops in ModifierUnsetClearValueTests filtered with `continue` guards instead of a predicate: * `Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear` - replaced `TryReadTypeGate(if, out string, out string) : bool` with `ReadTypeGate(if) : (string TypeName, string Variable)?` so the gate can be projected and filtered rather than tested with an `out`-param guard. * `ReadUnsetModifierNames` / `ReadDiffGuardedModifiers` - folded the guards into `.Where(...)` predicates. Phase-0 coupling (2 findings) - `initial.IsTabStop == false` and `button.IsTabStop != false` were flagged as boolean-literal comparisons. The literal is the *phase-0 bag value*, not a predicate: the release check asserts the effective value moved away from what the bag wrote, deliberately not towards a guessed WinUI default (guessing it once already produced a false failure in this suite). Collapsing to `!initial.IsTabStop` would have broken the five-row parallel table and hidden that coupling. Introduced `const bool Phase0IsTabStop` and wired it through the bag *and* both checks, so the coupling is now enforced by the compiler rather than only described in a comment. Mutation-verified after the refactor - both scanners still redden: * drop the `StackPanel` Padding clear -> type-parity test fails with `[Padding on StackPanel]` * drop the whole `IsTabStop` unset arm -> existence test fails with `[ApplyModifiers.IsTabStop]` Unit 7/7, selftest `--filter ModifierEvent` 150 ok / 0 not ok. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: pin the deferred LabeledBy staleness-guard ordering Round-3 Copilot review raised a *suppressed* finding (no inline thread) on the deferred LabeledBy Loaded handler: clearing `PendingLabeledBy` before checking whether `FindByAutomationId` resolves could `prematurely cancel the pending request`, including when `multiple parked handlers run in the same Loaded event`. The finding is incorrect as stated. The handler unsubscribes itself as its first statement, so it fires at most once - there is no later handler for the same request to prevent, and once it has run the token no longer describes anything outstanding whether or not the target resolved. Leaving it set on failure would make the token assert a parked handler that does not exist. The multi-handler case is real but already safe: the set arm's `a.LabeledBy != oldA?.LabeledBy` diff guard means a second park always carries a *different* labelId, so the stale handler's `PendingLabeledBy != labelId` check returns before it can touch the live request's token. But that safety rests entirely on statement *ordering*, and nothing pinned it. Hoist the retirement above the staleness guard and the stale handler cancels the live one, which then no-ops and leaves the label unresolved - the reviewer's scenario, reachable by a plausible edit. So the concern is now guarded. The first oracle for this was vacuous. Keying on the first `return` in the handler body passes under the hoist mutation, because the handler opens with an unrelated `TryGetReactorState` bail-out that still sorts before the clear. Replaced with a syntax-span comparison between the if-statement whose condition names `labelId` and the `PendingLabeledBy = null` assignment. Mutation-verified: hoisting the retirement above the staleness guard fails the test; the string-index version passed the same mutation. Unit 7/7, `dotnet build Reactor.slnx` exit 0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * fix: avoid allocating ReactorState on the sync LabeledBy path; document CenterPoint identity Three *suppressed* findings from the round-4 Copilot review (again emitted in the review body with no inline thread). All three are correct. **Reconciler.cs:4332** - the synchronous LabeledBy resolution path called `GetOrCreateReactorState(fe).PendingLabeledBy = null`, which allocates and attaches a ReactorState even when there was never a deferral. Synchronous resolution is the *common* path - deferral only happens when the element is not yet in the visual tree - so this cost one attached state per element that ever resolves a LabeledBy. With no state there is no parked handler and nothing to retire, so `TryGetReactorState` is both cheaper and semantically identical. This now matches the shape already used by the unset arm below it. **modifier-system template + generated guide** - the transform caveat named four modifiers but spelled identity resets for only three, which invites the reader to assume `.CenterPoint()` is safe to omit while the other three are not. Added `.CenterPoint(Vector3.Zero)`, and noted that unlike `.Translation()` it has no component-wise overload - the reason the example was easy to leave out. Edited the `.md.dt` template and regenerated via `mur docs compile --topic modifier-system`; the generated diff is the four intended lines with no snippet churn. Selftests after the Reconciler change: ModifierEvent 150 ok, ChartA11y 181 ok, Accessibility 28 ok, 0 not-ok across all three. Unit 7/7. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: fix a WaitFor short-circuit and split the release oracle from its non-vacuity guard Two more *suppressed* findings (round 5, again no inline threads). Both correct. **The WaitFor short-circuit is a real flake.** The predicate was `ReferenceEquals(AP.GetLabeledBy(applied), labelSource())`, and my own comment directly above it asserted this was `false at t=0 and converging, which is the one shape WaitFor is sound for`. That was wrong: `ReferenceEquals(null, null)` is **true**, so with the label source not yet discoverable and LabeledBy not yet resolved the wait returned immediately at zero elapsed time - having never waited - and the following check then failed spuriously on a still-null source. The predicate now requires the source inside the same lambda, which is what makes it genuinely eventual. This is the exact trap AGENTS.md documents for WaitFor; the comment claiming soundness made it harder to spot, not easier. **The release checks conflated two different failures.** Each read `ReadLocalValue(...) == UnsetValue && <effective> != <phase-0 value>`. The first clause is the product oracle; the second is a non-vacuity guard against a phase-0 value that happens to equal the control's default. Folded together, a fixture-design problem presented as a reconciler regression. Split into five `ModifierClear_*Cleared` checks (local-value oracle alone, as suggested) and five `ModifierClear_*Phase0WasDistinct` checks. The guard is kept rather than dropped - without it a property whose default already equals the phase-0 value would satisfy the release oracle trivially - but a `*Cleared` failure now means a product bug unambiguously. Applied to all five properties, not just the flagged `IsTabStop`: the shape occurred five times and the concern is structural. Mutation-verified: dropping the `IsTabStop` unset arm reddens `ModifierClear_IsTabStopCleared`, so the split did not weaken the oracle. Selftest `--filter ModifierEvent` 155 ok / 0 not-ok (150 before, +5 new checks), including all 14 `A11yClear_*`. ChartA11y 181 ok. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * docs: correct the transform-family caveat; fix a stale remark on the a11y bag Two more *suppressed* findings (round 6). Both are documentation-accuracy and both are right. **The transform caveat overstated the mechanism.** It claimed the four transform modifiers `write through the element's composition visual rather than through the XAML property system, so ClearValue has nothing to release`. Verified against `AnimationHelper`: `SetOrAnimateVector3` resolves the ambient curve first and, when there is none, falls through to `SetVector3Direct`, which assigns the XAML facade property (`element.Scale = value`) - an ordinary DP write that `ClearValue` would release. Only the *animated* path takes `ElementCompositionPreview.GetElementVisual` + `StartAnimation`. So the real reason these four are deferred to #1001 is that they have no unset arm **at all**, plus the animated case additionally needing the composition animation stopped before a reset can bite. The old wording would have told a reader - and whoever picks up #1001 - that the un-animated case is unfixable by the normal mechanism, which is false. Reworded, and applied the same correction to the `MissingUnsetArmExceptions` rationale, which carried the same claim. **Stale remark on ApplyAccessibilityModifiers.** The remarks said `Every property is therefore read through a?.X`, which stopped being true when the 12 set arms were rewritten to `a is not null && a.X…` to satisfy the CodeQL null-deref rule earlier in this PR. Now states the actual split: set arms guard with `a is not null`, reset arms read `a?.X`. Template edited and regenerated via `mur docs compile --topic modifier-system`. Unit 7/7, `dotnet build Reactor.slnx` exit 0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: locate the CleanElement FE-common boundary via Roslyn, not a text scan ReadCleanElementCommonBlock found the end of the FE-common region with source.IndexOf("switch (fe)"). Any comment mentioning the dispatch matches first, which silently truncates the region and makes every absence-shaped assertion over it vacuous while still reporting green. Measured on this tree: injecting a line comment naming the dispatch shrinks the region from 7796 to 40 characters. With a real violation placed below the comment, the old boundary reports PASS while the new one fails and names [AllowDrop]; same product file, only the boundary form differs. An anchored regex is not sufficient either -- a block comment whose inner line begins with the dispatch text still satisfies ^\s*switch. A SwitchStatementSyntax cannot be forged by a comment of any shape, so the boundary is now a syntax node matched on the method's actual parameter identifier. The file already parses with Roslyn elsewhere, so this introduces no new dependency. Region on a clean tree is unchanged at 7796 characters and the suite is unchanged at 13130 passing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * docs: name the four presence-shaped detectors the boundary does not replace The syntax-node boundary closes the comment-forgery route, but that is not a reason to drop the presence-shaped detectors: the pins guard this region's scope, the absence scan guards the pin list's closed world, and neither subsumes the other. Enumerate all four (one here, three in PoolResetSetConsistencyTests) so nobody deletes one believing the boundary made it redundant. Comment-only: 13 added lines, all ///, 0 removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * Calibrate the type-gated pair floor to the population it guards Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear carried a non-vacuity floor of `pairs >= 10` and a comment claiming 14 pairs. The oracle actually reads 28 pairs over 11 properties, so the floor tolerated losing 18 of them. That slack covered the exact arms the test exists for: dropping the Padding (6) and CornerRadius (5) chains outright still reports 17 and passes, and those are the two properties #970, #986 and #1003 each widened. A resolution that lost both chains was certified green. Raise the floor to 24 -- the loss of either chain now trips it, with four arms of slack for a legitimate narrowing, matching the ratio the per-method MinArms floors already use. Boundary-verified against the oracle's own count rather than a re-derivation: removing 4 CornerRadius arms leaves 24 and passes, removing 5 leaves 23 and fails. A line-scoped text scan reports 22 here because it misses writes nested inside a gate's block, so the comment now says to read the number out of the assertion message. Comment and constant only; no product code and no Reconciler.cs change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: give the type-gated parity test a clear-side non-vacuity floor Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear had a floor over the write side only. A failure confined to ReadClearedProperties therefore left pairs at 28 (floor green) while emptying clears, so every property took the continue and offenders came out empty -- both arms of the test reporting that nothing was wrong. ReadClearedProperties has exactly one call site, so nothing else in the suite covered it. Measured, not argued: breaking that reader alone and rebuilding produced Failed: 0, Passed: 7. With the clear-side floor it produces "Only 0 type-gated modifier clear(s) ... while the write side still reads 28". The population is read through this test's own parser rather than a text scan -- 28 clears over the same 11 properties, mirroring the writes exactly -- and the floor is calibrated identically to its sibling, so losing either of the two longest chains (Padding 6, CornerRadius 5) trips it. Test-only. No product code; Reconciler.cs deliberately untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * docs: correct a false premise in the clear-side skip comment The comment on the `clears.TryGetValue` continue claimed "several properties legitimately clear through an ungated fe.ClearValue(...)". Measured through the test's own parser, zero properties do: all 11 written properties are present in `clears`, so the skip count is 0. Two mutations settle what the branch is actually for: delete an unset arm, keep the write -> Every_Diff_Guarded_Modifier_Has_An_ Unset_Arm fails naming the property keep the arm, move the clear to an ungated fe.ClearValue(...) -> 7/7 green, correctly: the property is still released, it is just no longer a type-dispatch chain to compare against So the missing-arm case is covered by the sibling test, and the branch exists only for the ungated-but-correct shape. Left as a continue rather than tightened into an assertion: asserting the skip count is zero would fail on that correct shape, and the obvious remediation would be to re-gate a clear that never needed gating - churn in Reconciler.cs to fix nothing. Comment only; no assertion or logic change (0 non-comment lines in the diff). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: catch a deleted unset arm the exclusion list hid Every_Type_Gated_Write_Has_A_Matching_Type_Gated_Clear skipped any property absent from its `clears` map. That map is gate-bound, so a property whose whole unset arm was deleted vanished from it and was never compared -- the test certified the exact defect it exists to catch. The per-property sibling does not cover the gap. Its exclusion list names the three modifiers that compute a local before testing it (Margin, Padding, BorderThickness), whose conditions never mention `m.X`. Measured: deleting BorderThickness's whole unset arm while keeping both type-gated writes left all seven tests in this file green -- sibling excludes it, write floor untouched at 28, clear floor falls only 28 -> 26 against a floor of 24. Read ungated clears too and classify three ways instead of skipping: a type-gated chain is compared as before, an ungated `fe.ClearValue(...)` is accepted, and neither is an offender. Asserting nothing was skipped would have been wrong -- the ungated shape is semantically correct and would false-fail, and its obvious remediation is to re-gate a clear that never needed gating, in the most contended file in the repo. Mutation-verified both directions on the same property, with the aim measured out of band through the oracle's own parser: arm deleted, write kept gated 2->0, ungated 0 -> RED, names [BorderThickness] clear made ungated gated 2->0, ungated 1 -> GREEN, no false positive Reconciler.cs restored byte-identical after each arm; suite 13130/0/64. Refs #986 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: scope the unset-arm identifier names to both scanned methods ReadUnsetArms() scans ApplyModifiers AND ApplyAccessibilityModifiers, but three identifiers still carried an ApplyModifiers-only name. A coverage auditor reading them would conclude the accessibility half is unscanned. Every_ApplyModifiers_Unset_Arm_Clears_The_Dependency_Property -> Every_Unset_Arm_Clears_The_Dependency_Property Every_ApplyModifiers_Unset_Arm_Actually_Resets_Something -> Every_Unset_Arm_Actually_Resets_Something ApplyModifiersAssignmentExceptions -> UnsetArmAssignmentExceptions The dictionary was not merely broad but backwards: its sole entry, PendingLabeledBy, occurs five times in ApplyAccessibilityModifiers and zero times in ApplyModifiers. The mutation that removes that entry reports the offender as `ApplyAccessibilityModifiers.LabeledBy`, i.e. the excluded method. Tests only; no product code. 7/7 pass and the removed-entry mutation reddens exactly one, so the renamed dictionary is still consulted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee * test: hoist a per-method non-vacuity floor into ReadUnsetArms The unset-arm scan covers both ApplyModifiers and ApplyAccessibilityModifiers, but its only non-vacuity guard was a single combined `arms.Count >= 20` on one caller. ApplyModifiers alone contributes 36 arms, so that floor cleared on its own: if the matcher stopped recognizing ApplyAccessibilityModifiers, all 12 of its arms could vanish and the test stayed green. Every_Unset_Arm_Actually_Resets_Something consumes the same helper and had no floor at all -- strictly worse than the reported case, and not reported. The floor now lives inside ReadUnsetArms and is per method, so both current callers and any future one inherit it, and the failure message names the method and its count. The existing per-method MinArms floors did not cover this: ReadUnsetArms discarded MinArms, and that floor governs the set-arm scan in a different test over a different population. Measured unset-arm counts 36 (ApplyModifiers) and 12 (ApplyAccessibilityModifiers); floors 31 and 10, matching the ratios the existing floors in this file already use (32/37, 10/12, 24/28). Mutation receipt -- made the accessibility bag identifiers unmatchable: both consumers failed with "Only 0 modifier-unset arm(s) were read out of ApplyAccessibilityModifiers (expected at least 10)"; the removed combined floor would have read 36 >= 20 and passed. Restored byte-equal, 7/7. Tests only; no product code, and Reconciler.cs is deliberately untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: z <z@z.z> Copilot-Session: 0533f073-1d93-4f3a-9e34-85f02b4ddaee
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 4, 2026
…fier properties (#985) (#1015) * Fix ElementPool.CleanElement never releasing Control/Panel modifiers (#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 * Correct post-union receiver lists in three explanatory comments 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 * Assert the pool round-trip premise for both carriers in the Control+Panel fixture Copilot review at bc1fdb4 found that ModifierPoolClearValueControlPanel asserts the "returned to pool" premise only for the ScrollViewer carrier. The Grid carrier had no equivalent check, so the Panel.Background assertion could start passing via ApplyModifiers' unset arm instead of ElementPool.CleanElement if the Grid ever stopped being removed and re-added. The ReferenceEquals guards do not close this gap: an in-place update reuses the instance too, so instance identity cannot distinguish a pool round-trip from an update. Absence from the tree in phase 1 is the only thing that can. Confirmed by mutation rather than argument. Rendering the Grid in phase 1 instead of Empty() - so it is updated in place and never pooled: not ok PoolCp_Phase1_PanelReturned <- the new check reddens ok PoolCp_Phase2_PanelBackgroundCleared <- still passes, via the unset arm That is the vacuity the finding predicted: without the new check the panel half reports green while exercising the wrong code path. With it, the fixture fails loudly instead. Mutation reverted; all five ModifierEvent_PoolClearValue* fixtures pass (0 failures). Swept the other pool round-trip fixtures for the same asymmetry - PoolClear, PoolCtrl, PoolStack, PoolText and the pre-existing TipPool each have a single carrier, so ControlPanel was the only one able to have it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Cover the three Panel clears that only the source scan could see The union with #1003 widened CleanElement's Panel arm to clear Grid.Padding, Grid.CornerRadius and StackPanel.CornerRadius, but neither selftest carrier wrote those properties: the Grid carrier set only .Background(...) and the StackPanel carrier only .Padding(...) + .Background(...). Those three clears were therefore attested by the raw ClearValue source scan alone, which cannot tell a live clear from a commented-out one -- exactly the FALSE-GREEN shape AGENTS.md warns about, and the same blind spot that let TextBlock.Padding ship uncovered for a whole release. Both carriers now write the properties they are supposed to lose, with the matching phase-0 "local value written" premise (so the cleared assertions can come out the other way) and phase-2 ReadLocalValue == UnsetValue oracles. Mutation-checked per line, replacing each clear with `_ = 0;` rather than commenting it out, and asserting the mutation landed before reading the verdict: Grid.Padding clear removed -> only PoolCp_Phase2_PanelPaddingCleared reddens Grid.CornerRadius clear removed -> only PoolCp_Phase2_PanelCornerRadiusCleared reddens StackPanel.CornerRadius removed -> only PoolStack_Phase2_CornerRadiusCleared reddens Also corrects the StackPadding fixture's header comment, which claimed ApplyModifiers "only writes Padding to the StackPanel subset". That stopped being true when #1003 widened the gate to Grid. What actually keeps the Grid fixture from reaching the StackPanel clears is that CleanElement's Grid and StackPanel arms are mutually exclusive `else if` branches under one Panel receiver -- the exclusivity, not the property set, is what splits the fixtures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Classify Grid's instance DPs as instance, not suppressed-attached CleanElement's FE-common block clears WinUI.Grid.PaddingProperty and WinUI.Grid.CornerRadiusProperty, but "Grid" was absent from InstancePropertyOwners. PoolResetSetConsistencyTests builds both reset scans from that one list -- the attached scan collects every `RECEIVER.ClearValue(OWNER.PROPProperty)` whose OWNER is *not* in it, and the instance pattern is generated from it -- so both Grid clears were read as *attached* properties named Grid.Padding / Grid.CornerRadius, then silenced by adding them to ModifierTable.DeliberatelyExcludedAttached. The entries documented their own defect: both reasons read "Instance dependency property on Grid, not an attached property with a static setter." That is a classification error being recorded rather than fixed. DeliberatelyExcludedAttached exists for genuinely attached properties the `Owner.SetPROP(x, v)` rule cannot match -- the three AutomationProperties list-valued ones, which have no static setter at all. Parking a misclassification beside them means a genuinely attached Grid.* reset added later lands in the same bucket and reads as already-triaged. Neither branch alone had both halves, which is why it survived: #985 wrote InstancePropertyOwners (adding Border, Panel, StackPanel, TextBlock for exactly this reason), #1003 added the Grid clears, and the union inherited the clears without the owner. Note StackPanel.PaddingProperty is cleared four lines below Grid.PaddingProperty in an identically-shaped statement and was classified correctly -- the asymmetry had no rationale behind it. Grid joins InstancePropertyOwners and both exclusions are dropped. This is #1003's own resolution rule ("remove Owner.Prop from DeliberatelyExcludedAttached iff Owner is in the resolved InstancePropertyOwners") applied to the corrected list. The change is necessarily atomic, and the suite enforces that: doing only the first half fails Attached_Reset_Scan_Sees_Every_Owner_The_Table_Names with "found no ClearValue at all for these owners: [Grid]" -- verified by running it (1 failed / 53 passed) before completing the second half. There is no behavior change. DeliberatelyExcludedAttached is consulted only by the consistency tests; no diagnostic, code fix, or runtime path reads it, and CleanElement is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Guard the StackPanel absence assertion this PR disarms ElementPool.CleanElement now releases StackPanel.Padding, which turns Issue950_AllTypes_StackCleared fail-open. It asserts the local value is UnsetValue, but a recycled StackPanel reads UnsetValue *because the pool cleaned it* -- indistinguishable from the unset arm having cleared it on the live control. Before this PR the same assertion was fail-closed by accident: a stale reference still held Thickness(22) and failed loudly. Nothing in this PR's diff contains the defect, and the fixture is untouched by it, so neither a diff-scoped nor a blame-scoped review can see it -- the breaking commit is correct and the test is not edited. Adds the identity positive control the fixture already uses either side of it: Issue950_AllTypes_BorderSameInstance and, in the same file, Issue950_Unset_ControlSameInstance and Issue950_Ownership_SameInstance. Scope: exactly one assertion here is newly disarmed by this PR. The Control.Padding check is already guarded (Issue950_Unset_ControlSame- Instance); the three TextBlock.Padding checks are fail-open from the pre-existing TextBlock clear on main, not from this change, and are left for #965 where a reader arrives with the context to act. Verified: - the new guard reddens under mutation (re-find broken -> not ok), while StackCleared stays green -- which is the point - PoolStack_Phase2_PaddingCleared confirms a pooled StackPanel reads UnsetValue at HEAD, establishing the fail-open premise empirically - Issue950 selftests 1..10 green; Reactor.Tests 13043/0 in Release with TreatWarningsAsErrors and 0 warnings Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Correct a false claim in the poolReset rationale comment The comment introduced by this PR asserted "A .Set write to any of them is now unwound on pool return." That is false for one gated receiver. poolReset is a per-property flag, so PoolResetSetAnalyzer selects POOL_001 for every receiver the control gate admits, without consulting poolability. RelativePanel is in the Padding and CornerRadius gates but absent from ElementPool.PoolableTypes, so a .Set write there is never unwound on pool return -- POOL_001's leading clause and its whole Description are false for that receiver, and this PR is what escalates it from MOD_002 (Info) to POOL_001 (Warning) by flipping poolReset. The advice the diagnostic gives is still correct (the write is dropped by the next render, which is exactly the MOD_002 hazard), so this is a wrong-reason and wrong-severity defect rather than wrong advice. Fixing it needs PoolableTypes mirrored into the netstandard2.0 analyzer plus a parity gate and receiver-aware rule selection, so it is filed as #1051 rather than changed here. Comment-only: 11 comment lines added, 2 removed, no code lines touched. Release build 0 warnings / 0 errors (analyzer DLL timestamp confirmed to advance past the source edit, so the result is not a stale-binary skip), Reactor.Tests 13043/0/64, union checker 17/17. Raised twice by automated review, in suppressed-comment blocks carrying no thread. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Derive the pool-clear obligation from the gate and the poolable set (#1017) CleanElementRequiredClears is a hand-maintained snapshot of today's answer. It cannot notice that a receiver became poolable, because nothing in the file reads ElementPool.PoolableTypes -- the two halves of the invariant were both present and never joined. Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement computes the same obligation from the two sources that actually decide it: ModifierTable's control gates and the poolable set. Adding a type to PoolableTypes makes its gated clears mandatory on the next run with no edit to this file, which is the case the static rows structurally cannot cover. The intersection is by assignability, not by name. Control is a gate receiver and never appears in PoolableTypes itself -- Button, TextBox and ToggleSwitch do -- so a name-based intersection derives no Control.* obligation at all and reports no violation. The receiver universe is therefore each pooled type's own base chain, and derivedBySubclass guards that it stays that way. RelativePanel is excluded structurally rather than by a list: nothing poolable is a RelativePanel today, so no clear is owed. See #1051 for the analyzer-side consequence of the same asymmetry. Reading PoolableTypes reflectively throws on a missing, wrong-typed or empty field. A lookup that degrades to empty makes every derived obligation vanish and the test pass over nothing, which is precisely what a rename produces. Derived set is 16 pairs: Padding x5, CornerRadius x4, BorderThickness x2, BorderBrush x2, Background x3. Control.IsEnabled is not derivable -- it is poolReset with no control gate -- and stays covered by the static pin. Mutation-proved, each restored byte-identical and re-verified green: delete the Panel.Background clear AND its static pin -> 1 of 13108 tests fail, this one, naming [Panel.Background] collapse assignability to name equality -> fails on derivedBySubclass with the regression named simulate a PoolableTypes rename -> InvalidOperationException, not a vacuous pass Full suite 13044/0/64; union checker 17/17. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Select the pool-reset rule by receiver, not by property (#1051) REACTOR_POOL_001's leading clause asserts that the written property "is reset on pool return". Rule selection was per-property (`poolReset ? Rule : ModifierAvailableRule`), so the flip in this PR escalated *every* gate-admitted receiver to POOL_001 - including RelativePanel, which ApplyModifiers writes to but ElementPool never recycles. For that receiver the diagnostic's leading clause and its entire Description are false, and POOL_001 is a Warning, so a consumer building Release with TreatWarningsAsErrors would fail on a hazard they do not have. The advice was never wrong - the write really is dropped by the next render - only the reason and the severity were. So the fix narrows selection rather than suppressing anything: a new optional `poolResetGate` names the receivers the pool actually resets, and anything outside it falls to REACTOR_MOD_002 (Info), which is the hazard those receivers do have. `controlGate` is untouched, so which statements get reported does not move; only the label does. - ModifierInfo gains PoolResetGate; Padding and CornerRadius declare one (each is its existing gate minus RelativePanel). The other four pool-reset properties gate on Control/Border only, both poolable, so they need none - and absence of a gate is a pass, documented on PassesPoolResetGate because it reads identically to PassesControlGate and means the opposite. - Every_Pool_Reset_Gate_Matches_The_Poolable_Intersection pins the gates to ElementPool.PoolableTypes in BOTH directions: too wide is a false Warning and a downstream build break, too narrow silently downgrades a real hazard to Info. It also asserts at least one property actually declares a gate - without that, `PoolResetGate ?? ControlGate` compares the gate to itself and the whole test is true by construction. Mutation ledger (each restored byte-identical, site counts and artifact advance asserted; the Release build's CS0162 caught one mutant before it could run): revert to per-property selection -> 2 of 13109, both RelativePanel tests widen gate (+RelativePanel) -> parity gate tooWide + expectation test narrow gate (-Grid) -> parity gate tooNarrow, names [Grid] delete both poolResetGate args -> parity gate tooWide delete args AND pre-narrow the control gates so the ?? fallback coincidentally agrees -> the vacuity guard, in isolation The last one is the only way to reach that guard: on today's table tooWide fires first, so without it the guard would ship never having been executed. Fires_For_Padding_On_Grid_And_RelativePanel now pairs a pooled receiver with a non-pooled one under two different ids. That pairing is the point - it pins selection to the receiver, which a same-id pair could not distinguish. Closes #1051. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Address code-quality: filter the gate names explicitly github-code-quality flagged the `continue` guard in Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement as an implicit filter. It is right, and the explicit form is better here for a reason beyond style: the guard was carrying the derivation of the RelativePanel exclusion, and a reader looking for "what does this derivation range over?" looks at the loop header, not four lines into the body. Moved the reasoning there with the filter. Verified set-preserving rather than merely assertion-preserving - a rewrite of a filter can keep every assertion passing while quietly changing the set they range over, which is the failure this test exists to prevent elsewhere. Re-ran the load-bearing mutation (delete the Panel.Background clear AND its static pin, so the dynamic invariant is the sole detector): 1 of 13109, this test, still naming [Panel.Background]. Same result as before the refactor, so the derived set is unchanged. Full suite 13045/0/64, restore control clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * fix(analyzers): make REACTOR_POOL_001 receiver-aware via an exact-type pool mirror `PoolResetSetAnalyzer` selected `REACTOR_POOL_001` vs `REACTOR_MOD_002` from the per-property `poolReset` flag alone. `poolReset` says "CleanElement clears this property"; it does not say "this receiver is ever recycled". Those are different questions and they need different kinds of set. PassesControlGate / PassesPoolResetGate -> InheritsFrom (walks the BaseType chain) ElementPool.Rent / Return -> PoolableTypes.Contains(GetType()) (EXACT) A control gate names the base type an `ApplyModifiers` arm dispatches on; poolability is an exact-type set. `Control` admits `CheckBox`, `Panel` admits `RelativePanel`, and the pool holds fifteen exact types. No name-based gate can express "RelativePanel excluded for Background" without misdescribing the `Panel` arm, because `CleanElement`'s `Panel` arm genuinely does clear `Background` for every `Panel`. The only faithful test is an exact-type mirror of the pool's own membership test. Changes: - `ModifierTable.PoolableTypeNameSet` mirrors `ElementPool.PoolableTypes` by exact name (15 entries), exposed as `PoolableTypeNames` / `IsPoolableTypeName`. - `PoolResetSetAnalyzer` resolves the `.Set` lambda parameter's declared type once per body (`receiverIsPooled`) and requires exact membership plus the `Microsoft.UI.Xaml.Controls` namespace. A null or unresolved type yields `false`, which de-escalates -- the safe direction. - Both halves of the rule now resolve poolability the same way. The attached path previously hardcoded `PoolReset: true`, so a `.Set` body mixing an instance write with an attached write on one non-poolable receiver would have reported two different ids for the same control. Both paths are already gated to the lambda parameter, so poolability is one fact per body. Direction is strictly de-escalating (POOL_001 Warning -> MOD_002 Info) and the advice is unchanged: the write is still dropped on the next render, which is exactly what MOD_002 says. It cannot break a downstream Release build, only stop breaking one. Scope: this fixes 6 rows introduced by #985 and 12 pre-existing over-broad rows, plus the attached half. Measured 80 real `.Set` overloads, 0 base-typed, so exact matching loses no real-world signal; `class MyButton : Button` reports MOD_002, which is correct because the pool would not recycle it either. Tests: - `Analyzer_Poolable_Type_Mirror_Matches_ElementPool` asserts set equality in both directions against `ElementPool.PoolableTypes` read reflectively, and the reader throws on member-not-found / wrong-type / empty, so the parity cannot pass vacuously. - Two instance regressions (`IsEnabled` on Button vs CheckBox, `Background` on Grid vs RelativePanel) and one attached regression, each pairing the negative case with its poolable positive control in the same body. - Analyzer stubs now use a poolable `Microsoft.UI.Xaml.Controls.Button` as the `.Set` lambda parameter, mirroring the real DSL. Mutation-checked: widening the mirror, narrowing it, and disabling the receiver check each redden, and the receiver-check mutation reddens exactly 3 of 2756 analyzer tests -- precisely the three regressions. Full suite 13049 / 0 / 64. Release builds of `Reactor.slnx` and `Reactor.AppTests.Host` clean, 0 `REACTOR_*` diagnostics reaching any sample. Refs #1051 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * docs(pool): document PoolableTypes as an exact-type set and name its analyzer mirror `PoolableTypes` is now mirrored by name in `ModifierTable.PoolableTypeNameSet` so `PoolResetSetAnalyzer` can pick REACTOR_POOL_001 vs REACTOR_MOD_002 from the receiver. The parity gate lives in the test project, so someone editing this list learns about the mirror from a red test rather than from the code in front of them. Put the obligation at the site where the drift originates, and name both gates that fire. Also states the property the whole fix turns on: membership is tested with Contains on GetType(), so this is an exact-type set -- CheckBox and RelativePanel pass the Control and Panel gates ApplyModifiers dispatches on and are deliberately absent here. Comment-only. Suite 13049 / 0 / 64, union checker 17/17, Release build 0/0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * docs(analyzers): correct two POOL_001 statements the receiver-aware fix falsified `2ed8879d` made REACTOR_POOL_001 selection receiver-aware — the `.Set` lambda parameter's exact type must be in `ElementPool.PoolableTypes`, mirrored into the netstandard2.0 analyzer as `ModifierTable.PoolableTypeNameSet`. Two prose statements still described the old unconditional behaviour. `ModifierTable.AttachedModifierInfo` remarks claimed every attached write reports POOL_001 and that "there is deliberately no attached equivalent of the Info tier". Both are now false: the same commit changed the attached report path from a hardcoded `PoolReset: true` to the shared `receiverIsPooled`, so an attached write on an unpooled receiver reports REACTOR_MOD_002 — which is exactly what `Attached_Setter_Reports_ModifierAvailable_On_An_Unpooled_Receiver` asserts. Reworded to separate the two facts that were conflated: every *entry* is pool-reset as a property (hence no per-entry flag and no attached analogue of `PoolResetGate`), but the reported *id* also depends on the receiver. `reactor-build-and-check/SKILL.md` is shipped agent-kit guidance, so a stale rule there is acted on. Its POOL_001 row described the write as unconditionally "silently discarded when the control is reused"; it now names the fifteen pooled types and states that `CheckBox`, `RelativePanel` and user subclasses report REACTOR_MOD_002 instead. The row keeps its single-line, four-column shape and the `instance write` literal that `check-skillmd.ps1` matches on. Comment/documentation only — all 20 changed lines in `ModifierTable.cs` are `///` XML doc, 0 code lines, verified by classifying each +/- line rather than reading --stat. Verified: Reactor.Tests 13049/0/64 (unchanged); union checker 17/17 with the terminal RESULT: marker present; Release build 0 warnings / 0 errors. Positive control on the SKILL.md edit — neutralising the `instance write` literal drives the delegated gate to `RESULT: FAIL (16 passed, 1 failed)`, naming both failing checks, so the checker demonstrably ranges over the edited file. The fifteen type names in the SKILL.md row were compared by set membership against `PoolableTypeNameSet` (15 == 15, no missing, no extra, both sides conservation-clean, negative control returns False), and that set is in turn gated against the product by `Analyzer_Poolable_Type_Mirror_Matches_ElementPool`. Refs #985, #1051 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * docs(pool): name the analyzer mirror's real namespace, not its assembly name The doc block added in `9a7e48d2` wrote the mirror as `Reactor.Analyzers.ModifierTable.PoolableTypeNameSet`, which reads as a namespace-qualified type name and is not one: `Reactor.Analyzers` is the `AssemblyName`, while the `RootNamespace` and the declared namespace are `Microsoft.UI.Reactor.Analyzers` (`ModifierTable.cs:3`). A reader or tool resolving that string finds nothing. Repo convention measured before choosing the replacement rather than assumed: `src/**/*.cs` has 20 bare `ModifierTable` references and zero `Reactor.Analyzers.<Type>` ones, and every fully-qualified reference elsewhere (`StringSimilarityParityTests.cs:3`, `UseMemoCellsAnalyzerTests.cs:7-8`) uses the `Microsoft.UI.Reactor.` prefix. The only `Reactor.Analyzers.<Type>` spellings in the tree are in `docs/specs/`, pre-existing and out of scope. So this was a one-off introduced by this PR, not a house style. Now names the type bare and states namespace and assembly separately, since both are useful and conflating them is what produced the error. Comment-only: 3 changed lines, all `///` XML doc, 0 code lines, classified per-line rather than read off --stat. Verified: Reactor.Tests 13049/0/64 (unchanged); union checker 17/17 with the terminal RESULT: marker present; Release build 0 warnings / 0 errors. Refs #985 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * test(pool): record which detector the poolable-receiver mutation isolates `Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement` derives the same obligation that `CleanElementRequiredClears` pins by hand, so it reads as redundant — and the obvious experiment agrees with that reading. Deleting `Control.Padding`'s `ClearValue` reddens both tests, which says nothing about which one is load-bearing. The discriminating mutation deletes the clear *and* its pin row, blinding the static test. Measured on d26dcbd, all four legs rebuilt between runs: control ok=7 notOk=0 clear only ok=5 notOk=2 <- both detectors, confounded clear + pin ok=6 notOk=1 <- this test, alone restored ok=7 notOk=0 byte-identical, git clean Records the result in the test's remarks so the dedup argument — which looks reasonable until someone runs this — meets the measurement instead. Doc comment only: 10 added lines, all `///`, no code change. * docs(analyzers): stop describing MOD_002 as the opposite of what it does Five sites said an unpooled receiver falls to REACTOR_MOD_002 because "the write is dropped by the next render". MOD_002's own descriptor says the reverse — PoolResetSetAnalyzer.cs:53-56: "a fluent modifier exists for this property, but it is not pool-reset — the value is written correctly, it just costs the element its structural skip (Element.SettersEqual) and is never unwound when a later render drops it." The value persists; what is lost is the structural skip. The same file states the correct framing for this exact case at :490 — "only the claim about pool return is dropped" — so all five now say that instead. Sites, all introduced by this branch (none on origin/main, `git log -S`): src/Reactor.Analyzers/ModifierTable.cs 3 (PoolResetGate remarks, PoolableTypeNameSet remarks, the poolReset rationale comment) src/Reactor/Core/ElementPool.cs 1 (PoolableTypes remarks) plugins/.../reactor-build-and-check/ 1 (POOL_001 row) Copilot review flagged four; the repo-wide sweep found the fifth (ModifierTable.cs:82), which is not adjacent to the other two and was outside the reviewed hunks. Residual occurrences of the phrase: 0. Doc comments and one table cell: 13 added lines, none of them code. * test(pool): correct three claims that the reflective guard is the vacuity defence ReadPoolableTypes' remarks and both of its throw messages asserted that without the guard the callers would "assert over nothing and report green". Mutating the selection axis refutes it: deleting the empty check AND emptying PoolableTypes still reddens all three callers, because Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement asserts its own derivation is non-degenerate before asserting anything about it (derivedBySubclass.Count > 0, and the required.Count floor). So the guard is fail-fast diagnosis, not the vacuity defence. It converts a downstream failure that misdescribes a rename as a collapsed gate intersection into one that names the missing member. Worth keeping for the message; not what stops an empty poolable set reading as green. Stating it the other way round was the actual risk. It invites a later reader to simplify away the two non-degeneracy asserts as preamble in front of the "real" one -- deleting the detectors and keeping the decorative check. A comment now marks them as load-bearing at the site, since the guard's remarks are far away. Measured, 5 legs, every leg rebuilt, restore byte-identical, git clean: control 7 pass 0 fail field renamed 4 pass 3 fail guard text present set emptied 4 pass 3 fail guard text present emptied + empty-guard deleted 4 pass 3 fail <- predicted 1 restore 7 pass 0 fail Failure text was captured rather than counted, to confirm the three tests fail on their own assertions (mirror drift, gate mismatch, subclass-match regression) and not on a NullReferenceException -- a count cannot tell those apart. Full suite: 13049 passed, 0 failed, 64 skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * test: derive the pool-reset obligation for ungated modifiers too Every_Poolable_Gated_Receiver_Is_Released_By_CleanElement derives its obligation from the product, but only ranges over ModifierTable rows that carry a ControlGate - 5 of the 18 poolReset:true rows. The other 13, Margin through IsEnabled, had no derived obligation at all: their only protection was a row in CleanElementRequiredClears, and that list is consumed as "every listed entry must be cleared", so deleting a row removes the requirement and its detector in one edit. IsEnabled is one of issue #985's own six, so five of this fix was derived and the sixth rested on an erasable row. Every_Pool_Reset_Modifier_Is_Cleared_By_CleanElement closes it at property granularity, reading ModifierTable directly so it has no list to edit. The split between the two derivations is exact: the 5 gated properties are the same 5 cleared through more than one receiver, so only the gated test can pin each receiver; the 13 ungated ones are each cleared exactly once, which makes a property-level check receiver-strong for them by construction. Together they cover all 18 with no hardcoded names, and neither covers all 18 alone. Measured, 4 legs, every leg rebuilt and the restore byte-identical: LEG0 control 8 pass / 0 fail LEGA delete the IsEnabled clear 6 / 2 pin test and this one, so it cannot say which is load-bearing LEGB delete the clear AND its 7 / 1 this test ALONE pin row RESTORE 8 / 0 LEGB is the discriminating one - it is the silent-loss accident a merge resolution produces, and before this commit it was green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d * Correct ElementPool comment claiming gate widening is unguarded (S13) 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 * Correct three claims about what the CleanElement region scanners guarantee 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 * Refute the contravariant .Set-lambda misclassification by measurement 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 * test: pin control identity before the two remaining fail-open absence 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 * docs: correct the MOD_002 fallback wording and name the attached shape 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 * Resolve the gate closure once instead of ContainsKey plus indexer 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 * Close the LIFO hole in the Issue950 StackPanel pool guard 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 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: z <z@z.z> Copilot-Session: d1564805-7a78-452e-9c6e-ad1c81947d4d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ApplyModifiersandREACTOR_MOD_003together soPaddingreachesGridandRelativePanel, andCornerRadiusreachesGrid,StackPanel, andRelativePanelPanel: WinUI 2.3 declares these properties on those three panels, whilePanel,Canvas,VariableSizedWrapGrid, andItemsStackPaneldo notCornerRadiusunset arms toClearValue(...), restoring style/theme values instead of pinning a local zeroGrid/StackPanelinstances return;RelativePanelremains intentionally non-poolableFollow-up to #950 and #961. Uses #970 as the implementation template.
Measured sweep
Normalized MSBuild diagnostics to repo-relative paths and deduplicated on path + line + column:
apps/minesweeper/Components/StatusPanel.cs,apps/regedit/Components/ValueList.cs,CommandingDemo/App.cs,Reactor.TestApp/Demos/TransitionsDemo.cs,StylingGallery/App.csThe measured reduction is 7, matching the checked-in triage (3 StackPanel CornerRadius, 2 Grid CornerRadius, 2 Grid Padding). The sweep was repeated after rebasing onto current
mainand remained 19 across 9. Removing one still-dirty path from the exemption made the Release build fail onREACTOR_MOD_003, proving the narrowed list still has teeth.A separate tree-wide
REACTOR_MOD_002warning sweep found one existing, unrelatedHorizontalContentAlignmentsetter in wordpuzzle and no remaining.Set(...Padding...)/.Set(...CornerRadius...)panel sites. Regedit was the only production panel workaround enabled by this gate widening.Verification
mainafter Lint gallery SampleCard snippets against the live code beside them #974/Fix DataGrid row-edit keyboard falling into the single-cell commit path #977; resolved gallery source first, regenerated the search index secondWIN2D0001AnyCPU warnings)Index_IsUpToDatecomparison) and ApiIndexGeneratorTests 16/16; the wider Tooling selector also passed the gallery snippet-agreement lintRenderTargetBitmapproof confirms an 80×80 redGridpaints an opaque center (alpha > 240) while the(1,1)square-corner sample remains clipped (alpha < 15) withCornerRadius(30)ClearValuewith a zero write, and removal of pool cleanupThe first post-rebase Selftests attempt hit the known 300-second host process-budget timeout while
OptionalEchoStrandRegressionwas in flight. That fixture completed its 25 checks without anot ok, all three new issue #950 fixtures passed, and the unchanged-head rerun passed the full Selftests leg. The timed-out fixture exercises unrelated controlled-value echo paths, not these panel modifier gates. This PR does add three fixtures to the already budget-constrained suite, marginally increasing its duration; #988 tracks the underlying process-budget problem.Intentional boundary
BorderBrushandBorderThicknessremain gated toControl/Border. Consequently, alias calls such asStylingGallery's.WithBorder(...)remain outside this change and remain invisible toREACTOR_MOD_003, whose syntactic gate keys on modifier names inModifierTable.Properties. The remaining count is therefore still a floor, not a complete inventory.