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.
This commit is contained in:
@@ -75,23 +75,29 @@ The items below are still open.
|
|||||||
`journalStreamState.variableBases` so bases are restored exactly instead of
|
`journalStreamState.variableBases` so bases are restored exactly instead of
|
||||||
falling back to combined values.
|
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
|
- **Where:** `EvalContext.lookup` documents "Resolve `$var`", but
|
||||||
`parseFactor` calls `ctx.lookup(varName.slice(1))` (bare name), and every
|
`parseFactor` calls `ctx.lookup(varName.slice(1))` (bare name), and every
|
||||||
caller re-adds `$`.
|
caller re-adds `$`.
|
||||||
- **Symptom:** a caller following the doc would key variables with `$` and
|
- **Symptom:** a caller following the doc would key variables with `$` and
|
||||||
silently get `0`s.
|
silently get `0`s.
|
||||||
- **Fix:** update the doc comment to state the argument is the bare name (no
|
- **Fixed:** `EvalContext.lookup` now receives the full `$var` name (including
|
||||||
`$`), or stop stripping in `parseFactor` and update callers.
|
`$`), 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
|
- **Where:** `parseFactor` (`variable-expression.ts`) returns a numeric `0` for
|
||||||
an unresolved `$var`.
|
an unresolved `$var`.
|
||||||
- **Symptom:** `/set $hp $cn*5` (typo) silently sets `0` instead of failing.
|
- **Symptom:** `/set $hp $cn*5` (typo) silently sets `0` instead of failing.
|
||||||
- **Fix:** consider erroring on unknown variables in `/set` context while
|
- **Fixed:** `parseFactor` now throws `Unknown variable: $x` whenever `lookup`
|
||||||
keeping lenient behavior for display/roll contexts, or warn.
|
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
|
### 6. Tokenizer accepts malformed numbers — ✅ fixed
|
||||||
|
|
||||||
|
|||||||
@@ -41,6 +41,7 @@ import {
|
|||||||
extractDependencies,
|
extractDependencies,
|
||||||
rebuildReactivityFromStore,
|
rebuildReactivityFromStore,
|
||||||
getReactivityDiagnostics,
|
getReactivityDiagnostics,
|
||||||
|
resolveVariable,
|
||||||
} from "../../components/journal/var-reactivity";
|
} from "../../components/journal/var-reactivity";
|
||||||
import { parseInput } from "../../components/journal/command-parser";
|
import { parseInput } from "../../components/journal/command-parser";
|
||||||
import {
|
import {
|
||||||
@@ -273,32 +274,31 @@ describe("evaluateExpression", () => {
|
|||||||
test("evaluates variable references", () => {
|
test("evaluates variable references", () => {
|
||||||
const result = evaluateExpression("$con * 5 + $mod_hp", {
|
const result = evaluateExpression("$con * 5 + $mod_hp", {
|
||||||
lookup: (name) => {
|
lookup: (name) => {
|
||||||
if (name === "con") return "12";
|
if (name === "$con") return "12";
|
||||||
if (name === "mod_hp") return "20";
|
if (name === "$mod_hp") return "20";
|
||||||
return undefined;
|
return undefined;
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
expect(result.value).toEqual({ kind: "number", value: 80 }); // 12*5 + 20
|
expect(result.value).toEqual({ kind: "number", value: 80 }); // 12*5 + 20
|
||||||
});
|
});
|
||||||
|
|
||||||
test("returns 0 for undefined variables", () => {
|
test("throws on unknown variables", () => {
|
||||||
const result = evaluateExpression("$unknown + 5", {
|
expect(() =>
|
||||||
lookup: () => undefined,
|
evaluateExpression("$unknown + 5", { lookup: () => undefined }),
|
||||||
});
|
).toThrow("Unknown variable: $unknown");
|
||||||
expect(result.value).toEqual({ kind: "number", value: 5 });
|
|
||||||
});
|
});
|
||||||
|
|
||||||
test("throws on type mismatch (tagmap + number)", () => {
|
test("throws on type mismatch (tagmap + number)", () => {
|
||||||
expect(() =>
|
expect(() =>
|
||||||
evaluateExpression("$class + 5", {
|
evaluateExpression("$class + 5", {
|
||||||
lookup: (name) => (name === "class" ? "#warrior" : undefined),
|
lookup: (name) => (name === "$class" ? "#warrior" : undefined),
|
||||||
}),
|
}),
|
||||||
).toThrow("Type mismatch");
|
).toThrow("Type mismatch");
|
||||||
});
|
});
|
||||||
|
|
||||||
test("resolves tagmap variable without arithmetic", () => {
|
test("resolves tagmap variable without arithmetic", () => {
|
||||||
const result = evaluateExpression("$class", {
|
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 } });
|
expect(result.value).toEqual({ kind: "tagmap", value: { "#warrior": 1 } });
|
||||||
});
|
});
|
||||||
@@ -422,8 +422,8 @@ describe("evaluateExpression", () => {
|
|||||||
test("complex expression with variables and functions", () => {
|
test("complex expression with variables and functions", () => {
|
||||||
const result = evaluateExpression("floor($con / 2) + $mod", {
|
const result = evaluateExpression("floor($con / 2) + $mod", {
|
||||||
lookup: (name) => {
|
lookup: (name) => {
|
||||||
if (name === "con") return "15";
|
if (name === "$con") return "15";
|
||||||
if (name === "mod") return "3";
|
if (name === "$mod") return "3";
|
||||||
return undefined;
|
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", () => {
|
describe("rebuildReactivityFromStore", () => {
|
||||||
test("restores bases exactly without double-counting modifiers", () => {
|
test("restores bases exactly without double-counting modifiers", () => {
|
||||||
initReactivity({
|
initReactivity({
|
||||||
|
|||||||
@@ -11,7 +11,7 @@ import { parseInput } from "./command-parser";
|
|||||||
import { resolveRollPayload } from "./types/roll";
|
import { resolveRollPayload } from "./types/roll";
|
||||||
import { resolveSparkPayload } from "./types/spark";
|
import { resolveSparkPayload } from "./types/spark";
|
||||||
import { evaluateExpression, exprValueToString } from "./variable-expression";
|
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";
|
import { ensureReactivity } from "./completions";
|
||||||
|
|
||||||
// Tagmap pattern: "#warrior:1;#druid:2". Also accepts bare "#tag" as shorthand.
|
// Tagmap pattern: "#warrior:1;#druid:2". Also accepts bare "#tag" as shorthand.
|
||||||
@@ -131,7 +131,8 @@ export async function dispatchCommand(
|
|||||||
if (arg.includes("$")) {
|
if (arg.includes("$")) {
|
||||||
try {
|
try {
|
||||||
const ev = evaluateExpression(arg, {
|
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") {
|
if (ev.value.kind !== "number") {
|
||||||
return finish({ ok: false, error: "Roll expression must evaluate to a number" });
|
return finish({ ok: false, error: "Roll expression must evaluate to a number" });
|
||||||
@@ -204,11 +205,8 @@ function dispatchSet(
|
|||||||
} else if (p.expr) {
|
} else if (p.expr) {
|
||||||
// Evaluate expression — handles both numeric and tagmap values
|
// Evaluate expression — handles both numeric and tagmap values
|
||||||
const result = evaluateExpression(p.expr, {
|
const result = evaluateExpression(p.expr, {
|
||||||
lookup: (name: string) => {
|
// Strict: referencing an undefined variable is an error, not 0.
|
||||||
const k = "$" + name;
|
lookup: (name) => resolveVariable(name, ctx.variables),
|
||||||
// Use getCombined with stream fallback for accurate values
|
|
||||||
return k === key ? undefined : getCombined(k, ctx.variables);
|
|
||||||
},
|
|
||||||
});
|
});
|
||||||
newValue = exprValueToString(result.value);
|
newValue = exprValueToString(result.value);
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -60,7 +60,7 @@ export { parseDeclareCsv } from "./declare-parser";
|
|||||||
export type { VarDeclaration, TagModifier } from "./declare-parser";
|
export type { VarDeclaration, TagModifier } from "./declare-parser";
|
||||||
export { evaluateExpression, expressionIsTag, exprValueToString } from "./variable-expression";
|
export { evaluateExpression, expressionIsTag, exprValueToString } from "./variable-expression";
|
||||||
export type { EvalContext, EvalResult, ExprValue } 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 type { VarReactivityState, VariableStore, ReactivityDiagnostic } from "./var-reactivity";
|
||||||
export { JournalContext, useJournalContext } from "./JournalContext";
|
export { JournalContext, useJournalContext } from "./JournalContext";
|
||||||
export type { JournalContextValue } from "./JournalContext";
|
export type { JournalContextValue } from "./JournalContext";
|
||||||
|
|||||||
@@ -331,6 +331,24 @@ export function getCombined(key: string, fallback?: VariableStore): string {
|
|||||||
return "0";
|
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).
|
* Get the effective tagmap for a variable (base + tagmap mods).
|
||||||
* Returns null if the variable is not a tagmap variable.
|
* Returns null if the variable is not a tagmap variable.
|
||||||
@@ -472,10 +490,9 @@ export function computeInitialValues(
|
|||||||
|
|
||||||
try {
|
try {
|
||||||
const result = evaluateExpression(expr, {
|
const result = evaluateExpression(expr, {
|
||||||
lookup: (name: string) => {
|
// Declarations may reference runtime/external variables; getCombined
|
||||||
const k = "$" + name;
|
// defaults unknown keys to "0" rather than erroring.
|
||||||
return getCombined(k, currentVars);
|
lookup: (name) => getCombined(name, currentVars),
|
||||||
},
|
|
||||||
});
|
});
|
||||||
|
|
||||||
const rawValue = exprValueToString(result.value);
|
const rawValue = exprValueToString(result.value);
|
||||||
@@ -550,10 +567,8 @@ function applyTagMapActivations(
|
|||||||
}
|
}
|
||||||
|
|
||||||
const evalResult = evaluateExpression(mod.expression, {
|
const evalResult = evaluateExpression(mod.expression, {
|
||||||
lookup: (name: string) => {
|
// Modifier expressions tolerate undefined references (→ "0").
|
||||||
const k = "$" + name;
|
lookup: (name) => getCombined(name, fallback),
|
||||||
return getCombined(k, fallback);
|
|
||||||
},
|
|
||||||
});
|
});
|
||||||
|
|
||||||
// Modifier expressions must evaluate to a number
|
// Modifier expressions must evaluate to a number
|
||||||
@@ -716,10 +731,8 @@ function reevaluateDependents(
|
|||||||
if (!expr) continue;
|
if (!expr) continue;
|
||||||
|
|
||||||
const result = evaluateExpression(expr, {
|
const result = evaluateExpression(expr, {
|
||||||
lookup: (name: string) => {
|
// Declaration re-evaluation stays lenient for undeclared references.
|
||||||
const k = "$" + name;
|
lookup: (name) => getCombined(name, fallback),
|
||||||
return getCombined(k, fallback);
|
|
||||||
},
|
|
||||||
});
|
});
|
||||||
|
|
||||||
const rawValue = exprValueToString(result.value);
|
const rawValue = exprValueToString(result.value);
|
||||||
|
|||||||
@@ -16,6 +16,7 @@
|
|||||||
* - Circular variable references (detected by caller)
|
* - Circular variable references (detected by caller)
|
||||||
* - Division by zero
|
* - Division by zero
|
||||||
* - Unknown functions
|
* - Unknown functions
|
||||||
|
* - Unknown variables (lookup returned undefined)
|
||||||
* - Malformed expressions
|
* - Malformed expressions
|
||||||
* - Malformed numeric literals (e.g. "1.2.3")
|
* - Malformed numeric literals (e.g. "1.2.3")
|
||||||
*/
|
*/
|
||||||
@@ -32,8 +33,13 @@ export type ExprValue =
|
|||||||
| { kind: "tagmap"; value: Record<string, number> };
|
| { kind: "tagmap"; value: Record<string, number> };
|
||||||
|
|
||||||
export interface EvalContext {
|
export interface EvalContext {
|
||||||
/** Resolve $var → string (numeric or tagmap serialized form).
|
/** Resolve a variable reference — the full name including the leading `$`
|
||||||
* Return undefined if the variable doesn't exist. */
|
* (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;
|
lookup: (varName: string) => string | undefined;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -421,9 +427,9 @@ function parseFactor(
|
|||||||
// Variable reference: $var
|
// Variable reference: $var
|
||||||
if (tok.kind === "var") {
|
if (tok.kind === "var") {
|
||||||
const varName = tok.value; // includes $ prefix
|
const varName = tok.value; // includes $ prefix
|
||||||
const resolved = ctx.lookup(varName.slice(1)); // strip $ for lookup
|
const resolved = ctx.lookup(varName);
|
||||||
if (resolved === undefined) {
|
if (resolved === undefined) {
|
||||||
return { value: { kind: "number", value: 0 }, next: pos + 1 };
|
throw new Error(`Unknown variable: ${varName}`);
|
||||||
}
|
}
|
||||||
// Auto-detect: tagmap or numeric
|
// Auto-detect: tagmap or numeric
|
||||||
if (resolved.startsWith("#")) {
|
if (resolved.startsWith("#")) {
|
||||||
|
|||||||
Reference in New Issue
Block a user