Repository navigation
fix: unify export-status detection and fix ambient export handling (#1721) - #2896
johnhldiver-beep wants to merge 1 commit into
Conversation
…olbymchenry#1721) Export status — "is this name exported from this file" — was answered in three places that shared no code and disagreed: the AST predicate on the TS/JS extractors (exact for declaration forms, blind to everything else), a hand-mirrored regex that existed only because of that blindness and had one call site, and a coarser per-file check in the resolver. src/extraction/export-status.ts is now the single answer, mirrored for the native walker by codegraph-kernel/src/tsjs/export_status.rs. It closes the blind spots end to end: later `export { x }` / `export default x`, every CommonJS form (module.exports, exports.x, module.exports.x and the bracket spelling exports["x"]), and module.exports object literals. The scan is comment- and string-aware, so a commented-out or quoted export is not one. The resolver no longer treats a declaration exported only as a module's default as a named export, so `import { Widget } from './x'` stays unresolved when x.ts writes `class Widget {}` + `export default Widget`. Adds __tests__/export-status.test.ts, including a kernel<->wasm parity gate so the Rust mirror cannot drift from the source of truth. Co-authored-by: Perch <agent@perchai.app> Made-with: Perch
|
Thanks for pulling the export forms into one place. I tested the head (2f85e34) against 1. // util.js
function slugify(s) { return s; }
function pad(s) { return s; }
function formatDate(d) { return d; }
module.exports = { slugify, pad, formatDate };
// app.js
const util = require('./util');
function main() { util.formatDate(1); util.pad('x'); util.nonexistent(); }
if (stated !== 'unstated') return stated;
if (idx.defaultComponent) return idx.defaultComponent;
const source = context.readFile(filePath);
if (source && CJS_EXPORT.test(source) && !ESM_EXPORT_OR_DECLARE_GLOBAL.test(source)) return undefined;
return idx.defaultFnClass;With this change the fixture is back to no edges. The commonjs-*, require-default, default-export-declaration, import-default-as, esm-unexported-binding, store-exported-later, export-status, sync-default-export-change and the two reexport suites all pass (12 files, 72 passed / 10 skipped). 2. A store exported under another name loses its actions. // store.ts
const useStore = create((set) => ({ count: 0, increment: () => set((s: any) => ({ count: s.count + 1 })) }));
export { useStore as useAppStore };
// app.ts
import { useAppStore } from './store';
export function click() { useAppStore.getState().increment(); }On 3. Large files index much slower. I generated a 515 KB JS file with 16,000 top-level declarations and indexed it:
4. The CommonJS scan marks unexported functions as exported. const msg = "module.exports = { helper }";
function helper() { return msg; }
function render() {}
function ready() {}
function viaParam(exports) { exports.inner = 1; }
function inner() {}
const pkg = { exports: {} }; pkg.exports.render = 1;
const cfg = { exports: {} }; if (cfg.exports.ready == null) {}
Smaller: the kernel's |
…after its export (#3032) `exports.getItems = async (req, res) => {…}` names its function after the export property, in both engines (since the CommonJS controller fix), but the same export written with the name in quotes, `exports["getItems"] = …` or `module.exports['deleteItem'] = function () {…}`, left the function anonymous: no node, so nothing could find it or link to it, and its calls counted as the file's. commonJsExportName (tree-sitter.ts) and its kernel twin common_js_export_name (tsjs/extractors.rs) accepted only a member_expression on the left of the assignment. Both now also accept a subscript_expression on `exports` or `module.exports` whose index is a string literal spelling a plain identifier. The test for that string, which `this['svc']` receivers already used in both engines, is now one helper per engine (quotedIdentifier / quoted_identifier) shared by the two readers. A key that no identifier can spell (`exports["not-a-name"]`), a computed key and a template string still name nothing. Validation: the new commonjs-exports cases fail on main and pass in both engines, and a parity case compares the kernel's and wasm's nodes and refs for the JS and TS forms. Reimplements the bug part of PR #2896 by @johnhldiver-beep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @johnhldiver-beep for finding that |
|
Hi @inth3shadows — thanks for testing the head against main on both
the wasm and kernel paths, and for the detailed findings. Noted that
part of this landed separately in #3032. Thanks for the thorough
review.
…On Tue, 27 Oct 2026 14:55:56 +0000 (UTC), Eric Minish ***@***.***> wrote:
inth3shadows left a comment (colbymchenry/codegraph#2896)
Thanks for pulling the export forms into one place. I tested the head (2f85e34) against `main` (69af19a), on both the wasm path and a kernel built from this branch. The engine suite passed in an earlier run of ours (9,076 tests), but I found four problems, and the first one adds wrong call edges to ordinary CommonJS code.
**1. `require` + member calls now link to the wrong function.**
```js
// util.js
function slugify(s) { return s; }
function pad(s) { return s; }
function formatDate(d) { return d; }
module.exports = { slugify, pad, formatDate };
// app.js
const util = require('./util');
function main() { util.formatDate(1); util.pad('x'); util.nonexistent(); }
```
| | `main` calls |
|---|---|
| `main` | nothing |
| this PR | `slugify` ×3 (including `util.nonexistent()`) |
`require('./util')` resolves as a default import. With no `export default` in the file, `esmDefaultExport` falls back to the first exported function ([`import-resolver.ts#L302-L306`](https://github.com/colbymchenry/codegraph/blob/2f85e34ff51b416c685ad7a536a9be8fe22e259a/src/resolution/import-resolver.ts#L302-L306)). Before this PR, CommonJS functions weren't `isExported`, so the fallback found nothing. Now it finds `slugify`.
Fix: don't guess a default for a module that only exports through CommonJS:
```ts
if (stated !== 'unstated') return stated;
if (idx.defaultComponent) return idx.defaultComponent;
const source = context.readFile(filePath);
if (source && CJS_EXPORT.test(source) && !ESM_EXPORT_OR_DECLARE_GLOBAL.test(source)) return undefined;
return idx.defaultFnClass;
```
With this change the fixture is back to no edges. The commonjs-*, require-default, default-export-declaration, import-default-as, esm-unexported-binding, store-exported-later, export-status, sync-default-export-change and the two reexport suites all pass (12 files, 72 passed / 10 skipped).
**2. A store exported under another name loses its actions.**
```ts
// store.ts
const useStore = create((set) => ({ count: 0, increment: () => set((s: any) => ({ count: s.count + 1 })) }));
export { useStore as useAppStore };
// app.ts
import { useAppStore } from './store';
export function click() { useAppStore.getState().increment(); }
```
On `main`, `click` calls both `useStore` and `increment`. On this PR it calls only `useStore`, on both the wasm and kernel paths. The old `isExportedLater` accepted `useStore` inside any `export { … }`. `isExportedLaterInSource` deliberately rejects the local side of `A as B` (the ambient-module rule), and the store check at [`tree-sitter.ts#L3995`](https://github.com/colbymchenry/codegraph/blob/2f85e34ff51b416c685ad7a536a9be8fe22e259a/src/extraction/tree-sitter.ts#L3995) now relies on it.
Simply loosening `isExportedLaterInSource` breaks `ts-ambient-exports.test.ts` (2 failures), so the two questions need separate checks. Keep `isExported` as it is, and give the store branch its own check: does the name appear as the local side of any `export { … }` specifier? A ~10-line helper restores the edge, and export-status, store-exported-later, ts-ambient-exports and four related suites pass (50 passed / 11 skipped). Two things to watch: evaluate it after `objectOfFns && hasInlineFns`, so it only runs for store-shaped consts, and add the same check on the kernel side (I only prototyped the TS side).
**3. Large files index much slower.** I generated a 515 KB JS file with 16,000 top-level declarations and indexed it:
| | `main` | this PR |
|---|---|---|
| wasm | 64 s | 182 s |
| kernel | 38 s | 61 s |
[`declarationExportedBySource`](https://github.com/colbymchenry/codegraph/blob/2f85e34ff51b416c685ad7a536a9be8fe22e259a/src/extraction/export-status.ts#L239-L262) runs `isExportedLaterInSource` and `isCommonJsExportedName` once per declaration name. Each builds new RegExps and scans the whole file, so the cost is O(declarations × file size). The kernel's `export_status.rs` compiles `Regex::new` per name in the same way.
Fix: collect the file's exported names into one `Set` per source, memoized alongside `maskedFor`/`strippedFor`, and look names up in it. Skipping files that contain no `export`/`exports` at all also saves the common case.
**4. The CommonJS scan marks unexported functions as exported.**
```js
const msg = "module.exports = { helper }";
function helper() { return msg; }
function render() {}
function ready() {}
function viaParam(exports) { exports.inner = 1; }
function inner() {}
const pkg = { exports: {} }; pkg.exports.render = 1;
const cfg = { exports: {} }; if (cfg.exports.ready == null) {}
```
`helper`, `render`, `ready` and `inner` are all `is_exported = 1` on this PR, and all 0 on `main`. The scan in [`isCommonJsExportedName`](https://github.com/colbymchenry/codegraph/blob/2f85e34ff51b416c685ad7a536a9be8fe22e259a/src/extraction/export-status.ts#L268-L279) reads the comment-stripped source, so it still sees strings. Its `\b(?:module\.exports|exports)` also matches after a `.`, and its trailing `=` also matches `==`.
Fix (a sketch, not tested): require `(?<![\w$.])` before `module.exports`/`exports`, require `=(?!=)` after it, and run the object-literal form (`module.exports = { … }`) on the masked source. The bracket form `exports["x"]` is the only one that needs string contents kept. Until point 1 is fixed, each false "exported" here can also become the guessed default.
Smaller: the kernel's `masked_js_source` ([`export_status.rs#L86`](https://github.com/colbymchenry/codegraph/blob/2f85e34ff51b416c685ad7a536a9be8fe22e259a/codegraph-kernel/src/tsjs/export_status.rs#L86)) doesn't skip regex literals. So in a file with ``const BT = /`/;`` followed by `export { x }`, `x` reads as exported on wasm but not on the kernel (verified). By our reading, the backtick opens a "template string" that blanks the rest of the file. That's rare, but it is a wasm/kernel disagreement.
--
Reply to this email directly or view it on GitHub:
#2896 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
Closes #1721.
The problem
"Export status" — is this name exported from this file — was answered in three places, none sharing code, and they disagreed:
isExported, the AST predicate on the TS/JS language extractors, walks the parent chain for anexport_statementancestor. Exact for the declaration forms (export function f), blind to everything else:const x = …; export { x }and every CommonJS form left it false.isExportedLater, a regex over the file source intree-sitter.ts, hand-mirrored in the Rust kernel. It existed only because of that blindness, and had exactly one call site — the store-extraction condition — so the fix it represents was scoped to Zustand stores.isSealedModule), which needs "does this file export anything at all" and could use neither of the other two.The visible damage: a CommonJS module's exports were invisible to the graph as exports,
export { x }was visible only to store extraction, and the resolver re-derived a coarser version of the same fact from the same file text.The fix
src/extraction/export-status.tsis now the single answer, mirrored for the native walker bycodegraph-kernel/src/tsjs/export_status.rs. It is deliberately source-based rather than AST-based, because two of the three callers (the resolver's per-file question, and the kernel's decoded tables, which carry no tree) have no syntax tree to walk — and because the forms the AST walk misses are exactly the ones a source scan reads cleanly. The AST walk stays the fast path for the declaration forms it is exact on.Closed end to end:
export { x },export default x,export { x as default }.module.exports = { x },exports.x = …,module.exports.x = …, and the bracket spellingexports["x"] = …(which the dot-form-only check missed entirely, on both paths).module.exportsobject literals — shorthand, keyed and string-keyed, skipping a spread it cannot know.The scan is comment- and string-aware, so a commented-out or quoted
export { x }is not an export. Offsets are preserved, so a match index still maps to its line.Two consequences worth calling out:
import { Widget } from './x'names nothing whenx.tswritesclass Widget {}+export default Widget; answering it made the import resolve where a full index leaves it unresolved, so a sync that dropped the default kept the edge.isExportedLaterand its Rust mirror are deleted.Tests
__tests__/export-status.test.tscovers the blind forms, the declaration forms that must stay exported, the comment/string cases, the per-file questions, and the CommonJS name reading — through extraction, on both the wasm and kernel paths. It includes a kernel↔wasm parity gate that compares the wholeExtractionResultfor each form, so the Rust mirror cannot drift from the source of truth.npx tsc --noEmit: cleanresolution.test.ts,ts-ambient-exports.test.ts,ambient-module-imports.test.ts,sync-name-matched-imports.test.ts)export-status.test.ts+kernel-tsjs-parity.test.ts: 131 passedNote for the reviewer
#2754 (open) also edits
src/resolution/import-resolver.ts, in theexportedByName/ default-export region this change touches. The two are complementary — #2754 follows a default through wrappers and imports, this one stops a default from being read as a named export — but they will want a rebase against each other. Flagging it rather than guessing which lands first.