feat: reads/writes transitivity enforcement (DEP001/DEP002, ?bs 0.9)#51
feat: reads/writes transitivity enforcement (DEP001/DEP002, ?bs 0.9)#51marcelofarias wants to merge 25 commits into
Conversation
Adds reads { } and writes { } clauses to fn declarations. Both are parsed
from the function header (between uses {} and ->) and stored on FnDecl.
Metadata-only in 0.8: stripped from the TypeScript output, not yet
transitively enforced.
The annotations let bots declare which resource categories a function
reads from or writes to (user-defined labels: cache, db, metrics, etc.).
This is the first slice of the effect-signature design in issue #14.
Enforcement — transitive closure checking similar to cap-check — lands
in a follow-up once the syntax is settled in use.
Changes:
- parser/parse-fn.ts: FnDecl.reads / FnDecl.writes fields; parsed in a
small loop after uses {} that accepts reads, writes, and intent in any
order before ->
- passes/version.ts: add '0.8' to SUPPORTED_VERSIONS
- primer.ts: document reads {} / writes {} in the FUNCTIONS section
- tests/reads-writes.test.ts: 13 tests covering parse and TS-strip
- examples/node-app/src/user-cache.bs: end-to-end demo at ?bs 0.8
All 453 tests pass. pnpm -r build && pnpm test OK.
- detect duplicate reads/writes annotations and return null (parse error)
instead of silently overwriting the first occurrence
- add tests for duplicate reads {} / writes {} returning null
- fix primer.ts: give reads {} and writes {} separate, correctly aligned
description blocks so writes doesn't read as if it describes reads
- fix user-cache.bs example: mark fetchFromDb as async fn (body uses await)
455 tests pass.
…tes {}
Previously INT001 only caught uses {} vs pure intent. A function that
declares reads { cache } or writes { metrics } is also not pure — it
depends on or mutates external resources. The check now covers all three
annotation types.
Adds 4 tests; updates the error-codes rule/idiom/example to reflect the
wider contract. 460 tests pass.
- intent-check: use space-joined conflictsStr in rewrite field so the
suggested fn header is valid botscript syntax (clauses are
space-separated, not comma-separated)
- error-codes: INT001 rewrite option B now includes writes { ... }
alongside reads { ... } since the rule covers both annotations
- primer: clarify that uses {} (if present) must precede reads/writes/
intent — "any order" was inaccurate; uses {} is parsed in a fixed
position before the annotation loop
From ?bs 0.9, the compiler enforces that reads {} and writes {} annotations
are complete with respect to same-file callees. If fn A calls fn B and B
declares reads { db }, A must also declare reads { db } — the full resource
dependency surface must be visible from the caller's header alone.
This is the 'what comes next' step described in PR #47, implemented the same
way CAP001 handles capability transitivity.
- passes/dep-check.ts: new pass; fixed-point closure over the call graph,
same pattern as cap-check. Only activates at ?bs 0.9.
- DEP001: reads under-declared (callee reads a label caller doesn't declare)
- DEP002: writes under-declared (callee writes a label caller doesn't declare)
- Over-declaration is intentionally not checked (labels are user-defined
strings; the compiler cannot verify them against the fn body)
- version.ts: add '0.9' to SUPPORTED_VERSIONS
- transform.ts: wire passDepCheck after passCapCheck (minVersion: '0.9')
- error-codes.ts: DEP001 / DEP002 registry entries with examples
- primer.ts: update reads/writes docs from 'metadata-only in 0.8' to
document 0.9 transitivity enforcement
- tests/dep-check.test.ts: 18 tests covering DEP001, DEP002, mixed
reads+writes, transitive chains, version gating
- error-codes.test.ts, mcp/tests/server.test.ts: add DEP001/DEP002
- mcp/explanations.ts: DEP001/DEP002 long-form explanations with
fails/passes examples verified against the compiler
All 478 tests pass.
Partial progress on #14
There was a problem hiding this comment.
Pull request overview
Implements ?bs 0.9 enforcement for reads {} / writes {} transitivity across same-file calls by adding a new compiler pass (dep-check) and registering the new diagnostics (DEP001/DEP002) across the compiler + MCP explanation surfaces.
Changes:
- Add
passDepCheckto compute transitive reads/writes and emit DEP001/DEP002 for under-declarations (?bs 0.9+). - Register new error codes (compiler + MCP) and wire the pass into the transform pipeline; add
0.9to supported versions. - Add test coverage for dep-check behavior (including version gating) and update docs (primer) to reflect enforcement.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compiler/src/passes/dep-check.ts | New compiler pass implementing reads/writes transitivity closure + DEP001/DEP002 emission. |
| packages/compiler/src/transform.ts | Wires depCheck into the compiler pipeline (minVersion 0.9). |
| packages/compiler/src/passes/version.ts | Adds 0.9 to SUPPORTED_VERSIONS. |
| packages/compiler/src/error-codes.ts | Registers DEP001/DEP002 in the compiler error-code registry. |
| packages/compiler/src/primer.ts | Updates language primer to describe 0.9 enforcement behavior. |
| packages/compiler/tests/dep-check.test.ts | Adds tests for DEP001/DEP002, transitive chains, and version gating. |
| packages/compiler/tests/error-codes.test.ts | Ensures DEP001/DEP002 are present in the compiler-emitted code list. |
| packages/mcp/src/explanations.ts | Adds long-form MCP explanations for DEP001/DEP002 with examples. |
| packages/mcp/tests/server.test.ts | Updates MCP known-code list to include DEP001/DEP002. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip top-level fns whose annotations come only from their OWN declarations | ||
| // (no callee contribution) — those are always trivially consistent. |
| const tok = tokens[i]; | ||
| if (!tok || tok.kind !== "ident") continue; | ||
| if (!fnNames.has(tok.text)) continue; | ||
| if (tok.text === fn.name) continue; // skip self-references | ||
| // Must be followed by `(` to be a call (not just a reference). | ||
| const nextIdx = nextSignificant(tokens, i + 1); | ||
| const next = tokens[nextIdx]; | ||
| if (!next || next.kind !== "open" || next.text !== "(") continue; | ||
| callees.add(tok.text); |
| const message = | ||
| `fn '${rec.decl.name}' calls '${pathStr.split(" -> ").at(-1)}' which ${kind} { ${firstLabel} }, ` + |
| line, | ||
| column, | ||
| start: rec.decl.fnKeywordStart, | ||
| end: rec.decl.fnKeywordStart + 2, | ||
| message, |
| // Build a description of the first missing label and its path. | ||
| const firstLabel = missingLabels[0]!; | ||
| const firstPath = transitiveMap.get(firstLabel)!; | ||
| const pathStr = formatPath(firstPath); | ||
|
|
||
| const allMissing = missingLabels.map((l) => `"${l}"`).join(", "); | ||
| const currentDecl = |
| enforced (DEP001 / DEP002): if | ||
| fn A calls fn B and B declares | ||
| reads { x }, A must also declare | ||
| reads { x }. |
- intent-check: file header no longer claims body-shape verification is
implemented today; rewrite text broadened from 'remove resource
annotations' to 'remove the conflicting header clauses (uses/reads/writes)'
so the suggested fix is unambiguous when the conflict is just uses {}
- primer: same softening for the intent: description; INT001 entry now
explicitly mentions reads/writes as triggers, not just uses {}
- mcp/explanations: INT001 explanation updated to cover reads/writes
- examples/user-cache.bs: add a comment that lookupCache/writeCache are
intentionally stubbed (annotations are metadata-only in 0.8)
- README: list 0.7 / 0.8 as opt-in pins
- CHANGELOG: backfill 0.7 (intent + INT001) and 0.8 (reads/writes + INT001
extension) entries
- dep-check: remove misleading 'skip top-level fns' comment from the validation loop; the design already trivially handles self-only fns through the seed-then-check pattern - dep-check: collectCallees now skips property accesses (`obj.helper(...)` and `obj?.helper(...)`) so a top-level fn name accidentally matching a method name no longer triggers false DEP001/DEP002. Mirrors cap-check's member-access guard. Two new tests cover both . and ?. cases - dep-check: diagnostic message now says 'transitively calls' and includes the full call path in the main message (not just the leaf) when the dependency arrives via a chain. Trailing 'also missing: ...' tail lists remaining missing labels (replaces the dead 'allMissing' variable) - dep-check: diagnostic span now ends at the fn name (not just the `fn` keyword) for better editor/LSP highlighting — matches cap-check - primer: 0.9 enforcement description now explicitly covers writes (DEP002), not just reads (DEP001)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/compiler/src/passes/dep-check.ts:320
- The non-
viabranches forcallDescription/callPathappear unreachable for under-declaration errors (a missing label can't havekind: "declared"because declared labels are seeded intodeclaredReads/Writesand filtered out ofmissingLabels). Additionally, the fallbackcallPathtext says the label is "directly declared on" the caller (rec.decl.name), which is incorrect and would give wrong guidance if this branch ever became reachable. Recommend either removing the dead branches or fixing them to reference the leaf/callee function name.
// For via-paths, name the call chain in the main message so the caller
// sees that the dependency arrives through a callee, not from a direct call.
const callDescription =
firstPath.kind === "via"
? `${pathStr} — '${leaf}' ${kind} { ${firstLabel} }`
: `'${leaf}' which ${kind} { ${firstLabel} }`;
const message =
`fn '${rec.decl.name}'${transitively} calls ${callDescription}, ` +
`but '${rec.decl.name}' only declares ${kind} { ${currentDecl} }${otherTail}`;
const callPath =
firstPath.kind === "via"
? `call path: ${pathStr}`
: `directly declared on '${rec.decl.name}'`;
| const pathStr = formatPath(firstPath); | ||
| const leaf = pathStr.split(" -> ").at(-1)!; | ||
| const transitively = firstPath.kind === "via" ? " transitively" : ""; | ||
|
|
||
| const currentDecl = |
…otations # Conflicts: # packages/compiler/src/passes/intent-check.ts # packages/compiler/src/primer.ts # packages/compiler/tests/intent.test.ts
…check # Conflicts: # packages/compiler/src/transform.ts
The 'transitively' flag was always set because missingLabels are by construction always propagated from callees. Distinguish direct calls (one hop: caller -> declaring fn) from multi-hop chains so the message reads accurately in both cases.
| enforced: if fn A calls fn B and B | ||
| declares reads { x }, A must also | ||
| declare reads { x } (DEP001). The | ||
| same rule applies to writes { x } | ||
| (DEP002). |
|
|
||
| export const SUPPORTED_VERSIONS: ReadonlyArray<string> = ["0.1", "0.2", "0.3", "0.4", "0.5", "0.6", "0.7", "0.8"]; | ||
| export const SUPPORTED_VERSIONS: ReadonlyArray<string> = ["0.1", "0.2", "0.3", "0.4", "0.5", "0.6", "0.7", "0.8", "0.9"]; | ||
| export const LATEST_VERSION = "0.1"; |
…, fix Result.ok in examples
| export type { Program, Stmt, FnStmt, SourceRange } from "./parser/ast.js"; | ||
| export type { FnDecl, FnBody, ParseFnOptions } from "./parser/parse-fn.js"; | ||
| export { atLeast, LATEST_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js"; | ||
| export { atLeast, DEFAULT_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js"; |
| } | ||
|
|
||
| export { LATEST_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js"; | ||
| export { DEFAULT_VERSION, SUPPORTED_VERSIONS } from "./passes/version.js"; |
…rites to STDLIB.bs
- CHANGELOG: "user-defined strings" → "user-defined identifiers" (labels
inside reads {} / writes {} are identifier tokens, not quoted strings)
- CHANGELOG: compat note now correctly describes forward-compatible parsing
(reads/writes parse at any version; only enforcement is gated on 0.8)
- primer.ts: remove quotes around label examples so readers don't try
reads { "cache" } (identifiers only, parseCapList silently drops strings)
- STDLIB.bs: bump to ?bs 0.8; add canonical reads {} / writes {} example
(AGENTS.md §108: every new form must appear in STDLIB.bs exactly once)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removing LATEST_VERSION from the public exports was a breaking change for any consumer that imported it from @mbfarias/botscript-compiler or from ./transform. Add it back as a deprecated alias so downstream tooling does not break — the rename was a pure implementation detail. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| const tok = tokens[i]; | ||
| if (!tok || tok.kind !== "ident") continue; | ||
| if (!fnNames.has(tok.text)) continue; | ||
| if (tok.text === fn.name) continue; // skip self-references |
ed0f410 to
69eac1f
Compare
… rebase) Picks up the rebased botkowski/reads-writes-annotations, including: - fix: duplicate reads/writes/intent annotations use first-wins, not null return
| * | ||
| * DEP001 reads under-declared: fn A calls fn B which (transitively) | ||
| * reads a resource category that A does not declare. Diagnostic | ||
| * names the call path: "A -> B -> C [reads { x }]". |
| // For multi-hop chains, name the call chain in the main message so the | ||
| // caller sees that the dependency arrives through an intermediate callee. | ||
| // For direct calls (one hop), keep the message focused on the leaf. | ||
| const callDescription = directCall | ||
| ? `'${leaf}' which ${kind} { ${firstLabel} }` | ||
| : `${pathStr} — '${leaf}' ${kind} { ${firstLabel} }`; | ||
|
|
||
| const message = | ||
| `fn '${rec.decl.name}'${transitively} calls ${callDescription}, ` + | ||
| `but '${rec.decl.name}' only declares ${kind} { ${currentDecl} }${otherTail}`; | ||
|
|
For transitive chains (A -> B -> C), the diagnostic now shows "B -> C" as the display path rather than "A -> B -> C", avoiding the awkward "fn 'A' transitively calls A -> B -> C" where the caller appears twice. Also removes the unnecessary self-name skip in collectCallees — the closure step already ignores self-loops via `calleeDecl === rec.decl`, so skipping by name was redundant and incorrectly suppressed calls to shadowed nested fns with the same outer fn name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| for (let i = fn.tokenStart; i < fn.tokenEnd; i++) { | ||
| if (insideAny(i, inner)) continue; | ||
| const tok = tokens[i]; | ||
| if (!tok || tok.kind !== "ident") continue; | ||
| if (!fnNames.has(tok.text)) continue; | ||
| // Skip property accesses like `obj.helper(...)` or `obj?.helper(...)` — | ||
| // these are not same-file fn calls even if `helper` matches a top-level | ||
| // fn name. Mirrors cap-check's member-access guard. | ||
| 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 (not just a reference). | ||
| const nextIdx = nextSignificant(tokens, i + 1); | ||
| const next = tokens[nextIdx]; | ||
| if (!next || next.kind !== "open" || next.text !== "(") continue; | ||
| callees.add(tok.text); |
…l graph Mirrors cap-check's guard (tok.text !== fn.name). Without the skip, fn <name>( in the header was seen as an ident followed by (, creating a spurious self-edge that could propagate reads/writes from other same-named decls (e.g. nested fns) into the caller incorrectly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| // --------------------------------------------------------------------------- | ||
| // INT001 extended: reads {} / writes {} also contradict "pure" intent | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("INT001 — pure intent vs reads/writes annotations (?bs 0.8)", () => { |
Resolves conflict: error-codes.test.ts now checks for all codes (CAP001/002, DEP001/002, INT001, RES001, SYN001, UNS001-004). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The second describe block at line 240 was a verbatim duplicate of the one at line 166 (minus the 0.7 gating case) — kept the more complete first block. Addresses Copilot review on PR #51.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/mcp/src/explanations.ts:248
- This says “The message names the call path…”, but for direct calls DEP002’s diagnostic message does not include a path string (it only names the leaf callee); the call path appears in the rewrite hint and multi-hop messages. Please reword to reflect the actual diagnostic output.
"Add the missing label to the caller's `writes {}` clause to clear the diagnostic. " +
"The message names the call path and the specific missing label.",
| expect(() => compile(src)).toThrow(new RegExp(`\\[${code}\\]`)); | ||
| if (fragment) { | ||
| expect(() => compile(src)).toThrow(fragment); |
| "The fix is simple: add the missing label(s) to the caller's `reads {}` clause. " + | ||
| "The diagnostic message names the call path (\"A -> B\") and the specific label, " + | ||
| "so the rewrite is mechanical.", |
Summary
Implements the transitivity enforcement step described as "what comes next" in PR #47. Stacks on #47 (targets
botkowski/reads-writes-annotations).New behavior (
?bs 0.9)From
?bs 0.9, the compiler enforces thatreads {}andwrites {}annotations are complete with respect to same-file callees:reads { db }, fn A must also declarereads { db }writes {}This makes the full resource dependency surface visible from any function's header alone — callers don't need to trace through the call graph.
?bs 0.9 fn fetchFromDb(id: string) reads { db } -> string = id // DEP001: loadUser calls fetchFromDb which reads { db }, but loadUser only declares reads { cache } fn loadUser(id: string) reads { cache } -> string = fetchFromDb(id) // Fix: fn loadUser(id: string) reads { cache, db } -> string = fetchFromDb(id)Over-declaration is intentionally allowed — unlike capabilities, the compiler can't infer reads/writes labels from the body (they're user-defined strings), so conservative annotations are harmless.
Implementation
The pass mirrors
cap-check.ts:Only same-file resolution, same as cap-check.
Diagnostics
reads {}doesn't cover all same-file callees' reads (transitively)writes {}Both are compiler-inferred (no programmer annotation required to trigger them).
Files changed
passes/dep-check.ts: new pass (18 tests)passes/version.ts: add'0.9'to SUPPORTED_VERSIONStransform.ts: wirepassDepCheckafterpassCapCheck(minVersion:'0.9')error-codes.ts: DEP001 / DEP002 registry entriesprimer.ts: update reads/writes docs to describe 0.9 enforcementtests/dep-check.test.ts: 18 tests (DEP001, DEP002, transitive chains, version gating)error-codes.test.ts,mcp/tests/server.test.ts: add DEP001/DEP002mcp/explanations.ts: DEP001/DEP002 long-form explanations with verified examplesAll 478 tests pass.
pnpm -r build && pnpm test✓Partial progress on #14