feat(compiler): DEP003/DEP004 — warn when reads/writes labels are over-declared (?bs 0.9+)#139
Merged
Merged
Conversation
Closed
7 tasks
There was a problem hiding this comment.
Pull request overview
This PR adds compiler support for DEP003/DEP004: warning-level diagnostics (from ?bs 0.9) that detect likely-stale reads {} / writes {} labels when they aren’t justified by any tracked same-file (or moduleEffects) callee, with suppression when the body contains opaque/untracked external calls.
Changes:
- Add DEP003/DEP004 warning generation to the compiler dep-check pass (including
moduleEffectsintegration and warning rewrites). - Expand callgraph utilities to better detect “opaque” calls (including member/optional calls) and export stdlib namespace metadata used by the detection.
- Register DEP003/DEP004 across MCP
explain, error-code registry, docs, and add targeted tests (including moduleEffects empty-entry behavior).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents DEP003–DEP004 as supported explain codes. |
| packages/mcp/tests/server.test.ts | Ensures MCP explain covers DEP003/DEP004. |
| packages/mcp/src/explanations.ts | Adds long-form explanations + examples for DEP003/DEP004 and updates DEP001/DEP002 wording. |
| packages/compiler/tests/module-effects.test.ts | Updates moduleEffects expectations to include pure helpers as {} entries; adds DEP003/DEP004 cross-file tests. |
| packages/compiler/tests/dep-check.test.ts | Adds comprehensive test coverage for DEP003/DEP004 behavior and suppression rules. |
| packages/compiler/src/passes/dep-check.ts | Implements DEP003/DEP004 warning collection and returns warnings from the pass. |
| packages/compiler/src/passes/cap-check.ts | Clarifies stdlib namespace/capability mapping contract in docs. |
| packages/compiler/src/passes/_callgraph.ts | Adds stdlib namespace set, param/local name collection, and enhances opaque-call detection. |
| packages/compiler/src/module-effects.ts | Always includes exported fns in moduleEffects map, even with an empty surface {}. |
| packages/compiler/src/error-codes.ts | Registers DEP003/DEP004 in the compiler error-code registry. |
| AGENTS.md | Documents DEP003/DEP004 guidance in the diagnostics table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marcelofarias
force-pushed
the
botkowski/dep003-squash
branch
from
86959b5 to
8aa71e6
Compare
…r-declared (?bs 0.9+)
Squash-rebased onto current main. DEP003 fires when a fn declares
reads { x } but no same-file callee (transitively) reads { x } —
the annotation likely became stale after a refactor. DEP004 is the
symmetric check for writes { x }.
Both are warning-level (non-blocking). Leaf fns are excluded.
Adds DEP003/DEP004 to compiler registry, MCP explain, and tests.
Supersedes PR #100 (same content, rebased). All 996 tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…j.a.b() and obj.a?.b() Previous logic only checked one segment deep: obj.method() was detected but obj.a.b() was not, because afterMethod was `.` rather than `(`. Walk the full chain until a call or non-chain token is found. Adds two regression tests for the multi-segment cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rding - Add parens around single-param arrow functions in dep-check.ts (style consistency) - Clarify DEP001/DEP002 cross-refs: "may be warned about" with scope conditions, not unconditional Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…te drift risk STDLIB_NAMESPACES was a hardcoded duplicate of the key set of STDLIB_TO_CAP (_stdlib.ts). Adding or removing a stdlib namespace required updating two places and hasOpaqueCall behavior could silently diverge. Now derived from the single source of truth. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ms, collectFnBodyLocalNames handles var
collectTopLevelParamNames:
- Adds skipUntilNextParam flag so type annotations and default values
after the param name are not re-scanned as candidate param names.
Fixes false captures like treating `string` in `(name: string)` as a
param name when a untyped-param lookahead would have matched it.
- Adds fallback for completely untyped and default params: bare identifier
immediately before `,`, `)`, or `=` is collected as a param name.
Fixes the original Copilot gap: `fn f(x) -> void` left `x` unknown,
causing `x.trim()` to be treated as an opaque external call.
- Closing `}` at depth 1 sets skipUntilNextParam so the type annotation
after a destructured binding (`{a, b}: Context`) is not captured.
collectFnBodyLocalNames:
- Adds `var` alongside `const`/`let` so that `var x = ...` bindings are
recognised as local names and `x.method()` is not mistaken for an
opaque namespace call.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…suppression Two regression tests to cover the fixes in collectTopLevelParamNames and collectFnBodyLocalNames: - Untyped param `fn f(name)`: name.trim() must not suppress DEP003 - Var binding `var x = ...`: x.method() must not suppress DEP003 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ffects and opaque-call suppression Update DEP003/DEP004 entries in error-codes.ts (rule, idiom), MCP explanations, and AGENTS.md to accurately describe the two conditions under which the warning is suppressed: fns with opaque/untracked external calls, and tracked callees that include moduleEffects entries (not just same-file fns). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cts var The implementation collects `const`, `let`, and `var` bindings, but the doc comment only mentioned `const`/`let`. Updated to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l vars
collectTopLevelParamNames: when a parameter is a destructuring pattern
({ a, b: c } or [x]), extract the binding names (those NOT followed by
`:`) so method calls on destructured locals (e.g. name.trim()) are not
mistaken for opaque external calls that suppress DEP003/DEP004/THR004.
collectFnBodyLocalNames: add collectDestructuredTokenBindings helper that
uses the token matchedAt index to scan { } and [ ] destructuring patterns
recursively, collecting binding names by the same rule (ident not followed
by `:` is a binding, not a property key).
Adds two regression tests: one for destructured params and one for
destructured const locals — both confirm DEP003 still fires when the
only member calls in the body are on locally-scoped destructured names.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…uring depth
collectTopLevelParamNames: when skipping past a param's type annotation
(skipUntilNextParam=true), skip entire {}/[] groups rather than char-by-char
so commas inside `x: { a: string, b: number }` are not mistaken for
param separators.
collectDestructuringStringBindings: change depth guard from `depth !== 1`
to `depth < 1` so nested destructuring patterns like `{ a: { b } }` are
handled — binding names at depth > 1 are now correctly collected.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…collectors
Identifiers in `= <expr>` default values (e.g. `{ a = externalDb }`) were
incorrectly collected as binding names, causing hasOpaqueCall() to treat
member calls on those names as local and suppress DEP003/DEP004/THR004
when it shouldn't.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…file only Copilot pointed out the titles said "in the same file" but the implementation also counts moduleEffects entries as tracked callees. Updated titles in error-codes.ts and explanations.ts to say "any tracked callee" instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ection hasOpaqueCall suppresses DEP003/DEP004 when a fn contains a member call whose receiver is not in STDLIB_NAMESPACES or effectiveLocalNames. Module-level stdlib aliases like `const t = time` were not included in either set, causing `t.now()` to be treated as an opaque external call and incorrectly suppressing the warning. Fix: collect module-level stdlib alias names via collectStdlibAliases() once per pass invocation and add them to localNames at each hasOpaqueCall call site. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The JSDoc block for collectTopLevelParamNames was placed before collectDestructuringStringBindings, leaving both functions without proper documentation. Moved the doc block to attach to the correct symbol. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ession hasOpaqueCall in passThrCheck was called without localNames, so module-level stdlib aliases (e.g. `const t = time`) were treated as opaque external member calls and incorrectly suppressed THR004 warnings. passDepCheck already passed stdlibAliasNames correctly; align passThrCheck to match. - Import collectStdlibAliases from _alias.js and collectFnBodyLocalNames from _callgraph.js in thr-check.ts - Compute stdlibAliasNames once at passThrCheck entry - Build localNames = paramNames + bodyLocals + stdlibAliasNames for the hasOpaqueCall call (mirrors the pattern in passDepCheck) - Add regression test: fn with t.now() (stdlib alias) still fires THR004 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commas inside generic type argument lists like `Map<T, { a }>` were
incorrectly resetting skipUntilNextParam, causing the `{ a }` fragment to
be parsed as a destructuring parameter and adding `a` to the param-names
set. This made `a.method()` in the body appear local rather than opaque,
falsely suppressing DEP003/DEP004 when they should fire (and vice versa
— allowing opaque-call suppression to be defeated).
Fix: track `<`/`>` angle depth in skipUntilNextParam mode. Commas are
only treated as top-level param separators when angleDepth === 0.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The label had no corresponding `break chainWalk` or `continue chainWalk` usage; all break/continue statements in the loop are bare. Removing it eliminates the noise and avoids potential linter warnings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s for opaque-call check THR004 opaque-call suppression used allCalleeNames, which includes ALL import aliases. An alias to an untracked external (not in moduleEffects) is itself an opaque call — including it in knownForOpaque caused THR004 to fire for fns with genuinely unknown external dependencies (false warnings). Fix: derive opaqueKnownBase from fnNames + knownExternalNames + only the aliases whose resolved target exists in moduleEffects. allCalleeNames is preserved as-is for collectCallees (call graph building). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ot body scanner The previous wording implied the compiler scans fn bodies for direct resource access, which it does not. Reword rule and idiom to accurately describe the implementation: a call-graph justification check that fires only when all same-file callees are resolvable and none propagates the declared label.
marcelofarias
force-pushed
the
botkowski/dep003-squash
branch
from
5e8aa6f to
53d9e1e
Compare
Comment on lines
+481
to
+484
| rewrite: | ||
| proposed.length > 0 | ||
| ? `fn ${rec.decl.name}(...) ${kind} { ${proposed.join(", ")} } -> ... // remove stale label: ${labelList}` | ||
| : `fn ${rec.decl.name}(...) -> ... // remove stale ${kind} {} clause: ${labelList}`, |
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.
Summary
Squash-rebases DEP003/DEP004 onto current
main(which now includes SYN002, RES002, and several MAT/THR fixes).DEP003: fires when a
fndeclaresreads { x }but no same-file callee (transitively) also declaresreads { x }— the annotation likely became stale after a refactor removed the callee that originally justified it.DEP004: symmetric check for
writes { x }.Both are warning-level (non-blocking). Leaf fns (no tracked same-file callees) are excluded — they may be the actual resource access point. Fns with opaque external calls are also excluded.
Changes:
dep-check.tspass with DEP003/DEP004 warning collection_callgraph.tsexports:STDLIB_NAMESPACES,collectTopLevelParamNames,collectFnBodyLocalNames,hasOpaqueCall(sophisticated version handling member calls, optional calls, control-flow keywords)explaintool, and testsisDirectOrOptionalCall(from main's optional-call DEP001/DEP002 work) alongside DEP003's more sophisticatedhasOpaqueCallSupersedes: PR #100 (same content, squash-rebased). Please close #100 once this is reviewed.
All 996 tests pass.
🤖 Generated with Claude Code