diff --git a/AGENTS.md b/AGENTS.md index fd85169c..9dfc9d7d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -205,6 +205,8 @@ parse the resulting `{ ok: false, diagnostics: [...] }` envelope. | EFF004 | (0.9+) A callback parameter declares `writes { … }` labels not covered by the outer fn's `writes {}`. | Add the missing label(s) to the outer fn's `writes {}`, or narrow the callback annotation. | | DEP001 | (0.9+) A fn's body (or a callee in the same file) reads a resource label not declared in the fn's own `reads {}`. Transitivity is enforced: if `loadUser` calls `fetchRow` which reads `userDb`, `loadUser` must also declare `reads { userDb }`. | Add the missing label(s) to `reads {}`, or remove the undeclared read. | | DEP002 | (0.9+) Same as DEP001 but for `writes {}` labels. A fn whose callee writes a resource must declare that write in its own header. | Add the missing label(s) to `writes {}`, or remove the undeclared write. | +| DEP003 | (0.9+, warning) A fn declares `reads { x }` but no same-file callee (direct or transitive) also declares `reads { x }`. The annotation is likely stale — either a refactor removed the callee that justified it, or the fn is itself the access point. Leaf fns are excluded. Non-blocking. | Remove the stale label from `reads {}`, or verify the fn is the intended access point. | +| DEP004 | (0.9+, warning) Same as DEP003 but for `writes {}`. A fn declares a write label that no same-file callee justifies. Non-blocking. | Remove the stale label from `writes {}`, or verify the fn is the intended access point. | | THR001 | (0.9+) A fn's body (or a same-file callee) throws an exception type not declared in the fn's `throws {}`. Transitivity is enforced: if `loadUser` calls `fetchRow throws { NetworkError }`, `loadUser` must also declare `throws { NetworkError }`. | Add the missing type(s) to `throws {}`, or add a `match` / `unsafe` to suppress the propagation. | | THR002 | (0.9+) A fn body directly constructs `err(TypeName(...))`, `err(new TypeName(...))`, or `err(TypeName)` where `TypeName` (CapCase) is not declared in the fn's own `throws {}` clause. Producer-side complement to THR001. | Add `TypeName` to the fn's `throws {}`, or change the error construction to use a declared type. | | THR003 | (0.9+) A callback parameter declares `throws { … }` types not covered by the outer fn's `throws {}`. Same structural rule as THR001 applied to callback parameters. | Add the missing type(s) to the outer fn's `throws {}`, or narrow the callback annotation. | diff --git a/README.md b/README.md index 9dc0765b..1a4645a4 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,7 @@ claude mcp add botscript -- npx -y @mbfarias/botscript-mcp | ----------- | -------------------------------------- | --------------------------------------------------------------------------------------------------- | | `primer` | (no args) | The canonical language primer (same text the `?primer` directive emits). | | `transform` | `{ source: string, filename?: string }` | `{ ok: true, code, forms, version, warnings: [...] }` on success, or `{ ok: false, diagnostics: [...] }` on failure. `warnings` is an array of non-blocking diagnostics (e.g. CAP003). | -| `explain` | `{ code: string }` | Long-form explanation for any stable diagnostic code (`BS001`, `BS002`, `CAP001`–`CAP003`, `DEP001`, `DEP002`, `EFF002`–`EFF004`, `FMT001`, `INT001`–`INT005`, `MAT001`, `RES001`, `SYN001`, `THR001`–`THR003`, `UNS001`–`UNS005`, `VER001`–`VER003`) plus a fails/passes example pair. | +| `explain` | `{ code: string }` | Long-form explanation for any stable diagnostic code (`BS001`, `BS002`, `CAP001`–`CAP003`, `DEP001`–`DEP004`, `EFF002`–`EFF004`, `FMT001`, `INT001`–`INT005`, `MAT001`, `RES001`, `SYN001`, `THR001`–`THR003`, `UNS001`–`UNS005`, `VER001`–`VER003`) plus a fails/passes example pair. | A bot's loop becomes deterministic: `transform` → if `ok=false`, read `diagnostics[0].code` → `explain(code)` → apply `rewrite` → `transform` again. diff --git a/packages/compiler/src/error-codes.ts b/packages/compiler/src/error-codes.ts index 65325fa4..a2e58c11 100644 --- a/packages/compiler/src/error-codes.ts +++ b/packages/compiler/src/error-codes.ts @@ -453,6 +453,48 @@ const E: Record = { "fn helper(id: string) -> string = \"ok\"\n" + "fn load(id: string) -> string = helper(id)", }, + DEP003: { + code: "DEP003", + title: "fn declares reads {} label not justified by any callee in the same file (warning)", + rule: + "a declared reads {} label should reflect a resource the fn or its callees actually access; " + + "if no same-file callee (transitively) declares reads { x }, the label may be stale after a refactor", + idiom: + "remove the stale label from the reads {} clause, or verify that the fn itself directly accesses the resource; " + + "leaf fns that are the actual access point can safely declare the label even if no callee propagates it", + rewrite: + "fn name(...) reads { …remaining } -> ... // remove label not propagated by any callee", + example: + "// before — getUser calls helper() but helper() does not read userDb\n" + + "?bs 0.9\n" + + "fn helper(id: string) -> string = \"Alice\"\n" + + "fn getUser(id: string) reads { userDb } -> string { helper(id) } // DEP003\n\n" + + "// after — remove stale label\n" + + "?bs 0.9\n" + + "fn helper(id: string) -> string = \"Alice\"\n" + + "fn getUser(id: string) -> string { helper(id) }", + }, + DEP004: { + code: "DEP004", + title: "fn declares writes {} label not justified by any callee in the same file (warning)", + rule: + "a declared writes {} label should reflect a resource the fn or its callees actually modify; " + + "if no same-file callee (transitively) declares writes { x }, the label may be stale after a refactor", + idiom: + "remove the stale label from the writes {} clause, or verify that the fn itself directly modifies the resource; " + + "leaf fns that are the actual write point can safely declare the label even if no callee propagates it", + rewrite: + "fn name(...) writes { …remaining } -> ... // remove label not propagated by any callee", + example: + "// before — logEvent calls save() but save() does not write auditLog\n" + + "?bs 0.9\n" + + "fn save(msg: string) -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { save(msg) } // DEP004\n\n" + + "// after — remove stale label\n" + + "?bs 0.9\n" + + "fn save(msg: string) -> void { }\n" + + "fn logEvent(msg: string) -> void { save(msg) }", + }, THR003: { code: "THR003", title: "outer fn declares narrower throws than a callback parameter", diff --git a/packages/compiler/src/module-effects.ts b/packages/compiler/src/module-effects.ts index f541b2fd..4dadf07c 100644 --- a/packages/compiler/src/module-effects.ts +++ b/packages/compiler/src/module-effects.ts @@ -375,11 +375,12 @@ export function buildModuleEffects(sources: readonly string[]): ModuleEffects { if (decl.reads?.length) surface.reads = decl.reads; if (decl.writes?.length) surface.writes = decl.writes; if (decl.throws?.length) surface.throws = decl.throws; - if (Object.keys(surface).length > 0) { - effects[decl.name] = Object.hasOwn(effects, decl.name) - ? mergeEffectSurface(effects[decl.name]!, surface) - : surface; - } + // Always include the function, even when surface is empty {}. + // DEP003/DEP004 need to distinguish "known callee with no labels" from + // "unknown callee" — omitting pure helpers causes them to look opaque. + effects[decl.name] = Object.hasOwn(effects, decl.name) + ? mergeEffectSurface(effects[decl.name]!, surface) + : surface; } } return effects; diff --git a/packages/compiler/src/passes/_callgraph.ts b/packages/compiler/src/passes/_callgraph.ts index 61ab59cf..5c6e623d 100644 --- a/packages/compiler/src/passes/_callgraph.ts +++ b/packages/compiler/src/passes/_callgraph.ts @@ -87,64 +87,133 @@ export function collectCallees( return callees; } -export function nextSignificant(tokens: Token[], start: number): number { - let i = start; - while (i < tokens.length) { - const t = tokens[i]; - if (!t) return i; - if ( - t.kind === "whitespace" || - t.kind === "newline" || - t.kind === "lineComment" || - t.kind === "blockComment" - ) { +/** + * Botscript stdlib namespace names. Member calls on these (e.g. `time.now()`, + * `http.get()`) are handled by cap-check/uns-check, not by `hasOpaqueCall`. + * + * `cap-check.ts` exports `STDLIB_TO_CAP` whose keys must exactly match this set. + * Import from here rather than duplicating the list. + */ +export const STDLIB_NAMESPACES = new Set(["http", "time", "random", "fs", "stdout", "stderr"]); + +/** + * Botscript's lexer only promotes a small set of names to `keyword` tokens + * (see `KEYWORDS` in lex.ts). Most control-flow constructs (`if`, `while`, + * `for`, etc.) are tokenised as plain `ident` and would otherwise be treated + * as opaque function calls by `hasOpaqueCall`. Skip them explicitly. + */ +const CONTROL_FLOW_IDENTS = new Set([ + "if", "else", "while", "for", "do", "switch", "case", "default", + "return", "break", "continue", "throw", "try", "catch", "finally", + "typeof", "void", "delete", "new", "in", "of", "instanceof", +]); + + +/** + * Parse the top-level parameter names from a fn's `args` string (verbatim, + * including outer parens). Depth-tracks parentheses so names inside nested + * callback type annotations (e.g. `cb: (item: string) -> void`) are excluded. + * + * Used by `hasOpaqueCall` (and exported for callers that also need param names) + * to avoid treating method calls on fn parameters as opaque namespace calls. + */ +export function collectTopLevelParamNames(args: string): Set { + const names = new Set(); + let parenDepth = 0; + let braceDepth = 0; + let i = 0; + while (i < args.length) { + const c = args[i]!; + if (c === "(") { parenDepth++; i++; continue; } + if (c === ")") { parenDepth--; i++; continue; } + if (c === "{") { braceDepth++; i++; continue; } + if (c === "}") { braceDepth--; i++; continue; } + if (parenDepth !== 1 || braceDepth > 0) { i++; continue; } + const m = /^([a-zA-Z_$][a-zA-Z0-9_$]*)\s*:/.exec(args.slice(i)); + if (m) { + names.add(m[1]!); + i += m[0].length; + } else { i++; - continue; } - return i; } - return i; + return names; } -export function prevSignificant(tokens: Token[], start: number): number { - let i = start; - while (i >= 0) { - const t = tokens[i]; - if (!t) return i; - if ( - t.kind === "whitespace" || - t.kind === "newline" || - t.kind === "lineComment" || - t.kind === "blockComment" - ) { - i--; - continue; +/** + * Collect names of `const`/`let` simple-binding variables declared in `fn`'s + * body (excluding tokens inside nested fn declarations). + * + * Only plain `const name = ...` and `let name = ...` forms are collected — + * destructuring patterns are intentionally skipped. The result is used as + * the `localNames` set for `hasOpaqueCall` so that method calls on local + * variables (e.g. `name.trim()`) are not mistaken for opaque import calls. + */ +export function collectFnBodyLocalNames( + tokens: Token[], + fn: FnDecl, + inner: FnDecl[], +): Set { + const names = new Set(); + const open: FnDecl[] = []; + let nextInner = 0; + const start = fn.bodyTokenStart ?? fn.tokenStart; + + for (let i = start; i < fn.tokenEnd; i++) { + while (open.length > 0 && open[open.length - 1]!.tokenEnd <= i) open.pop(); + while (nextInner < inner.length && inner[nextInner]!.tokenStart <= i) { + open.push(inner[nextInner]!); + nextInner++; } - return i; + if (open.length > 0) continue; + + const tok = tokens[i]; + if (!tok || tok.kind !== "ident") continue; + if (tok.text !== "const" && tok.text !== "let") continue; + + const nameIdx = nextSignificant(tokens, i + 1); + const nameTok = tokens[nameIdx]; + // Only simple `const name` bindings — skip destructuring (`{`, `[`) + if (nameTok && nameTok.kind === "ident") names.add(nameTok.text); } - return i; -} -// Alias imported for opaque-call detection; derived from imports.ts to avoid drift. -const BOTSCRIPT_BUILTIN_CALLS = STDLIB_VALUE_CALL_NAMES; + return names; +} /** - * Returns true if `fn`'s body contains at least one function call whose callee - * name is NOT in `knownCalleeNames` (i.e. an opaque/external call whose effects - * are unknown to the compiler). + * Returns true if fn's body contains any opaque external call — either a bare + * function call (`ident(`) or a member/namespace call (`obj.method()`) — where + * the callee/receiver is NOT in `knownNames` and is not a compiler-known stdlib + * builtin, control-flow keyword, or CapCase error constructor. * - * A call is detected as `ident(` where the ident is not a property access and - * is not in `knownCalleeNames`. Inner fn declarations are excluded. + * Both patterns can reach external resources: a bare `fetchData()` or a member + * call `db.read()` on an imported object. Suppresses over-declaration warnings + * (DEP003/DEP004, THR004) when the fn has at least one such call whose effects + * are unknown to the compiler. * - * Botscript stdlib value helpers (ok, isOk, some, none, etc.) and CapCase type - * constructors are never treated as opaque — they are compiler-known builtins. + * `localNames` (optional) is a set of parameter or local-variable names that + * should not be treated as opaque namespace/object receivers. For example, + * `name.trim()` where `name` is a string parameter is not an opaque import + * method call and must not trigger suppression. */ export function hasOpaqueCall( tokens: Token[], fn: FnDecl, inner: FnDecl[], - knownCalleeNames: Set, + knownNames: Set, + localNames?: ReadonlySet, ): boolean { + // When localNames is not provided, lazily compute it so that method calls on + // fn parameters and local variables (e.g. `name.trim()`) are not mistaken for + // opaque namespace/object calls. + const effectiveLocalNames: ReadonlySet = + localNames ?? + (() => { + const names = collectTopLevelParamNames(fn.args); + for (const n of collectFnBodyLocalNames(tokens, fn, inner)) names.add(n); + return names; + })(); + const open: FnDecl[] = []; let nextInner = 0; @@ -158,27 +227,126 @@ export function hasOpaqueCall( const tok = tokens[i]; if (!tok || tok.kind !== "ident") continue; + if (tok.text === fn.name) continue; + if (knownNames.has(tok.text)) continue; - // Skip known callee names (same-file fns and listed external fns). - if (knownCalleeNames.has(tok.text)) continue; + // Skip control-flow identifiers that look like calls but aren't. + if (CONTROL_FLOW_IDENTS.has(tok.text)) continue; - // Skip compiler-known builtins (err, ok, some, none, etc.) and CapCase - // identifiers (error-type constructors like `NetworkError(...)`). - if (BOTSCRIPT_BUILTIN_CALLS.has(tok.text) || /^[A-Z]/.test(tok.text)) continue; + // Skip compiler-known stdlib builtins (ok, err, some, none, etc.). + if (STDLIB_VALUE_CALL_NAMES.has(tok.text)) continue; - // Skip property accesses: `obj.helper(...)` or `obj?.helper(...)`. + // Skip method identifiers that are part of a property/member access. const prevIdx = prevSignificant(tokens, i - 1); const prev = tokens[prevIdx]; if (prev && ((prev.kind === "punct" && prev.text === ".") || prev.kind === "questionDot")) continue; - // Must be followed by `(` to be a call. const nextIdx = nextSignificant(tokens, i + 1); const next = tokens[nextIdx]; + + // Detect member calls on unknown namespace objects: `db.read()` or `api.helper()`. + // If this ident is followed by `.`, it is used as a namespace/object receiver. + // Stdlib namespaces (time, http, etc.) are handled by cap-check — skip those. + // Local names (fn parameters, local variables) are also excluded: `name.trim()` + // is a method on a known local, not a call to an unknown namespace import. + if (next && ((next.kind === "punct" && next.text === ".") || next.kind === "questionDot")) { + const afterDotIdx = nextSignificant(tokens, nextIdx + 1); + const afterDot = tokens[afterDotIdx]; + + if (afterDot && afterDot.kind === "ident") { + // Member-access call: `db.read(...)` or optional member: `db?.read(...)` + if (STDLIB_NAMESPACES.has(tok.text)) continue; + if (effectiveLocalNames.has(tok.text)) continue; + const afterMethodIdx = nextSignificant(tokens, afterDotIdx + 1); + const afterMethod = tokens[afterMethodIdx]; + // Direct call: `db.read(...)` or optional call: `db.read?.(...)` + const isMethodCall = + (afterMethod?.kind === "open" && afterMethod.text === "(") || + (afterMethod?.kind === "questionDot" && + tokens[nextSignificant(tokens, afterMethodIdx + 1)]?.kind === "open" && + tokens[nextSignificant(tokens, afterMethodIdx + 1)]?.text === "("); + if (isMethodCall) return true; // Opaque namespace/object method call + continue; // Property access without a following call — not opaque + } + + if (afterDot && afterDot.kind === "open" && afterDot.text === "(") { + // Optional bare call: `fn?.()` — `?.` is immediately followed by `(`. + // Local variables and callback parameters are not opaque external callers. + if (effectiveLocalNames.has(tok.text)) continue; + return true; + } + + continue; // `?.` followed by something other than ident or `(` — not a call + } + + // Must be followed by `(` to be a bare function call. if (!next || next.kind !== "open" || next.text !== "(") continue; + // CapCase idents followed by `(` are opaque external calls UNLESS they appear + // inside `err(TypeName...)` or `err(new TypeName...)` — those are error-type + // constructors, not user-defined functions. + if (/^[A-Z]/.test(tok.text)) { + // err(TypeName(...)) — prev is `(`, prevprev is `err` + if (prev && prev.kind === "open" && prev.text === "(") { + const prevPrevIdx = prevSignificant(tokens, prevIdx - 1); + const prevPrev = tokens[prevPrevIdx]; + if (prevPrev && prevPrev.kind === "ident" && prevPrev.text === "err") continue; + } + // err(new TypeName(...)) — prev is `new`, prev-of-prev is `(`, prev3 is `err` + if (prev && prev.kind === "ident" && prev.text === "new") { + const prevNewIdx = prevIdx; + const prevParenIdx = prevSignificant(tokens, prevNewIdx - 1); + const prevParen = tokens[prevParenIdx]; + if (prevParen && prevParen.kind === "open" && prevParen.text === "(") { + const prevErrIdx = prevSignificant(tokens, prevParenIdx - 1); + const prevErr = tokens[prevErrIdx]; + if (prevErr && prevErr.kind === "ident" && prevErr.text === "err") continue; + } + } + } + return true; } return false; } + +export function nextSignificant(tokens: Token[], start: number): number { + let i = start; + while (i < tokens.length) { + const t = tokens[i]; + if (!t) return i; + if ( + t.kind === "whitespace" || + t.kind === "newline" || + t.kind === "lineComment" || + t.kind === "blockComment" + ) { + i++; + continue; + } + return i; + } + return i; +} + +export function prevSignificant(tokens: Token[], start: number): number { + let i = start; + while (i >= 0) { + const t = tokens[i]; + if (!t) return i; + if ( + t.kind === "whitespace" || + t.kind === "newline" || + t.kind === "lineComment" || + t.kind === "blockComment" + ) { + i--; + continue; + } + return i; + } + return i; +} + diff --git a/packages/compiler/src/passes/cap-check.ts b/packages/compiler/src/passes/cap-check.ts index 30235f8d..4d44ad52 100644 --- a/packages/compiler/src/passes/cap-check.ts +++ b/packages/compiler/src/passes/cap-check.ts @@ -35,7 +35,14 @@ import { nextSignificant } from "./_callgraph.js"; import { atLeast, type VersionInfo } from "./version.js"; import { buildImportAliasMap, type ModuleEffects } from "../module-effects.js"; -/** stdlib namespace -> capability it consumes. Canonical source; import from here to avoid drift. */ +/** + * stdlib namespace → capability it consumes. + * + * Keys must exactly match `STDLIB_NAMESPACES` exported from `_callgraph.ts`. + * `_callgraph.ts` is the canonical source for the namespace name list; this + * record adds the per-namespace capability string. If a new stdlib namespace is + * added, update both. + */ export const STDLIB_TO_CAP: Readonly> = { http: "net", time: "time", diff --git a/packages/compiler/src/passes/dep-check.ts b/packages/compiler/src/passes/dep-check.ts index 4c803a26..f49fff42 100644 --- a/packages/compiler/src/passes/dep-check.ts +++ b/packages/compiler/src/passes/dep-check.ts @@ -21,27 +21,29 @@ * * DEP002 writes under-declared: same for writes { ... }. * + * DEP003 reads over-declared (warning): fn has at least one same-file (or + * moduleEffects) callee but no callee (transitively) declares `reads { x }`. + * The label likely became stale after a refactor removed the callee that + * originally justified it. Leaf fns are excluded — they may be the actual + * access point and the compiler cannot verify the body directly. + * + * DEP004 writes over-declared (warning): same for writes { ... }. + * * Same-file call resolution is performed by default. Cross-file calls are * opaque unless the caller provides a `moduleEffects` map (via * `TransformOptions.moduleEffects`): any function listed there is treated as * if its declaration were in the current file, so a caller that omits its * reads/writes labels fires DEP001/DEP002 exactly as for a same-file callee. * Dynamic dispatch and higher-order function arguments are not tracked. - * - * Over-declaration is intentionally NOT checked here. The reads/writes labels - * are user-defined strings; unlike stdlib capability names, the compiler has - * no way to verify that a declared label is actually accessed in the body. - * Over-declaration is therefore always allowed (conservative declarations are - * harmless and may even be intentional for documentation purposes). */ -import { BotscriptError } from "../diagnostics.js"; +import { BotscriptError, type Diagnostic } from "../diagnostics.js"; import { getErrorCode } from "../error-codes.js"; import { parseProgram } from "../parser/parse.js"; import type { FnDecl } from "../parser/parse-fn.js"; import { atLeast, type VersionInfo } from "./version.js"; import { locationOf } from "./_location.js"; -import { computeNesting, collectCallees } from "./_callgraph.js"; +import { computeNesting, collectCallees, hasOpaqueCall, collectTopLevelParamNames, collectFnBodyLocalNames } from "./_callgraph.js"; import { buildImportAliasMap, type ModuleEffects } from "../module-effects.js"; // --------------------------------------------------------------------------- @@ -66,19 +68,24 @@ interface FnRecord { // Entry point // --------------------------------------------------------------------------- +export interface DepCheckResult { + code: string; + warnings: ReadonlyArray; +} + export function passDepCheck( src: string, version: VersionInfo, moduleEffects?: ModuleEffects, -): string { - if (!atLeast(version.resolved, "0.9")) return src; +): DepCheckResult { + if (!atLeast(version.resolved, "0.9")) return { code: src, warnings: [] }; const allowGenerics = atLeast(version.resolved, "0.4"); const program = parseProgram(src, { allowGenerics, includeNestedFns: true }); const tokens = program.tokens; const decls = program.fns.map((s) => s.decl); - if (decls.length === 0) return src; + if (decls.length === 0) return { code: src, warnings: [] }; // Resolve import aliases: `import { fetchRow as fetchUser }` means a call // to `fetchUser` in this file should look up `fetchRow` in moduleEffects. @@ -87,8 +94,13 @@ export function passDepCheck( // Build maps for external (cross-file) effect declarations. const extReads = new Map>(); const extWrites = new Map>(); + // All declared names in moduleEffects (regardless of whether they have reads/writes). + // Used by DEP003/DEP004 to distinguish "known callee with no labels" from "unknown callee" + // — only known callees count as non-self callees; unknown ones are opaque. + const knownExternalNames = new Set(); if (moduleEffects) { for (const [name, eff] of Object.entries(moduleEffects)) { + knownExternalNames.add(name); if (eff.reads?.length) extReads.set(name, new Set(eff.reads)); if (eff.writes?.length) extWrites.set(name, new Set(eff.writes)); } @@ -100,11 +112,21 @@ export function passDepCheck( const fnNames = new Set(decls.map((d) => d.name)); // Include external function names (and their local aliases) in the callee - // scan so cross-file calls appear in the callees set. - const aliasedLocalNames = new Set(importAliases.keys()); + // scan so cross-file calls appear in the callees set. knownExternalNames + // covers moduleEffects entries with no reads/writes so calls to them are + // collected and treated as non-self callees by DEP003/DEP004. + // + // Only include aliases whose resolved target is actually present in + // moduleEffects. An alias for an un-listed import is opaque — including it + // would mask the opaque call from hasOpaqueCall and produce false warnings. + const aliasedKnownNames = new Set( + [...importAliases.entries()] + .filter(([, resolved]) => knownExternalNames.has(resolved)) + .map(([alias]) => alias), + ); const allCalleeNames = - extReads.size > 0 || extWrites.size > 0 || aliasedLocalNames.size > 0 - ? new Set([...fnNames, ...extReads.keys(), ...extWrites.keys(), ...aliasedLocalNames]) + knownExternalNames.size > 0 || aliasedKnownNames.size > 0 + ? new Set([...fnNames, ...knownExternalNames, ...aliasedKnownNames]) : fnNames; // Precompute each fn's nested (descendant) decls once via a single sweep, @@ -236,7 +258,92 @@ export function passDepCheck( } } - return src; + // 5. DEP003/DEP004: over-declared (warning-level). + // For each fn that has at least one same-file (or moduleEffects) non-self callee, + // compute which declared reads/writes labels are NOT justified by any callee. + // Leaf fns (no tracked callees) and self-only-recursive fns are excluded — + // they may be the actual access point and the compiler can't verify the body. + // Callback parameter annotations (paramReads/paramWrites) are treated as + // implicit callee justification: a wrapper that receives a reads { db } callback + // and propagates that label upward is not over-declaring. + // Fns that call any opaque (unlisted) function are also excluded — the unknown + // callee may be the actual read/write point that justifies the label. + // + // Justification uses a DFS over the callee graph rooted at rec.decl's direct + // callees, collecting each reachable fn's DECLARED reads/writes. The root fn + // itself is pre-added to `visited` so the DFS stops at it during mutual recursion + // (f calls g, g calls f → DFS skips f when encountered via g, preventing + // f from justifying its own labels through the cycle). + const warnings: Diagnostic[] = []; + for (const rec of records.values()) { + if (rec.callees.size === 0) continue; + + // Seed justification from callback parameter annotations. A wrapper fn that + // accepts a `fn() reads { db } -> T` callback and propagates reads { db } + // upward is not over-declaring, even if regular callees don't read db. + const calleeReads = new Set(rec.decl.paramReads); + const calleeWrites = new Set(rec.decl.paramWrites); + + // DFS from direct callees, collecting their declared reads/writes transitively. + // rec.decl is pre-added so the DFS stops there, preventing circular justification. + const visited = new Set([rec.decl]); + let hasNonSelfCallee = false; + + const dfsStack: string[] = [...rec.callees]; + while (dfsStack.length > 0) { + const calleeName = dfsStack.pop()!; + const calleeDecls = declsByName.get(calleeName); + if (calleeDecls) { + for (const calleeDecl of calleeDecls) { + if (visited.has(calleeDecl)) continue; + visited.add(calleeDecl); + hasNonSelfCallee = true; + const callee = records.get(calleeDecl); + if (!callee) continue; + for (const label of callee.decl.reads ?? []) calleeReads.add(label); + for (const label of callee.decl.writes ?? []) calleeWrites.add(label); + for (const nextCallee of callee.callees) dfsStack.push(nextCallee); + } + continue; + } + const resolvedCallee = importAliases.get(calleeName) ?? calleeName; + if (!knownExternalNames.has(resolvedCallee)) continue; + hasNonSelfCallee = true; + const extR = extReads.get(resolvedCallee); + if (extR) for (const label of extR) calleeReads.add(label); + const extW = extWrites.get(resolvedCallee); + if (extW) for (const label of extW) calleeWrites.add(label); + } + + // Self-only-recursive fns are treated like leaves. + if (!hasNonSelfCallee) continue; + + // Suppress when the fn calls any opaque (unlisted) external function — that + // unknown callee may be the actual read/write point that justifies the label. + // Combine param names and local variable names so member calls on any known + // local (e.g. `str.trim()`, `name.length`) are not mistaken for opaque + // namespace/import method calls and don't suppress DEP003/DEP004. + const inner = innerByDecl.get(rec.decl) ?? []; + const paramNames = collectTopLevelParamNames(rec.decl.args); + const bodyLocals = collectFnBodyLocalNames(tokens, rec.decl, inner); + const localNames = new Set([...paramNames, ...bodyLocals]); + // Include param names in knownNames so bare calls to callback parameters + // (e.g. `loader()`) are not treated as opaque external calls — their effects + // are already captured via paramReads/paramWrites. + const knownWithParams = new Set([...allCalleeNames, ...paramNames]); + if (hasOpaqueCall(tokens, rec.decl, inner, knownWithParams, localNames)) continue; + + const overDeclaredReads = [...rec.declaredReads].filter(l => !calleeReads.has(l)).sort(); + if (overDeclaredReads.length > 0) { + warnings.push(mkOverDeclaredWarning(src, rec, "reads", overDeclaredReads)); + } + const overDeclaredWrites = [...rec.declaredWrites].filter(l => !calleeWrites.has(l)).sort(); + if (overDeclaredWrites.length > 0) { + warnings.push(mkOverDeclaredWarning(src, rec, "writes", overDeclaredWrites)); + } + } + + return { code: src, warnings }; } // --------------------------------------------------------------------------- @@ -327,3 +434,48 @@ function mkError( return new BotscriptError([diagnostic]); } + +function mkOverDeclaredWarning( + src: string, + rec: FnRecord, + kind: "reads" | "writes", + overLabels: string[], +): Diagnostic { + const code = kind === "reads" ? "DEP003" : "DEP004"; + const entry = getErrorCode(code)!; + const { line, column } = locationOf(src, rec.decl.fnKeywordStart); + const nameEnd = rec.decl.nameStart + rec.decl.name.length; + + const labelList = overLabels.join(", "); + const firstLabel = overLabels[0]!; + + const notPropagated = + overLabels.length === 1 + ? `'${firstLabel}' is not declared by any tracked callee` + : `[${labelList}] are not declared by any tracked callee`; + const message = + `fn '${rec.decl.name}' declares ${kind} { ${labelList} } but ${notPropagated}; ` + + `annotation may be stale`; + + const proposed = + kind === "reads" + ? [...rec.declaredReads].filter((l) => !overLabels.includes(l)).sort() + : [...rec.declaredWrites].filter((l) => !overLabels.includes(l)).sort(); + + return { + code, + severity: "warning", + file: null, + line, + column, + start: rec.decl.fnKeywordStart, + end: nameEnd, + message, + rule: entry.rule, + idiom: entry.idiom, + rewrite: + proposed.length > 0 + ? `fn ${rec.decl.name}(...) ${kind} { ${proposed.join(", ")} } -> ... // remove stale label: ${labelList}` + : `fn ${rec.decl.name}(...) -> ... // remove stale ${kind} {} clause: ${labelList}`, + }; +} diff --git a/packages/compiler/tests/dep-check.test.ts b/packages/compiler/tests/dep-check.test.ts index 688f2cfb..556498e1 100644 --- a/packages/compiler/tests/dep-check.test.ts +++ b/packages/compiler/tests/dep-check.test.ts @@ -3,6 +3,8 @@ * * DEP001: fn A calls fn B which reads { x }, but A doesn't declare reads { x }. * DEP002: fn A calls fn B which writes { x }, but A doesn't declare writes { x }. + * DEP003: fn declares reads { x } but no callee (transitively) reads { x }. + * DEP004: fn declares writes { x } but no callee (transitively) writes { x }. */ import { describe, expect, it } from "vitest"; @@ -219,3 +221,429 @@ describe("parameter-default exclusion (issue #70)", () => { expect(() => compile(src)).toThrow("DEP001"); }); }); + +// --------------------------------------------------------------------------- +// DEP003: reads over-declared +// --------------------------------------------------------------------------- + +describe("DEP003: reads over-declared (0.9+)", () => { + it("fires when a fn declares reads { x } but no callee declares reads { x }", () => { + const src = + "?bs 0.9\n" + + "fn formatName(s: string) -> string = s\n" + + "fn getUserName(id: string) reads { userDb } -> string = formatName(\"Alice\")\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + expect(result.warnings.find((w) => w.code === "DEP003")!.message).toContain("userDb"); + }); + + it("does NOT fire when the declared label is justified by a same-file callee", () => { + const src = + "?bs 0.9\n" + + "fn getUser(id: string) reads { userDb } -> string = id\n" + + "fn getUserName(id: string) reads { userDb } -> string = getUser(id)\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does NOT fire when the declared label is justified transitively", () => { + const src = + "?bs 0.9\n" + + "fn readDb(id: string) reads { userDb } -> string = id\n" + + "fn getUser(id: string) reads { userDb } -> string = readDb(id)\n" + + "fn getUserName(id: string) reads { userDb } -> string = getUser(id)\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("fires a single warning listing all over-declared labels when multiple are stale", () => { + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db, cache } -> string = helper()\n"; + const result = transform(src); + const dep3warns = result.warnings.filter((w) => w.code === "DEP003"); + expect(dep3warns).toHaveLength(1); + expect(dep3warns[0]!.message).toContain("db"); + expect(dep3warns[0]!.message).toContain("cache"); + }); + + it("fires warning with severity 'warning'", () => { + const src = + "?bs 0.9\n" + + "fn noop() -> string = \"ok\"\n" + + "fn f() reads { x } -> string = noop()\n"; + const result = transform(src); + const w = result.warnings.find((w) => w.code === "DEP003"); + expect(w).toBeDefined(); + expect(w!.severity).toBe("warning"); + }); + + it("does not fire below ?bs 0.9", () => { + const src = + "?bs 0.8\n" + + "fn helper() -> string = \"ok\"\n" + + "fn f() reads { x } -> string = helper()\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does not throw — DEP003 is non-blocking", () => { + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"Alice\"\n" + + "fn f() reads { userDb } -> string = helper()\n"; + expect(() => compile(src)).not.toThrow(); + }); +}); + +// --------------------------------------------------------------------------- +// DEP004: writes over-declared +// --------------------------------------------------------------------------- + +describe("DEP004: writes over-declared (0.9+)", () => { + it("fires when a fn declares writes { x } but no callee declares writes { x }", () => { + const src = + "?bs 0.9\n" + + "fn noop(msg: string) -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { noop(msg); }\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(true); + expect(result.warnings.find((w) => w.code === "DEP004")!.message).toContain("auditLog"); + }); + + it("does NOT fire when the declared label is justified by a same-file callee", () => { + const src = + "?bs 0.9\n" + + "fn writeAudit(msg: string) writes { auditLog } -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { writeAudit(msg) }\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); + }); + + it("fires with severity 'warning' and does not throw", () => { + const src = + "?bs 0.9\n" + + "fn noop(msg: string) -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { noop(msg); }\n"; + expect(() => compile(src)).not.toThrow(); + const result = transform(src); + expect(result.warnings.find((w) => w.code === "DEP004")!.severity).toBe("warning"); + }); + + it("does not fire below ?bs 0.9", () => { + const src = + "?bs 0.8\n" + + "fn noop(msg: string) -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { noop(msg); }\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// DEP003/DEP004: self-recursive fns and callback parameter justification +// --------------------------------------------------------------------------- + +describe("DEP003/DEP004: transitive callee justification", () => { + it("does NOT fire DEP003 when a multi-hop callee chain justifies the label", () => { + // f reads { db } calls g, g calls h, h reads { db }. + // The DFS must reach h through g to find the justification. + const src = + "?bs 0.9\n" + + "fn h() reads { db } -> string = \"row\"\n" + + "fn g() reads { db } -> string = h()\n" + + "fn f() reads { db } -> string = g()\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does NOT fire DEP003 when a callee (leaf) declares the same label", () => { + // f reads { db } calls g, g reads { db } (leaf). f is justified by g. + const src = + "?bs 0.9\n" + + "fn g() reads { db } -> string = \"row\"\n" + + "fn f() reads { db } -> string = g()\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); +}); + +describe("DEP003/DEP004: self-recursive fn exclusion", () => { + it("does not fire DEP003 for a self-recursive fn — treated as leaf/access-point", () => { + const src = + "?bs 0.9\n" + + "fn countdown(n: number) reads { store } -> void {\n" + + " if (n > 0) countdown(n - 1);\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does not fire DEP004 for a self-recursive fn — treated as leaf/access-point", () => { + const src = + "?bs 0.9\n" + + "fn accumulate(n: number) writes { store } -> void {\n" + + " if (n > 0) accumulate(n - 1);\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); + }); +}); + +describe("DEP003/DEP004: callback parameter justification", () => { + it("does not fire DEP003 when reads label is justified by a callback parameter annotation", () => { + const src = + "?bs 0.9\n" + + "fn noop() -> void { }\n" + + "fn withCache(loader: () reads { cache } -> string) reads { cache } -> string {\n" + + " noop();\n" + + " loader()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does not fire DEP004 when writes label is justified by a callback parameter annotation", () => { + const src = + "?bs 0.9\n" + + "fn noop() -> void { }\n" + + "fn withAudit(cb: () writes { auditLog } -> void) writes { auditLog } -> void {\n" + + " noop();\n" + + " cb()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// DEP003/DEP004: opaque external call suppression +// --------------------------------------------------------------------------- + +describe("DEP003/DEP004: opaque external call suppression", () => { + it("does not fire DEP003 when fn also calls an unlisted external helper", () => { + // localHelper is tracked (same-file), unknownHelper is NOT in allCalleeNames. + // DEP003 must be suppressed because unknownHelper may be the actual reader. + const src = + "?bs 0.9\n" + + "fn localHelper() -> string = \"x\"\n" + + "fn f() reads { db } -> string { localHelper(); unknownHelper() }\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does not fire DEP004 when fn also calls an unlisted external helper", () => { + const src = + "?bs 0.9\n" + + "fn localHelper() -> void { }\n" + + "fn f() writes { db } -> void { localHelper(); unknownHelper() }\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); + }); + + it("fires DEP003 when all callees are tracked and none declare the label", () => { + const src = + "?bs 0.9\n" + + "fn localHelper() -> string = \"x\"\n" + + "fn f() reads { db } -> string = localHelper()\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("fires DEP003 even when fn body contains if/while control flow", () => { + // `if (cond)` must not be treated as an opaque function call — it's + // control flow, not an external callee. + const src = + "?bs 0.9\n" + + "fn localHelper(x: number) -> string = \"ok\"\n" + + "fn f(flag: bool) reads { db } -> string {\n" + + " if (flag) localHelper(1);\n" + + " localHelper(2)\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("fires DEP004 even when fn body contains while control flow", () => { + const src = + "?bs 0.9\n" + + "fn step(n: number) -> void { }\n" + + "fn f(n: number) writes { log } -> void {\n" + + " while (n > 0) step(n);\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(true); + }); + + it("does not fire DEP003 when fn calls an unknown namespace object method", () => { + // `dbClient.query()` is a member call on an unknown namespace import. + // hasOpaqueCall must treat it as opaque so DEP003 is suppressed. + const src = + "?bs 0.9\n" + + "fn helper() -> void { }\n" + + "fn f() reads { db } -> string {\n" + + " helper();\n" + + " dbClient.query()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("does not fire DEP004 when fn calls an unknown namespace object method", () => { + const src = + "?bs 0.9\n" + + "fn step() -> void { }\n" + + "fn f() writes { log } -> void {\n" + + " step();\n" + + " logger.write()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); + }); + + it("does not suppress DEP003 for stdlib namespace method calls (time.now)", () => { + // Stdlib namespaces are excluded from opaque detection — they are handled + // by cap-check, not by DEP003 suppression. + // Use `unsafe` to suppress UNS005 so the DEP003 check is reached. + const src = + "?bs 0.9\n" + + "fn helper() -> void { }\n" + + "fn f() uses { time } reads { db } -> number {\n" + + " helper();\n" + + " unsafe \"known\" { time.now() }\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("fires DEP003 even when fn calls a method on a string parameter (name.trim())", () => { + // `name.trim()` is a method call on a fn parameter — it is NOT an opaque + // external import. hasOpaqueCall must exclude parameter names from the + // namespace-receiver check so DEP003 is not incorrectly suppressed. + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f(name: string) reads { db } -> string {\n" + + " helper();\n" + + " name.trim()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("fires DEP004 even when fn calls a method on an array parameter (items.map())", () => { + const src = + "?bs 0.9\n" + + "fn step() -> void { }\n" + + "fn f(items: string[]) writes { log } -> void {\n" + + " step();\n" + + " items.map(step)\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP004")).toBe(true); + }); + + it("fires DEP003 even when fn calls a method on a local const binding (name.trim())", () => { + // Local `const name = ...` followed by `name.trim()` is NOT an opaque external call. + // Without collectFnBodyLocalNames, `name` is absent from localNames and name.trim() + // incorrectly suppresses DEP003. + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db } -> string {\n" + + " const name = helper();\n" + + " name.trim()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("fires DEP003 even when fn constructs an error type (err(NetworkError{...})) as an arg", () => { + // CapCase error-type constructors inside err(...) must NOT suppress DEP003. + // Standalone CapCase calls like `LoadUser()` SHOULD suppress (opaque external). + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db } -> string {\n" + + " helper();\n" + + " err(NotFoundError { msg: \"x\" })\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("suppresses DEP003 when fn calls an unknown CapCase external function (LoadUser())", () => { + // A CapCase function call that is NOT inside err(...) is a genuine opaque + // external and should suppress DEP003 — the external may perform the read. + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db } -> string {\n" + + " helper();\n" + + " LoadUser()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("suppresses DEP003 when param has object type annotation — inner idents are not param names", () => { + // `opts: { dbClient: string }` — `dbClient` is a property name in the type + // annotation, not an actual fn parameter. Before the brace-depth fix, + // collectTopLevelParamNames would add `dbClient` to localNames, causing + // `dbClient.query()` in the body to look like a local param method call + // rather than an opaque external call, which incorrectly allowed DEP003 to fire. + const src = + "?bs 0.9\n" + + "fn helper() -> void { }\n" + + "fn f(opts: { dbClient: string }) reads { db } -> void {\n" + + " helper();\n" + + " dbClient.query()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("suppresses DEP003 when fn uses optional bare call to unknown external fn?.()", () => { + // `externalLib?.()` is an optional call to a function not declared in this file — opaque, + // so DEP003 must be suppressed (the external may perform the read). + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db } -> void {\n" + + " helper();\n" + + " externalLib?.()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("suppresses DEP003 when fn calls an unknown namespace via optional member call obj.method?.()", () => { + // `db.read?.()` — optional method call on an unknown namespace object. + // hasOpaqueCall must detect the call pattern even with the `?.` operator. + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db } -> void {\n" + + " helper();\n" + + " externalDb.read?.()\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("fires DEP003 when fn uses err(new TypeName(...)) — new-form error construction is not opaque", () => { + // `err(new NetworkError(...))` is the botscript `new`-form error constructor. + // It must NOT suppress DEP003 — the error construction is not an external read. + // (throws { NetworkError } declared so THR002 does not fire independently) + const src = + "?bs 0.9\n" + + "fn helper() -> string = \"x\"\n" + + "fn f() reads { db } throws { NetworkError } -> Result {\n" + + " helper();\n" + + " return err(new NetworkError(\"failed\"))\n" + + "}\n"; + const result = transform(src); + expect(result.warnings.some((w) => w.code === "DEP003")).toBe(true); + }); +}); diff --git a/packages/compiler/tests/module-effects.test.ts b/packages/compiler/tests/module-effects.test.ts index ba1e776e..8d64972b 100644 --- a/packages/compiler/tests/module-effects.test.ts +++ b/packages/compiler/tests/module-effects.test.ts @@ -16,6 +16,10 @@ function compile(src: string, moduleEffects?: ModuleEffects): string { return transform(src, { moduleEffects }).code; } +function compileWarnings(src: string, moduleEffects?: ModuleEffects) { + return transform(src, { moduleEffects }).warnings; +} + // --------------------------------------------------------------------------- // DEP001: cross-file reads transitivity // --------------------------------------------------------------------------- @@ -310,11 +314,14 @@ describe("buildModuleEffects builder", () => { expect(Object.hasOwn(effects, "__proto__")).toBe(true); }); - it("omits fns with no declared effects", () => { + it("includes fns with no declared effects as empty entries", () => { + // Pure helpers (no reads/writes/throws) are included so DEP003/DEP004 can + // distinguish "known callee with no labels" from "unknown opaque callee". const effects = buildModuleEffects([ - "?bs 0.9\nfn pure(x: string) -> string = x\n", + "?bs 0.9\nfn helper(x: string) -> string = x\n", ]); - expect(Object.keys(effects)).toEqual([]); + expect(Object.hasOwn(effects, "helper")).toBe(true); + expect(effects.helper).toEqual({}); }); it("includes only exported fns when the source has export statements", () => { @@ -564,3 +571,41 @@ describe("CAP001: cross-file capability transitivity via moduleEffects", () => { expect(() => compile(src, moduleEffects)).not.toThrow(); }); }); + +// --------------------------------------------------------------------------- +// DEP003/DEP004: cross-file callee with no labels counts as non-self callee +// --------------------------------------------------------------------------- + +describe("DEP003/DEP004: moduleEffects entry with no reads/writes is a known callee", () => { + it("fires DEP003 when fn calls a moduleEffects entry with no reads and declares reads { x }", () => { + // `helper` is a known external callee ({} — no declared reads) so the + // caller's `reads { userDb }` is over-declared. Before the fix, + // `helper` was absent from allCalleeNames and the fn was treated as a + // leaf, suppressing the warning. + const src = + "?bs 0.9\n" + + "fn load(id: string) reads { userDb } -> string = helper(id)\n"; + const mods: ModuleEffects = { helper: {} }; + const warnings = compileWarnings(src, mods); + expect(warnings.some((w) => w.code === "DEP003")).toBe(true); + }); + + it("does NOT fire DEP003 when the external callee is unknown (not in moduleEffects)", () => { + // Unknown callees are opaque: we cannot tell whether they read anything, + // so we must not warn about the caller's annotation being stale. + const src = + "?bs 0.9\n" + + "fn load(id: string) reads { userDb } -> string = helper(id)\n"; + const warnings = compileWarnings(src); + expect(warnings.some((w) => w.code === "DEP003")).toBe(false); + }); + + it("fires DEP004 when fn calls a moduleEffects entry with no writes and declares writes { x }", () => { + const src = + "?bs 0.9\n" + + "fn persist(id: string) writes { auditLog } -> string = helper(id)\n"; + const mods: ModuleEffects = { helper: {} }; + const warnings = compileWarnings(src, mods); + expect(warnings.some((w) => w.code === "DEP004")).toBe(true); + }); +}); diff --git a/packages/mcp/src/explanations.ts b/packages/mcp/src/explanations.ts index 2a0ca810..aacf12b0 100644 --- a/packages/mcp/src/explanations.ts +++ b/packages/mcp/src/explanations.ts @@ -563,9 +563,8 @@ export const EXPLANATIONS: Readonly> = { "`reads { y }`, then both B and A must declare `reads { y }`.\n\n" + "The purpose is completeness: reading A's header should tell you every resource " + "category A (or anything it calls) touches, without tracing through the call graph.\n\n" + - "Over-declaration is always allowed — declaring more than the minimum is conservative " + - "and harmless. DEP001 only fires on under-declaration (a label that is reachable but " + - "not declared).", + "Over-declaration is warned about from `?bs 0.9` (see DEP003). DEP001 only fires on " + + "under-declaration (a label that is reachable but not declared).", example: { fails: "?bs 0.9\n" + @@ -629,7 +628,7 @@ export const EXPLANATIONS: Readonly> = { "`writes { x }`. The rule extends to any depth.\n\n" + "The purpose is completeness: reading A's header should tell you every resource " + "category A (or anything it calls) writes to, without tracing through the call graph.\n\n" + - "Over-declaration is always allowed. DEP002 only fires on under-declaration.", + "Over-declaration is warned about from `?bs 0.9` (see DEP004). DEP002 only fires on under-declaration.", example: { fails: "?bs 0.9\n" + @@ -641,6 +640,58 @@ export const EXPLANATIONS: Readonly> = { "fn recordEvent(id: string) writes { metrics } -> void { updateMetrics(id); }\n", }, }, + DEP003: { + code: "DEP003", + title: "fn declares reads {} label not justified by any callee in the same file (warning)", + body: + "From `?bs 0.9`, DEP003 fires as a **warning** (not an error) when fn A has same-file " + + "callees but no callee (direct or transitive) declares `reads { x }` that A also declares. " + + "The label is not justified by the call graph — a common sign of a refactor that removed " + + "the callee that originally justified the annotation.\n\n" + + "**Scope:** only fires for fns with at least one same-file (or moduleEffects) callee. " + + "Leaf fns are excluded — they may be the actual access point, and the compiler cannot " + + "scan the body for direct resource accesses (reads {} labels are user-defined strings, " + + "not stdlib references).\n\n" + + "DEP003 is also suppressed when the function body contains any opaque or untracked external " + + "call (a call to a function not visible to the same-file call graph) — the unknown callee may " + + "be the actual read site, so the warning is withheld to avoid false positives.\n\n" + + "DEP003 is gated on `?bs 0.9`. The symmetrical under-declaration check is DEP001.", + example: { + fails: + "?bs 0.9\n" + + "fn helper(id: string) -> string = id\n" + + "fn getUserName(id: string) reads { userDb } -> string = helper(id)\n", + passes: + "?bs 0.9\n" + + "fn getUser(id: string) reads { userDb } -> string = id\n" + + "fn getUserName(id: string) reads { userDb } -> string = getUser(id)\n", + }, + }, + DEP004: { + code: "DEP004", + title: "fn declares writes {} label not justified by any callee in the same file (warning)", + body: + "From `?bs 0.9`, DEP004 fires as a **warning** (not an error) when fn A has same-file " + + "callees but no callee (direct or transitive) declares `writes { x }` that A also declares. " + + "The label is not justified by the call graph and may be stale.\n\n" + + "**Scope:** only fires for fns with at least one same-file (or moduleEffects) callee. " + + "Leaf fns are excluded — the compiler cannot scan the body for direct resource modifications " + + "(writes {} labels are user-defined strings).\n\n" + + "DEP004 is also suppressed when the function body contains any opaque or untracked external " + + "call (a call to a function not visible to the same-file call graph) — the unknown callee may " + + "be the actual write site, so the warning is withheld to avoid false positives.\n\n" + + "DEP004 is gated on `?bs 0.9`. The symmetrical under-declaration check is DEP002.", + example: { + fails: + "?bs 0.9\n" + + "fn helper(msg: string) -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { helper(msg) }\n", + passes: + "?bs 0.9\n" + + "fn writeAudit(msg: string) writes { auditLog } -> void { }\n" + + "fn logEvent(msg: string) writes { auditLog } -> void { writeAudit(msg) }\n", + }, + }, THR001: { code: "THR001", title: "fn transitively throws an exception type not declared in its header", diff --git a/packages/mcp/tests/server.test.ts b/packages/mcp/tests/server.test.ts index 23d79133..cdf61759 100644 --- a/packages/mcp/tests/server.test.ts +++ b/packages/mcp/tests/server.test.ts @@ -52,6 +52,8 @@ describe("botscript-mcp explanations", () => { "CAP003", "DEP001", "DEP002", + "DEP003", + "DEP004", "EFF002", "EFF003", "EFF004",