feat(analyzers): REACTOR_MEMO_001 — keyed-Memo wrapper modifier opts out of recycle cache#807
Open
azchohfi wants to merge 3 commits into
Open
Conversation
Modifiers on a keyed Memo(key, factory) wrapper set Modifiers/Key/Extensions, which makes ElementFactory.BuildOrCache skip the cross-recycle LRU (it only memoizes a bare KeyedMemoElement). The row then rebuilds + re-diffs on every recycle, silently losing the perf benefit. REACTOR_MEMO_001 (Reactor.Performance, Info) detects a fluent Element modifier applied to a semantically-confirmed keyed Memo overload and offers a code-fix that moves the modifier chain inside the factory body (Memo(id, () => Row(item)).Padding(8) -> Memo(id, () => Row(item).Padding(8))), which is behavior-equivalent (the reconciler re-applies wrapper modifiers to the inner element) and keeps the wrapper cacheable. - Semantic keyed/non-keyed discrimination via KeyedMemoElement return type - Low-FP: bails on non-keyed Memo, opaque factory, and non-Element modifiers - Adds Unshipped release row + cheat-table + analyzer-architecture template rows - 9 tests: positive, fire-once, negatives, near-misses, fix round-trips Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Roslyn analyzer + code fix (REACTOR_MEMO_001) to detect when fluent modifiers are applied to the keyed Memo(key, factory) wrapper (which prevents KeyedMemoElement from being served from the cross-recycle LRU), and provides an automated rewrite to move the modifier chain into the factory body so the wrapper remains cacheable.
Changes:
- Introduces
MemoWrapperModifierAnalyzerto semantically discriminate keyed vs non-keyedMemoand report the diagnostic on the innermost wrapper-decorating modifier. - Adds
MemoWrapperModifierCodeFixto re-root the modifier chain onto the factory body. - Adds unit tests + updates analyzer release tracking and internal docs/skill tables.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/AnalyzerTests/MemoWrapperModifierAnalyzerTests.cs | Adds analyzer/code-fix tests for REACTOR_MEMO_001 scenarios. |
| src/Reactor.Analyzers/MemoWrapperModifierAnalyzer.cs | New analyzer implementing the keyed-Memo wrapper-modifier rule. |
| src/Reactor.Analyzers/MemoWrapperModifierCodeFix.cs | New code fix that moves wrapper modifier chains into the memo factory. |
| src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md | Registers the new analyzer in the unshipped release log. |
| plugins/reactor/skills/reactor-build-and-check/SKILL.md | Documents REACTOR_MEMO_001 in the build-and-check skill cheat table. |
| docs/_pipeline/templates/analyzer-architecture.md.dt | Adds REACTOR_MEMO_001 to the analyzer architecture rules table template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ale-cache messaging pr-review + multi-model (gpt-5.4) cross-check found two HIGH issues: 1. Code-fix over-climb: the outermost-walk absorbed ANY trailing invocation, so Memo(id, () => Row(item)).Padding(8).ToString() moved .ToString() into the factory, breaking Func<Element>. The walk now stops at the last Element-returning modifier (semantic-checked), leaving non-Element trailing calls on the now-bare wrapper. 2. Description overclaimed 'renders identically'. Moving a modifier whose argument reads unkeyed state (e.g. .Background(isSelected ? A : B)) changes evaluation: the bare wrapper becomes cacheable, so a cache hit serves stale content. Reworded the Description, code-action title, and cheat-table row to require folding moved-modifier inputs into the key (matches the documented keyed-Memo purity contract in Dsl.cs). Test coverage (3 medium gaps closed): a negative that actually reaches the semantic return-type guard (synthetic non-keyed 2-arg look-alike), a NeedsParentheses fix round-trip (conditional factory body), a member-access callee positive (Factories.Memo(...)), and an over-climb fix round-trip. 13/13 pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot review flagged that TryGetMovableFactory and the code-fix assumed the factory is the second positional argument, so keyed-Memo calls with named / out-of-order arguments (Memo(factory: () => ..., key: id)) were a false negative and would have rewritten the wrong argument if fired. Locate the factory argument BY SHAPE — the single parameterless-lambda argument among the two — instead of by position. The keyed overload's TKey key can never be a bare lambda (no natural type for inference), so exactly one argument is the factory; ambiguity bails. The code-fix now rewrites that argument by identity. Adds a positive + fix round-trip for the out-of-order named-args form. 15/15 pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
REACTOR_MEMO_001 — modifiers on a keyed
Memo(key, factory)wrapper opt out of the recycle cachePart of spec 060 (Analyzer Suite Expansion), Wave E / Batch 2. Stacked on
azchohfi-spec-060-analyzer-suite.The bug it catches
The opt-in keyed memo
Factories.Memo<TKey>(TKey key, Func<Element> factory)→KeyedMemoElementis only served from the virtualizedElementFactory's bounded cross-recycle LRU when the wrapper is bare:Decorating the wrapper —
Memo(item.Id, () => Row(item)).Padding(8)— setsModifiers(orKey/Extensions), so the row bypasses the cache and rebuilds + re-diffs on every recycle, silently losing the perf benefit the keyed memo exists to provide.The rule
REACTOR_MEMO_001·Reactor.Performance· Info · code-fix ✔Memooverload (confirmed via theKeyedMemoElementreturn type — distinct from the non-keyedMemo(Func<RenderContext,Element>, …)→MemoElement). Fires once, on the innermost modifier whose receiver is the bareMemo(...)call.Memo(id, () => Row(item)).Padding(8)→Memo(id, () => Row(item).Padding(8)).Behavior-equivalent (the reconciler re-applies wrapper modifiers to the inner element on mount/update, so render output is identical) and keeps the wrapper cacheable.
Memo, opaque factory (method group / multi-statement — nothing to move the modifier into), non-Element trailing call (e.g..ToString()), arg count ≠ 2.Premise verification (Batch 2 verify-first gate)
Confirmed against source — no mismatch, so no orchestrator stop was needed:
Dsl.cs:1276→KeyedMemoElement(Element.cs:1543).Memo(Func<RenderContext,Element>, params object?[])→MemoElement(Dsl.cs:1239); overload resolution + return type separate them cleanly.ElementFactory.cs:151-158. The fix is behavior-equivalent per the transparent-unwrap paths (Reconciler.Mount.cs:111,Reconciler.cs:2474).Changes
src/Reactor.Analyzers/MemoWrapperModifierAnalyzer.cs,MemoWrapperModifierCodeFix.cs(new)AnalyzerReleases.Unshipped.mdrelease row (RS2000/RS2002 coupling — verified: analyzer builds with 0 warnings)reactor-build-and-check/SKILL.mdcheat table +analyzer-architecture.md.dttemplate rules tabletests/Reactor.Tests/AnalyzerTests/MemoWrapperModifierAnalyzerTests.cs— 9 tests (positive, fire-once, 2 negatives, 2 near-misses, 3 fix round-trips)Validation
dotnet test tests/Reactor.Tests -p:Platform=x64 --filter FullyQualifiedName~MemoWrapperModifierAnalyzerTests→ 9/9 passedVirtualizationDemo.cs:46) already uses the good "modifiers-inside-factory" form, so the rule correctly does not fire (§2.4 — no deliberately-bad sample added).