Skip to content

fix(reference): ignore lexically shadowed require lookups - #1657

Closed
rudycelekli wants to merge 4 commits into
morluto:mainfrom
rudycelekli:fix/reference-shadowed-require-20261010
Closed

rudycelekli wants to merge 4 commits into
morluto:mainfrom
rudycelekli:fix/reference-shadowed-require-20261010

Conversation

@rudycelekli

Copy link
Copy Markdown
Contributor

Reference source import currently treats locally bound require calls as CommonJS dependencies. For example, function load(require) { return require("./throwing.cjs"); } creates a dependency edge even when the caller supplies its own loader. The JavaScript semantic analyzer already understands lexical bindings; this change reuses it to accept only unbound global require references, including the supported resolve and main forms.

The regression executes owned Node fixtures and imports them through the production resource-owned source importer. It covers parameters, destructuring, hoisted declarations, catch bindings, dynamic lookup, genuine CommonJS controls, and static/dynamic ESM controls. Existing parser diagnostics are preserved. This addresses source-import relationships; the earlier semantic-analyzer fix in #509 is reused.

Validation on application commit 8de54e086308a2386a0fd0b898d7e2764e36ca4c:

  • The official focused regression fails on the unchanged parser at the intended relationship assertion; an independent structured classifier confirms that failure.
  • Four focused boundary files: 16 tests passed.
  • npm run verify:test-discovery, npm run check, and npm run build:cached passed with locked dependencies, Node 24.18.0 and npm 11.16.0.
  • Fork validation run explicitly checked out the application SHA and verified its source, test, and lockfile hashes. Its workflow-only child is separate from this PR.

The tested base is 4999e4be; main subsequently advanced to 26038912 without changes to either PR file. The shared semantic projection change only removes the bare CommonJS exports alias and does not alter reference collection. Full-suite and native-provider verification are not claimed. Prepared with AI assistance.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@CassieFox23

Copy link
Copy Markdown
Contributor

TL;DR: The fix works. In my 6 shadowed-require cases, Node loads nothing and head makes no dependency edge, where base made one. It also drops the edge in 3 cases where Node does load the file: createRequire twice and a top-level var require.

Thanks, rudycelekli, for the clear PR text and the test that runs the fixtures in Node first. I ran your new test and a second reader through rea import-reference-source.

Results

Check Node 22.23.1 Node 24.18.0 Node 26.11.1
New test on head 8de54e0 pass pass (3 of 3 runs) pass
Same test on base 4999e4b (#1651) not run fails: 10 edges, expected 2 not run
15 referenceSource* boundary files (61 tests) on head pass pass pass
src/domain/referenceSource* unit tests (13) on head not run pass not run
Head files laid over main d21132f (boundary files and unit tests) not run pass not run
Six parts of npm run check on head not run pass not run

On base, the 8 extra edges all point at throwing.cjs. They come from 8 of your fixture files: parameter.cjs, destructured.cjs, hoisted.cjs, catch.cjs, members.cjs, dynamic.cjs, control.cjs and imports.mjs.

On /tmp (a RAM disk), one unrelated test fails. It is the "applies exclusions" test in referenceSourceReader.test.ts. It expects one directory listing order, and a RAM disk lists in the other order. It passes on btrfs, so the table above uses btrfs.

Second reader

I wrote 15 small files and ran each one in Node 22.23.1, 24.18.0 and 26.11.1. All three Node versions agreed on every file. Each file requires its own target, and the target prints a line when Node loads it. Then I ran rea import-reference-source (under Node 24.18.0) on the folder at base and at head.

Case Node loads the file? Base edge Head edge
Six shadowed forms (rows 01, 02, 03, 06, 08 and 13 in the fold) no yes no
outer unbound require, inner function with a require parameter outer only both outer only
plain require("./f.cjs") control yes yes yes
require.resolve("./d.cjs") no, it only resolves yes yes
createRequire in an .mjs yes yes no
createRequire in a block of a .cjs yes yes no
var require; at the top of a .cjs yes yes no

The last three rows are the ones to look at. In a .cjs file, Node wraps the code in a function that takes require as a parameter. A top-level var require; only redeclares that parameter, so the real require still works. Head sees a local binding and drops the edge.

For createRequire, the binding is local but it is a real loader. This is the design in the PR text, so it may be fine. A control for it in the test would show the choice was on purpose.

Cases this PR does not change, same at base and head: module.require("./e.cjs") and require.main.require(...) load in Node but make no edge. A bare require(...) in an .mjs makes an edge, and Node throws ReferenceError there.

All 15 cases, one row per file

Edges to node:module from the createRequire files are the same at base and head and are left out.

File Node loads Base Head
01-param.cjs none edge none
02-destructure-block.cjs none edge none
03-destructure-esm.mjs none edge none
04-createRequire.mjs t04 edge none
05-createRequire-block.cjs t05 edge none
06-catch.cjs none edge none
07-var-toplevel.cjs t07 edge none
08-var-in-function.cjs none edge none
09-resolve.cjs none edge edge
10-module-require.cjs t10 none none
11-plain-control.cjs t11 edge edge
12-nested-shadow.cjs t12a t12a and t12b t12a
13-function-decl-toplevel.cjs none edge none
14-esm-bare-require.mjs none (ReferenceError) edge edge
15-require-main.cjs t15 none none

In total, base made 16 relationships and head made 6.

Not run

  • The full test suite, and any native-provider checks.
  • npm run check as one command. Its lint step runs a module-boundary script that needs a git checkout, and my test trees have none. I ran the other parts one by one. I ran the module-boundary script on the one changed source file only.
  • The MCP route. I used the CLI only.
  • Your fork CI run. I did not open it.
  • macOS and Windows.
Test setup
Commands
git archive 8de54e086 | tar -x -C /tmp/p9-1657/head   # same for 4999e4be and d21132fc9
npm ci && npm run build && npm run test:prepare
node node_modules/vitest/vitest.mjs run --project boundary tests/boundary/filesystem/referenceSourceShadowedRequire.test.ts
node node_modules/vitest/vitest.mjs run --project boundary tests/boundary/filesystem/referenceSource
node node_modules/vitest/vitest.mjs run src/domain/referenceSource
node scripts/rea.mjs import-reference-source <folder> --format json

🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊

@morluto

morluto commented Oct 11, 2026

Copy link
Copy Markdown
Owner

Integrated the lexical-binding fix on main in f998fcb, with co-author credit for this PR.

The implementation reuses the shared lexical analyzer, preserves the CommonJS wrapper binding after an uninitialized top-level var require, and recognizes direct createRequire(import.meta.url) bindings. It also keeps candidate dependency edges when .js/.ts runtime mode is unknown; import syntax alone does not establish the compilation target.

All 30 tests across the import, computed require, dynamic import, shadowed require, and TypeScript-extension boundary suites passed. check:fast passed. The regression executes real owned Node fixtures and exercises the production source importer. Full-suite/provider coverage is not claimed.

Closing this PR as integrated.

@morluto morluto closed this Oct 11, 2026
@CassieFox23

Copy link
Copy Markdown
Contributor

TL;DR: Two of the three missing edges are fixed on main. They are createRequire in an .mjs and a top-level var require;. The third, createRequire(__filename) in a block of a .cjs, still makes no edge.

Thanks, morluto, for the fix and the note. Thanks, rudycelekli, for the PR and its test. I reran my 15 small files on main 83e5255. It holds f998fcb and three later commits. Of those, only 83e5255 changes the loader files. It only reformats them. Main has since moved to 3749cce, which does not touch them. Node 22.23.1, 24.18.0 and 26.11.1 agreed on which files load. The edges come from rea import-reference-source under Node 24.18.0.

Case Node loads the file? PR head 8de54e0 (my earlier run) Main 83e5255
createRequire(import.meta.url) in an .mjs (04) yes no edge edge
var require; at the top of a .cjs (07) yes no edge edge
createRequire(__filename) in a block of a .cjs (05) yes no edge no edge
Bare require(...) in an .mjs (14) no, ReferenceError edge no edge

The six shadowed forms (01, 02, 03, 06, 08 and 13) still make no edge on main. Node loads none of them.

Rows that did not change from 8de54e0:

  • The plain require control (11) makes an edge, and the nested case (12) makes one edge.
  • module.require (10) and require.main.require (15) load in Node but make no edge.
  • require.resolve (09) makes an edge, though Node only resolves there.

The 05 gap fits the note: the code only matches a top-level const require = createRequire(import.meta.url). That comes from reading referenceSourceRequireBindings.ts, not from a test. It may be a fair limit. The new test covers a top-level var require;, an .mjs createRequire, and a bare require in an .mjs, but no .cjs createRequire case.

Tests on main 83e5255:

Check Node 22.23.1 Node 24.18.0 Node 26.11.1
referenceSourceShadowedRequire.test.ts (1 test) pass pass pass
15 referenceSource* boundary files (61 tests) not run pass not run
referenceSourceGraph.test.ts unit file (13 tests) not run pass not run

Not run

  • The full test suite, and any native-provider checks.
  • npm run check as one command.
  • The MCP route. I used the CLI only.
  • macOS and Windows.
  • The PR's own CI.
All 15 cases, one row per file

Edges to node:module from the createRequire files are left out. The PR head column is from my earlier run, not rerun.

File Node loads PR head 8de54e0 Main 83e5255
01-param.cjs none none none
02-destructure-block.cjs none none none
03-destructure-esm.mjs none none none
04-createRequire.mjs t04 none t04
05-createRequire-block.cjs t05 none none
06-catch.cjs none none none
07-var-toplevel.cjs t07 none t07
08-var-in-function.cjs none none none
09-resolve.cjs none t09 t09
10-module-require.cjs t10 none none
11-plain-control.cjs t11 t11 t11
12-nested-shadow.cjs t12a t12a t12a
13-function-decl-toplevel.cjs none none none
14-esm-bare-require.mjs none t14 none
15-require-main.cjs t15 none none

In total, main made 7 relationships and 8de54e0 made 6. Two of the 7 on main point at node:module.

Test setup
  • Box: Nobara 44 (Fedora 44 base), Linux 7.2.9
  • CPU: AMD Ryzen 9 5900X, 12 cores / 24 threads
  • RAM: 32 GB DDR4-3600
  • Node: 22.23.1, 24.18.0, 26.11.1
  • Main: 83e5255
  • Earlier PR head: 8de54e0

🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊

@morluto

morluto commented Oct 15, 2026

Copy link
Copy Markdown
Owner

@rudycelekli, thanks for showing that lexically shadowed require calls were creating false dependency edges. The importer now uses lexical binding facts and retains coverage for both shadowed and genuine CommonJS lookups (main).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants