Skip to content

feat(analyzers): REACTOR_DSL_003 — constant / param-ignoring keySelector#806

Merged
azchohfi merged 4 commits into
azchohfi-spec-060-analyzer-suitefrom
azchohfi-dsl-003-constant-keyselector
Jul 6, 2026
Merged

feat(analyzers): REACTOR_DSL_003 — constant / param-ignoring keySelector#806
azchohfi merged 4 commits into
azchohfi-spec-060-analyzer-suitefrom
azchohfi-dsl-003-constant-keyselector

Conversation

@azchohfi

@azchohfi azchohfi commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

REACTOR_DSL_003 (Reactor.Dsl, Warning) — spec 060 Batch 2

A typed Reactor collection factory (ListView<T>, GridView<T>, FlipView<T>, TreeView<T>, LazyVStack<T>, LazyHStack<T>, ItemsRepeater<T>, ItemsView<T>) called with a keySelector that returns a constant / literal / null, or that never reads its item parameter, maps every item to the same (or a null) key.

ListView(items, _ => "row", (item, i) => Row(item));   // ⚠ every row collides
ListView(items, x => null,  (item, i) => Row(item));   // ⚠ null keys bail the diff
ListView(items, i => i.Id,  (item, i) => Row(item));   // ✓ stable, unique per-item key

Why it matters (premise, verified against source)

  • keySelector is the 2nd positional parameter (Func<T,string>) of the typed collection factories — src/Reactor/Elements/Dsl.cs (ListView<T> at :1464, GridView<T> :1495, FlipView<T> :1525, TreeView<T> :1017, LazyVStack<T> :1560, LazyHStack<T> :1590, ItemsRepeater<T> :1623, ItemsView<T> :1810).
  • Duplicate and null keys force a correctness-preserving bailout in KeyedListDiff (the DuplicateKey / NullKey ReportBailout paths, src/Reactor/Core/Internal/KeyedListDiff.cs:194-246), which re-realizes the entire list on every change — losing focus, selection, and animation state.
  • docs/guide/collections.md:74-92: keys must be stable, unique, non-null.
  • Distinct from REACTOR_DSL_001 (.WithKey on Select-projected layout children).

Detection (low false-positive)

Fires only when the keySelector is a single-parameter lambda whose body provably does not depend on the item — the parameter is never referenced and the body contains no invocation / object-creation / await (any of which could vary per item). It then semantically confirms the argument binds to a keySelector parameter of type Func<T,string> on Microsoft.UI.Reactor.Factories. This rejects:

  • selectors that read the item (directly, in interpolation, or via a helper),
  • opaque helper calls like _ => Guid.NewGuid().ToString() (could be unique),
  • the untyped selection overloads' onSelectedIndexChanged (e.g. ListView(0, _ => {}, …)),
  • the IReactorKeyed two-parameter viewBuilder overload,
  • method-group selectors, and same-named methods on unrelated types.

No mechanical code-fix (~): there is no obviously-correct key to substitute — the author must choose a stable per-item property. The diagnostic says so.

Changes

  • src/Reactor.Analyzers/ConstantKeySelectorAnalyzer.cs (new)
  • src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md — descriptor + release row added together (RS2000/RS2002 coupled; build is 0-warning)
  • plugins/reactor/skills/reactor-build-and-check/SKILL.md — app-author cheat-table row
  • docs/_pipeline/templates/analyzer-architecture.md.dt — rules-table row (template, not generated docs)
  • tests/Reactor.Tests/AnalyzerTests/ConstantKeySelectorAnalyzerTests.cs (new, 15 tests)

Verification

  • dotnet test tests/Reactor.Tests -p:Platform=x64 --filter FullyQualifiedName~AnalyzerTests230 passed (15 new).
  • Samples sweep: every real keySelector reads its item; the IReactorKeyed overloads are correctly not keySelector calls → no false positives. Firing nowhere in samples is expected for a hygiene rule (spec-060 §2.4).

…tor rule

A typed Reactor collection factory (ListView<T>, GridView<T>, FlipView<T>,
TreeView<T>, LazyVStack<T>, LazyHStack<T>, ItemsRepeater<T>, ItemsView<T>)
called with a keySelector that returns a constant/literal/null or never reads
its item parameter maps every item to the same (or null) key. Duplicate/null
keys force a keyed-list diff bailout (KeyedListDiff) that re-realizes the whole
list on every change, losing focus, selection, and animation state.

Low false-positive posture: fires only on a single-parameter lambda whose body
provably ignores the item (parameter never referenced AND no invocation /
object-creation / await), then semantically confirms the argument binds to a
keySelector parameter of type Func<T,string> on Microsoft.UI.Reactor.Factories
- so the untyped selection overloads' onSelectedIndexChanged, the IReactorKeyed
viewBuilder overload, method-group selectors, and item-reading selectors never
trip it. No mechanical code-fix (there is no obviously-correct key to substitute).

Adds the descriptor + AnalyzerReleases.Unshipped row (RS2000/RS2002 coupled),
the app-author cheat-table row (SKILL.md) and the analyzer-architecture template
row, and 15 tests (positive: const/null/param-ignoring/named-arg/block-body;
negative + near-miss: reads item, helper, IReactorKeyed overload, selection
discard, method group, unrelated type).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new Roslyn analyzer rule (REACTOR_DSL_003) to the Reactor analyzer pack to detect typed collection factory calls where the keySelector is provably constant / null / or otherwise ignores the item parameter, which can cause duplicate/null keys and force keyed diff bailout (full re-realization of the list).

Changes:

  • Introduces ConstantKeySelectorAnalyzer (REACTOR_DSL_003) with a cheap syntactic pre-gate and semantic confirmation against Microsoft.UI.Reactor.Factories keySelector: Func<T,string>.
  • Adds a dedicated analyzer test suite (15 tests) using a minimal stubbed Factories surface to validate both positive hits and key false-positive exclusions.
  • Updates analyzer release notes + contributor-facing docs (skill cheat table + analyzer architecture template) to include the new diagnostic.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/Reactor.Tests/AnalyzerTests/ConstantKeySelectorAnalyzerTests.cs Adds coverage for the new diagnostic, including overload-shape and “unrelated same-name method” negatives.
src/Reactor.Analyzers/ConstantKeySelectorAnalyzer.cs Implements REACTOR_DSL_003 (syntactic pre-gate + semantic binding confirmation) and reports a warning on the keySelector lambda.
src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md Registers the new diagnostic in the unshipped analyzer release table.
plugins/reactor/skills/reactor-build-and-check/SKILL.md Documents REACTOR_DSL_003 in the “common build errors” cheat table with guidance.
docs/_pipeline/templates/analyzer-architecture.md.dt Adds the new rule to the analyzer architecture rules table (template source).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pr-review (correctness): a keySelector that ignores its item but mutates state
per call (e.g. an interpolated post-increment) produces unique keys, not the
duplicate-key bug the rule reports. Add increment / decrement / assignment to
BodyProvablyIgnoresItem's disqualifier set so only bodies that are provably
constant across items fire.

pr-review (test-coverage): add a positive test for the IsMemberName arm
(x => Config.x, member name equals the parameter name) and a negative test for
the per-call-mutation guard.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/Reactor.Analyzers/ConstantKeySelectorAnalyzer.cs
Copilot review: the syntactic pre-gate did the lambda/body scan and a semantic
GetSymbolInfo on any invocation with a param-ignoring 2nd-arg lambda (e.g.
Enumerable.Select(source, _ => const)) before rejecting it. Add a hash-set name
filter over the typed collection factory names as the first gate, matching the
PoolResetSet/MissingWithKey reference analyzers. Precision is unchanged - the
semantic containing-type/parameter confirmation still runs. Adds a name-gate
negative test.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Copilot review: positive-path tests only exercised ListView and GridView. Add a
positive test for TreeView<T>, which has a different signature (childrenSelector
at index 2, a single-parameter viewBuilder at index 3) - guarding both the
name-gate set entry and the positional keySelector-argument selection.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@azchohfi azchohfi merged commit 31fe0ae into azchohfi-spec-060-analyzer-suite Jul 6, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants