From 1386b9c31cfff3f33849dfc755779fb9fad1da8d Mon Sep 17 00:00:00 2001 From: Brenley Dueck Date: Tue, 18 Aug 2026 23:19:38 -0500 Subject: [PATCH 1/3] feat(signals): "why did this run" dev attribution prototype Dev-only re-run attribution for the reactive graph, off by default and fully tree-shaken from prod builds. Every value commit (setSignal, memo change, async landing, refresh) stamps its node; every recompute diffs its previous deps against those stamps, producing a causal chain from the scope that ran down to the root write: [why-run] effect "title-effect" ran (run 2, 0.42ms) <- memo "label" changed (#6) <- signal "notifications" write (#5) 1 -> 2 Surfaced as DEV.attribution (enable/disable/subscribe/history/why/ subscriptions/costs/format). Layers on top of the core mechanism: - Hot-scope warnings: HOT_SCOPE_RERUNS (N runs per window) and HOT_SCOPE_TIME (self-time budget per window) via the existing diagnostics channel, warned once per window with the leaking signal named. - Wide-scope warning: WIDE_SCOPE_DEPS when a scope's dep count reaches a limit (creation runs included) - the coarse-read / helper-leak signature. Store property nodes get dev names ("store.count") so the evidence reads as paths. - Subscription diffing: RerunEvent.depsAdded/depsRemoved/depCount per run, plus DEV.attribution.subscriptions(node). - Timings: per-run selfMs (child-subtracted via a timing stack) and totalMs; costs() aggregates by scope (with wastedMs = time spent on unchanged-value runs) and by root write (downstream fan-out cost per write). - Static prototype: lint/reactive-helpers.ts (TS compiler API) flags reactive reads in un-annotated helpers with call-graph propagation; declaration sites are @reactive JSDoc, a Reactive<> return type, or being passed to a tracking primitive. Notable integration constraints: - rollup's tryCatchDeoptimization retains any function referenced inside a try block even behind a folded __DEV__ guard, so the asyncWrite lane-branch stamp sets a local flag inside the try and stamps after the catch (keeps prod at zero attribution bytes). - dev.ts exposes attribution through a getter: attribution.ts imports emitDiagnostic back from dev.ts, and under bundler transforms the cyclic binding is undefined (not a TDZ throw) at DEV construction. Co-Authored-By: Claude Fable 5 --- .../solid-signals/lint/reactive-helpers.ts | 256 +++++++++ packages/solid-signals/src/core/async.ts | 28 +- .../solid-signals/src/core/attribution.ts | 538 ++++++++++++++++++ packages/solid-signals/src/core/core.ts | 50 ++ packages/solid-signals/src/core/dev.ts | 25 +- packages/solid-signals/src/store/store.ts | 3 + .../solid-signals/tests/attribution.test.ts | 468 +++++++++++++++ .../tests/lint-reactive-helpers.test.ts | 112 ++++ 8 files changed, 1476 insertions(+), 4 deletions(-) create mode 100644 packages/solid-signals/lint/reactive-helpers.ts create mode 100644 packages/solid-signals/src/core/attribution.ts create mode 100644 packages/solid-signals/tests/attribution.test.ts create mode 100644 packages/solid-signals/tests/lint-reactive-helpers.test.ts diff --git a/packages/solid-signals/lint/reactive-helpers.ts b/packages/solid-signals/lint/reactive-helpers.ts new file mode 100644 index 000000000..7a5b0fa41 --- /dev/null +++ b/packages/solid-signals/lint/reactive-helpers.ts @@ -0,0 +1,256 @@ +import ts from "typescript"; + +/** + * Prototype static rule: reactive reads in un-annotated helpers. + * + * The runtime hazard this checks for: calling a function inside a tracked + * scope subscribes the CALLER to every signal/store read the function + * performs, but the call site gives no hint — `getUserLabel()` and + * `formatDate(d)` look identical, and only one wires you into the reactive + * graph. This rule makes the declaration site carry that information: any + * function that (transitively) reads reactive state must say so. + * + * A function counts as READING reactive state when it: + * - calls an accessor bound from `createSignal` / `createMemo` / + * `createAsync` (`count()`), + * - accesses a property on a proxy bound from `createStore` / + * `createProjection` (`state.notifications`), + * - calls a parameter typed `Accessor<...>` (the type-based heuristic), or + * - calls another module-local function already classified reactive + * (propagation up the call graph — the `getUserLabel` drift case). + * + * A reactive function is EXEMPT (already declared) when it: + * - has a `@reactive` JSDoc tag, + * - declares a `Reactive<...>` return type (type-level branding), or + * - is passed directly to a tracking primitive (`createMemo`, + * `createEffect`, ...) — being a tracked scope is its declaration. + * + * This is deliberately the "useful 80%": single-module, syntactic, no type + * checker. Cross-module propagation would key off the `Reactive<...>` brand + * in signatures — the brand is what makes the analysis composable. + */ + +export interface LintFinding { + functionName: string; + line: number; // 1-based + column: number; // 1-based + /** Human-readable evidence: what it reads, directly or via which callee. */ + evidence: string[]; + message: string; +} + +const SIGNAL_SOURCES = new Set(["createSignal"]); +const ACCESSOR_SOURCES = new Set(["createMemo", "createAsync"]); +const STORE_SOURCES = new Set(["createStore", "createProjection", "createOptimisticStore"]); +/** Functions whose function-arguments are tracked scopes (exempt as callees). */ +const TRACKING_PRIMITIVES = new Set([ + "createMemo", + "createAsync", + "createEffect", + "createRenderEffect", + "createComputed", + "createReaction", + "mapArray", + "repeat" +]); + +interface FnInfo { + name: string; + node: ts.SignatureDeclaration & { body?: ts.Node }; + directReads: string[]; // evidence strings for direct reactive reads + calls: Set; // names of module-local functions it calls + exempt: boolean; + reactive: boolean; + via: string | null; // callee that made it reactive transitively +} + +function hasReactiveJsDoc(node: ts.Node): boolean { + return ts.getJSDocTags(node).some(tag => tag.tagName.text === "reactive"); +} + +function hasReactiveReturnType(node: ts.SignatureDeclaration): boolean { + const t = node.type; + return ( + t !== undefined && + ts.isTypeReferenceNode(t) && + ts.isIdentifier(t.typeName) && + t.typeName.text === "Reactive" + ); +} + +function isAccessorTypedParam(p: ts.ParameterDeclaration): boolean { + const t = p.type; + return ( + t !== undefined && + ts.isTypeReferenceNode(t) && + ts.isIdentifier(t.typeName) && + (t.typeName.text === "Accessor" || t.typeName.text === "SourceAccessor") + ); +} + +export function analyzeReactiveHelpers(source: string, fileName = "module.tsx"): LintFinding[] { + const sf = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + + // Pass 1: module-level reactive bindings. + const accessors = new Set(); // count, label, ... + const stores = new Set(); // state, ... + const trackedScopeFns = new Set(); // function args to tracking primitives + + const calleeName = (call: ts.CallExpression): string | null => + ts.isIdentifier(call.expression) ? call.expression.text : null; + + const bindingVisitor = (node: ts.Node): void => { + if ( + ts.isVariableDeclaration(node) && + node.initializer && + ts.isCallExpression(node.initializer) + ) { + const callee = calleeName(node.initializer); + if (callee !== null) { + if (SIGNAL_SOURCES.has(callee) || STORE_SOURCES.has(callee)) { + // const [get, set] = createSignal(...) / const [state, setState] = createStore(...) + if (ts.isArrayBindingPattern(node.name) && node.name.elements.length > 0) { + const first = node.name.elements[0]; + if (ts.isBindingElement(first) && ts.isIdentifier(first.name)) { + (SIGNAL_SOURCES.has(callee) ? accessors : stores).add(first.name.text); + } + } + } else if (ACCESSOR_SOURCES.has(callee) && ts.isIdentifier(node.name)) { + accessors.add(node.name.text); + } + } + } + if (ts.isCallExpression(node)) { + const callee = calleeName(node); + if (callee !== null && TRACKING_PRIMITIVES.has(callee)) { + for (const arg of node.arguments) { + if (ts.isArrowFunction(arg) || ts.isFunctionExpression(arg)) trackedScopeFns.add(arg); + else if (ts.isIdentifier(arg)) trackedScopeFns.add(arg); // marker; resolved by name below + } + } + } + ts.forEachChild(node, bindingVisitor); + }; + bindingVisitor(sf); + + const trackedScopeNames = new Set(); + for (const n of trackedScopeFns) if (ts.isIdentifier(n)) trackedScopeNames.add(n.text); + + // Pass 2: collect module-local functions and their reads/calls. + const fns = new Map(); + + const registerFn = ( + name: string, + fnNode: ts.FunctionDeclaration | ts.ArrowFunction | ts.FunctionExpression, + docHost: ts.Node + ): void => { + const info: FnInfo = { + name, + node: fnNode, + directReads: [], + calls: new Set(), + exempt: + hasReactiveJsDoc(docHost) || + hasReactiveJsDoc(fnNode) || + hasReactiveReturnType(fnNode) || + trackedScopeFns.has(fnNode) || + trackedScopeNames.has(name), + reactive: false, + via: null + }; + const paramAccessors = new Set(); + for (const p of fnNode.parameters) { + if (isAccessorTypedParam(p) && ts.isIdentifier(p.name)) paramAccessors.add(p.name.text); + } + const bodyVisitor = (n: ts.Node): void => { + // Nested callbacks (`.map(x => ...)`) run during the call, so their + // reads belong to this function — but a tracked scope created inside + // (`createEffect(() => ...)` in a component body) owns its own reads. + if (trackedScopeFns.has(n)) return; + if (ts.isCallExpression(n) && ts.isIdentifier(n.expression)) { + const callee = n.expression.text; + if (accessors.has(callee)) info.directReads.push(`signal read "${callee}()"`); + else if (paramAccessors.has(callee)) + info.directReads.push(`accessor-typed parameter "${callee}()"`); + else info.calls.add(callee); + } + if ( + (ts.isPropertyAccessExpression(n) || ts.isElementAccessExpression(n)) && + ts.isIdentifier(n.expression) && + stores.has(n.expression.text) + ) { + const prop = + ts.isPropertyAccessExpression(n) && ts.isIdentifier(n.name) ? `.${n.name.text}` : "[...]"; + info.directReads.push(`store read "${n.expression.text}${prop}"`); + } + ts.forEachChild(n, bodyVisitor); + }; + if (fnNode.body) bodyVisitor(fnNode.body); + fns.set(name, info); + }; + + const fnVisitor = (node: ts.Node): void => { + if (ts.isFunctionDeclaration(node) && node.name !== undefined) { + registerFn(node.name.text, node, node); + } else if (ts.isVariableStatement(node) && node.declarationList.declarations.length === 1) { + const decl = node.declarationList.declarations[0]; + if ( + ts.isIdentifier(decl.name) && + decl.initializer !== undefined && + (ts.isArrowFunction(decl.initializer) || ts.isFunctionExpression(decl.initializer)) + ) { + registerFn(decl.name.text, decl.initializer, node); + } + } + ts.forEachChild(node, fnVisitor); + }; + fnVisitor(sf); + + // Pass 3: fixpoint propagation up the call graph. + let changedInPass = true; + while (changedInPass) { + changedInPass = false; + for (const info of fns.values()) { + if (info.reactive) continue; + if (info.directReads.length > 0) { + info.reactive = true; + changedInPass = true; + continue; + } + for (const callee of info.calls) { + const c = fns.get(callee); + if (c !== undefined && c.reactive) { + info.reactive = true; + info.via = callee; + changedInPass = true; + break; + } + } + } + } + + // Pass 4: report reactive, un-annotated functions. + const findings: LintFinding[] = []; + for (const info of fns.values()) { + if (!info.reactive || info.exempt) continue; + // Components (PascalCase) are exempt: their bodies run untracked by + // convention, and the runtime strict-read diagnostics own that case. + if (/^[A-Z]/.test(info.name)) continue; + const pos = sf.getLineAndCharacterOfPosition(info.node.getStart(sf)); + const evidence = + info.directReads.length > 0 + ? [...new Set(info.directReads)] + : [`calls reactive function "${info.via}()"`]; + findings.push({ + functionName: info.name, + line: pos.line + 1, + column: pos.character + 1, + evidence, + message: + `Function "${info.name}" reads reactive state (${evidence.join("; ")}) but does not ` + + `declare it — every tracked caller silently subscribes to those sources. ` + + `Annotate it /** @reactive */, give it a Reactive<...> return type, or wrap it in createMemo.` + }); + } + return findings.sort((a, b) => a.line - b.line); +} diff --git a/packages/solid-signals/src/core/async.ts b/packages/solid-signals/src/core/async.ts index 780083d0c..137a88987 100644 --- a/packages/solid-signals/src/core/async.ts +++ b/packages/solid-signals/src/core/async.ts @@ -12,6 +12,7 @@ import { STATUS_PENDING, STATUS_UNINITIALIZED } from "./constants.js"; +import { attributionActive, NO_VALUES, stampWrite } from "./attribution.js"; import { context, setSignal, untrack } from "./core.js"; import { devTrackHeldPending } from "./invariants.js"; import { emitDiagnostic } from "./dev.js"; @@ -364,6 +365,9 @@ export function handleAsync( clearStatus(el); const lane = resolveLane(el as any); if (lane) lane._pendingAsync.delete(el); + // Attribution: remember the newest stamp before the landing branches so a + // committed change (and only a committed change) can be relabeled below. + const devStampSeq = __DEV__ && attributionActive ? ((el as any)._devChange?.seq ?? 0) : 0; if (setter) { setter(value); if (wasUninitialized) clearStatus(el, true); @@ -388,15 +392,24 @@ export function handleAsync( // override every reader sees the override (A17), so waking subs would // re-show an unchanged view — the revert is the notification point. GlobalQueue._syncCompanions !== null && GlobalQueue._syncCompanions(el, value); - if (!hasActiveOverride(el)) insertSubs(el); + if (!hasActiveOverride(el)) { + if (__DEV__ && attributionActive) stampWrite(el, "async", NO_VALUES, value); + insertSubs(el); + } el._time = clock; } else if (lane) { // Route through lane's effect queue for independent flushing const isEffect = (el as any)._type; const prevValue = el._value; const equals = el._equals; + // Attribution flag only — the stamp itself must stay OUTSIDE the try: + // rollup's tryCatchDeoptimization retains any function referenced + // inside a try block even behind a folded __DEV__ guard, which would + // re-couple the dev-only attribution module into prod bundles (#2883). + let devChanged = false; try { if ((!isEffect && wasUninitialized) || !equals || !equals(value, prevValue)) { + if (__DEV__) devChanged = true; el._value = value; el._time = clock; // The latest() shadow write gives latest() effects independent lanes; the @@ -412,6 +425,7 @@ export function handleAsync( // rejection (#2837). notifyStatus(el, STATUS_ERROR, e); } + if (__DEV__ && attributionActive && devChanged) stampWrite(el, "async", prevValue, value); } else { try { setSignal(el, () => value); @@ -421,6 +435,18 @@ export function handleAsync( notifyStatus(el, STATUS_ERROR, e); } } + // Attribution: the plain path landed through setSignal, which stamps the + // generic "write" kind on a committed change. Relabel it as an async + // landing — but only if setSignal actually stamped (i.e. the value + // changed); a no-change landing must leave no fresh stamp behind, or a + // later unrelated re-run of a subscriber would mis-attribute to it. + if ( + __DEV__ && + attributionActive && + ((el as any)._devChange?.seq ?? 0) > devStampSeq && + (el as any)._devChange.kind === "write" + ) + stampWrite(el, "async", NO_VALUES, value); // First real answer landing: the window closes when the answer becomes // OBSERVABLE. A direct commit is observable now; a transition-held write // (`_pendingValue` set above or inside setSignal) is not — the verdict's diff --git a/packages/solid-signals/src/core/attribution.ts b/packages/solid-signals/src/core/attribution.ts new file mode 100644 index 000000000..42cf285e2 --- /dev/null +++ b/packages/solid-signals/src/core/attribution.ts @@ -0,0 +1,538 @@ +import { $REFRESH } from "./constants.js"; +// Cycle note: dev.ts imports this module for the `attribution` object, and we +// import its hoisted emitDiagnostic back — safe (only called at runtime) and +// treeshake-neutral (dev.ts is already reachable from the core). +import { emitDiagnostic } from "./dev.js"; +import type { Computed, Signal } from "./types.js"; + +/** + * Dev-mode "why did this run" attribution. + * + * The runtime already knows the full dependency set of every scope; this + * module surfaces it. Every value commit stamps its node with a ChangeRecord + * (a write, an async landing, a refresh() invalidation, or a derived change + * whose `causes` chain back to root writes). When a computation re-executes, + * the deps whose stamp is newer than the node's last run are its causes, so + * each re-run can be explained as a chain down to the originating write: + * + * [why-run] effect "docTitle" ran (run 4) + * ← memo "userLabel" changed (#6) + * ← signal "notifications" write (#5) 2 → 3 + * + * Everything here is dev-only and off by default: with attribution disabled + * the only cost is one boolean check per recompute/write. + */ + +export type ChangeKind = "write" | "derived" | "async" | "refresh"; + +export interface ChangeRecord { + /** Global monotonic change sequence — orders causes across the app. */ + seq: number; + kind: ChangeKind; + name: string; + /** Short previews of the value transition (writes only). */ + prev?: string; + value?: string; + /** First user frames of the triggering write's stack (opt-in). */ + stack?: string[]; + /** For derived changes: the upstream changes that produced this one. */ + causes?: ChangeRecord[]; +} + +export interface RerunEvent { + /** Global monotonic run sequence. */ + run: number; + /** How many times this node has re-run since attribution was enabled. */ + nodeRuns: number; + nodeKind: "effect" | "memo"; + nodeName: string; + node: Computed; + /** + * The deps that changed since this node's previous run. Empty means the + * re-run was not triggered by a tracked value change (creation-adjacent + * pull, error retry, or a cause this prototype does not stamp yet). + */ + causes: ChangeRecord[]; + /** Dependency count after this run. */ + depCount: number; + /** Names of deps this run subscribed to that the previous run did not. */ + depsAdded: string[]; + /** Names of deps the previous run had that this run dropped. */ + depsRemoved: string[]; + /** Wall time of this run excluding nested recomputes (ms). */ + selfMs: number; + /** Wall time of this run including nested recomputes (ms). */ + totalMs: number; + /** + * Whether the run committed a changed value. A memo run with + * `changed: false` was pure waste — the equality cutoff stopped it from + * notifying anyone; an effect run with `changed: false` computed without + * firing its effect phase. Summed as `wastedMs` in costs(). + */ + changed: boolean; +} + +export interface AttributionOptions { + /** Pretty-print each re-run to the console (default true). */ + log?: boolean; + /** Capture the user stack frame of each write — slow (default false). */ + stacks?: boolean; + /** Ring-buffer size for `history()` (default 200). */ + historyLimit?: number; + /** + * Hot-scope warning: emit a diagnostic when one scope re-runs `count` + * times within `windowMs` (default 60 runs / 1000ms). `false` disables. + */ + hotRuns?: { count: number; windowMs: number } | false; + /** + * Wide-scope warning: emit a diagnostic when a scope's dependency count + * reaches this (default 30) — the coarse-read / helper-leak signature. + * Re-warns only if the count then grows by another 50%. `false` disables. + */ + wideDeps?: number | false; + /** + * Time-budget warning: emit a diagnostic when one scope's summed self-time + * inside `windowMs` exceeds `budgetMs` (default 8ms / 1000ms — half a frame + * spent in one scope). Unlike `hotRuns` this catches the few-but-expensive + * scope that run counts miss. `false` disables. + */ + hotTime?: { budgetMs: number; windowMs: number } | false; +} + +interface AttributedNode { + _devChange?: ChangeRecord; + _devSeenSeq?: number; + _devRunCount?: number; + _devWinStart?: number; + _devWinCount?: number; + _devHotWarned?: boolean; + _devWideWarnedAt?: number; + _devTimeWinStart?: number; + _devTimeWinMs?: number; + _devTimeWarned?: boolean; +} + +export let attributionActive = false; + +let changeSeq = 0; +let runSeq = 0; +const defaultOptions = { + log: true, + stacks: false, + historyLimit: 200, + hotRuns: { count: 60, windowMs: 1000 } as { count: number; windowMs: number } | false, + wideDeps: 30 as number | false, + hotTime: { budgetMs: 8, windowMs: 1000 } as { budgetMs: number; windowMs: number } | false +}; +let options: typeof defaultOptions = { ...defaultOptions }; +const listeners = new Set<(event: RerunEvent) => void>(); +let history: RerunEvent[] = []; + +const now: () => number = + typeof performance !== "undefined" ? () => performance.now() : () => Date.now(); + +// Self-time bookkeeping: recomputes nest (pulls, child creation inside a +// parent's fn), so each frame tracks the time its children consumed and the +// parent subtracts it — the same discipline every profiler uses. +const timeStarts: number[] = []; +const childTimes: number[] = []; + +/** Open a timing frame for a recompute. Must be paired with endTiming(). */ +export function beginTiming(): void { + timeStarts.push(now()); + childTimes.push(0); +} + +/** Close the current timing frame; credits total time to the parent frame. */ +export function endTiming(): { selfMs: number; totalMs: number } { + const totalMs = now() - timeStarts.pop()!; + const childMs = childTimes.pop()!; + if (childTimes.length > 0) childTimes[childTimes.length - 1] += totalMs; + return { selfMs: Math.max(0, totalMs - childMs), totalMs }; +} + +// Cost aggregates, reset on enable()/disable(). +export interface ScopeCost { + name: string; + kind: "effect" | "memo"; + runs: number; + selfMs: number; + /** Self-time of runs that produced an unchanged value — recoverable. */ + wastedMs: number; +} +export interface WriteCost { + /** Root cause name (a signal write, async landing, or refresh target). */ + name: string; + /** Number of downstream re-runs this root triggered. */ + runs: number; + /** Summed self-time of every downstream re-run it caused. */ + downstreamMs: number; +} +const scopeCosts = new Map, ScopeCost>(); +const writeCosts = new Map(); + +function rootsOf(causes: ChangeRecord[], out: Set): void { + for (const c of causes) { + if (c.kind === "derived" && c.causes && c.causes.length > 0) rootsOf(c.causes, out); + else out.add(c.name); + } +} + +function recordCosts(event: RerunEvent): void { + let scope = scopeCosts.get(event.node); + if (scope === undefined) { + scope = { name: event.nodeName, kind: event.nodeKind, runs: 0, selfMs: 0, wastedMs: 0 }; + scopeCosts.set(event.node, scope); + } + scope.runs++; + scope.selfMs += event.selfMs; + if (!event.changed) scope.wastedMs += event.selfMs; + const roots = new Set(); + rootsOf(event.causes, roots); + for (const name of roots) { + let write = writeCosts.get(name); + if (write === undefined) writeCosts.set(name, (write = { name, runs: 0, downstreamMs: 0 })); + write.runs++; + write.downstreamMs += event.selfMs; + } +} + +function nodeName(node: Signal | Computed): string { + return (node as AttributedNode & { _name?: string })._name ?? "anonymous"; +} + +function preview(v: unknown): string { + if (v === null) return "null"; + switch (typeof v) { + case "undefined": + return "undefined"; + case "string": + return JSON.stringify(v.length > 40 ? v.slice(0, 40) + "…" : v); + case "number": + case "boolean": + case "bigint": + return String(v); + case "function": + return "[function]"; + case "symbol": + return v.toString(); + default: + return Array.isArray(v) ? `Array(${v.length})` : `[${v.constructor?.name ?? "object"}]`; + } +} + +function captureStack(): string[] | undefined { + if (!options.stacks) return undefined; + const raw = new Error().stack?.split("\n") ?? []; + // Drop the message line and every frame inside the reactive core; the first + // remaining frames are the user code that performed the write. + return raw + .slice(1) + .filter(line => !/solid-signals[/\\](src|dist)[/\\]/.test(line)) + .slice(0, 3) + .map(line => line.trim()); +} + +/** Sentinel for "no value transition to record" (refresh() stamps). */ +export const NO_VALUES = Symbol("no-values"); + +/** Record a root change (setSignal / refresh / async landing) on the node. */ +export function stampWrite( + node: Signal | Computed, + kind: Exclude, + prev: unknown = NO_VALUES, + value: unknown = NO_VALUES +): void { + const record: ChangeRecord = { seq: ++changeSeq, kind, name: nodeName(node) }; + if (value !== NO_VALUES) { + record.prev = prev === NO_VALUES ? undefined : preview(prev); + record.value = preview(value); + } + record.stack = captureStack(); + (node as AttributedNode)._devChange = record; +} + +/** Record a derived change (memo produced a new value) with its causes. */ +export function stampDerived(node: Computed, causes: ChangeRecord[]): void { + (node as AttributedNode)._devChange = { + seq: ++changeSeq, + kind: "derived", + name: nodeName(node), + causes + }; +} + +/** + * Collect the deps whose committed change is newer than this node's previous + * run. Called at recompute entry, while `_deps` still holds the previous + * run's links. A refresh() stamp on the node itself also counts — that is a + * self-invalidation, not a dep change. + */ +export function collectCauses(el: Computed): ChangeRecord[] { + const seen = (el as AttributedNode)._devSeenSeq ?? 0; + const causes: ChangeRecord[] = []; + const self = (el as AttributedNode)._devChange; + if (self !== undefined && self.seq > seen && self.kind === "refresh") causes.push(self); + for (let l = el._deps; l !== null; l = l._nextDep) { + const change = (l._dep as AttributedNode)._devChange; + if (change !== undefined && change.seq > seen) causes.push(change); + } + return causes; +} + +/** Advance the node's seen-cursor to the present. Call after every run. */ +export function markSeen(el: Computed): void { + (el as AttributedNode)._devSeenSeq = changeSeq; +} + +/** Snapshot the node's current dep identities (call before a run replaces them). */ +export function captureDeps(el: Computed): unknown[] { + const deps: unknown[] = []; + for (let l = el._deps; l !== null; l = l._nextDep) deps.push(l._dep); + return deps; +} + +/** + * Wide-scope warning — the coarse-read / helper-leak signature: one scope + * subscribed to dozens of sources re-runs when ANY of them change. Fired from + * recordRerun for re-runs and directly from recompute for creation runs (a + * memo can be born too wide). Re-warns only on 50% further growth. + */ +export function checkDepWidth(el: Computed): void { + const limit = options.wideDeps; + if (limit === false) return; + let count = 0; + const names: string[] = []; + for (let l = el._deps; l !== null; l = l._nextDep) { + count++; + if (names.length < 12) names.push(nodeName(l._dep)); + } + const node = el as AttributedNode; + if (count < limit || count < (node._devWideWarnedAt ?? 0) * 1.5) return; + node._devWideWarnedAt = count; + const kind = (el as { _type?: number })._type ? "effect" : "memo"; + const message = + `[WIDE_SCOPE_DEPS] ${kind} "${nodeName(el)}" is subscribed to ${count} sources — ` + + `it re-runs when any of them change. Narrow its reads or split it into smaller memos. ` + + `Sources: ${names.join(", ")}${count > names.length ? ", …" : ""}`; + emitDiagnostic({ + code: "WIDE_SCOPE_DEPS", + kind: "perf", + severity: "warn", + message, + nodeName: nodeName(el), + data: { depCount: count, deps: names } + }); + console.warn(message); +} + +/** + * Hot-scope warning — flags a scope that re-ran more than `count` times + * inside one `windowMs` window. Warned once per window, with the most recent + * cause chain named so the leaking signal is identified in the message. + */ +function checkHotRuns(el: Computed, event: RerunEvent): void { + const cfg = options.hotRuns; + if (cfg === false) return; + const node = el as AttributedNode; + const now = Date.now(); + if (node._devWinStart === undefined || now - node._devWinStart > cfg.windowMs) { + node._devWinStart = now; + node._devWinCount = 0; + node._devHotWarned = false; + } + node._devWinCount = (node._devWinCount ?? 0) + 1; + if (node._devHotWarned || node._devWinCount < cfg.count) return; + node._devHotWarned = true; + const rootCause = event.causes.map(c => `"${c.name}" (${c.kind})`).join(", "); + const message = + `[HOT_SCOPE_RERUNS] ${event.nodeKind} "${event.nodeName}" re-ran ${node._devWinCount} times ` + + `in ${Math.max(1, now - node._devWinStart)}ms — a hot signal is likely leaking into this ` + + `scope. Latest cause: ${rootCause || "(untracked pull)"}`; + emitDiagnostic({ + code: "HOT_SCOPE_RERUNS", + kind: "perf", + severity: "warn", + message, + nodeName: event.nodeName, + data: { + runs: node._devWinCount, + windowMs: cfg.windowMs, + causes: event.causes.map(c => c.name) + } + }); + console.warn(message); +} + +/** + * Time-budget warning — the counterpart of checkHotRuns for the + * few-but-expensive scope: warns when one scope's summed self-time within a + * window exceeds the budget. Warned once per window. + */ +function checkHotTime(el: Computed, event: RerunEvent): void { + const cfg = options.hotTime; + if (cfg === false) return; + const node = el as AttributedNode; + const at = now(); + if (node._devTimeWinStart === undefined || at - node._devTimeWinStart > cfg.windowMs) { + node._devTimeWinStart = at; + node._devTimeWinMs = 0; + node._devTimeWarned = false; + } + node._devTimeWinMs = (node._devTimeWinMs ?? 0) + event.selfMs; + if (node._devTimeWarned || node._devTimeWinMs < cfg.budgetMs) return; + node._devTimeWarned = true; + const rootCause = event.causes.map(c => `"${c.name}" (${c.kind})`).join(", "); + const message = + `[HOT_SCOPE_TIME] ${event.nodeKind} "${event.nodeName}" spent ` + + `${node._devTimeWinMs.toFixed(1)}ms of compute inside one ${cfg.windowMs}ms window ` + + `(budget ${cfg.budgetMs}ms). Latest cause: ${rootCause || "(untracked pull)"}`; + emitDiagnostic({ + code: "HOT_SCOPE_TIME", + kind: "perf", + severity: "warn", + message, + nodeName: event.nodeName, + data: { + spentMs: node._devTimeWinMs, + budgetMs: cfg.budgetMs, + windowMs: cfg.windowMs, + causes: event.causes.map(c => c.name) + } + }); + console.warn(message); +} + +export function recordRerun( + el: Computed, + causes: ChangeRecord[], + prevDeps: unknown[], + timing: { selfMs: number; totalMs: number }, + changed: boolean +): void { + const node = el as AttributedNode; + // Subscription diff: `prevDeps` was captured at run entry; `_deps` now + // holds the fresh set. A changed set is the "helper edit changed distant + // call sites" signal — surfaced per-event and in the console format. + const newDeps = captureDeps(el); + const prevSet = new Set(prevDeps); + const newSet = new Set(newDeps); + const depsAdded: string[] = []; + const depsRemoved: string[] = []; + for (const d of newDeps) if (!prevSet.has(d)) depsAdded.push(nodeName(d as Signal)); + for (const d of prevDeps) if (!newSet.has(d)) depsRemoved.push(nodeName(d as Signal)); + const event: RerunEvent = { + run: ++runSeq, + nodeRuns: (node._devRunCount = (node._devRunCount ?? 0) + 1), + nodeKind: (el as { _type?: number })._type ? "effect" : "memo", + nodeName: nodeName(el), + node: el, + causes, + depCount: newDeps.length, + depsAdded, + depsRemoved, + selfMs: timing.selfMs, + totalMs: timing.totalMs, + changed + }; + history.push(event); + if (history.length > options.historyLimit) history.shift(); + recordCosts(event); + checkHotRuns(el, event); + checkHotTime(el, event); + checkDepWidth(el); + for (const listener of listeners) listener(event); + if (options.log) console.log(formatRerun(event)); +} + +function formatCause(cause: ChangeRecord, depth: number, out: string[]): void { + const pad = " ".repeat(depth + 1); + let line = `${pad}← ${cause.kind === "derived" ? "memo" : "signal"} "${cause.name}" ${ + cause.kind === "derived" ? "changed" : cause.kind + } (#${cause.seq})`; + if (cause.prev !== undefined) line += ` ${cause.prev} → ${cause.value}`; + out.push(line); + if (cause.stack) for (const frame of cause.stack) out.push(`${pad} ${frame}`); + if (cause.causes && depth < 10) { + for (const upstream of cause.causes) formatCause(upstream, depth + 1, out); + } +} + +export function formatRerun(event: RerunEvent): string { + const out = [ + `[why-run] ${event.nodeKind} "${event.nodeName}" ran (run ${event.nodeRuns}, ` + + `${event.selfMs.toFixed(2)}ms${event.changed ? "" : ", unchanged"})` + + (event.causes.length === 0 ? " — no tracked cause (pull or retry)" : "") + ]; + for (const cause of event.causes) formatCause(cause, 0, out); + if (event.depsAdded.length > 0 || event.depsRemoved.length > 0) { + const delta = [ + ...event.depsAdded.map(n => `+"${n}"`), + ...event.depsRemoved.map(n => `-"${n}"`) + ].join(" "); + out.push(` deps changed: ${delta} (${event.depCount} total)`); + } + return out.join("\n"); +} + +export interface Attribution { + enable(opts?: AttributionOptions): void; + disable(): void; + subscribe(listener: (event: RerunEvent) => void): () => void; + history(): readonly RerunEvent[]; + /** Re-run history for one node — pass a memo/effect accessor or raw node. */ + why(target: unknown): RerunEvent[]; + /** Current dependency names of one scope — the devtools subscription view. */ + subscriptions(target: unknown): string[]; + /** + * Aggregated cost tables since enable(): `scopes` ranked by self-time + * (with `wastedMs` = time spent on unchanged-value runs), `writes` ranked + * by total downstream re-run time each root write caused. + */ + costs(): { scopes: ScopeCost[]; writes: WriteCost[] }; + format: typeof formatRerun; +} + +export const attribution: Attribution = { + enable(opts?: AttributionOptions) { + options = { ...defaultOptions, ...opts }; + attributionActive = true; + timeStarts.length = 0; + childTimes.length = 0; + scopeCosts.clear(); + writeCosts.clear(); + }, + disable() { + attributionActive = false; + listeners.clear(); + history = []; + timeStarts.length = 0; + childTimes.length = 0; + scopeCosts.clear(); + writeCosts.clear(); + }, + subscribe(listener) { + listeners.add(listener); + return () => listeners.delete(listener); + }, + history() { + return history; + }, + why(target: unknown) { + const node = ((target as Record)?.[$REFRESH] ?? target) as Computed; + return history.filter(event => event.node === node); + }, + subscriptions(target: unknown) { + const node = ((target as Record)?.[$REFRESH] ?? target) as Computed; + const names: string[] = []; + for (let l = node?._deps ?? null; l !== null; l = l._nextDep) names.push(nodeName(l._dep)); + return names; + }, + costs() { + return { + scopes: [...scopeCosts.values()].sort((a, b) => b.selfMs - a.selfMs), + writes: [...writeCosts.values()].sort((a, b) => b.downstreamMs - a.downstreamMs) + }; + }, + format: formatRerun +}; diff --git a/packages/solid-signals/src/core/core.ts b/packages/solid-signals/src/core/core.ts index ecf57d653..3abfcb018 100644 --- a/packages/solid-signals/src/core/core.ts +++ b/packages/solid-signals/src/core/core.ts @@ -57,6 +57,19 @@ import { throwPendingUntrackedRead, warnStrictReadUntracked } from "./dev.js"; +import { + attributionActive, + beginTiming, + captureDeps, + checkDepWidth, + collectCauses, + endTiming, + markSeen, + recordRerun, + stampDerived, + stampWrite, + type ChangeRecord +} from "./attribution.js"; import { devTrackHeldPending } from "./invariants.js"; import { cleanup, disposeChildren, inheritId, markDisposal } from "./owner.js"; import { @@ -170,6 +183,22 @@ export function clearSnapshots(): void { export function recompute(el: Computed, create: boolean = false): void { const isEffect = (el as any)._type; + // Attribution snapshot must happen before this run touches the dep list: + // `_deps` still holds the previous run's links, which are exactly the + // subscriptions that could have triggered this run — and the baseline for + // the subscription diff after the run. + let devCauses: ChangeRecord[] | null = null; + let devPrevDeps: unknown[] | null = null; + let devChanged = false; + if (__DEV__ && attributionActive) { + // Every recompute opens a timing frame (create runs included) so nested + // computes are subtracted from the parent's self-time. + beginTiming(); + if (!create) { + devCauses = collectCauses(el); + devPrevDeps = captureDeps(el); + } + } if (!create) { if (el._transition && (!isEffect || activeTransition) && activeTransition !== el._transition) globalQueue.initTransition(el._transition); @@ -350,6 +379,13 @@ export function recompute(el: Computed, create: boolean = false): void { notifyStatus(el, STATUS_ERROR, e); } + // A committed derived change becomes a cause for this node's subscribers, + // chaining their attribution through this node to the root write. + if (__DEV__ && attributionActive) { + devChanged = valueChanged && !el._error; + if (devCauses !== null && devChanged && !isEffect) stampDerived(el, devCauses); + } + // Effects use `_equals: false` (no per-effect closure). The side effects that // the equals closure used to perform — flagging the effect dirty and enqueueing // its runner — happen here instead. `!create` matches the previous `initialized` @@ -434,6 +470,14 @@ export function recompute(el: Computed, create: boolean = false): void { if (outgoingError !== undefined && !valueChanged && !el._error) settleErroredDependents(el, outgoingError); } + if (__DEV__ && attributionActive) { + const devTiming = endTiming(); + if (devCauses !== null) recordRerun(el, devCauses, devPrevDeps!, devTiming, devChanged); + // Creation runs still get the wide-scope check: a memo can be born with + // its coarse-read problem already in place. + else checkDepWidth(el); + markSeen(el); + } currentOptimisticLane = prevLane; const needsPendingCommit = el._pendingValue !== NOT_PENDING || @@ -1046,6 +1090,9 @@ export function setSignal(el: Signal | Computed, v: T | ((prev: T) => T !el._equals(currentValue, v); if (!valueChanged) return v; + // Root cause for attribution: this write is where a re-run chain begins. + if (__DEV__ && attributionActive) stampWrite(el, "write", currentValue, v); + if (el._pendingValue === NOT_PENDING) queuePendingNode(el); el._pendingValue = v; if (__DEV__) devTrackHeldPending(el); @@ -1217,6 +1264,9 @@ export function refresh(target: Refreshable): void { armReaskClear(); } node._flags = (node._flags & ~REACTIVE_CHECK) | REACTIVE_DIRTY; + // A refresh() self-invalidation is a root cause too — the target's next + // run has no changed dep to point at, so it points here instead. + if (__DEV__ && attributionActive) stampWrite(node, "refresh"); insertIntoHeap(node, queueFor(node)); schedule(); } diff --git a/packages/solid-signals/src/core/dev.ts b/packages/solid-signals/src/core/dev.ts index e51038504..e5942faf1 100644 --- a/packages/solid-signals/src/core/dev.ts +++ b/packages/solid-signals/src/core/dev.ts @@ -1,3 +1,4 @@ +import { attribution, type Attribution } from "./attribution.js"; import type { Computed, Link, Owner, Signal } from "./types.js"; export interface DevHooks { @@ -29,9 +30,19 @@ export type DiagnosticCode = | "MISSING_EFFECT_FN" | "SYNC_NODE_RECEIVED_ASYNC" | "REACTIVITY_HALTED" - | "INVARIANT_VIOLATION"; - -export type DiagnosticKind = "strict-read" | "async" | "write" | "lifecycle" | "owner" | "error"; + | "INVARIANT_VIOLATION" + | "HOT_SCOPE_RERUNS" + | "HOT_SCOPE_TIME" + | "WIDE_SCOPE_DEPS"; + +export type DiagnosticKind = + | "strict-read" + | "async" + | "write" + | "lifecycle" + | "owner" + | "error" + | "perf"; export interface DiagnosticEvent { sequence: number; @@ -61,6 +72,8 @@ export interface Diagnostics { export interface Dev { hooks: DevHooks; diagnostics: Diagnostics; + /** "Why did this run" re-run attribution — see attribution.ts. */ + attribution: Attribution; getChildren: typeof getChildren; getSignals: typeof getSignals; getParent: typeof getParent; @@ -100,6 +113,12 @@ export const DEV: Dev = __DEV__ ? { hooks, diagnostics, + // Getter: attribution.ts imports emitDiagnostic back from this module, + // so when attribution.ts evaluates first the `attribution` binding is + // still uninitialized here — defer the read to access time. + get attribution() { + return attribution; + }, getChildren, getSignals, getParent, diff --git a/packages/solid-signals/src/store/store.ts b/packages/solid-signals/src/store/store.ts index 42e9038d1..d1541f9d6 100644 --- a/packages/solid-signals/src/store/store.ts +++ b/packages/solid-signals/src/store/store.ts @@ -502,6 +502,9 @@ function getNode( const s = signal( value, { + // Dev-only: name store property nodes by path segment so attribution + // chains and wide-scope warnings read "store.todos", not "signal". + name: __DEV__ ? "store." + String(property) : undefined, equals: equals, unobserved() { if (nodes[property] === s) { diff --git a/packages/solid-signals/tests/attribution.test.ts b/packages/solid-signals/tests/attribution.test.ts new file mode 100644 index 000000000..c63f4f533 --- /dev/null +++ b/packages/solid-signals/tests/attribution.test.ts @@ -0,0 +1,468 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + createEffect, + createMemo, + createRoot, + createSignal, + createStore, + DEV, + flush, + refresh +} from "../src/index.js"; +import type { AttributionOptions, RerunEvent } from "../src/core/attribution.js"; + +afterEach(() => { + DEV!.attribution.disable(); + flush(); + vi.restoreAllMocks(); +}); + +/** Enable quietly and collect every rerun event. */ +function collect(opts?: AttributionOptions): RerunEvent[] { + DEV!.attribution.enable({ log: false, ...opts }); + const events: RerunEvent[] = []; + DEV!.attribution.subscribe(e => events.push(e)); + return events; +} + +describe("why-did-this-run attribution", () => { + it("attributes an effect re-run to the triggering signal write", () => { + const [count, setCount] = createSignal(0, { name: "count" }); + createRoot(() => + createEffect( + () => count(), + () => {}, + { name: "counter-effect" } + ) + ); + flush(); + + const events = collect(); + setCount(1); + flush(); + + expect(events).toHaveLength(1); + expect(events[0].nodeKind).toBe("effect"); + expect(events[0].nodeName).toBe("counter-effect"); + expect(events[0].causes).toHaveLength(1); + expect(events[0].causes[0]).toMatchObject({ + kind: "write", + name: "count", + prev: "0", + value: "1" + }); + }); + + it("chains attribution through a memo to the root write", () => { + const [n, setN] = createSignal(1, { name: "notifications" }); + const label = createMemo(() => `msgs: ${n()}`, { name: "label" }); + createRoot(() => + createEffect( + () => label(), + () => {}, + { name: "title-effect" } + ) + ); + flush(); + + const events = collect(); + setN(2); + flush(); + + const effectRun = events.find(e => e.nodeName === "title-effect")!; + expect(effectRun).toBeDefined(); + expect(effectRun.causes).toHaveLength(1); + const cause = effectRun.causes[0]; + expect(cause.kind).toBe("derived"); + expect(cause.name).toBe("label"); + // The derived cause chains to the root write. + expect(cause.causes).toHaveLength(1); + expect(cause.causes![0]).toMatchObject({ kind: "write", name: "notifications" }); + + // The memo's own re-run is attributed directly to the write. + const memoRun = events.find(e => e.nodeName === "label")!; + expect(memoRun.causes[0]).toMatchObject({ kind: "write", name: "notifications" }); + }); + + it("does not attribute downstream re-runs past an equality cutoff", () => { + const [n, setN] = createSignal(1, { name: "n" }); + const parity = createMemo(() => n() % 2, { name: "parity" }); + createRoot(() => + createEffect( + () => parity(), + () => {}, + { name: "parity-effect" } + ) + ); + flush(); + + const events = collect(); + setN(3); // parity unchanged: memo re-runs, effect must not + flush(); + + expect(events.map(e => e.nodeName)).toEqual(["parity"]); + + setN(4); // parity flips: both run, effect attributed through the memo + flush(); + const effectRun = events.find(e => e.nodeName === "parity-effect")!; + expect(effectRun.causes[0]).toMatchObject({ kind: "derived", name: "parity" }); + }); + + it("attributes refresh() re-runs to the self-invalidation", () => { + const [n] = createSignal(1, { name: "n" }); + const doubled = createMemo(() => n() * 2, { name: "doubled" }); + createRoot(() => + createEffect( + () => doubled(), + () => {}, + { name: "consumer" } + ) + ); + flush(); + + const events = collect(); + refresh(doubled); + flush(); + + const memoRun = events.find(e => e.nodeName === "doubled")!; + expect(memoRun.causes).toHaveLength(1); + expect(memoRun.causes[0]).toMatchObject({ kind: "refresh", name: "doubled" }); + }); + + it("attributes async landings distinctly from sync writes", async () => { + let resolve!: (v: string) => void; + const [trigger, setTrigger] = createSignal(0, { name: "trigger" }); + const data = createMemo( + () => { + trigger(); + return new Promise(r => (resolve = r)); + }, + { name: "data" } + ); + createRoot(() => + createEffect( + () => data(), + () => {}, + { name: "data-effect" } + ) + ); + flush(); + resolve("first"); + await Promise.resolve(); + flush(); + + const events = collect(); + setTrigger(1); + flush(); + resolve("second"); + await Promise.resolve(); + flush(); + + const effectRun = events.filter(e => e.nodeName === "data-effect").at(-1)!; + expect(effectRun).toBeDefined(); + expect(effectRun.causes.some(c => c.kind === "async" && c.name === "data")).toBe(true); + }); + + it("exposes per-node history via why()", () => { + const [n, setN] = createSignal(0, { name: "n" }); + const doubled = createMemo(() => n() * 2, { name: "doubled" }); + createRoot(() => + createEffect( + () => doubled(), + () => {}, + { name: "consumer" } + ) + ); + flush(); + + collect(); + setN(1); + setN(2); + flush(); + + const runs = DEV!.attribution.why(doubled); + expect(runs.length).toBeGreaterThanOrEqual(1); + expect(runs.every(e => e.nodeName === "doubled")).toBe(true); + }); + + it("formats a readable cause chain", () => { + const [n, setN] = createSignal(1, { name: "notifications" }); + const label = createMemo(() => `msgs: ${n()}`, { name: "label" }); + createRoot(() => + createEffect( + () => label(), + () => {}, + { name: "title-effect" } + ) + ); + flush(); + + const events = collect(); + setN(2); + flush(); + + const text = DEV!.attribution.format(events.find(e => e.nodeName === "title-effect")!); + expect(text).toContain('effect "title-effect" ran'); + expect(text).toContain('memo "label" changed'); + expect(text).toContain('signal "notifications" write'); + expect(text).toContain("1 → 2"); + }); + + it("diffs subscriptions across runs (conditional deps)", () => { + const [flag, setFlag] = createSignal(true, { name: "flag" }); + const [a] = createSignal("a", { name: "a" }); + const [b] = createSignal("b", { name: "b" }); + createRoot(() => + createEffect( + () => (flag() ? a() : b()), + () => {}, + { name: "branchy" } + ) + ); + flush(); + + const events = collect(); + setFlag(false); + flush(); + + const run = events.find(e => e.nodeName === "branchy")!; + expect(run.depsAdded).toEqual(["b"]); + expect(run.depsRemoved).toEqual(["a"]); + expect(run.depCount).toBe(2); // flag + b + expect(DEV!.attribution.format(run)).toContain('deps changed: +"b" -"a" (2 total)'); + + // A run with an unchanged dep set reports no diff. + setFlag(true); + flush(); + setFlag(false); + flush(); + const last = events.filter(e => e.nodeName === "branchy").at(-1)!; + expect(last.depsAdded).toEqual(["b"]); + expect(DEV!.attribution.subscriptions(run.node)).toEqual(["flag", "b"]); + }); + + it("warns on hot scopes, once per window", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const [n, setN] = createSignal(0, { name: "n" }); + createRoot(() => + createEffect( + () => n(), + () => {}, + { name: "hot-effect" } + ) + ); + flush(); + + collect({ hotRuns: { count: 3, windowMs: 60_000 }, wideDeps: false }); + const capture = DEV!.diagnostics.capture(); + for (let i = 1; i <= 5; i++) { + setN(i); + flush(); + } + + const hot = capture.stop().filter(e => e.code === "HOT_SCOPE_RERUNS"); + expect(hot).toHaveLength(1); // warned at the 3rd run, muted after + expect(hot[0].nodeName).toBe("hot-effect"); + expect(hot[0].data).toMatchObject({ runs: 3, windowMs: 60_000 }); + expect(hot[0].message).toContain('"n" (write)'); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("warns on wide scopes and re-warns only on 50% growth", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const signals = Array.from({ length: 5 }, (_, i) => createSignal(i, { name: `s${i}` })); + const [bump, setBump] = createSignal(0, { name: "bump" }); + const wide = createMemo(() => bump() + signals.reduce((sum, [get]) => sum + get(), 0), { + name: "wide-memo" + }); + createRoot(() => + createEffect( + () => wide(), + () => {}, + { name: "consumer" } + ) + ); + flush(); + + collect({ wideDeps: 4, hotRuns: false }); + const capture = DEV!.diagnostics.capture(); + setBump(1); + flush(); + setBump(2); // still 6 deps — under the 1.5x re-warn bar + flush(); + + const wideEvents = capture.stop().filter(e => e.code === "WIDE_SCOPE_DEPS"); + expect(wideEvents).toHaveLength(1); + expect(wideEvents[0].nodeName).toBe("wide-memo"); + expect(wideEvents[0].data!.depCount).toBe(6); // bump + s0..s4 + expect(wideEvents[0].data!.deps as string[]).toContain("s3"); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("warns on wide scopes at creation time", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const signals = Array.from({ length: 5 }, (_, i) => createSignal(i, { name: `c${i}` })); + + collect({ wideDeps: 4, hotRuns: false }); + const capture = DEV!.diagnostics.capture(); + const wide = createMemo(() => signals.reduce((sum, [get]) => sum + get(), 0), { + name: "born-wide" + }); + wide(); // pull once so a lazy creation path still computes + + const wideEvents = capture.stop().filter(e => e.code === "WIDE_SCOPE_DEPS"); + expect(wideEvents).toHaveLength(1); + expect(wideEvents[0].nodeName).toBe("born-wide"); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("names store property nodes by path in attribution output", () => { + const [state, setState] = createStore({ count: 1, other: "x" }); + createRoot(() => + createEffect( + () => state.count, + () => {}, + { name: "store-reader" } + ) + ); + flush(); + + const events = collect(); + setState(s => { + s.count = 2; + }); + flush(); + + const run = events.find(e => e.nodeName === "store-reader")!; + expect(run).toBeDefined(); + expect(run.causes.some(c => c.name === "store.count")).toBe(true); + }); + + it("measures self-time and aggregates costs by scope and root write", () => { + const spin = (ms: number) => { + const end = performance.now() + ms; + while (performance.now() < end); + }; + const [n, setN] = createSignal(0, { name: "n" }); + const slow = createMemo( + () => { + spin(10); + return n(); + }, + { name: "slow-memo" } + ); + createRoot(() => + createEffect( + () => slow(), + () => {}, + { name: "cheap-effect" } + ) + ); + flush(); + + const events = collect({ hotTime: false }); + setN(1); + flush(); + + const memoRun = events.find(e => e.nodeName === "slow-memo")!; + const effectRun = events.find(e => e.nodeName === "cheap-effect")!; + expect(memoRun.selfMs).toBeGreaterThanOrEqual(5); + expect(memoRun.totalMs).toBeGreaterThanOrEqual(memoRun.selfMs); + expect(memoRun.changed).toBe(true); + expect(effectRun.selfMs).toBeLessThan(memoRun.selfMs); + + const { scopes, writes } = DEV!.attribution.costs(); + expect(scopes[0].name).toBe("slow-memo"); // ranked by self-time + expect(scopes[0].selfMs).toBeGreaterThanOrEqual(5); + expect(scopes[0].wastedMs).toBe(0); // value changed — not waste + const rootWrite = writes.find(w => w.name === "n")!; + expect(rootWrite).toBeDefined(); + expect(rootWrite.downstreamMs).toBeGreaterThanOrEqual(memoRun.selfMs); + expect(rootWrite.runs).toBeGreaterThanOrEqual(2); // memo + effect + }); + + it("counts unchanged-value runs as wasted time", () => { + const spin = (ms: number) => { + const end = performance.now() + ms; + while (performance.now() < end); + }; + const [n, setN] = createSignal(1, { name: "n" }); + const wastefulMemo = createMemo( + () => { + n(); + spin(6); + return "constant"; + }, + { name: "wasteful" } + ); + createRoot(() => + createEffect( + () => wastefulMemo(), + () => {}, + { name: "w-consumer" } + ) + ); + flush(); + + collect({ hotTime: false }); + setN(2); // memo re-runs, produces the same value — pure waste + flush(); + + const { scopes } = DEV!.attribution.costs(); + const wasteful = scopes.find(s => s.name === "wasteful")!; + expect(wasteful.wastedMs).toBeGreaterThanOrEqual(4); + expect(wasteful.wastedMs).toBe(wasteful.selfMs); + }); + + it("warns when a scope exceeds its time budget in one window", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const spin = (ms: number) => { + const end = performance.now() + ms; + while (performance.now() < end); + }; + const [n, setN] = createSignal(0, { name: "n" }); + createRoot(() => + createEffect( + () => { + n(); + spin(6); + }, + () => {}, + { name: "budget-buster" } + ) + ); + flush(); + + collect({ hotRuns: false, hotTime: { budgetMs: 5, windowMs: 60_000 } }); + const capture = DEV!.diagnostics.capture(); + setN(1); + flush(); + setN(2); // still inside the window — warned once, then muted + flush(); + + const timeEvents = capture.stop().filter(e => e.code === "HOT_SCOPE_TIME"); + expect(timeEvents).toHaveLength(1); + expect(timeEvents[0].nodeName).toBe("budget-buster"); + expect(timeEvents[0].data!.spentMs as number).toBeGreaterThanOrEqual(5); + expect(timeEvents[0].message).toContain('"n" (write)'); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("is inert when disabled", () => { + const [n, setN] = createSignal(0, { name: "n" }); + createRoot(() => + createEffect( + () => n(), + () => {}, + { name: "e" } + ) + ); + flush(); + const events: RerunEvent[] = []; + DEV!.attribution.subscribe(e => events.push(e)); + setN(1); + flush(); + expect(events).toHaveLength(0); + expect(DEV!.attribution.history()).toHaveLength(0); + }); +}); diff --git a/packages/solid-signals/tests/lint-reactive-helpers.test.ts b/packages/solid-signals/tests/lint-reactive-helpers.test.ts new file mode 100644 index 000000000..42762643e --- /dev/null +++ b/packages/solid-signals/tests/lint-reactive-helpers.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from "vitest"; +import { analyzeReactiveHelpers } from "../lint/reactive-helpers.js"; + +describe("lint: reactive reads in un-annotated helpers", () => { + it("flags a helper that reads a store (the getUserLabel drift)", () => { + const findings = analyzeReactiveHelpers(` + const [state, setState] = createStore({ + user: { name: "Ada" }, + notifications: [] + }); + + function getUserLabel() { + const badge = state.notifications.length > 0 ? " •" : ""; + return state.user.name + badge; + } + `); + expect(findings).toHaveLength(1); + expect(findings[0].functionName).toBe("getUserLabel"); + expect(findings[0].evidence.join(" ")).toContain('store read "state.notifications"'); + expect(findings[0].message).toContain("silently subscribes"); + }); + + it("flags signal-accessor reads in plain helpers, including arrow consts", () => { + const findings = analyzeReactiveHelpers(` + const [count, setCount] = createSignal(0); + const doubled = () => count() * 2; + function formatDate(d: Date) { return d.toISOString(); } + `); + expect(findings.map(f => f.functionName)).toEqual(["doubled"]); + expect(findings[0].evidence).toEqual(['signal read "count()"']); + }); + + it("propagates reactivity up the call graph", () => { + const findings = analyzeReactiveHelpers(` + const [n] = createSignal(1); + function inner() { return n(); } + function middle() { return inner() + 1; } + function outer() { return middle() * 2; } + function unrelated() { return 42; } + `); + expect(findings.map(f => f.functionName).sort()).toEqual(["inner", "middle", "outer"]); + const outer = findings.find(f => f.functionName === "outer")!; + expect(outer.evidence).toEqual(['calls reactive function "middle()"']); + }); + + it("accepts the declaration sites: @reactive JSDoc and Reactive<> return type", () => { + const findings = analyzeReactiveHelpers(` + const [count] = createSignal(0); + + /** @reactive */ + function labeled() { return count(); } + + const branded = (): Reactive => count() * 2; + + function undeclared() { return count(); } + `); + expect(findings.map(f => f.functionName)).toEqual(["undeclared"]); + }); + + it("exempts functions passed directly to tracking primitives", () => { + const findings = analyzeReactiveHelpers(` + const [count] = createSignal(0); + const doubled = createMemo(() => count() * 2); + function computeLabel() { return count() + "!"; } + createEffect(computeLabel, v => console.log(v)); + `); + // The inline memo arrow is a tracked scope; computeLabel is passed to + // createEffect by name, which IS its declaration as a tracked compute. + expect(findings).toHaveLength(0); + }); + + it("uses the type-based heuristic for Accessor-typed parameters", () => { + const findings = analyzeReactiveHelpers(` + function summarize(items: Accessor) { + return items().join(", "); + } + `); + expect(findings).toHaveLength(1); + expect(findings[0].evidence).toEqual(['accessor-typed parameter "items()"']); + }); + + it("does not flag component bodies or tracked scopes created inside them", () => { + const findings = analyzeReactiveHelpers(` + const [count] = createSignal(0); + function Counter() { + createEffect(() => count(), v => console.log(v)); + return "ui"; + } + `); + expect(findings).toHaveLength(0); + }); + + it("attributes inline callback reads to the enclosing helper", () => { + const findings = analyzeReactiveHelpers(` + const [items] = createSignal([1, 2, 3]); + const [factor] = createSignal(2); + function scaled() { + return items().map(x => x * factor()); + } + `); + expect(findings).toHaveLength(1); + expect(findings[0].evidence.join(" ")).toContain('signal read "factor()"'); + }); + + it("reports position and a fix suggestion", () => { + const findings = analyzeReactiveHelpers( + `const [n] = createSignal(1);\nfunction f() { return n(); }` + ); + expect(findings[0].line).toBe(2); + expect(findings[0].message).toContain("createMemo"); + }); +}); From 6d637d9cfda1307d408ff2b270b3ef456ff20a49 Mon Sep 17 00:00:00 2001 From: Brenley Dueck Date: Wed, 19 Aug 2026 00:11:11 -0500 Subject: [PATCH 2/3] refactor(signals): extract attribution hook surface, tag run phases Restructure per review feedback: - Lane/transition truthfulness: recomputeEnd reports optimistic/ transition/held facts (captured before the lane restore), RerunEvent carries phase + held, and wastedMs counts only plain non-held unchanged runs. Overlay time is reported separately as ScopeCost.overlayMs - an optimistic recompute landing back on the committed value is the mechanism working, not waste. - Hook surface instead of weaving: new attribution-hooks.ts defines seven narrow dev-only hook points (recomputeStart/End, derivedChanged, write, refreshed, asyncStart/End) with an install/uninstall slot, following the GlobalQueue._* precedent. Core's obligation is one-line calls with true facts; ALL attribution semantics (stamps, cause collection, dep diffing, timing frames, warnings, costs) live in the engine, which installs itself on enable(). Core no longer imports the engine. The hook surface is the intended devtools substrate - one mechanism, DEV.attribution is its first consumer. - Thresholds: hotRuns default raised 60 -> 120/1000ms, deliberately above animation-frame cadence so a legitimate rAF-driven scope does not cry wolf. All warnings remain opt-in behind enable(). - Lint rule removed from this branch; it is a language-convention discussion (eslint-plugin-solid), not an observability feature. Both dev modules render 0 bytes in prod bundles; suite 1280 green. Co-Authored-By: Claude Fable 5 --- .../solid-signals/lint/reactive-helpers.ts | 256 ------------------ packages/solid-signals/src/core/async.ts | 31 +-- .../src/core/attribution-hooks.ts | 61 +++++ .../solid-signals/src/core/attribution.ts | 184 ++++++++++--- packages/solid-signals/src/core/core.ts | 65 ++--- .../solid-signals/tests/attribution.test.ts | 55 ++++ .../tests/lint-reactive-helpers.test.ts | 112 -------- 7 files changed, 294 insertions(+), 470 deletions(-) delete mode 100644 packages/solid-signals/lint/reactive-helpers.ts create mode 100644 packages/solid-signals/src/core/attribution-hooks.ts delete mode 100644 packages/solid-signals/tests/lint-reactive-helpers.test.ts diff --git a/packages/solid-signals/lint/reactive-helpers.ts b/packages/solid-signals/lint/reactive-helpers.ts deleted file mode 100644 index 7a5b0fa41..000000000 --- a/packages/solid-signals/lint/reactive-helpers.ts +++ /dev/null @@ -1,256 +0,0 @@ -import ts from "typescript"; - -/** - * Prototype static rule: reactive reads in un-annotated helpers. - * - * The runtime hazard this checks for: calling a function inside a tracked - * scope subscribes the CALLER to every signal/store read the function - * performs, but the call site gives no hint — `getUserLabel()` and - * `formatDate(d)` look identical, and only one wires you into the reactive - * graph. This rule makes the declaration site carry that information: any - * function that (transitively) reads reactive state must say so. - * - * A function counts as READING reactive state when it: - * - calls an accessor bound from `createSignal` / `createMemo` / - * `createAsync` (`count()`), - * - accesses a property on a proxy bound from `createStore` / - * `createProjection` (`state.notifications`), - * - calls a parameter typed `Accessor<...>` (the type-based heuristic), or - * - calls another module-local function already classified reactive - * (propagation up the call graph — the `getUserLabel` drift case). - * - * A reactive function is EXEMPT (already declared) when it: - * - has a `@reactive` JSDoc tag, - * - declares a `Reactive<...>` return type (type-level branding), or - * - is passed directly to a tracking primitive (`createMemo`, - * `createEffect`, ...) — being a tracked scope is its declaration. - * - * This is deliberately the "useful 80%": single-module, syntactic, no type - * checker. Cross-module propagation would key off the `Reactive<...>` brand - * in signatures — the brand is what makes the analysis composable. - */ - -export interface LintFinding { - functionName: string; - line: number; // 1-based - column: number; // 1-based - /** Human-readable evidence: what it reads, directly or via which callee. */ - evidence: string[]; - message: string; -} - -const SIGNAL_SOURCES = new Set(["createSignal"]); -const ACCESSOR_SOURCES = new Set(["createMemo", "createAsync"]); -const STORE_SOURCES = new Set(["createStore", "createProjection", "createOptimisticStore"]); -/** Functions whose function-arguments are tracked scopes (exempt as callees). */ -const TRACKING_PRIMITIVES = new Set([ - "createMemo", - "createAsync", - "createEffect", - "createRenderEffect", - "createComputed", - "createReaction", - "mapArray", - "repeat" -]); - -interface FnInfo { - name: string; - node: ts.SignatureDeclaration & { body?: ts.Node }; - directReads: string[]; // evidence strings for direct reactive reads - calls: Set; // names of module-local functions it calls - exempt: boolean; - reactive: boolean; - via: string | null; // callee that made it reactive transitively -} - -function hasReactiveJsDoc(node: ts.Node): boolean { - return ts.getJSDocTags(node).some(tag => tag.tagName.text === "reactive"); -} - -function hasReactiveReturnType(node: ts.SignatureDeclaration): boolean { - const t = node.type; - return ( - t !== undefined && - ts.isTypeReferenceNode(t) && - ts.isIdentifier(t.typeName) && - t.typeName.text === "Reactive" - ); -} - -function isAccessorTypedParam(p: ts.ParameterDeclaration): boolean { - const t = p.type; - return ( - t !== undefined && - ts.isTypeReferenceNode(t) && - ts.isIdentifier(t.typeName) && - (t.typeName.text === "Accessor" || t.typeName.text === "SourceAccessor") - ); -} - -export function analyzeReactiveHelpers(source: string, fileName = "module.tsx"): LintFinding[] { - const sf = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); - - // Pass 1: module-level reactive bindings. - const accessors = new Set(); // count, label, ... - const stores = new Set(); // state, ... - const trackedScopeFns = new Set(); // function args to tracking primitives - - const calleeName = (call: ts.CallExpression): string | null => - ts.isIdentifier(call.expression) ? call.expression.text : null; - - const bindingVisitor = (node: ts.Node): void => { - if ( - ts.isVariableDeclaration(node) && - node.initializer && - ts.isCallExpression(node.initializer) - ) { - const callee = calleeName(node.initializer); - if (callee !== null) { - if (SIGNAL_SOURCES.has(callee) || STORE_SOURCES.has(callee)) { - // const [get, set] = createSignal(...) / const [state, setState] = createStore(...) - if (ts.isArrayBindingPattern(node.name) && node.name.elements.length > 0) { - const first = node.name.elements[0]; - if (ts.isBindingElement(first) && ts.isIdentifier(first.name)) { - (SIGNAL_SOURCES.has(callee) ? accessors : stores).add(first.name.text); - } - } - } else if (ACCESSOR_SOURCES.has(callee) && ts.isIdentifier(node.name)) { - accessors.add(node.name.text); - } - } - } - if (ts.isCallExpression(node)) { - const callee = calleeName(node); - if (callee !== null && TRACKING_PRIMITIVES.has(callee)) { - for (const arg of node.arguments) { - if (ts.isArrowFunction(arg) || ts.isFunctionExpression(arg)) trackedScopeFns.add(arg); - else if (ts.isIdentifier(arg)) trackedScopeFns.add(arg); // marker; resolved by name below - } - } - } - ts.forEachChild(node, bindingVisitor); - }; - bindingVisitor(sf); - - const trackedScopeNames = new Set(); - for (const n of trackedScopeFns) if (ts.isIdentifier(n)) trackedScopeNames.add(n.text); - - // Pass 2: collect module-local functions and their reads/calls. - const fns = new Map(); - - const registerFn = ( - name: string, - fnNode: ts.FunctionDeclaration | ts.ArrowFunction | ts.FunctionExpression, - docHost: ts.Node - ): void => { - const info: FnInfo = { - name, - node: fnNode, - directReads: [], - calls: new Set(), - exempt: - hasReactiveJsDoc(docHost) || - hasReactiveJsDoc(fnNode) || - hasReactiveReturnType(fnNode) || - trackedScopeFns.has(fnNode) || - trackedScopeNames.has(name), - reactive: false, - via: null - }; - const paramAccessors = new Set(); - for (const p of fnNode.parameters) { - if (isAccessorTypedParam(p) && ts.isIdentifier(p.name)) paramAccessors.add(p.name.text); - } - const bodyVisitor = (n: ts.Node): void => { - // Nested callbacks (`.map(x => ...)`) run during the call, so their - // reads belong to this function — but a tracked scope created inside - // (`createEffect(() => ...)` in a component body) owns its own reads. - if (trackedScopeFns.has(n)) return; - if (ts.isCallExpression(n) && ts.isIdentifier(n.expression)) { - const callee = n.expression.text; - if (accessors.has(callee)) info.directReads.push(`signal read "${callee}()"`); - else if (paramAccessors.has(callee)) - info.directReads.push(`accessor-typed parameter "${callee}()"`); - else info.calls.add(callee); - } - if ( - (ts.isPropertyAccessExpression(n) || ts.isElementAccessExpression(n)) && - ts.isIdentifier(n.expression) && - stores.has(n.expression.text) - ) { - const prop = - ts.isPropertyAccessExpression(n) && ts.isIdentifier(n.name) ? `.${n.name.text}` : "[...]"; - info.directReads.push(`store read "${n.expression.text}${prop}"`); - } - ts.forEachChild(n, bodyVisitor); - }; - if (fnNode.body) bodyVisitor(fnNode.body); - fns.set(name, info); - }; - - const fnVisitor = (node: ts.Node): void => { - if (ts.isFunctionDeclaration(node) && node.name !== undefined) { - registerFn(node.name.text, node, node); - } else if (ts.isVariableStatement(node) && node.declarationList.declarations.length === 1) { - const decl = node.declarationList.declarations[0]; - if ( - ts.isIdentifier(decl.name) && - decl.initializer !== undefined && - (ts.isArrowFunction(decl.initializer) || ts.isFunctionExpression(decl.initializer)) - ) { - registerFn(decl.name.text, decl.initializer, node); - } - } - ts.forEachChild(node, fnVisitor); - }; - fnVisitor(sf); - - // Pass 3: fixpoint propagation up the call graph. - let changedInPass = true; - while (changedInPass) { - changedInPass = false; - for (const info of fns.values()) { - if (info.reactive) continue; - if (info.directReads.length > 0) { - info.reactive = true; - changedInPass = true; - continue; - } - for (const callee of info.calls) { - const c = fns.get(callee); - if (c !== undefined && c.reactive) { - info.reactive = true; - info.via = callee; - changedInPass = true; - break; - } - } - } - } - - // Pass 4: report reactive, un-annotated functions. - const findings: LintFinding[] = []; - for (const info of fns.values()) { - if (!info.reactive || info.exempt) continue; - // Components (PascalCase) are exempt: their bodies run untracked by - // convention, and the runtime strict-read diagnostics own that case. - if (/^[A-Z]/.test(info.name)) continue; - const pos = sf.getLineAndCharacterOfPosition(info.node.getStart(sf)); - const evidence = - info.directReads.length > 0 - ? [...new Set(info.directReads)] - : [`calls reactive function "${info.via}()"`]; - findings.push({ - functionName: info.name, - line: pos.line + 1, - column: pos.character + 1, - evidence, - message: - `Function "${info.name}" reads reactive state (${evidence.join("; ")}) but does not ` + - `declare it — every tracked caller silently subscribes to those sources. ` + - `Annotate it /** @reactive */, give it a Reactive<...> return type, or wrap it in createMemo.` - }); - } - return findings.sort((a, b) => a.line - b.line); -} diff --git a/packages/solid-signals/src/core/async.ts b/packages/solid-signals/src/core/async.ts index 137a88987..48bea3755 100644 --- a/packages/solid-signals/src/core/async.ts +++ b/packages/solid-signals/src/core/async.ts @@ -12,7 +12,7 @@ import { STATUS_PENDING, STATUS_UNINITIALIZED } from "./constants.js"; -import { attributionActive, NO_VALUES, stampWrite } from "./attribution.js"; +import { attrHooks } from "./attribution-hooks.js"; import { context, setSignal, untrack } from "./core.js"; import { devTrackHeldPending } from "./invariants.js"; import { emitDiagnostic } from "./dev.js"; @@ -365,9 +365,10 @@ export function handleAsync( clearStatus(el); const lane = resolveLane(el as any); if (lane) lane._pendingAsync.delete(el); - // Attribution: remember the newest stamp before the landing branches so a - // committed change (and only a committed change) can be relabeled below. - const devStampSeq = __DEV__ && attributionActive ? ((el as any)._devChange?.seq ?? 0) : 0; + // Attribution hook: lets the engine snapshot state before the landing + // branches, so it can tell whether the plain path's setSignal committed a + // change (and only then classify it as an async landing). + if (__DEV__ && attrHooks !== null) attrHooks.asyncStart(el); if (setter) { setter(value); if (wasUninitialized) clearStatus(el, true); @@ -393,7 +394,7 @@ export function handleAsync( // re-show an unchanged view — the revert is the notification point. GlobalQueue._syncCompanions !== null && GlobalQueue._syncCompanions(el, value); if (!hasActiveOverride(el)) { - if (__DEV__ && attributionActive) stampWrite(el, "async", NO_VALUES, value); + if (__DEV__ && attrHooks !== null) attrHooks.asyncEnd(el, undefined, value, true); insertSubs(el); } el._time = clock; @@ -425,7 +426,8 @@ export function handleAsync( // rejection (#2837). notifyStatus(el, STATUS_ERROR, e); } - if (__DEV__ && attributionActive && devChanged) stampWrite(el, "async", prevValue, value); + if (__DEV__ && attrHooks !== null && devChanged) + attrHooks.asyncEnd(el, prevValue, value, true); } else { try { setSignal(el, () => value); @@ -434,19 +436,12 @@ export function handleAsync( // pre-commit failure here, and there is no user callsite to throw to. notifyStatus(el, STATUS_ERROR, e); } + // Attribution hook: this path landed through setSignal, whose write + // hook already saw any committed change — direct=false lets the engine + // reclassify that write as an async landing iff it actually committed. + // Outside the try (#2883 — see attribution-hooks.ts). + if (__DEV__ && attrHooks !== null) attrHooks.asyncEnd(el, undefined, value, false); } - // Attribution: the plain path landed through setSignal, which stamps the - // generic "write" kind on a committed change. Relabel it as an async - // landing — but only if setSignal actually stamped (i.e. the value - // changed); a no-change landing must leave no fresh stamp behind, or a - // later unrelated re-run of a subscriber would mis-attribute to it. - if ( - __DEV__ && - attributionActive && - ((el as any)._devChange?.seq ?? 0) > devStampSeq && - (el as any)._devChange.kind === "write" - ) - stampWrite(el, "async", NO_VALUES, value); // First real answer landing: the window closes when the answer becomes // OBSERVABLE. A direct commit is observable now; a transition-held write // (`_pendingValue` set above or inside setSignal) is not — the verdict's diff --git a/packages/solid-signals/src/core/attribution-hooks.ts b/packages/solid-signals/src/core/attribution-hooks.ts new file mode 100644 index 000000000..5afe79a70 --- /dev/null +++ b/packages/solid-signals/src/core/attribution-hooks.ts @@ -0,0 +1,61 @@ +import type { Computed, Signal } from "./types.js"; + +/** + * Dev-only observability hook points for the reactive core. + * + * Core's obligation is to call these with true facts at the moments they + * happen; ALL attribution semantics (stamps, cause chains, timings, warnings) + * live in the engine that installs them (attribution.ts — same pattern as the + * GlobalQueue._* feature slots). `attrHooks` is null unless an engine is + * installed, so the disabled cost is one null check per site, and prod builds + * fold every site out behind __DEV__. + * + * IMPORTANT for implementers of call sites: a hook call must never sit inside + * a `try` block — rollup's tryCatchDeoptimization retains functions referenced + * inside `try` even behind a folded __DEV__ guard, which re-couples the dev + * engine into prod bundles (#2883 harness). Set a local flag inside the try + * and call the hook after the catch. + */ +export interface AttributionHooks { + /** + * A recompute is starting; `el._deps` still holds the previous run's links. + * Always paired with `recomputeEnd` (recompute has no early returns). + */ + recomputeStart(el: Computed, create: boolean): void; + /** + * The recompute finished. `changed` = committed a changed value (false for + * errored runs); `optimistic` = ran under an optimistic lane / lane-dirty + * posture; `transition` = a transition was active or owns this node; + * `held` = the value went to `_pendingValue` (a transition hold) rather + * than committing directly — its reveal happens later on the transition's + * own schedule. + */ + recomputeEnd( + el: Computed, + create: boolean, + changed: boolean, + optimistic: boolean, + transition: boolean, + held: boolean + ): void; + /** A non-effect computed committed a changed value during a re-run. */ + derivedChanged(el: Computed): void; + /** A signal write committed (value passed the equality gate). */ + write(el: Signal | Computed, prev: unknown, value: unknown): void; + /** refresh() invalidated this node (self-invalidation, no dep changed). */ + refreshed(el: Computed): void; + /** An async landing is about to apply its value (before any branch). */ + asyncStart(el: Computed): void; + /** + * The async landing applied. `direct` = the value was committed by this + * landing itself (lane/override paths); false = it went through setSignal, + * whose own `write` hook already saw any committed change. + */ + asyncEnd(el: Computed, prev: unknown, value: unknown, direct: boolean): void; +} + +export let attrHooks: AttributionHooks | null = null; + +export function setAttributionHooks(hooks: AttributionHooks | null): void { + attrHooks = hooks; +} diff --git a/packages/solid-signals/src/core/attribution.ts b/packages/solid-signals/src/core/attribution.ts index 42cf285e2..8f2074184 100644 --- a/packages/solid-signals/src/core/attribution.ts +++ b/packages/solid-signals/src/core/attribution.ts @@ -1,3 +1,4 @@ +import { setAttributionHooks, type AttributionHooks } from "./attribution-hooks.js"; import { $REFRESH } from "./constants.js"; // Cycle note: dev.ts imports this module for the `attribution` object, and we // import its hoisted emitDiagnostic back — safe (only called at runtime) and @@ -19,8 +20,13 @@ import type { Computed, Signal } from "./types.js"; * ← memo "userLabel" changed (#6) * ← signal "notifications" write (#5) 2 → 3 * - * Everything here is dev-only and off by default: with attribution disabled - * the only cost is one boolean check per recompute/write. + * This module is the attribution ENGINE: all semantics live here, and it is + * decoupled from the core. `enable()` installs it into the core's narrow + * dev-only hook points (attribution-hooks.ts); core's only obligation is to + * call those hooks with true facts. Disabled cost is one null check per hook + * site; prod builds fold the sites out entirely. The same hook surface is the + * intended substrate for external consumers (devtools) — one mechanism, two + * front-ends. */ export type ChangeKind = "write" | "derived" | "async" | "refresh"; @@ -64,12 +70,28 @@ export interface RerunEvent { /** Wall time of this run including nested recomputes (ms). */ totalMs: number; /** - * Whether the run committed a changed value. A memo run with + * Whether the run committed a changed value. A PLAIN memo run with * `changed: false` was pure waste — the equality cutoff stopped it from * notifying anyone; an effect run with `changed: false` computed without - * firing its effect phase. Summed as `wastedMs` in costs(). + * firing its effect phase. Summed as `wastedMs` in costs() (plain, + * non-held runs only — see `phase`). */ changed: boolean; + /** + * Which posture this run executed under. "optimistic" = under an + * optimistic lane (overlay recompute); "transition" = a transition was + * active or owns the node (the run may be replayed/settled later); + * "plain" = an ordinary committed run. Overlay runs are real work (they + * count toward time budgets) but are never blamed as waste, and costs() + * reports their time separately as `overlayMs`. + */ + phase: "plain" | "transition" | "optimistic"; + /** + * The changed value was held in `_pendingValue` (a transition hold) rather + * than committed directly; its reveal happens on the transition's own + * schedule. Held runs are excluded from waste accounting. + */ + held: boolean; } export interface AttributionOptions { @@ -81,7 +103,9 @@ export interface AttributionOptions { historyLimit?: number; /** * Hot-scope warning: emit a diagnostic when one scope re-runs `count` - * times within `windowMs` (default 60 runs / 1000ms). `false` disables. + * times within `windowMs` (default 120 runs / 1000ms — deliberately above + * animation-frame cadence, so a legitimate rAF-driven scope at 60/s does + * not cry wolf). `false` disables. */ hotRuns?: { count: number; windowMs: number } | false; /** @@ -112,7 +136,7 @@ interface AttributedNode { _devTimeWarned?: boolean; } -export let attributionActive = false; +let attributionActive = false; let changeSeq = 0; let runSeq = 0; @@ -120,7 +144,7 @@ const defaultOptions = { log: true, stacks: false, historyLimit: 200, - hotRuns: { count: 60, windowMs: 1000 } as { count: number; windowMs: number } | false, + hotRuns: { count: 120, windowMs: 1000 } as { count: number; windowMs: number } | false, wideDeps: 30 as number | false, hotTime: { budgetMs: 8, windowMs: 1000 } as { budgetMs: number; windowMs: number } | false }; @@ -131,25 +155,17 @@ let history: RerunEvent[] = []; const now: () => number = typeof performance !== "undefined" ? () => performance.now() : () => Date.now(); -// Self-time bookkeeping: recomputes nest (pulls, child creation inside a -// parent's fn), so each frame tracks the time its children consumed and the -// parent subtracts it — the same discipline every profiler uses. -const timeStarts: number[] = []; -const childTimes: number[] = []; - -/** Open a timing frame for a recompute. Must be paired with endTiming(). */ -export function beginTiming(): void { - timeStarts.push(now()); - childTimes.push(0); -} - -/** Close the current timing frame; credits total time to the parent frame. */ -export function endTiming(): { selfMs: number; totalMs: number } { - const totalMs = now() - timeStarts.pop()!; - const childMs = childTimes.pop()!; - if (childTimes.length > 0) childTimes[childTimes.length - 1] += totalMs; - return { selfMs: Math.max(0, totalMs - childMs), totalMs }; +// Per-recompute frames: recomputes nest (pulls, child creation inside a +// parent's fn), so each frame carries the causes/prev-deps snapshot from +// recomputeStart plus the time its children consumed — the parent subtracts +// child time for honest self-time, the same discipline every profiler uses. +interface RunFrame { + start: number; + childMs: number; + causes: ChangeRecord[] | null; // null on create runs + prevDeps: unknown[] | null; } +const frames: RunFrame[] = []; // Cost aggregates, reset on enable()/disable(). export interface ScopeCost { @@ -157,8 +173,15 @@ export interface ScopeCost { kind: "effect" | "memo"; runs: number; selfMs: number; - /** Self-time of runs that produced an unchanged value — recoverable. */ + /** + * Self-time of PLAIN, non-held runs that produced an unchanged value — + * the recoverable number. Overlay runs (optimistic/transition) are never + * counted here: an optimistic recompute landing back on the committed + * value is the mechanism working, not waste. + */ wastedMs: number; + /** Self-time spent in optimistic/transition (overlay) runs. */ + overlayMs: number; } export interface WriteCost { /** Root cause name (a signal write, async landing, or refresh target). */ @@ -181,12 +204,20 @@ function rootsOf(causes: ChangeRecord[], out: Set): void { function recordCosts(event: RerunEvent): void { let scope = scopeCosts.get(event.node); if (scope === undefined) { - scope = { name: event.nodeName, kind: event.nodeKind, runs: 0, selfMs: 0, wastedMs: 0 }; + scope = { + name: event.nodeName, + kind: event.nodeKind, + runs: 0, + selfMs: 0, + wastedMs: 0, + overlayMs: 0 + }; scopeCosts.set(event.node, scope); } scope.runs++; scope.selfMs += event.selfMs; - if (!event.changed) scope.wastedMs += event.selfMs; + if (event.phase !== "plain") scope.overlayMs += event.selfMs; + else if (!event.changed && !event.held) scope.wastedMs += event.selfMs; const roots = new Set(); rootsOf(event.causes, roots); for (const name of roots) { @@ -234,10 +265,10 @@ function captureStack(): string[] | undefined { } /** Sentinel for "no value transition to record" (refresh() stamps). */ -export const NO_VALUES = Symbol("no-values"); +const NO_VALUES = Symbol("no-values"); /** Record a root change (setSignal / refresh / async landing) on the node. */ -export function stampWrite( +function stampWrite( node: Signal | Computed, kind: Exclude, prev: unknown = NO_VALUES, @@ -253,7 +284,7 @@ export function stampWrite( } /** Record a derived change (memo produced a new value) with its causes. */ -export function stampDerived(node: Computed, causes: ChangeRecord[]): void { +function stampDerived(node: Computed, causes: ChangeRecord[]): void { (node as AttributedNode)._devChange = { seq: ++changeSeq, kind: "derived", @@ -268,7 +299,7 @@ export function stampDerived(node: Computed, causes: ChangeRecord[]): void * run's links. A refresh() stamp on the node itself also counts — that is a * self-invalidation, not a dep change. */ -export function collectCauses(el: Computed): ChangeRecord[] { +function collectCauses(el: Computed): ChangeRecord[] { const seen = (el as AttributedNode)._devSeenSeq ?? 0; const causes: ChangeRecord[] = []; const self = (el as AttributedNode)._devChange; @@ -281,12 +312,12 @@ export function collectCauses(el: Computed): ChangeRecord[] { } /** Advance the node's seen-cursor to the present. Call after every run. */ -export function markSeen(el: Computed): void { +function markSeen(el: Computed): void { (el as AttributedNode)._devSeenSeq = changeSeq; } /** Snapshot the node's current dep identities (call before a run replaces them). */ -export function captureDeps(el: Computed): unknown[] { +function captureDeps(el: Computed): unknown[] { const deps: unknown[] = []; for (let l = el._deps; l !== null; l = l._nextDep) deps.push(l._dep); return deps; @@ -298,7 +329,7 @@ export function captureDeps(el: Computed): unknown[] { * recordRerun for re-runs and directly from recompute for creation runs (a * memo can be born too wide). Re-warns only on 50% further growth. */ -export function checkDepWidth(el: Computed): void { +function checkDepWidth(el: Computed): void { const limit = options.wideDeps; if (limit === false) return; let count = 0; @@ -403,12 +434,14 @@ function checkHotTime(el: Computed, event: RerunEvent): void { console.warn(message); } -export function recordRerun( +function recordRerun( el: Computed, causes: ChangeRecord[], prevDeps: unknown[], timing: { selfMs: number; totalMs: number }, - changed: boolean + changed: boolean, + phase: "plain" | "transition" | "optimistic", + held: boolean ): void { const node = el as AttributedNode; // Subscription diff: `prevDeps` was captured at run entry; `_deps` now @@ -433,7 +466,9 @@ export function recordRerun( depsRemoved, selfMs: timing.selfMs, totalMs: timing.totalMs, - changed + changed, + phase, + held }; history.push(event); if (history.length > options.historyLimit) history.shift(); @@ -461,7 +496,8 @@ function formatCause(cause: ChangeRecord, depth: number, out: string[]): void { export function formatRerun(event: RerunEvent): string { const out = [ `[why-run] ${event.nodeKind} "${event.nodeName}" ran (run ${event.nodeRuns}, ` + - `${event.selfMs.toFixed(2)}ms${event.changed ? "" : ", unchanged"})` + + `${event.selfMs.toFixed(2)}ms${event.changed ? "" : ", unchanged"}` + + `${event.phase === "plain" ? "" : `, ${event.phase}`}${event.held ? ", held" : ""})` + (event.causes.length === 0 ? " — no tracked cause (pull or retry)" : "") ]; for (const cause of event.causes) formatCause(cause, 0, out); @@ -493,23 +529,85 @@ export interface Attribution { format: typeof formatRerun; } +// The engine's implementation of the core's dev hook points. Installed by +// enable(), uninstalled by disable() — while uninstalled the core pays one +// null check per site and nothing else. +let asyncStartSeq = 0; +const engineHooks: AttributionHooks = { + recomputeStart(el, create) { + frames.push({ + start: now(), + childMs: 0, + causes: create ? null : collectCauses(el), + prevDeps: create ? null : captureDeps(el) + }); + }, + derivedChanged(el) { + const frame = frames[frames.length - 1]; + stampDerived(el, frame !== undefined && frame.causes !== null ? frame.causes : []); + }, + recomputeEnd(el, _create, changed, optimistic, transition, held) { + const frame = frames.pop(); + // enable() can land mid-recompute: no opening frame, nothing to report. + if (frame === undefined) return; + const totalMs = now() - frame.start; + if (frames.length > 0) frames[frames.length - 1].childMs += totalMs; + const selfMs = Math.max(0, totalMs - frame.childMs); + if (frame.causes !== null) + recordRerun( + el, + frame.causes, + frame.prevDeps!, + { selfMs, totalMs }, + changed, + optimistic ? "optimistic" : transition ? "transition" : "plain", + held + ); + // Creation runs still get the wide-scope check: a memo can be born with + // its coarse-read problem already in place. + else checkDepWidth(el); + markSeen(el); + }, + write(el, prev, value) { + stampWrite(el, "write", prev, value); + }, + refreshed(el) { + stampWrite(el, "refresh"); + }, + asyncStart(el) { + asyncStartSeq = (el as AttributedNode)._devChange?.seq ?? 0; + }, + asyncEnd(el, prev, value, direct) { + if (direct) { + stampWrite(el, "async", prev === undefined ? NO_VALUES : prev, value); + return; + } + // Landed through setSignal: reclassify its "write" stamp as an async + // landing — but only if it actually stamped (the value changed) since + // asyncStart; a no-change landing must leave no fresh stamp behind. + const change = (el as AttributedNode)._devChange; + if (change !== undefined && change.seq > asyncStartSeq && change.kind === "write") + stampWrite(el, "async", NO_VALUES, value); + } +}; + export const attribution: Attribution = { enable(opts?: AttributionOptions) { options = { ...defaultOptions, ...opts }; attributionActive = true; - timeStarts.length = 0; - childTimes.length = 0; + frames.length = 0; scopeCosts.clear(); writeCosts.clear(); + setAttributionHooks(engineHooks); }, disable() { attributionActive = false; listeners.clear(); history = []; - timeStarts.length = 0; - childTimes.length = 0; + frames.length = 0; scopeCosts.clear(); writeCosts.clear(); + setAttributionHooks(null); }, subscribe(listener) { listeners.add(listener); diff --git a/packages/solid-signals/src/core/core.ts b/packages/solid-signals/src/core/core.ts index 3abfcb018..d97a56801 100644 --- a/packages/solid-signals/src/core/core.ts +++ b/packages/solid-signals/src/core/core.ts @@ -57,19 +57,7 @@ import { throwPendingUntrackedRead, warnStrictReadUntracked } from "./dev.js"; -import { - attributionActive, - beginTiming, - captureDeps, - checkDepWidth, - collectCauses, - endTiming, - markSeen, - recordRerun, - stampDerived, - stampWrite, - type ChangeRecord -} from "./attribution.js"; +import { attrHooks } from "./attribution-hooks.js"; import { devTrackHeldPending } from "./invariants.js"; import { cleanup, disposeChildren, inheritId, markDisposal } from "./owner.js"; import { @@ -183,22 +171,11 @@ export function clearSnapshots(): void { export function recompute(el: Computed, create: boolean = false): void { const isEffect = (el as any)._type; - // Attribution snapshot must happen before this run touches the dep list: - // `_deps` still holds the previous run's links, which are exactly the - // subscriptions that could have triggered this run — and the baseline for - // the subscription diff after the run. - let devCauses: ChangeRecord[] | null = null; - let devPrevDeps: unknown[] | null = null; + // Attribution hook: fired before this run touches the dep list — `_deps` + // still holds the previous run's links (the subscriptions that could have + // triggered this run, and the baseline for the engine's subscription diff). let devChanged = false; - if (__DEV__ && attributionActive) { - // Every recompute opens a timing frame (create runs included) so nested - // computes are subtracted from the parent's self-time. - beginTiming(); - if (!create) { - devCauses = collectCauses(el); - devPrevDeps = captureDeps(el); - } - } + if (__DEV__ && attrHooks !== null) attrHooks.recomputeStart(el, create); if (!create) { if (el._transition && (!isEffect || activeTransition) && activeTransition !== el._transition) globalQueue.initTransition(el._transition); @@ -381,9 +358,9 @@ export function recompute(el: Computed, create: boolean = false): void { // A committed derived change becomes a cause for this node's subscribers, // chaining their attribution through this node to the root write. - if (__DEV__ && attributionActive) { + if (__DEV__ && attrHooks !== null) { devChanged = valueChanged && !el._error; - if (devCauses !== null && devChanged && !isEffect) stampDerived(el, devCauses); + if (devChanged && !isEffect && !create) attrHooks.derivedChanged(el); } // Effects use `_equals: false` (no per-effect closure). The side effects that @@ -470,14 +447,20 @@ export function recompute(el: Computed, create: boolean = false): void { if (outgoingError !== undefined && !valueChanged && !el._error) settleErroredDependents(el, outgoingError); } - if (__DEV__ && attributionActive) { - const devTiming = endTiming(); - if (devCauses !== null) recordRerun(el, devCauses, devPrevDeps!, devTiming, devChanged); - // Creation runs still get the wide-scope check: a memo can be born with - // its coarse-read problem already in place. - else checkDepWidth(el); - markSeen(el); - } + // Attribution hook: fired before the lane restore so `currentOptimisticLane` + // still reflects THIS run's posture. The facts distinguish an overlay + // recompute (optimistic lane, transition replay, transition-held commit) + // from a plain committed one — the engine must not blame overlay runs as + // waste or double-count them against plain aggregates. + if (__DEV__ && attrHooks !== null) + attrHooks.recomputeEnd( + el, + create, + devChanged, + isOptimisticDirty || currentOptimisticLane !== null, + activeTransition !== null || el._transition !== null, + el._pendingValue !== NOT_PENDING + ); currentOptimisticLane = prevLane; const needsPendingCommit = el._pendingValue !== NOT_PENDING || @@ -1090,8 +1073,8 @@ export function setSignal(el: Signal | Computed, v: T | ((prev: T) => T !el._equals(currentValue, v); if (!valueChanged) return v; - // Root cause for attribution: this write is where a re-run chain begins. - if (__DEV__ && attributionActive) stampWrite(el, "write", currentValue, v); + // Attribution hook: this committed write is where a re-run chain begins. + if (__DEV__ && attrHooks !== null) attrHooks.write(el, currentValue, v); if (el._pendingValue === NOT_PENDING) queuePendingNode(el); el._pendingValue = v; @@ -1266,7 +1249,7 @@ export function refresh(target: Refreshable): void { node._flags = (node._flags & ~REACTIVE_CHECK) | REACTIVE_DIRTY; // A refresh() self-invalidation is a root cause too — the target's next // run has no changed dep to point at, so it points here instead. - if (__DEV__ && attributionActive) stampWrite(node, "refresh"); + if (__DEV__ && attrHooks !== null) attrHooks.refreshed(node); insertIntoHeap(node, queueFor(node)); schedule(); } diff --git a/packages/solid-signals/tests/attribution.test.ts b/packages/solid-signals/tests/attribution.test.ts index c63f4f533..d4da9ec4c 100644 --- a/packages/solid-signals/tests/attribution.test.ts +++ b/packages/solid-signals/tests/attribution.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createEffect, createMemo, + createOptimistic, createRoot, createSignal, createStore, @@ -448,6 +449,60 @@ describe("why-did-this-run attribution", () => { expect(warn).toHaveBeenCalledTimes(1); }); + it("tags optimistic runs with their phase and never blames them as waste", () => { + const spin = (ms: number) => { + const end = performance.now() + ms; + while (performance.now() < end); + }; + const [x, setX] = createOptimistic(1, { name: "opt" }); + createRoot(() => + createEffect( + () => { + spin(3); + return x(); + }, + () => {}, + { name: "opt-effect" } + ) + ); + flush(); + + const events = collect({ hotRuns: false, hotTime: false }); + setX(2); + flush(); + + const runs = events.filter(e => e.nodeName === "opt-effect"); + expect(runs.length).toBeGreaterThanOrEqual(1); + // Every run under the optimistic write is tagged as overlay work. + for (const run of runs) expect(run.phase).not.toBe("plain"); + + const { scopes } = DEV!.attribution.costs(); + const scope = scopes.find(s => s.name === "opt-effect")!; + expect(scope.overlayMs).toBeGreaterThan(0); + expect(scope.wastedMs).toBe(0); // overlay runs are never waste + expect(scope.selfMs).toBeGreaterThanOrEqual(scope.overlayMs); + }); + + it("keeps plain runs untagged", () => { + const [n, setN] = createSignal(0, { name: "n" }); + createRoot(() => + createEffect( + () => n(), + () => {}, + { name: "plain-effect" } + ) + ); + flush(); + + const events = collect(); + setN(1); + flush(); + + const run = events.find(e => e.nodeName === "plain-effect")!; + expect(run.phase).toBe("plain"); + expect(run.held).toBe(false); + }); + it("is inert when disabled", () => { const [n, setN] = createSignal(0, { name: "n" }); createRoot(() => diff --git a/packages/solid-signals/tests/lint-reactive-helpers.test.ts b/packages/solid-signals/tests/lint-reactive-helpers.test.ts deleted file mode 100644 index 42762643e..000000000 --- a/packages/solid-signals/tests/lint-reactive-helpers.test.ts +++ /dev/null @@ -1,112 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { analyzeReactiveHelpers } from "../lint/reactive-helpers.js"; - -describe("lint: reactive reads in un-annotated helpers", () => { - it("flags a helper that reads a store (the getUserLabel drift)", () => { - const findings = analyzeReactiveHelpers(` - const [state, setState] = createStore({ - user: { name: "Ada" }, - notifications: [] - }); - - function getUserLabel() { - const badge = state.notifications.length > 0 ? " •" : ""; - return state.user.name + badge; - } - `); - expect(findings).toHaveLength(1); - expect(findings[0].functionName).toBe("getUserLabel"); - expect(findings[0].evidence.join(" ")).toContain('store read "state.notifications"'); - expect(findings[0].message).toContain("silently subscribes"); - }); - - it("flags signal-accessor reads in plain helpers, including arrow consts", () => { - const findings = analyzeReactiveHelpers(` - const [count, setCount] = createSignal(0); - const doubled = () => count() * 2; - function formatDate(d: Date) { return d.toISOString(); } - `); - expect(findings.map(f => f.functionName)).toEqual(["doubled"]); - expect(findings[0].evidence).toEqual(['signal read "count()"']); - }); - - it("propagates reactivity up the call graph", () => { - const findings = analyzeReactiveHelpers(` - const [n] = createSignal(1); - function inner() { return n(); } - function middle() { return inner() + 1; } - function outer() { return middle() * 2; } - function unrelated() { return 42; } - `); - expect(findings.map(f => f.functionName).sort()).toEqual(["inner", "middle", "outer"]); - const outer = findings.find(f => f.functionName === "outer")!; - expect(outer.evidence).toEqual(['calls reactive function "middle()"']); - }); - - it("accepts the declaration sites: @reactive JSDoc and Reactive<> return type", () => { - const findings = analyzeReactiveHelpers(` - const [count] = createSignal(0); - - /** @reactive */ - function labeled() { return count(); } - - const branded = (): Reactive => count() * 2; - - function undeclared() { return count(); } - `); - expect(findings.map(f => f.functionName)).toEqual(["undeclared"]); - }); - - it("exempts functions passed directly to tracking primitives", () => { - const findings = analyzeReactiveHelpers(` - const [count] = createSignal(0); - const doubled = createMemo(() => count() * 2); - function computeLabel() { return count() + "!"; } - createEffect(computeLabel, v => console.log(v)); - `); - // The inline memo arrow is a tracked scope; computeLabel is passed to - // createEffect by name, which IS its declaration as a tracked compute. - expect(findings).toHaveLength(0); - }); - - it("uses the type-based heuristic for Accessor-typed parameters", () => { - const findings = analyzeReactiveHelpers(` - function summarize(items: Accessor) { - return items().join(", "); - } - `); - expect(findings).toHaveLength(1); - expect(findings[0].evidence).toEqual(['accessor-typed parameter "items()"']); - }); - - it("does not flag component bodies or tracked scopes created inside them", () => { - const findings = analyzeReactiveHelpers(` - const [count] = createSignal(0); - function Counter() { - createEffect(() => count(), v => console.log(v)); - return "ui"; - } - `); - expect(findings).toHaveLength(0); - }); - - it("attributes inline callback reads to the enclosing helper", () => { - const findings = analyzeReactiveHelpers(` - const [items] = createSignal([1, 2, 3]); - const [factor] = createSignal(2); - function scaled() { - return items().map(x => x * factor()); - } - `); - expect(findings).toHaveLength(1); - expect(findings[0].evidence.join(" ")).toContain('signal read "factor()"'); - }); - - it("reports position and a fix suggestion", () => { - const findings = analyzeReactiveHelpers( - `const [n] = createSignal(1);\nfunction f() { return n(); }` - ); - expect(findings[0].line).toBe(2); - expect(findings[0].message).toContain("createMemo"); - }); -}); From b11702efba80b20ecfc0ea50cc3f555e68209527 Mon Sep 17 00:00:00 2001 From: Brenley Dueck Date: Wed, 19 Aug 2026 07:55:07 -0500 Subject: [PATCH 3/3] fix(signals): gate store node naming on the attribution engine Store property-node naming ran on every node creation in dev builds, engine installed or not - the one place the attribution work broke its own "one null check when disabled" discipline, on the hottest store path. Gate the name on attrHooks !== null. Tradeoff, flagged for review: the property key is NOT recoverable from the node at format time (it lives only in the node's unobserved() closure), so fully lazy derivation would need a dev-only field on the node. With the gate, nodes created before enable() stay generically named - enable attribution before creating state for full path names. If late-enable path names matter (attach-to-running-app debugging), the field is the middle ground: one monomorphic write per store node creation in dev, concat deferred to format time. Co-Authored-By: Claude Fable 5 --- packages/solid-signals/src/store/store.ts | 11 ++++++++--- packages/solid-signals/tests/attribution.test.ts | 5 ++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/solid-signals/src/store/store.ts b/packages/solid-signals/src/store/store.ts index d1541f9d6..7963da765 100644 --- a/packages/solid-signals/src/store/store.ts +++ b/packages/solid-signals/src/store/store.ts @@ -1,3 +1,4 @@ +import { attrHooks } from "../core/attribution-hooks.js"; import { STATUS_ERROR, STATUS_PENDING, @@ -502,9 +503,13 @@ function getNode( const s = signal( value, { - // Dev-only: name store property nodes by path segment so attribution - // chains and wide-scope warnings read "store.todos", not "signal". - name: __DEV__ ? "store." + String(property) : undefined, + // Attribution-only: name store property nodes by path segment so + // attribution chains and wide-scope warnings read "store.todos", not + // "signal". Gated on the engine being installed — node creation is the + // hottest store path, and the disabled cost must stay one null check + // (nodes created before enable() stay generically named; enable + // attribution before creating state for full path names). + name: __DEV__ && attrHooks !== null ? "store." + String(property) : undefined, equals: equals, unobserved() { if (nodes[property] === s) { diff --git a/packages/solid-signals/tests/attribution.test.ts b/packages/solid-signals/tests/attribution.test.ts index d4da9ec4c..fab939231 100644 --- a/packages/solid-signals/tests/attribution.test.ts +++ b/packages/solid-signals/tests/attribution.test.ts @@ -318,6 +318,10 @@ describe("why-did-this-run attribution", () => { }); it("names store property nodes by path in attribution output", () => { + // Store node naming is gated on the engine being installed (node + // creation is the hottest store path), so enable BEFORE the first read + // creates the property nodes. + const events = collect(); const [state, setState] = createStore({ count: 1, other: "x" }); createRoot(() => createEffect( @@ -328,7 +332,6 @@ describe("why-did-this-run attribution", () => { ); flush(); - const events = collect(); setState(s => { s.count = 2; });