Flag a top-level action form that is stored as data instead of run

MeTTa evaluates a top-level form only when it carries a leading `!`; every other
form is added to the space as data. That is what you want for a fact or a rule,
but an op whose signature returns the unit type (->) produces nothing a later
query can match, so the stored call is inert: the assertion never checks, the
add-atom never adds, the println! never prints. The failure is silent, which is
why an unbanged assertEqualToResult reads as a passing test.

The check reads the unit return type from the ops' own signatures rather than
from a list of builtin names, so a user op declared (-> ... (->)) is covered on
the same footing. It is a warning, so it never changes the check exit code, it
skips an op that also carries a non-arrow type, and it defers to the arity error
when the call matches no declared overload.

Author: MesTTo

packages/core/src/diagnose.test.ts

@@ -150,6 +150,75 @@ describe("analyzeSource — imported declarations", () => {
   });
 });
 
+describe("analyzeSource — top-level actions the interpreter never runs", () => {
+  // MeTTa evaluates a top-level form only when it carries `!`; every other form is added to the space as
+  // data. That is right for facts and rules, but an op whose signature returns the unit type `(->)` yields
+  // nothing to match on, so storing the call is inert: the assertion never checks, the `add-atom` never adds,
+  // the `println!` never prints. Confirmed against the runtime: a file holding `(assertEqual 1 2)` runs clean,
+  // while `!(assertEqual 1 2)` reports `results-are-not-equal`.
+  const actionsOf = (src: string) =>
+    analyzeSource(src, cfg).filter((d) => d.code === "unevaluated-action");
+
+  it("flags an unbanged assertion and underlines the whole form", () => {
+    const src = "(assertEqualToResult (retract a) (1))";
+    const diags = actionsOf(src);
+    expect(diags).toHaveLength(1);
+    expect(diags[0]!.severity).toBe(DiagnosticSeverity.Warning);
+    expect(diags[0]!.message).toContain("assertEqualToResult");
+    expect(diags[0]!.range.start).toEqual({ line: 0, character: 0 });
+    expect(diags[0]!.range.end).toEqual({ line: 0, character: src.length });
+  });
+
+  it("suggests inserting `!` before the form", () => {
+    const fix = actionsOf("(assertEqual 1 1)")[0]!.suggestions?.[0];
+    expect(fix?.replacement).toBe("!");
+    // a zero-width span at the form's start, so applying it inserts rather than replaces
+    expect(fix?.span.start).toEqual({ line: 0, character: 0 });
+    expect(fix?.span.end).toEqual({ line: 0, character: 0 });
+  });
+
+  it("stays silent when the form carries its `!`", () => {
+    expect(actionsOf("!(assertEqual 1 1)")).toEqual([]);
+  });
+
+  it("stays silent when a comment sits between the `!` and its form", () => {
+    // The parser binds a pending top-level `!` to the next form across comments, so this banner layout runs.
+    expect(actionsOf("!;;; banner\n(assertEqual 1 1)")).toEqual([]);
+  });
+
+  it("flags space mutation and output ops the same way", () => {
+    expect(actionsOf("(add-atom &self (fact a))")).toHaveLength(1);
+    expect(actionsOf('(println! "hi")')).toHaveLength(1);
+  });
+
+  it("reads the unit return type from a user declaration, not a list of builtin names", () => {
+    const diags = actionsOf("(: my-effect (-> Number (->)))\n(my-effect 1)");
+    expect(diags).toHaveLength(1);
+    expect(diags[0]!.range.start.line).toBe(1);
+  });
+
+  it("reads it from an imported declaration too", () => {
+    const imported = atomsOf("(: remote-log (-> String (->)))");
+    expect(analyzeSource('(remote-log "hi")', cfg, imported)).toHaveLength(1);
+    // without the signature in scope the head is untyped data, so nothing is reported
+    expect(analyzeSource('(remote-log "hi")', cfg)).toEqual([]);
+  });
+
+  it("leaves rules, type declarations, and doc atoms alone", () => {
+    expect(actionsOf("(= (run) (assertEqual 1 1))")).toEqual([]);
+    expect(actionsOf("(: assertEqual (-> Atom Atom (->)))")).toEqual([]);
+    expect(actionsOf('(@doc my-add (@desc "adds"))')).toEqual([]);
+  });
+
+  it("leaves plain data and untyped heads alone", () => {
+    expect(actionsOf("(likes Sam pizza)")).toEqual([]);
+  });
+
+  it("defers to the arity error when the call matches no declared overload", () => {
+    expect(analyzeSource("(assertEqual 1)", cfg).map((d) => d.code)).toEqual(["arity-mismatch"]);
+  });
+});
+
 describe("importedDefinitions", () => {
   it("flattens a resolved import map and de-duplicates aliased module entries", () => {
     const module = {

packages/core/src/diagnose.ts

@@ -121,6 +121,67 @@ function checkArity(src: string, node: SpannedNode, env: MinEnv, out: Diagnostic
   });
 }
 
+/** The unit type `(->)`: an arrow expression with no parameters and no result. The stdlib gives it as the
+ *  return type of every op that exists only for its effect, so a call to one yields nothing to match on. */
+function isUnitType(t: Atom | undefined): boolean {
+  return (
+    t?.kind === "expr" &&
+    t.items.length === 1 &&
+    t.items[0]?.kind === "sym" &&
+    t.items[0].name === "->"
+  );
+}
+
+/** Does every arrow type declared for `name` return the unit type? True for the assert family,
+ *  `add-atom`/`remove-atom`/`add-reduct`, `print!`/`println!`, and any user op declared `(-> ... (->))`.
+ *  Read from the signatures themselves, so it needs no list of builtin names. */
+function isActionOp(env: MinEnv, name: string): boolean {
+  const arrows = (env.types.get(name) ?? []).filter(
+    (t) => typeHead(t) === "->" && t.kind === "expr" && t.items.length >= 2,
+  );
+  return (
+    arrows.length > 0 &&
+    arrows.every((t) => t.kind === "expr" && isUnitType(t.items[t.items.length - 1]))
+  );
+}
+
+/** A top-level action form the interpreter stores instead of running. MeTTa evaluates a top-level form only
+ *  when it carries `!`; every other form is added to the space as data. That is what you want for a fact or a
+ *  rule, but an op returning the unit type produces nothing a later query can match, so the stored call is
+ *  inert: the assertion never checks, the `add-atom` never adds, the `println!` never prints. The failure is
+ *  silent, which is why an unbanged `assertEqualToResult` reads as a passing test. */
+function checkUnevaluatedAction(
+  src: string,
+  node: SpannedNode,
+  env: MinEnv,
+  out: Diagnostic[],
+): void {
+  if (node.bang === true || node.children === undefined) return;
+  const name = headName(node);
+  if (name === undefined) return;
+  // An op that also carries a non-arrow type can legitimately be stored as data, matching checkArity.
+  if (hasTupleType(env, name)) return;
+  if (!isActionOp(env, name)) return;
+  // A call matching no declared overload is already reported as an arity mismatch; that is the real problem.
+  if (!declaredArities(env, name).has(node.children.length - 1)) return;
+  const range = spanToRange(src, node.span.start, node.span.end);
+  out.push({
+    range,
+    severity: DiagnosticSeverity.Warning,
+    code: "unevaluated-action",
+    message: `\`${name}\` runs for its effect, so this top-level form is stored as data and never evaluated`,
+    relatedInformation: [{ message: "a top-level form runs only when it carries a leading `!`" }],
+    suggestions: [
+      {
+        span: { start: range.start, end: range.start },
+        replacement: "!",
+        applicability: Applicability.MachineApplicable,
+        message: "prefix it with `!` to run it",
+      },
+    ],
+  });
+}
+
 /** Parameter types the interpreter passes UNEVALUATED: `Atom` (spec `metta`: `$type == Atom` returns the
  *  argument as-is) and the `Variable`/`Expression` meta-types a form binds or matches. A call sitting at
  *  such a position, a case/if/let branch, a match/unify pattern, or a quoted term, is data the interpreter
@@ -183,7 +244,11 @@ export function analyze(
   const known = config.undefinedSymbols ? knownNames(env) : new Set<string>();
   const matcher = config.undefinedSymbols ? new FuzzyMatcher(known) : undefined;
   const dataPositions = unevaluatedArgPositions(env);
-  for (const node of cst) walk(src, node, env, config, known, matcher, dataPositions, false, out);
+  for (const node of cst) {
+    // Only a top-level node carries the `!` marker, so this check sits outside the recursive walk.
+    checkUnevaluatedAction(src, node, env, out);
+    walk(src, node, env, config, known, matcher, dataPositions, false, out);
+  }
   const seen = new Set<string>();
   const deduped = out.filter((d) => {
     const k = `${d.range.start.line}:${d.range.start.character}:${d.code}`;