diff --git a/docs/variable-store-plan.md b/docs/variable-store-plan.md new file mode 100644 index 0000000..d5dc978 --- /dev/null +++ b/docs/variable-store-plan.md @@ -0,0 +1,129 @@ +# Variable store — outstanding fixes + +Notes from an audit of the client-side variable / tag reactivity system: + +- `src/components/journal/var-reactivity.ts` — engine (dependency graph, base/mod + separation, threshold-gated tag activation, cascade). +- `src/components/journal/variable-expression.ts` — expression evaluator. +- `src/components/journal/command-dispatcher.ts` — `/set`, `/rolltag`, cascade. +- `src/components/journal/completions.ts` — init + seed. +- `src/components/stores/journalStream.ts` — replay + hydration rebuild. +- `src/components/journal/types/var.tsx` — `var` message + reducer. + +## Already fixed (context) + +Commit `a26d53c` (`fix(journal): keep variable bases in the stream`): + +- Hydration double-counted active tag modifiers because the stream carried only + combined values. `var` messages now carry `base` alongside `value`, the store + keeps `variableBases`, and `rebuildReactivityFromStore` restores exact bases. +- Seeding ran at import against an empty store (clobbering persisted values and + appending duplicate `var` messages on every load). It now runs after hydration + and only for keys absent from the store. + +The items below are still open. + +## Open issues + +### 1. Modifier and declaration errors are swallowed — ✅ fixed + +- **Where:** `computeInitialValues` (`catch {}`, "skip failed evaluations at init + time") and `applyTagMapActivations` (`catch {}`, "skip failed modifier") in + `var-reactivity.ts`; `initReactivity` throws are reduced to `console.warn` in + `completions.ts`. +- **Symptom:** a typo'd declaration or modifier expression silently yields a + wrong value (often `0`) with no author-facing feedback. +- **Fixed:** the engine now collects `ReactivityDiagnostic` entries for failed + declarations (`computeInitialValues`, `reevaluateDependents`), failed modifier + expressions (`applyTagMapActivations`), and circular declarations + (`initReactivity`). They are deduped, logged via `console.warn` with + key/expression/tag context, cleared on each `initReactivity`, and bridged into + a signal exposed as `useJournalCompletions().diagnostics()`. `VariableView` + renders them as a warning banner. A bad declaration no longer aborts the rest + of a cascade. + +### 2. `var del` does not reach the engine — medium + +- **Where:** reducer in `types/var.tsx` supports `action: "del"`; the parser + (`command-parser.ts`) has no `/del`; `var-reactivity.ts` never clears + `baseValues` and `rebuildReactivityFromStore` never removes stale keys. +- **Symptom:** a deleted variable still resolves locally via `getCombined`, + because it reads `baseValues` first. +- **Fix:** either wire a `/del` command and clear engine state (base + mods) on + delete, or drop `del` from the schema/reducer if it is intentionally unused. + +### 3. Reactivity is initialized once, at import — medium + +- **Where:** `bootstrapReactivity` in `completions.ts` runs `initReactivity` + once; nothing re-runs it when declare blocks change or a session/document is + switched. +- **Also:** `DispatchContext.declarations` / `tagModifiers` + (`command-dispatcher.ts`) are declared but never read — a vestige of a + per-dispatch init that was never wired up. +- **Symptom:** editing `role=declare` content requires a full reload; stale + graph/expressions persist. +- **Fix:** expose a re-init entry point keyed to content/session changes, and + either use or remove the unused context fields. + +### 4. `lookup` contract disagrees with its docs — low + +- **Where:** `EvalContext.lookup` documents "Resolve `$var`", but + `parseFactor` calls `ctx.lookup(varName.slice(1))` (bare name), and every + caller re-adds `$`. +- **Symptom:** a caller following the doc would key variables with `$` and + silently get `0`s. +- **Fix:** update the doc comment to state the argument is the bare name (no + `$`), or stop stripping in `parseFactor` and update callers. + +### 5. Undefined variables evaluate to `0` — low + +- **Where:** `parseFactor` (`variable-expression.ts`) returns a numeric `0` for + an unresolved `$var`. +- **Symptom:** `/set $hp $cn*5` (typo) silently sets `0` instead of failing. +- **Fix:** consider erroring on unknown variables in `/set` context while + keeping lenient behavior for display/roll contexts, or warn. + +### 6. Tokenizer accepts malformed numbers — ✅ fixed + +- **Where:** the number branch of `tokenize` consumes `[0-9.]` greedily, so + `1.2.3` becomes one "number" and `parseFloat` truncates it. +- **Fixed:** the tokenizer now validates the scanned chunk against + `^\d+(\.\d+)?$` and throws `Invalid number literal: "…"` instead of pushing a + silently-truncated value. The error propagates through the diagnostics channel + (#1) for declarations/modifiers and through `dispatchError` for `/set` and + `/roll`. + +### 7. `topoSortAffected` re-evaluates untouched declarations — low + +- **Where:** in `var-reactivity.ts`, the visit condition + `affected.has(dep) || declExprs.has(dep)` pulls declared dependencies that are + not in `affected` into the result. +- **Symptom:** `reevaluateDependents` re-evaluates and re-publishes variables + that did not change. Mostly harmless, but noisy. +- **Fix:** only visit/topologically order keys in `affected`. + +### 8. Failed `initReactivity` leaves partial state — low + +- **Where:** `initReactivity` populates `depGraph` / `declExprs` / `tagModMap` + before `checkCircular()` throws; `inFlight` is never cleared on re-init. +- **Fix:** build into locals and commit on success, or roll back on failure. + Clear `inFlight` in `initReactivity`. + +### 9. Residual base / tagmap-target edges — low + +- **Base fallback:** `rebuildReactivityFromStore` falls back to the combined + `value` when a message has no `base`, which double-counts mods. In-repo + emitters always send `base`; only hand-crafted/external `var` messages hit + this. +- **Tagmap targets:** rebuild derives activations from each source's *base* + tagmap (this fixed a feedback loop where a tagmap-valued target re-triggered + its own modifier on every rebuild). If a variable's effective count crosses a + threshold purely via mods, that activation is not reconstructed on hydration. + Revisit if tagmap-valued modifier targets come into use. + +## Suggested order + +1. #3 (re-init) and #2 (`del`) — correctness of engine lifecycle. +2. #1 (diagnostics) — authoring feedback. +3. #4–#8 — cleanup and hardening. +4. #9 — only if the tagmap-target pattern is adopted. diff --git a/src/cli/completions/variable-system.test.ts b/src/cli/completions/variable-system.test.ts index cff1e37..2575aac 100644 --- a/src/cli/completions/variable-system.test.ts +++ b/src/cli/completions/variable-system.test.ts @@ -40,6 +40,7 @@ import { getDeclExpr, extractDependencies, rebuildReactivityFromStore, + getReactivityDiagnostics, } from "../../components/journal/var-reactivity"; import { parseInput } from "../../components/journal/command-parser"; import { @@ -396,6 +397,15 @@ describe("evaluateExpression", () => { ).toThrow("Unexpected end"); }); + test("throws on malformed number literals", () => { + expect(() => + evaluateExpression("1.2.3", { lookup: () => undefined }), + ).toThrow("Invalid number literal"); + expect(() => + evaluateExpression("1.", { lookup: () => undefined }), + ).toThrow("Invalid number literal"); + }); + test("handles decimal numbers", () => { const result = evaluateExpression("3.5 + 2.5", { lookup: () => undefined }); expect(result.value).toEqual({ kind: "number", value: 6 }); @@ -574,6 +584,113 @@ describe("var-reactivity", () => { }); }); + describe("diagnostics", () => { + test("reports a declaration that fails to evaluate", () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + initReactivity({ + declarations: [ + { key: "$hp", expression: "10 + 5" }, + { key: "$bad", expression: "#warrior * 2" }, + ], + tagModifiers: [], + }); + + const results = computeInitialValues(store()); + expect(results.find((r) => r.key === "$hp")?.value).toBe("15"); + expect(results.find((r) => r.key === "$bad")).toBeUndefined(); + + const diags = getReactivityDiagnostics(); + expect(diags).toHaveLength(1); + expect(diags[0].kind).toBe("declaration"); + expect(diags[0].key).toBe("$bad"); + expect(diags[0].expression).toBe("#warrior * 2"); + warn.mockRestore(); + }); + + test("dedupes repeated failures of the same expression", () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + initReactivity({ + declarations: [{ key: "$bad", expression: "#warrior * 2" }], + tagModifiers: [], + }); + + computeInitialValues(store()); + computeInitialValues(store()); + expect(getReactivityDiagnostics()).toHaveLength(1); + warn.mockRestore(); + }); + + test("clears diagnostics on re-init", () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + initReactivity({ + declarations: [{ key: "$bad", expression: "#warrior * 2" }], + tagModifiers: [], + }); + computeInitialValues(store()); + expect(getReactivityDiagnostics()).toHaveLength(1); + + initReactivity({ declarations: [], tagModifiers: [] }); + expect(getReactivityDiagnostics()).toHaveLength(0); + warn.mockRestore(); + }); + + test("records a diagnostic when declarations are circular", () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + expect(() => + initReactivity({ + declarations: [ + { key: "$a", expression: "$b + 1" }, + { key: "$b", expression: "$a + 1" }, + ], + tagModifiers: [], + }), + ).toThrow(); + + const diags = getReactivityDiagnostics(); + expect(diags).toHaveLength(1); + expect(diags[0].kind).toBe("init"); + warn.mockRestore(); + }); + + test("reports a tag modifier that fails to evaluate", () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + initReactivity({ + declarations: [], + tagModifiers: [ + { tag: "#warrior", target: "$hp", expression: "#x * 2", threshold: 1 }, + ], + }); + + setBase("$class", "#warrior:1"); + computeCascade("$class", undefined, store({ $class: "#warrior:1" })); + + const diags = getReactivityDiagnostics(); + expect(diags).toHaveLength(1); + expect(diags[0].kind).toBe("modifier"); + expect(diags[0].key).toBe("$hp"); + expect(diags[0].tag).toBe("#warrior"); + expect(diags[0].source).toBe("$class"); + warn.mockRestore(); + }); + + test("keeps cascading other dependents when a declaration fails", () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + initReactivity({ + declarations: [ + { key: "$bad", expression: "$con * #warrior" }, + { key: "$good", expression: "$con + 1" }, + ], + tagModifiers: [], + }); + + const cascade = computeCascade("$con", undefined, store({ $con: "5" })); + expect(cascade.find((r) => r.key === "$good")?.value).toBe("6"); + expect(cascade.find((r) => r.key === "$bad")).toBeUndefined(); + expect(getReactivityDiagnostics().some((d) => d.key === "$bad")).toBe(true); + warn.mockRestore(); + }); + }); + describe("getCombined / setBase", () => { test("returns base value when no mods", () => { initReactivity({ declarations: [], tagModifiers: [] }); diff --git a/src/components/journal/VariableView.tsx b/src/components/journal/VariableView.tsx index 5ae7910..288e296 100644 --- a/src/components/journal/VariableView.tsx +++ b/src/components/journal/VariableView.tsx @@ -17,6 +17,9 @@ export const VariableView: Component = () => { const values = createMemo(() => stream.variables); + /** Runtime engine diagnostics (bad declarations / modifiers). */ + const diagnostics = createMemo(() => comp.diagnostics()); + /** Flat list of all variables */ const items = createMemo(() => { const vars = values(); @@ -32,6 +35,20 @@ export const VariableView: Component = () => { return (
+ 0}> +
+
+ 变量求值有 {diagnostics().length} 个问题 +
+
    + + {(d) => ( +
  • {d.message}
    • + )} +     +
+
+ ({ status: "loading", }); +// Runtime diagnostics from the variable engine (bad declarations, modifier +// expressions, circular deps). Bridged into a signal so components can render +// them; the engine itself stays Solid-free. +const [reactivityDiagnostics, setReactivityDiagnostics] = createSignal< + ReactivityDiagnostic[] +>(getReactivityDiagnostics()); +onReactivityDiagnostics((list) => setReactivityDiagnostics(list)); + // The registry backing the completions. Populated in both CLI and client // modes so inline content ids can be resolved at runtime (e.g. spark rolls). let activeRegistry: ContentRegistry = { pathIndex: {}, docContent: {} }; @@ -225,13 +233,16 @@ export function ensureCompletions(): Promise { export function useJournalCompletions(): { state: CompletionsState; data: JournalCompletions; + /** Reactive accessor for runtime variable-engine diagnostics. */ + diagnostics: () => ReactivityDiagnostic[]; } { const s = completionsState(); if (s.status === "loaded") { - return { state: s, data: s.data }; + return { state: s, data: s.data, diagnostics: reactivityDiagnostics }; } return { state: s, + diagnostics: reactivityDiagnostics, data: { dice: [], links: [], diff --git a/src/components/journal/index.ts b/src/components/journal/index.ts index 86c83e2..1a4eb02 100644 --- a/src/components/journal/index.ts +++ b/src/components/journal/index.ts @@ -60,7 +60,7 @@ export { parseDeclareCsv } from "./declare-parser"; export type { VarDeclaration, TagModifier } from "./declare-parser"; export { evaluateExpression, expressionIsTag, exprValueToString } from "./variable-expression"; export type { EvalContext, EvalResult, ExprValue } from "./variable-expression"; -export { initReactivity, computeCascade, getCombined, getMods, getDeclExpr, setBase, extractDependencies, computeInitialValues } from "./var-reactivity"; -export type { VarReactivityState, VariableStore } from "./var-reactivity"; +export { initReactivity, computeCascade, getCombined, getMods, getDeclExpr, setBase, extractDependencies, computeInitialValues, getReactivityDiagnostics, clearReactivityDiagnostics, onReactivityDiagnostics } from "./var-reactivity"; +export type { VarReactivityState, VariableStore, ReactivityDiagnostic } from "./var-reactivity"; export { JournalContext, useJournalContext } from "./JournalContext"; export type { JournalContextValue } from "./JournalContext"; diff --git a/src/components/journal/var-reactivity.ts b/src/components/journal/var-reactivity.ts index 71098f8..1f3ebf6 100644 --- a/src/components/journal/var-reactivity.ts +++ b/src/components/journal/var-reactivity.ts @@ -56,6 +56,26 @@ export interface ActiveMod { kind: "numeric" | "tagmap"; } +/** + * A problem detected while evaluating declarations or tag modifiers at + * runtime. These are benign to the engine (the offending entry is skipped) but + * signal an authoring mistake that would otherwise silently produce a wrong + * value, so they are collected for surfacing in the UI. + */ +export interface ReactivityDiagnostic { + kind: "init" | "declaration" | "modifier"; + /** Human-readable description, already including the offending context. */ + message: string; + /** The variable the diagnostic concerns (e.g. "$hp"). */ + key?: string; + /** The expression that failed to evaluate. */ + expression?: string; + /** For modifiers: the tag whose activation triggered the modifier ("#warrior"). */ + tag?: string; + /** For modifiers: the variable whose tagmap activated the tag ("$class"). */ + source?: string; +} + // --------------------------------------------------------------------------- // Tagmap helpers // --------------------------------------------------------------------------- @@ -133,6 +153,61 @@ const sourceActivations = new Map(); +/** Diagnostics collected since the last init, keyed by a stable id to dedupe. */ +const diagnostics = new Map(); + +/** Listeners notified whenever the diagnostic set changes. */ +const diagnosticListeners = new Set<(diagnostics: ReactivityDiagnostic[]) => void>(); + +function errorText(e: unknown): string { + return e instanceof Error ? e.message : String(e); +} + +function diagnosticId(d: ReactivityDiagnostic): string { + return [d.kind, d.key ?? "", d.expression ?? "", d.source ?? "", d.tag ?? "", d.message].join("\u0000"); +} + +function notifyDiagnostics(): void { + const list = [...diagnostics.values()]; + for (const listener of diagnosticListeners) listener(list); +} + +/** Record a diagnostic, dedupe by identity, and notify listeners. */ +function reportDiagnostic(d: ReactivityDiagnostic): void { + const id = diagnosticId(d); + if (diagnostics.has(id)) return; + diagnostics.set(id, d); + notifyDiagnostics(); + console.warn(`[var-reactivity] ${d.message}`); +} + +/** + * Diagnostics collected by the engine (bad declarations, modifier expressions, + * circular dependencies). Cleared by `initReactivity` so a re-init reports a + * fresh set. Repeated failures of the same expression are reported once. + */ +export function getReactivityDiagnostics(): ReactivityDiagnostic[] { + return [...diagnostics.values()]; +} + +/** Remove all collected diagnostics. */ +export function clearReactivityDiagnostics(): void { + if (diagnostics.size === 0) return; + diagnostics.clear(); + notifyDiagnostics(); +} + +/** + * Subscribe to diagnostic changes. The listener receives the current list on + * every change. Returns an unsubscribe function. + */ +export function onReactivityDiagnostics( + listener: (diagnostics: ReactivityDiagnostic[]) => void, +): () => void { + diagnosticListeners.add(listener); + return () => diagnosticListeners.delete(listener); +} + // --------------------------------------------------------------------------- // Public API // --------------------------------------------------------------------------- @@ -151,6 +226,7 @@ export function initReactivity(state: VarReactivityState): void { numericMods.clear(); tagMapMods.clear(); sourceActivations.clear(); + clearReactivityDiagnostics(); // Index tag modifiers for (const tm of state.tagModifiers) { @@ -176,7 +252,15 @@ export function initReactivity(state: VarReactivityState): void { } } - checkCircular(); + try { + checkCircular(); + } catch (e) { + reportDiagnostic({ + kind: "init", + message: `Declarations contain an unresolvable dependency: ${errorText(e)}`, + }); + throw e; + } } /** Extract $var names from an expression string. */ @@ -400,8 +484,13 @@ export function computeInitialValues( if (!results.some((x) => x.key === key)) { results.push({ key, value: combined, base: rawValue }); } - } catch { - // skip failed evaluations at init time + } catch (e) { + reportDiagnostic({ + kind: "declaration", + key, + expression: expr, + message: `Declaration ${key} failed to evaluate "${expr}": ${errorText(e)}`, + }); } } @@ -506,8 +595,17 @@ function applyTagMapActivations( } else { results.push({ key: mod.target, value: combined, base }); } - } catch { - // skip failed modifier + } catch (e) { + reportDiagnostic({ + kind: "modifier", + key: mod.target, + expression: mod.expression, + tag, + source, + message: + `Modifier ${tag} → ${mod.target} failed to evaluate ` + + `"${mod.expression}" (activated by ${source}): ${errorText(e)}`, + }); } } else if (wasActive && !isActive) { // Deactivate this modifier @@ -635,6 +733,14 @@ function reevaluateDependents( if (!results.some((x) => x.key === key)) { results.push({ key, value: combined, base: rawValue }); } + } catch (e) { + // Report the bad declaration but keep evaluating unrelated dependents. + reportDiagnostic({ + kind: "declaration", + key, + expression: declExprs.get(key), + message: `Declaration ${key} failed to re-evaluate "${declExprs.get(key) ?? ""}": ${errorText(e)}`, + }); } finally { inFlight.delete(key); } diff --git a/src/components/journal/variable-expression.ts b/src/components/journal/variable-expression.ts index be78fb9..fba1f45 100644 --- a/src/components/journal/variable-expression.ts +++ b/src/components/journal/variable-expression.ts @@ -17,6 +17,7 @@ * - Division by zero * - Unknown functions * - Malformed expressions + * - Malformed numeric literals (e.g. "1.2.3") */ import { rollFormula } from "../md-commander/hooks"; @@ -168,6 +169,9 @@ interface Token { /** Dice pattern: e.g. "3d6", "2d8kh1", "d20" */ const DICE_RE = /^\d*d\d+(?:[kdh]\d+)*$/i; +/** Numeric literal: integer or decimal with digits on both sides of the dot. */ +const NUMBER_RE = /^\d+(?:\.\d+)?$/; + /** Single tagmap entry: "#warrior" or "#warrior:1" */ const TAGMAP_ENTRY_RE = /^#([a-zA-Z_][a-zA-Z0-9_]*)(?::(\d+))?/; @@ -205,6 +209,12 @@ function tokenize(input: string): Token[] { tokens.push({ kind: "number", value: String(rollDice(dice)), raw: dice }); continue; } + // The scanner above consumes digits and dots greedily, so "1.2.3" and + // "1." arrive here as a single chunk. Reject them rather than letting + // parseFloat silently truncate to a wrong value. + if (!NUMBER_RE.test(num)) { + throw new Error(`Invalid number literal: "${num}"`); + } tokens.push({ kind: "number", value: num, raw: num }); continue; }