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

Open
#1,017 8 comments 0 reactions 1 assignee Claimed by @azchohfi View on GitHub
enhancement needs-triage
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.