microsoft / microsoft/microsoft-ui-reactor

[Bug] Attached-property exclusion list accepts instance DPs: the diagnostic recommends it, and no test reads the reason

Open
#1,048 1 comment 0 reactions 0 assignees View on GitHub
bug needs-triage
Dominant language
C#
Stars
646
Forks
54
Avg merge
1d 3h
Merged PRs (30d)
84

Description

### What happened?

`PoolResetSetConsistencyTests.Every_Reset_Attached_Property_Is_Classified` fires whenever a property cleared in `ElementPool.CleanElement` has an owner that is not in the test's `InstancePropertyOwners` set. Its failure message offers **two** remedies:

> These attached properties are cleared in ElementPool.CleanElement but are in neither `ModifierTable.AttachedProperties` nor `DeliberatelyExcludedAttached`: [Grid.CornerRadius, Grid.Padding]. **Either map them** (so REACTOR_POOL_001 fires on `.Set(fe => Owner.SetPROP(fe, ...))`), **or exclude them with a documented reason.**

There is a **third** remedy, and for this failure it was the correct one: add the owner to `InstancePropertyOwners`, because the property is not attached at all — the attached scan is merely the *fallback* for owners that list doesn't name. The message never mentions the list that decides whether you land in the fallback.

Taking the recommended "exclude" branch for an instance DP produces a self-contradicting entry that **the entire suite accepts**. `ModifierTableIntegrityTests.Every_Attached_Exclusion_Carries_A_Reason` is the only test that reads exclusion *values*, and it only asserts the reason is non-blank:

```csharp
var blank = ModifierTable.DeliberatelyExcludedAttached
.Where(kvp => string.IsNullOrWhiteSpace(kvp.Value))
```

**This is not hypothetical — it happened, and the reason text stated the bug in plain English while every test stayed green:**

```csharp
["Grid.Padding"] = "Instance dependency property on Grid, not an attached property with a static setter.",
["Grid.CornerRadius"] = "Instance dependency property on Grid, not an attached property with a static setter.",
```

An exclusion whose documented reason is *"this is an instance dependency property, not an attached property"* is prima facie evidence that the exclusion is the wrong mechanism — `DeliberatelyExcludedAttached` is a suppression list for genuinely-attached properties that can't be written through `Owner.SetPROP(x, v)`, not a classification list. Both entries were removed in PR #1015 (`ddb8b4bd`) in favour of adding `Grid` to `InstancePropertyOwners`; this issue is about the guidance and the missing gate that led there, both of which are still live on `main`.

The consequence of leaving it: a genuinely attached `Grid.*` reset added later lands in the same bucket as these two and reads as already-triaged, so `REACTOR_POOL_001` silently stops firing for it.

### Steps to reproduce

1. Check out `main` (or PR #1015's head, where the classification is already correct).
2. Delete the `"Grid",` entry from `InstancePropertyOwners` in `tests/Reactor.Tests/AnalyzerTests/PoolResetSetConsistencyTests.cs` — this simulates the state any new panel receiver starts in.
3. Run:

```
dotnet test tests/Reactor.Tests --filter "FullyQualifiedName~PoolResetSetConsistencyTests" -c Release -p:Platform=x64
```

→ `Failed: 1, Passed: 53, Total: 54`, naming `[Grid.CornerRadius, Grid.Padding]`.
4. Follow the message's second recommendation — add both keys to `DeliberatelyExcludedAttached` with an honest reason such as *"Instance dependency property on Grid, not an attached property with a static setter."*
5. Re-run the full suite → **green**. The misclassification is now permanent, documented, and invisible.

Suggested fix, both halves:

- **Message.** Name the third remedy: *"…or, if this is an ordinary instance dependency property rather than an attached one, add its owner to `InstancePropertyOwners`."* Step 4 above is only attractive because the message presents exclusion as the general escape hatch.
- **Gate.** Add a test asserting no exclusion reason declares its key an instance property (e.g. reject reasons matching `instance (dependency )?propert`). That is a *content* oracle rather than a *presence* oracle, and it is what distinguishes the three legitimate `AutomationProperties` rows — whose reasons cite the absence of a static setter — from a misfiled instance DP. A count- or non-blank-based check passes on a wrong-but-stable set by construction.

The general shape, which is what makes this worth fixing rather than just correcting the two rows: **consistency derived from a classification list cannot prove that list is complete.** Every existing test here checks that the tables agree with each other, so a property missing from `InstancePropertyOwners` and suppressed in `DeliberatelyExcludedAttached` is perfectly self-consistent. Only an oracle over reason *content* is independent of the classification it is checking.

### Reactor version / commit

`1ed2644e7d98e310f0efb6db4c33c5f0ec2f2568` (`main`). The diagnostic text was introduced in `96cadfe3` ("feat(analyzers): extend REACTOR_POOL_001 to attached-property writes in .Set", 2026-07-29); the `Grid.*` exclusions arrived later and were removed in PR #1015.

### Platform

x64

### .NET SDK version

10.0.302

### Windows version

Windows 11 build 26310

### Windows App SDK version

N/A — headless `Reactor.Tests` tier only; no Windows App SDK surface involved.

### Logs / stack trace

```text
Microsoft.UI.Reactor.Tests.AnalyzerTests.PoolResetSetConsistencyTests.Every_Reset_Attached_Property_Is_Classified [FAIL]
Error Message:
These attached properties are cleared in ElementPool.CleanElement but are in neither
ModifierTable.AttachedProperties nor DeliberatelyExcludedAttached: [Grid.CornerRadius, Grid.Padding].
Either map them (so REACTOR_POOL_001 fires on '.Set(fe => Owner.SetPROP(fe, ...))'),
or exclude them with a documented reason.
at ...PoolResetSetConsistencyTests.Every_Reset_Attached_Property_Is_Classified()
in tests\Reactor.Tests\AnalyzerTests\PoolResetSetConsistencyTests.cs:line 219

Failed! - Failed: 1, Passed: 53, Skipped: 0, Total: 54
```

### Confirmation

- [x] I have searched existing issues and this isn't a duplicate.
- [x] This bug reproduces against the current `main` branch.

Contributor guide

Open the contributing guide

Research direction

Start in tests/Reactor.Tests/AnalyzerTests/PoolResetSetConsistencyTests.cs and run the focused PoolResetSetConsistencyTests command from the issue. Read the attached-property diagnostic and ModifierTableIntegrityTests.Every_Attached_Exclusion_Carries_A_Reason. Done means the guidance names InstancePropertyOwners as a third remedy and the suite rejects exclusion reasons that classify a key as an instance dependency property.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.