microsoft / microsoft/microsoft-ui-reactor
Enforce the FlyoutBase.Placement choke point with an internal analyzer instead of a syntactic test scan
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
Follow-up from #953, out of scope there. Not urgent — the current guard works and is mutation-checked. Filing it so the idea isn't lost.
## Background
`FlyoutBase.Placement` must never be written directly: `FlyoutPlacementMode.Auto` (enum value 13) falls outside WinUI's `ValidateAndSetParameters` 0..12 range, so it reaches the validator as `E_INVALIDARG` and **terminates the process** when the flyout is shown. Reactor's element records default to `Auto`, so every write has to route through `FlyoutPlacement.Apply`, which clears the DP instead.
#953 retired the second helper that also wrote this DP (`Reconciler.ApplyFlyoutPlacement`) and tightened `tests/Reactor.Tests/FlyoutPlacementGuardTests.cs` so a second one cannot reappear unnoticed — the two had already diverged silently once.
## The problem with the current mechanism
The guard is a Roslyn **syntactic** scan (`CSharpSyntaxTree.ParseText` over every `.cs` under `src/Reactor`), deliberately avoiding a semantic model so it needs no compilation. That buys speed and simplicity, and costs precision. Known limits, all documented in the file:
- **No symbol resolution.** `IsFlyoutObjectInitializer` decides by type *name* — anything whose leaf name ends in `Flyout`. A DP alias (`var dp = FlyoutBase.PlacementProperty; x.SetValue(dp, ...)`), a `using FB = ...FlyoutBase;`, or a same-named type in an unrelated namespace all read wrong.
- **Target-typed `new()` needs hand-rolled inference.** #953 had to add `DeclaredTypeName` / `EnclosingReturnTypeName` to walk up to the declaration, return type, or cast supplying the type — reimplementing, badly, what the semantic model already knows. It fails closed when it can't tell, which is correct but means the shape list is load-bearing.
- **Method granularity.** `Every_Flyout_Site_Routes_Through_The_Choke_Point` asserts each of six methods calls `FlyoutPlacement.Apply`; it cannot tell *which branch* of a multi-branch method the call sits in. Deleting one of `UpdateCommandBarFlyout`'s two calls leaves the method "routed". Per-branch coverage is pushed to the selftests (this is exactly how the fresh-flyout arm ended up with no placement assertion at all — caught and fixed in #953, but only by review).
- **`PlacementApplyingMethods` is hand-maintained.** A new flyout site added without an entry is unguarded, and nothing says so at the time.
- **Failure arrives late.** A test failure after the fact, not a squiggle while typing.
## Proposal
Replace the scan with a `DiagnosticAnalyzer` in `src/Reactor.Analyzers.Internal` — the project already exists for exactly this class of repo-internal invariant (`XmlDocSummaryAnalyzer` / `REACTOR_DOC_001` is the precedent).
Roughly:
- Register a syntax-node action for assignments and invocations; use the semantic model to identify writes to `Microsoft.UI.Xaml.Controls.Primitives.FlyoutBase.PlacementProperty` (or the `Placement` property on a symbol convertible to `FlyoutBase`) — by symbol, so aliases, target-typed `new()`, and `var` all resolve for free.
- Report unless the containing file is the choke point.
- Severity `Error` in `src/Reactor` (the core library already promotes analyzer warnings to errors), so a bypass fails the build at the offending line rather than in a distant test.
Open questions worth settling before starting:
1. **Does it subsume the positive half?** The "did a site *stop* applying placement" invariant is not a diagnostic — an analyzer sees writes, not absences. `PlacementApplyingMethods` (or the selftests) likely has to survive in some form. Worth deciding whether that's a second analyzer keyed on `CommandBarFlyoutElement`/`FlyoutElement` mount+update handlers, or just stays a test.
2. **Scoping.** The analyzer would run over all of `src/Reactor`; make sure it doesn't fire on element records' `init` properties (`ContentFlyoutElement.Placement` etc.), which are immutable descriptions, not live controls.
3. **Generality.** Same defect class shows up elsewhere — #952 is `ClearValue`-vs-local-value in `Reactor.ApplyModifiers`. If a general "clear, don't write a sentinel" rule is feasible, this is a better vehicle than a per-DP guard.
## Cost/benefit
The current guard is 25 tests, mutation-checked at both tiers, and green. This is a mechanism upgrade, not a bug fix — worth doing when someone is already in `Reactor.Analyzers.Internal`, not on its own.
Credit: raised by the multi-model cross-check during #953.
Contributor guide
Research direction
Start in src/Reactor.Analyzers.Internal, using XmlDocSummaryAnalyzer and REACTOR_DOC_001 as the analyzer precedent, and review tests/Reactor.Tests/FlyoutPlacementGuardTests.cs for the current invariant. Resolve the open questions around positive coverage, scoping, and generality, then ensure direct writes to FlyoutBase.Placement fail at the offending source line while required placement-routing coverage remains enforced.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, tooling
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100