feat(compiler): DEP003/DEP004 — warn when reads/writes labels are over-declared (?bs 0.9+)#100
Open
marcelofarias wants to merge 16 commits into
Open
feat(compiler): DEP003/DEP004 — warn when reads/writes labels are over-declared (?bs 0.9+)#100marcelofarias wants to merge 16 commits into
marcelofarias wants to merge 16 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds warning-level DEP003/DEP004 diagnostics for over-declared reads {} / writes {} labels in the compiler and exposes them through registry, tests, and MCP explanations.
Changes:
- Implements DEP003/DEP004 warning collection in
dep-check. - Adds registry and MCP explanation entries for the new diagnostics.
- Adds compiler and MCP tests covering the new diagnostic codes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/compiler/src/passes/dep-check.ts |
Adds over-declaration warning logic and result shape with warnings. |
packages/compiler/src/error-codes.ts |
Registers DEP003/DEP004 metadata and examples. |
packages/compiler/tests/dep-check.test.ts |
Adds tests for DEP003/DEP004 warning behavior. |
packages/mcp/src/explanations.ts |
Adds MCP explanations and examples for DEP003/DEP004. |
packages/mcp/tests/server.test.ts |
Adds DEP003/DEP004 to known MCP explanation codes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+279
to
+283
| 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(); |
Comment on lines
+429
to
+434
| "// before — getUserName declares reads { userDb } but no callee reads userDb\n" + | ||
| "?bs 0.9\n" + | ||
| "fn getUserName(id: string) reads { userDb } -> string = \"Alice\" // DEP003\n\n" + | ||
| "// after — remove stale label\n" + | ||
| "?bs 0.9\n" + | ||
| "fn getUserName(id: string) -> string = \"Alice\"", |
Comment on lines
+448
to
+453
| "// before — logEvent declares writes { auditLog } but no callee writes auditLog\n" + | ||
| "?bs 0.9\n" + | ||
| "fn logEvent(msg: string) writes { auditLog } -> void { } // DEP004\n\n" + | ||
| "// after — remove stale label\n" + | ||
| "?bs 0.9\n" + | ||
| "fn logEvent(msg: string) -> void { }", |
Comment on lines
+253
to
+277
| const warnings: Diagnostic[] = []; | ||
| for (const rec of records.values()) { | ||
| if (rec.callees.size === 0) continue; | ||
|
|
||
| // Collect labels that propagate from callees (excluding this fn's own declaration). | ||
| const calleeReads = new Set<string>(); | ||
| const calleeWrites = new Set<string>(); | ||
| for (const calleeName of rec.callees) { | ||
| const calleeDecls = declsByName.get(calleeName); | ||
| if (calleeDecls) { | ||
| for (const calleeDecl of calleeDecls) { | ||
| if (calleeDecl === rec.decl) continue; | ||
| const callee = records.get(calleeDecl); | ||
| if (!callee) continue; | ||
| for (const label of callee.transitiveReads.keys()) calleeReads.add(label); | ||
| for (const label of callee.transitiveWrites.keys()) calleeWrites.add(label); | ||
| } | ||
| continue; | ||
| } | ||
| const resolvedCallee = importAliases.get(calleeName) ?? calleeName; | ||
| 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); | ||
| } |
marcelofarias
pushed a commit
that referenced
this pull request
May 30, 2026
…mWrites, fix DEP003/DEP004 examples - Self-only-recursive fns (callees contains only the fn itself) are now treated as leaves and excluded from DEP003/DEP004, consistent with the intent that leaf/access-point fns should be exempt. - Seed calleeReads/calleeWrites from rec.decl.paramReads/paramWrites so wrapper fns that receive a callback with reads/writes annotations are not falsely warned — the annotation is justified by the callback contract. - Fix DEP003/DEP004 `example` entries in error-codes.ts: the prior examples used leaf fns (no callees), which are excluded from the check and would never actually produce the diagnostic. Replaced with non-leaf examples. - Add 4 new tests: self-recursive DEP003/DEP004 suppression, and callback parameter reads/writes justification for both variants. Addresses Copilot review feedback on PR #100. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+417
to
+419
| DEP003: { | ||
| code: "DEP003", | ||
| title: "fn declares reads {} label not justified by any callee in the same file (warning)", |
Comment on lines
+281
to
+286
| it("does not fire below ?bs 0.9", () => { | ||
| const src = | ||
| "?bs 0.8\n" + | ||
| "fn f() reads { x } -> string = \"ok\"\n"; | ||
| const result = transform(src); | ||
| expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); |
Comment on lines
+289
to
+293
| it("does not throw — DEP003 is non-blocking", () => { | ||
| const src = | ||
| "?bs 0.9\n" + | ||
| "fn f() reads { userDb } -> string = \"Alice\"\n"; | ||
| expect(() => compile(src)).not.toThrow(); |
Comment on lines
+331
to
+336
| it("does not fire below ?bs 0.9", () => { | ||
| const src = | ||
| "?bs 0.8\n" + | ||
| "fn logEvent(msg: string) writes { auditLog } -> void { }\n"; | ||
| const result = transform(src); | ||
| expect(result.warnings.some((w) => w.code === "DEP004")).toBe(false); |
| expect(result.warnings.some((w) => w.code === "DEP003")).toBe(false); | ||
| }); | ||
|
|
||
| it("fires for each over-declared label separately when multiple are stale", () => { |
Comment on lines
+278
to
+283
| hasNonSelfCallee = true; | ||
| const resolvedCallee = importAliases.get(calleeName) ?? calleeName; | ||
| 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); |
Comment on lines
+614
to
+616
| DEP003: { | ||
| code: "DEP003", | ||
| title: "fn declares reads {} label not justified by any callee in the same file (warning)", |
Comment on lines
+97
to
+100
| // 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<string>(); |
Comment on lines
+300
to
+304
| 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(); |
Comment on lines
+114
to
+128
| const tok = tokens[i]; | ||
| if (!tok || tok.kind !== "ident") continue; | ||
| if (tok.text === fn.name) continue; | ||
| if (knownNames.has(tok.text)) continue; | ||
|
|
||
| // Skip property accesses: `obj.helper(...)` or `obj?.helper(...)`. | ||
| 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]; | ||
| if (!next || next.kind !== "open" || next.text !== "(") continue; |
Comment on lines
118
to
122
| const aliasedLocalNames = new Set(importAliases.keys()); | ||
| const allCalleeNames = | ||
| extReads.size > 0 || extWrites.size > 0 || aliasedLocalNames.size > 0 | ||
| ? new Set([...fnNames, ...extReads.keys(), ...extWrites.keys(), ...aliasedLocalNames]) | ||
| knownExternalNames.size > 0 || aliasedLocalNames.size > 0 | ||
| ? new Set([...fnNames, ...knownExternalNames, ...aliasedLocalNames]) | ||
| : fnNames; |
Comment on lines
+290
to
+291
| for (const label of callee.transitiveReads.keys()) calleeReads.add(label); | ||
| for (const label of callee.transitiveWrites.keys()) calleeWrites.add(label); |
Comment on lines
+431
to
+434
| const message = | ||
| `fn '${rec.decl.name}' declares ${kind} { ${labelList} } ` + | ||
| `but no callee in this file transitively declares ${kind} { ${firstLabel} }; ` + | ||
| `annotation may be stale`; |
Comment on lines
+149
to
+153
| // Skip property accesses: `obj.helper(...)` or `obj?.helper(...)`. | ||
| const prevIdx = prevSignificant(tokens, i - 1); | ||
| const prev = tokens[prevIdx]; | ||
| if (prev && ((prev.kind === "punct" && prev.text === ".") || prev.kind === "questionDot")) | ||
| continue; |
Comment on lines
+97
to
+103
| // 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<string>(); | ||
| if (moduleEffects) { | ||
| for (const [name, eff] of Object.entries(moduleEffects)) { | ||
| knownExternalNames.add(name); |
Comment on lines
+168
to
+177
| if (next && ((next.kind === "punct" && next.text === ".") || next.kind === "questionDot")) { | ||
| if (STDLIB_NAMESPACES.has(tok.text)) continue; | ||
| const methodIdx = nextSignificant(tokens, nextIdx + 1); | ||
| const methodTok = tokens[methodIdx]; | ||
| if (methodTok && methodTok.kind === "ident") { | ||
| const afterMethodIdx = nextSignificant(tokens, methodIdx + 1); | ||
| const afterMethod = tokens[afterMethodIdx]; | ||
| if (afterMethod && afterMethod.kind === "open" && afterMethod.text === "(") { | ||
| return true; // Opaque namespace/object method call | ||
| } |
| * Botscript stdlib namespace names. Member calls on these (e.g. `time.now()`, | ||
| * `http.get()`) are handled by cap-check/uns-check, not by `hasOpaqueCall`. | ||
| */ | ||
| const STDLIB_NAMESPACES = new Set(["http", "time", "random", "fs", "stdout", "stderr"]); |
Comment on lines
+251
to
+264
| if (next && ((next.kind === "punct" && next.text === ".") || next.kind === "questionDot")) { | ||
| if (STDLIB_NAMESPACES.has(tok.text)) continue; | ||
| if (localNames?.has(tok.text)) continue; | ||
| const methodIdx = nextSignificant(tokens, nextIdx + 1); | ||
| const methodTok = tokens[methodIdx]; | ||
| if (methodTok && methodTok.kind === "ident") { | ||
| const afterMethodIdx = nextSignificant(tokens, methodIdx + 1); | ||
| const afterMethod = tokens[afterMethodIdx]; | ||
| if (afterMethod && afterMethod.kind === "open" && afterMethod.text === "(") { | ||
| return true; // Opaque namespace/object method call | ||
| } | ||
| } | ||
| continue; // Property access without a following call — not opaque | ||
| } |
…r-declared (?bs 0.9+) Closes #99. DEP003 fires when fn A has same-file callees but none (transitively) declares `reads { x }` that A also declares — the annotation is likely stale after a refactor removed the callee that originally justified it. DEP004 is the symmetric check for `writes { ... }`. Both are warning-level (non-blocking). Leaf fns are excluded because they may be the actual resource access point and the compiler cannot verify the body directly. Also adds DEP003/DEP004 entries to the MCP `explain` tool. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mWrites, fix DEP003/DEP004 examples - Self-only-recursive fns (callees contains only the fn itself) are now treated as leaves and excluded from DEP003/DEP004, consistent with the intent that leaf/access-point fns should be exempt. - Seed calleeReads/calleeWrites from rec.decl.paramReads/paramWrites so wrapper fns that receive a callback with reads/writes annotations are not falsely warned — the annotation is justified by the callback contract. - Fix DEP003/DEP004 `example` entries in error-codes.ts: the prior examples used leaf fns (no callees), which are excluded from the check and would never actually produce the diagnostic. Replaced with non-leaf examples. - Add 4 new tests: self-recursive DEP003/DEP004 suppression, and callback parameter reads/writes justification for both variants. Addresses Copilot review feedback on PR #100. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d add docs Tests: version-gate and non-blocking tests for DEP003/DEP004 used leaf fns (excluded by design), so they tested the leaf carve-out rather than the version gate. Replace with non-self callees so the assertions actually cover the `?bs 0.9` gate. Rename multi-label test to match grouped behavior. Docs: add DEP003/DEP004 to AGENTS.md diagnostic table and README MCP explain code list — both were missing despite the codes being registered. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…P001/DEP002 over-declaration wording - DEP003/DEP004: external callees not present in moduleEffects are now treated as opaque (don't count as non-self callees for over-declaration check). Prevents false warnings when a fn imports a helper that is absent from the effects map — unknown cross-file callees cannot confirm or deny labels. - DEP001/DEP002 MCP explanations: replace 'over-declaration is always allowed' with a reference to DEP003/DEP004 (?bs 0.9+) to avoid contradicting the new over-declaration warnings.
…DEP003/DEP004 moduleEffects entries with no reads/writes were missing from allCalleeNames, so calls to them never appeared in rec.callees, and the knownExternalNames guard at line 287 was unreachable — the fn was falsely treated as a leaf. Now knownExternalNames drives allCalleeNames directly so any external call to a listed entry counts as a non-self callee. Adds 3 tests covering the bug case and its opaque-callee inverse. Co-Authored-By: Botkowski <noreply@anthropic.com>
…calls A fn that calls both a tracked local helper and an unlisted external helper (not in moduleEffects) could get a false DEP003/DEP004 warning — the unknown callee may be the actual read/write point that justifies the declared label. Add `hasOpaqueCall` to `_callgraph.ts`: scans the fn body for any direct function call to a name not in `allCalleeNames`. Use it in the DEP003/DEP004 loop to skip fns whose body contains at least one such opaque call. Adds 3 tests: two no-fire cases (opaque callee present) and one fire case (all callees tracked, none declare the label). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s gating - hasOpaqueCall: add CONTROL_FLOW_IDENTS set so `if(`, `while(`, `for(` and similar control-flow constructs are not mistaken for opaque external calls, which was incorrectly suppressing DEP003/DEP004 on any fn with an if-branch - allCalleeNames: only include import aliases whose resolved target is present in moduleEffects; an alias for an unlisted import is opaque — including it masked the opaque call from hasOpaqueCall and produced false warnings - Add regression tests: control-flow-is-not-opaque for DEP003 and DEP004 Co-Authored-By: Botkowski <noreply@openclaw.ai>
- Replace transitiveReads/transitiveWrites justification with DFS over callee-declared reads/writes; root fn is excluded from DFS to prevent circular self-justification through mutual recursion (thread 16) - Fix warning message: 'no callee in this file' → 'not declared by any tracked callee' to cover moduleEffects callees too (thread 17) - Add BOTSCRIPT_BUILTIN_CALLS to hasOpaqueCall to prevent ok(), some(), isOk(), etc. from being treated as opaque external calls (thread 5/14) - Add cache-label check to multi-stale-label test (thread 9) - Add transitive callee justification tests (DFS multi-hop coverage) 752 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d buildModuleEffects - buildModuleEffects now includes all exported fns (even pure helpers with no reads/writes/throws) so DEP003/DEP004 can distinguish "known callee with no labels" from "unknown opaque callee" in real CLI/Vite builds; previously pure helpers were omitted, causing them to look opaque and suppressing the warning - hasOpaqueCall now detects member calls on unknown namespace objects (e.g. `dbClient.query()`, `api.helper()`): an ident followed by `.method(` where the ident is not in knownNames is treated as an opaque call — the resource access point may be the namespace method, not a tracked callee - Stdlib namespaces (time, http, random, etc.) are explicitly excluded from the new opaque-member heuristic — they are handled by cap-check - Update module-effects test: "omits fns with no declared effects" → "includes fns with no declared effects as empty entries" to reflect the new contract - Add DEP003/DEP004 tests for namespace member call suppression 755 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…canonical stdlib source - Export STDLIB_NAMESPACES from _callgraph.ts as canonical source; update cap-check.ts to import it and note that STDLIB_TO_CAP keys must match - Export collectTopLevelParamNames from _callgraph.ts (depth-safe param parser, same logic as thr-check's collectParamNames) - Add optional localNames param to hasOpaqueCall; member calls on local variables (e.g. name.trim(), items.map()) no longer trigger opaque suppression — only genuine unknown namespace imports do - Pass fn param names as localNames in passDepCheck - Add tests: name.trim() and items.map() on params no longer suppress DEP003/DEP004; external dbClient.query() still suppresses correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s body locals to hasOpaqueCall Two Copilot review fixes: 1. `hasOpaqueCall` was skipping ALL CapCase calls (`/^[A-Z]/`) including untracked external functions like `LoadUser()` or `DbClient()`, causing DEP003/DEP004 to fire when a function calls an unknown CapCase callee that may be performing the actual resource access. Fix: only skip CapCase idents that appear immediately inside `err(...)` — those are error-type constructors, not user functions. 2. `dep-check.ts` only passed `paramNames` to `hasOpaqueCall`, so method calls on local `const`/`let` bindings (e.g. `name.trim()`) were treated as opaque namespace calls, incorrectly suppressing DEP003/DEP004. Fix: new `collectFnBodyLocalNames` helper collects simple-binding local variable names from the fn body; these are combined with `paramNames`. Adds three regression tests covering both behaviors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lude object type annotation properties
Property names inside `opts: { dbClient: string }` type annotations were
incorrectly added to the param name set, causing `dbClient.query()` in the
body to look like a local method call rather than an opaque external call,
which allowed DEP003/DEP004 to fire even though the call might justify the
label. Now braceDepth is tracked and identifiers inside `{...}` are skipped.
Adds regression test.
Co-Authored-By: Botkowski <noreply@botkowski.ai>
…ueCall doc cap-check.ts imported STDLIB_NAMESPACES but never used it in code (only in a comment). Remove from import to avoid unused-import lint noise. hasOpaqueCall docstring only described bare ident( calls but the implementation also detects member/namespace calls (obj.method()). Updated to match behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Handles two cases that were previously missed: - `fn?.()`: optional bare call — `?.` immediately followed by `(` on the receiver - `obj.method?.()`: optional method call — method name followed by `?.(` Both patterns mean the callee's effects are unknown to the compiler and should suppress DEP003/DEP004 (and THR004) the same way as direct calls. Added two regression tests for optional-call opaque suppression.
The rebase onto main merged two versions of hasOpaqueCall — the older 4-arg form (at the bottom of the file) and the newer 5-arg form with localNames and optional-call detection. Remove the duplicate; the comprehensive version at the top already handles all callers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
marcelofarias
force-pushed
the
botkowski/dep003-dep004-over-declared
branch
from
5a763d2 to
d1c993c
Compare
Comment on lines
+116
to
+120
| const BOTSCRIPT_BUILTIN_CALLS = new Set([ | ||
| "err", | ||
| "isErr", "isOk", "mapErr", "mapResult", "ok", "unwrap", | ||
| "isNone", "isSome", "mapOption", "none", "optionFromNullable", "some", "unwrapOption", "unwrapOr", | ||
| ]); |
Comment on lines
+651
to
+655
| "**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 gated on `?bs 0.9`. The symmetrical under-declaration check is DEP001.", |
Comment on lines
+674
to
+677
| "**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 gated on `?bs 0.9`. The symmetrical under-declaration check is DEP002.", |
- Remove duplicate BOTSCRIPT_BUILTIN_CALLS constant from _callgraph.ts and use the canonical STDLIB_VALUE_CALL_NAMES from imports.ts instead - Add note to DEP003 MCP explanation that the warning is also suppressed when the function body has opaque/untracked external calls Co-Authored-By: Botkowski <noreply@openclaw.ai>
Comment on lines
+326
to
+330
| 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]); | ||
| if (hasOpaqueCall(tokens, rec.decl, inner, allCalleeNames, localNames)) continue; |
Comment on lines
+679
to
+680
| "(writes {} labels are user-defined strings).\n\n" + | ||
| "DEP004 is gated on `?bs 0.9`. The symmetrical under-declaration check is DEP002.", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #99.
Summary
reads { x }that the fn also declares — annotation likely became stale after a refactor removed the callee that originally justified itwrites { ... }explaintoolTest plan
pnpm test— 738 tests pass (31 test files)?bs 0.9explain DEP003/explain DEP004return correct entries with valid fails/passes examples🤖 Generated with Claude Code