microsoft / microsoft/microsoft-ui-reactor
[Feature] Add the reverse pool-reset invariant: a `poolReset: true` modifier must be cleared on every receiver its gate admits
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
### Problem
`PoolResetSetConsistencyTests` already enforces the pool ⇄ analyzer relationship in **both** directions — but only at **property** granularity:
> - cleared in `CleanElement` ⇒ marked `poolReset: true` — `Every_Reset_Property_With_Matching_Modifier_Is_Tracked` (`:117`)
> - marked `poolReset: true` ⇒ cleared **somewhere** in the FE-common block — `Every_TrappedProperty_Is_Reset_In_CleanElement` (`:86`), `Every_TrappedAttachedProperty_Is_Reset_In_CleanElement` (`:185`)
> [!IMPORTANT]
> **The gap is granularity, not direction.** Do not implement this as "add the missing reverse direction" — that test already exists, a new one will pass immediately, and the hole stays open.
At *(property × gated receiver)* granularity the invariant **is** one-directional:
```
exists : "Padding is cleared SOMEWHERE in the FE-common block" -> passes, via the Control|Border|Panel chain
missing : "Padding is cleared for EVERY receiver its gate admits" -> RelativePanel is gated in, never cleared
```
This is also why the file's other guards cannot catch it: `Every_ClearValue_In_CleanElement_Is_Recognized_By_The_Reset_Scan` and `Attached_Reset_Scan_Sees_Every_Owner_The_Table_Names` protect the *scan*, and a scan that reads the block perfectly still cannot notice a receiver that was never written to.
That asymmetry makes a whole defect class structurally invisible: **widen a modifier's gate (or its `ApplyModifiers` write) to a new receiver type without adding the matching `ClearValue`.** Nothing fails. The analyzer starts telling users the value "is reset on pool return" for a receiver the pool never resets, and the day that receiver is added to `PoolableTypes` it silently leaks its previous renter's local value — which outranks every `Style` setter in WinUI's DP precedence order.
This is not hypothetical. It has now appeared three times:
- **#952** — reset present but the wrong *shape* (assigning a default instead of `ClearValue`).
- **#985** — six modifiers written by `ApplyModifiers` with no reset at all on `Control`/`Border`/`Panel`/`StackPanel`.
- **#1003 + #985 merged** (found by trial-merging the two branches, [detail on the PR](https://github.com/microsoft/microsoft-ui-reactor/pull/1003#issuecomment-5148009922)) — `RelativePanel` gains write **and** unset arms in `Reconciler.cs` and is added to the `Padding`/`CornerRadius` gates, but `ElementPool`'s clear covers only `Grid` and `StackPanel`. The merged `ElementPool.cs` has **zero** `RelativePanel` matches. Latent today only because `RelativePanel` isn't in `PoolableTypes`.
The #985 case additionally shows the cost of the blind spot. During PR #984's development a `Control`/`StackPanel` clear block was removed in `14c8aaf4` — **this was not a revert of #984**, which merged intact. It was a scope narrowing, and that commit's own message says the work was *"filed as a follow-up instead"*; #985 is that follow-up. The stated reason for removing it was that the block *"forced six modifiers from `REACTOR_MOD_002` to `REACTOR_POOL_001` and invalidated 27 analyzer expectations"* — i.e. the existing invariants fired loudly on the **marking**, which is the expensive half, while the receiver-coverage gap that marking was supposed to describe stayed unmeasured. A receiver-granular invariant would have named that gap directly.
### Proposed solution
Add a test alongside the existing ones in `tests/Reactor.Tests/AnalyzerTests/PoolResetSetConsistencyTests.cs`:
```
Every_PoolReset_Modifier_Is_Cleared_On_Every_Gated_Receiver
```
For each `ModifierTable` entry with `poolReset: true`, expand its `controlGate` type list to the concrete receiver types (`Control`, `Border`, `Panel`, `Grid`, `StackPanel`, `RelativePanel`, `TextBlock`, …) and assert the `CleanElement` FE-common region contains a `ClearValue` reaching each one — directly, or via a base type that provably covers it (a `Control` clear covers every `Control` subclass; a `Panel` clear covers `Grid`/`StackPanel`/`RelativePanel`).
Two details worth getting right, because they are what makes it non-vacuous:
1. **Subsumption must be explicit, not assumed.** `if (fe is Control c)` genuinely covers all `Control` subclasses, but `if (fe is StackPanel s)` does not cover `Grid`. The test needs a small "does clear on type X satisfy gate entry Y" relation rather than a name match, or it will either miss real gaps or demand redundant clears.
2. **It must fail if the clear moves out of the scanned region.** `ReadCleanElementCommonBlock` already delimits at the first `switch ()`; reusing it means the #984 placement mistake reddens this test too, which is the outcome we want.
Failure message should name the modifier, the gate entry that has no clear, and the file/line of the gate — so the fix is mechanical.
### Alternatives considered
- **Analyzer-side check instead of a test.** Wrong layer: `ModifierTable` is the analyzer's own data, and it can't see `ElementPool.cs` at analysis time.
- **Extend `ModifierUnsetClearValueTests`' pin list instead.** That file pins an explicit receiver/property list (14 pins today, added in #985). Pins catch *deletion* of a known clear; they don't catch *addition* of a new gated receiver, which is precisely this bug. Complementary, not a substitute.
- **Rely on selftest fixtures.** They prove a specific pooled control releases a specific property, which is valuable, but coverage is per-fixture and per-type — the gap only shows up if someone thinks to write the fixture for the new receiver, which is the same omission being guarded against.
- **Do nothing, treat it as review discipline.** Three occurrences say otherwise, and the merged-tree case was invisible to both PRs' CI: disjoint file sets, clean auto-merge, both green independently.
### Additional context
- Source scanning helpers already exist and are line-anchored: `ReadCleanElementCommonBlock`, `ReadResetProperties`, `ReadResetAttachedProperties`, `InstancePropertyOwners` in `PoolResetSetConsistencyTests.cs`. Note the trap documented there — an unanchored regex let a *comment* containing `switch (fe)` truncate the scanned block and produce 32 misleading failures; both scanners are now `RegexOptions.Multiline`-anchored.
- Related: #952 (reset shape), #985 (missing resets), #1013 (`HorizontalContentAlignment`/`VerticalContentAlignment` still unreset — deliberately deferred, and would be reported by this new invariant once they're marked).
- The gate type-group arrays live at the top of `ModifierTable.cs` (`ControlBorderGridStackRelativeText`, `ControlBorder`, `PanelControlBorder`, …), which gives the test a single source of truth to enumerate from.
### Confirmation
- [x] I have searched existing issues and specs for prior discussion of this idea.
Contributor guide
Assessment
This issue has not been assessed yet.