microsoft / microsoft/microsoft-ui-reactor
[Feature] Architecture test: no static ConditionalWeakTable keyed by a poolable control type may live inside a generic type
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
### Problem
A `static` field declared inside a generic type gets **one copy per closed generic instantiation**. `ElementPool` recycles by **CLR type** (`ElementPool.cs:45` `PoolableTypes`, and `TryRent(Type)`/`Return` key on `element.GetType()` at `:105`/`:123`). When per-control bookkeeping is stored in a `static ConditionalWeakTable` declared on a generic type, those two identity keys **disagree by exactly the generic parameter** — and the bookkeeping fragments silently.
Found while fixing #976. `DataGridComponent` held its grid-root KeyDown wiring flag in a `static ConditionalWeakTable` declared on the generic component. `Microsoft.UI.Xaml.Controls.Grid` is on the poolable list (`ElementPool.cs:50`), so the *same pooled grid root* can be returned and remounted under a **different** `DataGridComponent`. The new instantiation's table has no record of the handler the previous one attached, so handlers accumulate and a stale closure keeps driving the previous grid's state — which is the exact defect the wiring guard exists to prevent. Fixed on #976 by moving the table to a non-generic holder (`DataGridKeyDownWiring`).
The failure is invisible to every existing tier: it compiles, it produces no conflict, and it only manifests when two *different* closed generics share a recycled control — which no single-`T` fixture exercises.
### Proposed solution
An architecture test in the existing `tests/Reactor.Tests/Architecture/` style (`CoreControlFamilyBoundaryTests`, `SetEventSubscriptionConsistencyTests`), asserting:
> A `static` `ConditionalWeakTable` whose `TKey` is a type on `ElementPool.PoolableTypes` — or a base of one, e.g. `FrameworkElement`/`UIElement` — must not be declared inside a generic type, or inside a type nested in one.
**The narrowing is the load-bearing part.** A blanket "no static `ConditionalWeakTable` in a generic type" rule would be wrong, and a blanket "no static state in a generic type" rule would be very wrong — per-instantiation statics are usually *the mechanism*, not the bug:
- `Reg<…>.Done`, `RegBase<…>.Done`, `RegDecorator<…>.Done`, `RegBaseDecorator<…>.Done`, `ItemsRepeaterDescriptor…Done`, `ItemsViewDescriptor…Done` — `internal static readonly byte Done = Init();` is the run-once-per-closed-generic module-init idiom. It *needs* one copy per `T`.
- `ElementFactory.cs:194` — `private static readonly bool s_valueTypeItem = typeof(T).IsValueType;` is per-instantiation by definition, and the comment says so: *"JIT-folded per instantiation, so the guard is free."*
- `PropEntry.cs:277` — `static readonly EventHandler StaticTrampoline`, one trampoline per `TArgs`.
The defect is not "static state in a generic type." It is a **disagreement between two identity keys**: the static's implicit partition (closed generic) being *finer* than the partition of the subject it describes (CLR type). Keying the rule on `ElementPool`'s poolable list expresses exactly that and cannot fire on any of the legitimate idioms above.
### Alternatives considered
**Per-control guard instead of an architecture test.** #976 already carries one — a unit test pinning the wiring table to a non-generic holder. It covers DataGrid only, and the next control to make the same mistake gets no warning. One structural assertion covering every holder is the same trade the capture-site guard on #1010 made, and it's the reason the architecture-test directory exists.
**Do nothing — the population is currently zero.** Defensible, but the finding then lives only in a PR description and a doc comment. The value here is purely regression-prevention, and it is cheap: the rule has exactly one historical violation and, once #976 lands, zero current ones.
**Roslyn source-parsing vs. IL scanning.** Source-parsing (`RepoRootFinder.FindRepoRoot()` + `CSharpSyntaxTree.ParseText`) is the right tool. It is immune to prose: `ConditionalWeakTable` appears **50 times across 32 files** under `src/`, and many of those are doc comments and rationale, not declarations. Comments are trivia to a parse tree. Note also the repo's two existing IL scanners (`Architecture/CoreControlFamilyBoundaryTests.cs`, `Docking/DockHostFocusFallbackTests.cs`) each carry their own ~110-line opcode decoder — a third copy would be the expensive option.
### Additional context
Related: #976 (origin of the finding), #987.
**What was measured**, so the scope of the claim is clear:
- All `ConditionalWeakTable` references under `src/` — 50 matches in 32 files; of those, 30 are actual field declarations, 27 of them `static`.
- Cross-referenced against files declaring any generic type, using two probes with complementary blind spots (a strict modifier-anchored declaration regex, and a deliberately over-broad `class|struct|record|interface … <` sweep). **Both agree.** The strict probe has a built-in positive control: it *did* find the two real generic declarations, so the zero elsewhere is not a dead-probe zero.
- Result: exactly **two** files contain both a static `ConditionalWeakTable` and a generic type declaration.
- `Reconciler.cs` — the generic type is `TypeRegistration` (`:1364-1402`), which declares **no static members at all**; the table at `:4958` sits at non-generic `Reconciler` scope.
- `DataGridComponent.cs` — the #976 case, already fixed.
**So there is no second violation.** This proposal is regression-prevention, not a bug report.
**What was *not* measured:** I checked static fields inside generic types only in the control-touching layer (`Core/V1Protocol/`, `Component.cs`, `ElementFactory.cs`, `PoolPolicy.cs`, `Reconciler.KeyedItemsBinding.cs`). 72 files under `src/` declare generic types; I have not audited all of them for non-`ConditionalWeakTable` static state with the same key-disagreement shape. The one mutable non-`readonly` case I did find — `ReactorBinding.s_referenceSlotCount` (`ReactorBindingT.cs:31`) — is `[ThreadStatic]` and reset to `0` in the constructor (`:42`) before any read, so its per-instantiation split has no observable consequence.
If the rule is worth having, widening `TKey` beyond `ConditionalWeakTable` to any static keyed store over a poolable control type (e.g. a `static Dictionary`) would be the natural second step.
---
## Update — the Roslyn requirement is now demonstrated, not preferred
The "Roslyn source-parsing vs. IL scanning" note above called parse-trees *the right tool*. Three
successive regex approximations have now been tried and **every one is fail-open** — each reports
"no violation" or "cannot tell" on the historical violation this test exists to catch. Recording
them so an implementer doesn't rediscover them:
**Positive control used throughout:** the *same file* at two revisions —
`a490d603` (`s_keyDownWiring` on generic `DataGridComponent`, the violation) and `ee6fcd2f`
(moved to non-generic `DataGridKeyDownWiring`, the fix). A sound probe must return **opposite**
verdicts. Anything that doesn't cannot be the test.
| # | probe | failure | verdict on the known violation |
|---|---|---|---|
| 1 | `(class\|struct\|record\|interface)\s+\w+\s*<` intersected per-file | matches **English prose** — `"Record that `. A capture of the form
`<[^>(]*>` cannot match it, because the attribute contains both `(` and `>`. So the probe found
the correct enclosing type on the pre-fix revision **and still concluded "no type parameters."**
Both are free in a syntax tree. `AncestorsAndSelf().OfType().Any(t => t.TypeParameterList is not null)`
expresses "lexically inside a generic type" exactly, is immune to prose because comments are
trivia, and is immune to attribute syntax because the parser has already handled it.
**Consequence for the earlier measurement in this issue.** The "exactly two files contain both a
static `ConditionalWeakTable` and a generic type declaration" figure was produced by a **probe-2
class** instrument. Its *answer* survives — the two hits were triaged by hand and neither is a
violation — but it was hand-triage that established that, not the probe. An implementer must not
treat that figure as evidence a file-scoped check is adequate.
## Update — the rule should be keyed on the key domain, not the container
The final paragraph above suggested widening beyond `ConditionalWeakTable` as "the natural second
step." Sharper phrasing, from review discussion on #987/#1010: **the container is not what makes it
wrong — the key domain is.** A `static ConcurrentDictionary` or `HashSet` inside a
generic type has the identical defect, and a `ConditionalWeakTable`-phrased rule cannot see it.
Phrase the assertion as:
> A `static` field whose type is a keyed store over a **CLR-type-or-poolable-control key domain**
> must not be declared lexically inside a type that has type parameters.
**That wider net has now been measured, and the population is still zero.** Broadening to
`ConditionalWeakTable<` + `Dictionary` gives
32 files with such a static and 79 declaring a generic type, intersecting in **4** — of which
`Element.cs` (a static *method* with Type-keyed parameters), `V1Protocol/ControlRegistry.cs` and
`Hosting/BackdropApplier.cs` (both matched on **prose** in doc comments) are probe artifacts, not
instances. This remains regression-prevention with zero current violations.
Contributor guide
Research direction
Start in tests/Reactor.Tests/Architecture/, following CoreControlFamilyBoundaryTests.cs and SetEventSubscriptionConsistencyTests.cs, and inspect ElementPool.cs for the poolable key domain. Use RepoRootFinder.FindRepoRoot() with CSharpSyntaxTree.ParseText, then walk syntax ancestors to identify static keyed stores inside generic types. Done means the test catches the historical DataGridComponent violation while excluding legitimate per-generic static state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- testing-qa
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100