Add REACTOR_LIFECYCLE_002: UseEffect missing-cleanup analyzer#805
Merged
azchohfi merged 9 commits intoJul 6, 2026
Merged
Conversation
Flags an Action-overload UseEffect whose body allocates a long-lived producer (PeriodicTimer/Timer, an IObservable .Subscribe, or a CLR event +=) but returns no cleanup. Only the Func<Action> overload can carry a teardown, so the Action overload leaks the producer past unmount, firing the state setter on a dead RenderContext (docs/guide/effects.md 'Missing cleanup'). Conservative, low-FP: bails on method-group effects, any in-body using/Dispose/DisposeAsync or matching event -=, and allocations nested inside their own lambda/local function. Nudge-only (no code fix) since the correct teardown differs per resource. Couples the descriptor with its AnalyzerReleases.Unshipped.md row and appends the shared cheat-table + analyzer-architecture template rows. 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 to the Reactor analyzer suite to warn when UseEffect(Action, …) allocates a long-lived producer (timer / subscription / event wiring) without any cleanup mechanism, aligning with the “Missing cleanup” guidance in the effects documentation.
Changes:
- Introduces
REACTOR_LIFECYCLE_002(EffectCleanupAnalyzer) with conservative detection (ReactorUseEffectAction overload + top-level producer allocation + no teardown signals). - Adds a dedicated unit test suite (
EffectCleanupAnalyzerTests) covering positive/negative scenarios and near-misses. - Registers the new diagnostic in release notes + docs/skill tables (
AnalyzerReleases.Unshipped.md, skill cheat-sheet, analyzer architecture template).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/AnalyzerTests/EffectCleanupAnalyzerTests.cs | New analyzer tests with a stubbed minimal Reactor surface and producer types. |
| src/Reactor.Analyzers/EffectCleanupAnalyzer.cs | New REACTOR_LIFECYCLE_002 analyzer implementation and diagnostic text. |
| src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md | Adds an unshipped release note row for the new diagnostic. |
| plugins/reactor/skills/reactor-build-and-check/SKILL.md | Documents the new warning in the skill’s “common diagnostics” table. |
| docs/_pipeline/templates/analyzer-architecture.md.dt | Adds the analyzer to the architecture template’s diagnostic table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Correctness (both confirmed by the multi-model cross-check):
- Resolve the constructed timer type semantically so target-typed
'new(...)' (ImplicitObjectCreationExpressionSyntax) is covered.
- Filter a user type that merely shares a timer's simple name: the
distinctive names (PeriodicTimer + the dispatcher-timer family) match
by name, but the common 'Timer' name now requires the type to be
IDisposable, which the real System timers are.
Also swap the bespoke subtree walker for Roslyn's built-in
DescendantNodesAndSelf(descendIntoChildren:) (alternative-solution nit).
Tests: +12 cases (28 total) covering target-typed new, the
dispatcher-timer name, Subscribe return-type branches (concrete
IDisposable / void / non-disposable), the typed arity overloads, the
using(){} block form, DisposeAsync, an anonymous-method effect, the
user-Timer negative, and single-diagnostic-for-multiple-producers.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- GetInvokedMethodName now handles MemberBindingExpressionSyntax so a conditional-access call (ctx?.UseEffect(...)) passes the fast path (mirrors CommandDebounceAnalyzer). - HasCleanupSignal and the .Subscribe detection route through GetInvokedMethodName, so timer?.Dispose() / source?.Subscribe(...) are recognized — fixes a potential false positive where a conditional disposal path exists. - Reword the diagnostic message/description: the analyzer can't prove a state setter is called, so frame the guaranteed problem (the resource outlives the component and its callback can run after unmount) and present the dead-RenderConext setter as the conditional worse case. Tests: +2 (32 total) for conditional-access UseEffect (fires) and conditional-access Dispose (does not fire). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The .Subscribe detection is a heuristic (any Subscribe(...) that returns IDisposable), not strictly IObservable. Reword to match what's actually detected and align the SKILL row with the diagnostic's own conditional phrasing: - diagnostic payload: 'an IObservable subscription' -> 'a disposable subscription' - description: clarify it's a .Subscribe(...) returning IDisposable - SKILL cheat-table row: drop 'IObservable'; the producer 'outlives the component and can keep firing after unmount' rather than asserting a dead-context hit as a certainty. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Doc-comment-only: the <summary>/<remarks> still said 'IObservable subscription', asserted the dead-RenderContext state-setter hit as a certainty, and described 'a matching event -='. Reword to match the implementation: a '.Subscribe(...) returning IDisposable' heuristic; the guaranteed problem is the resource outliving the component with callbacks that may run after unmount (dead-context/state-setter only as the conditional worse case); and any event '-=' is accepted (not verified against the specific '+=' handler). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DispatcherQueueTimer and ThreadPoolTimer have no public constructor (they are factory-created via DispatcherQueue.CreateTimer() and ThreadPoolTimer.CreatePeriodicTimer(...)), so a new-only detection could never match them — they were dead entries. Remove them from KnownTimerTypes, leaving PeriodicTimer / Timer / DispatcherTimer (all genuinely constructible with new, as the samples show). Note the factory-method pattern as a possible future invocation-based extension. Test uses DispatcherTimer (new-able) for the distinctive-name branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A stray second <summary> line slipped in during the round-4 reword. Remove it so the XML doc is well-formed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
IsReactorUseEffect walked the base chain, so a user-declared UseEffect on a Component subclass (a shadow of the non-virtual framework hook, with unknown cleanup semantics) was treated as the Reactor hook — a potential false positive. Tighten to an exact-type match on Component / Component<T> / RenderContext (the types that actually declare the Action-vs-Func<Action> overloads). The idiomatic unqualified call still binds to Component.UseEffect (containing type exactly Component), so no real call is lost. Test: user-shadowed single-arg UseEffect on a Component subclass no longer fires (31 total). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Substantive: HasCleanupSignal recognized only Dispose/DisposeAsync and event -=, so stopping a timer in the body (DispatcherTimer's teardown is Stop(), not Dispose) still fired — a false positive. Treat Stop/Cancel as teardown signals too (consistent with how Dispose/-= are handled: any teardown-looking call bails the rule). Also make the wording resource-agnostic: the message/description said 'stops/disposes' / 'cancels/disposes', which doesn't fit an event subscription (torn down via -=). Now 'tears it down' / 'tears the resource down (stop/dispose the timer or subscription, or unsubscribe the event)'. Test: creating a DispatcherTimer and calling Stop() in-body no longer fires (32 total). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
azchohfi
merged commit a8f06c5
into
azchohfi-spec-060-analyzer-suite
28 of 29 checks passed
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.
Implements REACTOR_LIFECYCLE_002 (Reactor.Lifecycle, Warning, 4/5) for spec 060 (Analyzer Suite Expansion), stacked on
azchohfi-spec-060-analyzer-suite.What it catches
A
UseEffect(Action, …)whose body allocates a long-lived producer —new PeriodicTimer/Timer/DispatcherTimer, a.Subscribe(...)that returnsIDisposable, or a CLRevent +=— but returns no cleanup. Only theFunc<Action>overload can carry a teardown, so theActionoverload leaks the producer past unmount: it keeps running and its callback can still fire against a torn-down component (the "Missing cleanup" pitfall documented indocs/guide/effects.md, lines 340-376). Nudge-only — no code fix (the correct teardown differs per resource).Premise verification (STEP 1 gate)
Every cited anchor confirmed against real source:
RenderContext.cs:363→UseEffect(Action, params object[])(no-cleanup overload);:379→UseEffect(Func<Action>, …)(cleanup overload). Generic arity-1..3 forms exist for both.Component.cs:48-77re-exposes every overload asprotectedwrappers → confirms Component-or-RenderContext anchoring (§2.1).effects.md:340-376shows the exact pitfall + fix.The
ActionvsFunc<Action>overload split is the clean causal discriminator, resolved semantically.Detection (conservative, low-FP)
Fires only when: the call resolves exactly to the Reactor
Component/Component<T>/RenderContextUseEffectAction overload (a user's ownUseEffectshadow on a subclass is excluded); the effect arg is a lambda with a visible body; a known-lifetime allocation appears at the top level of that body (not inside a nested lambda / local function); and there is no teardown signal anywhere in the body (using, aDispose/DisposeAsync/Stop/Cancelcall — including conditional-accesstimer?.Stop()— or any event-=). Timer types are resolved semantically (covers target-typednew(...)and filters a user type that merely shares a timer's simple name). Method-group effects bail (unprovable).Tests
EffectCleanupAnalyzerTests— 32 tests: positives (PeriodicTimer / Timer / DispatcherTimer / target-typednew()/ subscription (exact + concrete IDisposable) / event / RenderContext-receiver / expression-bodied / anonymous-method / typed-arity / conditional-access), negatives (cleanup returned,usingdecl +using(){},Dispose/DisposeAsync, event unsubscribe,Stop(), numeric-=still fires, no resource, resource nested in a continuation, non-ReactorUseEffect, user-shadowedUseEffect, user non-disposableTimer, void/non-disposableSubscribe, conditional-accessDispose), and near-misses (method-group effect, similarly-named hook). All pass; fullAnalyzerTestssuite 247/247.Samples sweep
CounterDemo.cs,particle-storm/Sidebar.cs,ReactorCharting/Program.csall return() => timer.Dispose()/Stop()→Func<Action>overload → correctly skipped (0 FP). Non-UseEffecttimers ignored.Shared "append" docs (kept all existing rows)
AnalyzerReleases.Unshipped.mdrow (coupled with the descriptor — 0-warning build, no RS2000/RS2002), thereactor-build-and-checkSKILL cheat-table row, and theanalyzer-architecture.md.dttemplate row.Review
Passed the
pr-reviewskill (8 dimensions + a GPT multi-model cross-check that found no critical/high) and the GitHub Copilot review loop (8 rounds, every comment addressed; a review on the latest HEAD adds no new comments; all threads resolved).