From 16f2038fd86ee58d870d994a6ba915fd6074dd4a Mon Sep 17 00:00:00 2001 From: hyper Date: Sat, 12 Sep 2026 22:34:12 +0800 Subject: [PATCH] fix(journal): unify $var lookup and error EvalContext.lookup now receives the full $var name, matching getCombined, setBase, and getMods, and parseFactor throws "Unknown variable" when the lookup returns undefined. /set and /roll use the new resolveVariable() so a typo errors instead of silently evaluating to 0. Declarations and tag modifiers stay lenient (unknown -> 0): the documented $hp = $con*5+$mod_hp pattern depends on external/undeclared variables defaulting to 0. --- docs/variable-store-plan.md | 18 +++++--- src/cli/completions/variable-system.test.ts | 41 ++++++++++++++----- src/components/journal/command-dispatcher.ts | 12 +++--- src/components/journal/index.ts | 2 +- src/components/journal/var-reactivity.ts | 37 +++++++++++------ src/components/journal/variable-expression.ts | 14 +++++-- 6 files changed, 83 insertions(+), 41 deletions(-) diff --git a/docs/variable-store-plan.md b/docs/variable-store-plan.md index f35f0b1..72b9ed6 100644 --- a/docs/variable-store-plan.md +++ b/docs/variable-store-plan.md @@ -75,23 +75,29 @@ The items below are still open. `journalStreamState.variableBases` so bases are restored exactly instead of falling back to combined values. -### 4. `lookup` contract disagrees with its docs — low +### 4. `lookup` contract disagrees with its docs — ✅ fixed - **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. +- **Fixed:** `EvalContext.lookup` now receives the full `$var` name (including + `$`), consistent with every other variable API (`getCombined`, `setBase`, + `getMods`, `resolveVariable`). Callers no longer add or strip `$`. -### 5. Undefined variables evaluate to `0` — low +### 5. Undefined variables evaluate to `0` — ✅ fixed (strict for commands) - **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. +- **Fixed:** `parseFactor` now throws `Unknown variable: $x` whenever `lookup` + returns `undefined`, and `resolveVariable()` distinguishes unknown from a real + `0`. `/set` and `/roll` use the strict resolver, so typos fail loudly via + `dispatchError`. Declarations and tag modifiers stay lenient on purpose: they + resolve unknown references to `0` through `getCombined`, because the + documented pattern (`$hp = $con*5+$mod_hp`) relies on external/undeclared + variables defaulting to 0. ### 6. Tokenizer accepts malformed numbers — ✅ fixed diff --git a/src/cli/completions/variable-system.test.ts b/src/cli/completions/variable-system.test.ts index 4de48b8..ee0b3ff 100644 --- a/src/cli/completions/variable-system.test.ts +++ b/src/cli/completions/variable-system.test.ts @@ -41,6 +41,7 @@ import { extractDependencies, rebuildReactivityFromStore, getReactivityDiagnostics, + resolveVariable, } from "../../components/journal/var-reactivity"; import { parseInput } from "../../components/journal/command-parser"; import { @@ -273,32 +274,31 @@ describe("evaluateExpression", () => { test("evaluates variable references", () => { const result = evaluateExpression("$con * 5 + $mod_hp", { lookup: (name) => { - if (name === "con") return "12"; - if (name === "mod_hp") return "20"; + if (name === "$con") return "12"; + if (name === "$mod_hp") return "20"; return undefined; }, }); expect(result.value).toEqual({ kind: "number", value: 80 }); // 12*5 + 20 }); - test("returns 0 for undefined variables", () => { - const result = evaluateExpression("$unknown + 5", { - lookup: () => undefined, - }); - expect(result.value).toEqual({ kind: "number", value: 5 }); + test("throws on unknown variables", () => { + expect(() => + evaluateExpression("$unknown + 5", { lookup: () => undefined }), + ).toThrow("Unknown variable: $unknown"); }); test("throws on type mismatch (tagmap + number)", () => { expect(() => evaluateExpression("$class + 5", { - lookup: (name) => (name === "class" ? "#warrior" : undefined), + lookup: (name) => (name === "$class" ? "#warrior" : undefined), }), ).toThrow("Type mismatch"); }); test("resolves tagmap variable without arithmetic", () => { const result = evaluateExpression("$class", { - lookup: (name) => (name === "class" ? "#warrior:1" : undefined), + lookup: (name) => (name === "$class" ? "#warrior:1" : undefined), }); expect(result.value).toEqual({ kind: "tagmap", value: { "#warrior": 1 } }); }); @@ -422,8 +422,8 @@ describe("evaluateExpression", () => { test("complex expression with variables and functions", () => { const result = evaluateExpression("floor($con / 2) + $mod", { lookup: (name) => { - if (name === "con") return "15"; - if (name === "mod") return "3"; + if (name === "$con") return "15"; + if (name === "$mod") return "3"; return undefined; }, }); @@ -742,6 +742,25 @@ describe("var-reactivity", () => { }); }); + describe("resolveVariable", () => { + test("returns undefined for unknown variables", () => { + initReactivity({ declarations: [], tagModifiers: [] }); + expect(resolveVariable("$nope")).toBeUndefined(); + expect(resolveVariable("$nope", {})).toBeUndefined(); + }); + + test("returns a real 0 rather than undefined", () => { + initReactivity({ declarations: [], tagModifiers: [] }); + setBase("$zero", "0"); + expect(resolveVariable("$zero")).toBe("0"); + }); + + test("resolves keys present only in the fallback store", () => { + initReactivity({ declarations: [], tagModifiers: [] }); + expect(resolveVariable("$hp", { $hp: "12" })).toBe("12"); + }); + }); + describe("rebuildReactivityFromStore", () => { test("restores bases exactly without double-counting modifiers", () => { initReactivity({ diff --git a/src/components/journal/command-dispatcher.ts b/src/components/journal/command-dispatcher.ts index d99d4b1..e696bf9 100644 --- a/src/components/journal/command-dispatcher.ts +++ b/src/components/journal/command-dispatcher.ts @@ -11,7 +11,7 @@ import { parseInput } from "./command-parser"; import { resolveRollPayload } from "./types/roll"; import { resolveSparkPayload } from "./types/spark"; import { evaluateExpression, exprValueToString } from "./variable-expression"; -import { computeCascade, getCombined, setBase } from "./var-reactivity"; +import { computeCascade, getCombined, resolveVariable, setBase } from "./var-reactivity"; import { ensureReactivity } from "./completions"; // Tagmap pattern: "#warrior:1;#druid:2". Also accepts bare "#tag" as shorthand. @@ -131,7 +131,8 @@ export async function dispatchCommand( if (arg.includes("$")) { try { const ev = evaluateExpression(arg, { - lookup: (name: string) => getCombined("$" + name, ctx.variables), + // Strict: referencing an undefined variable is an error, not 0. + lookup: (name) => resolveVariable(name, ctx.variables), }); if (ev.value.kind !== "number") { return finish({ ok: false, error: "Roll expression must evaluate to a number" }); @@ -204,11 +205,8 @@ function dispatchSet( } else if (p.expr) { // Evaluate expression — handles both numeric and tagmap values const result = evaluateExpression(p.expr, { - lookup: (name: string) => { - const k = "$" + name; - // Use getCombined with stream fallback for accurate values - return k === key ? undefined : getCombined(k, ctx.variables); - }, + // Strict: referencing an undefined variable is an error, not 0. + lookup: (name) => resolveVariable(name, ctx.variables), }); newValue = exprValueToString(result.value); } else { diff --git a/src/components/journal/index.ts b/src/components/journal/index.ts index a37c7e3..76ef653 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, getReactivityDiagnostics, clearReactivityDiagnostics, onReactivityDiagnostics } from "./var-reactivity"; +export { initReactivity, computeCascade, getCombined, getMods, getDeclExpr, setBase, extractDependencies, computeInitialValues, getReactivityDiagnostics, clearReactivityDiagnostics, onReactivityDiagnostics, resolveVariable } 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 1efa101..344cb35 100644 --- a/src/components/journal/var-reactivity.ts +++ b/src/components/journal/var-reactivity.ts @@ -331,6 +331,24 @@ export function getCombined(key: string, fallback?: VariableStore): string { return "0"; } +/** + * Resolve a `$var` to its combined value, distinguishing an unknown variable + * (undefined) from a real value of "0". Strict contexts such as `/set` and + * `/roll` use this so a typo'd or undeclared reference errors instead of + * silently evaluating to 0. + */ +export function resolveVariable( + key: string, + fallback?: VariableStore, +): string | undefined { + const known = + baseValues.has(key) || + numericMods.has(key) || + tagMapMods.has(key) || + (fallback !== undefined && fallback[key] !== undefined); + return known ? getCombined(key, fallback) : undefined; +} + /** * Get the effective tagmap for a variable (base + tagmap mods). * Returns null if the variable is not a tagmap variable. @@ -472,10 +490,9 @@ export function computeInitialValues( try { const result = evaluateExpression(expr, { - lookup: (name: string) => { - const k = "$" + name; - return getCombined(k, currentVars); - }, + // Declarations may reference runtime/external variables; getCombined + // defaults unknown keys to "0" rather than erroring. + lookup: (name) => getCombined(name, currentVars), }); const rawValue = exprValueToString(result.value); @@ -550,10 +567,8 @@ function applyTagMapActivations( } const evalResult = evaluateExpression(mod.expression, { - lookup: (name: string) => { - const k = "$" + name; - return getCombined(k, fallback); - }, + // Modifier expressions tolerate undefined references (→ "0"). + lookup: (name) => getCombined(name, fallback), }); // Modifier expressions must evaluate to a number @@ -716,10 +731,8 @@ function reevaluateDependents( if (!expr) continue; const result = evaluateExpression(expr, { - lookup: (name: string) => { - const k = "$" + name; - return getCombined(k, fallback); - }, + // Declaration re-evaluation stays lenient for undeclared references. + lookup: (name) => getCombined(name, fallback), }); const rawValue = exprValueToString(result.value); diff --git a/src/components/journal/variable-expression.ts b/src/components/journal/variable-expression.ts index fba1f45..9616b73 100644 --- a/src/components/journal/variable-expression.ts +++ b/src/components/journal/variable-expression.ts @@ -16,6 +16,7 @@ * - Circular variable references (detected by caller) * - Division by zero * - Unknown functions + * - Unknown variables (lookup returned undefined) * - Malformed expressions * - Malformed numeric literals (e.g. "1.2.3") */ @@ -32,8 +33,13 @@ export type ExprValue = | { kind: "tagmap"; value: Record }; export interface EvalContext { - /** Resolve $var → string (numeric or tagmap serialized form). - * Return undefined if the variable doesn't exist. */ + /** Resolve a variable reference — the full name including the leading `$` + * (e.g. `"$hp"`) — to its serialized value: numeric ("12") or tagmap + * ("#warrior:1;#druid:2"). + * + * Return undefined for an unknown variable; the evaluator then throws + * `Unknown variable`. Contexts that want a default (e.g. undeclared + * variables in declarations resolving to 0) should return a value instead. */ lookup: (varName: string) => string | undefined; } @@ -421,9 +427,9 @@ function parseFactor( // Variable reference: $var if (tok.kind === "var") { const varName = tok.value; // includes $ prefix - const resolved = ctx.lookup(varName.slice(1)); // strip $ for lookup + const resolved = ctx.lookup(varName); if (resolved === undefined) { - return { value: { kind: "number", value: 0 }, next: pos + 1 }; + throw new Error(`Unknown variable: ${varName}`); } // Auto-detect: tagmap or numeric if (resolved.startsWith("#")) {