morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #2989] [CLOSED] fix(graph): skip import edges for lexically shadowed require (#2890) #5543

New issue

Closed

opened 2026-10-31 01:07:27 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:07:27 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2989
Author: @hussnainahmedd
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/2890-shadowed-require

📝 Commits (10+)

40df65e fix(graph): skip import edges for lexically shadowed require (#2890)
4c4c273 fix(graph): address review findings on shadowed-require detection
8ef0dd1 Merge branch 'main' into fix/2890-shadowed-require
4c7faad Merge branch 'main' into fix/2890-shadowed-require
3e26716 fix(graph): address greptile review findings on loader exemptions
92e2783 Merge branch 'main' into fix/2890-shadowed-require
e8b8bed Merge branch 'main' into fix/2890-shadowed-require
3d0f980 fix(graph): verify loader-parameter bindings positionally and accept more createRequire forms
4173418 Merge branch 'main' into fix/2890-shadowed-require
84ea39f fix(graph): make require-loader checks positional for writes, keys, and rest copies

📊 Changes

8 files changed (+1148 additions, -5 deletions)

View changed files

📝 src/domain/javascript/javascriptSemanticAnalysis.ts (+18 -1)
📝 src/domain/javascript/javascriptSemanticIr.ts (+6 -0)
📝 src/domain/javascript/javascriptStaticAnalysisBundler.ts (+6 -0)
📝 src/domain/javascript/javascriptStaticAnalysisCalls.ts (+671 -3)
📝 src/domain/javascript/javascriptStaticAnalysisHelpers.ts (+1 -1)
📝 src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts (+4 -0)
➕ src/domain/javascript/javascriptStaticAnalysisShadowedRequire.test.ts (+434 -0)
📝 src/domain/javascript/javascriptStaticAnalysisState.ts (+8 -0)

📄 Description

Problem

analyze-javascript-application (CLI and stdio MCP) emits a resolved, high-confidence imports edge for a require(...) call even when require resolves to a local binding — e.g. a function parameter, a hoisted function declaration, a catch binding, or a block-scoped binding. That is a confident-looking false fact in the evidence graph: lexical binding already establishes the call is not the CommonJS loader.

Minimal repro from the issue (still failing on current main):

// main.cjs
function run(require) {
  return require("./ghost.cjs");
}
console.log(run(() => "local"));

Node prints local, but REA returns an imports edge (kind: "require", resolution_status: "resolved").

Root cause: inspectCall already receives the lexical binding index (JavaScriptSemanticAnalysisState), but requireCallSpecifier in src/domain/javascript/javascriptStaticAnalysisCalls.ts recognizes the bare require name syntactically without consulting it. JavaScriptArtifactGraphFindings then resolves the candidate specifier to an inventoried file and emits the edge.

Fix

requireCallSpecifier now consults the binding index at the call site before emitting a require reference for a bare require/require?.() callee:

Shadowed (parameter, hoisted function, catch, block binding, initialized var/let/const, import binding) → no reference, no edge.
Unbound (Node's wrapper loader) → reference kept.
Uninitialized top-level var require → reference kept (retains the wrapper loader, per the issue spec).
Direct createRequire(import.meta.url) alias (const/let, incl. behind erased TS syntax like as NodeRequire, incl. aliased imports) → reference kept.
Writes are handled positionally, not with a file-wide flag: a call that runs before every write to require still sees the loader, so require?.("./dep"); require = localLoader and eagerly evaluated computed method keys ({ [require?.("./dep")]() {} }) keep their edges. A call at/after a write is treated as shadowed.

The check is local to the call site (resolveSemanticBindingState, O(scope depth)); no file-wide AST walk is introduced. The gate applies only to the bare require name — __webpack_require__, module-scoped require names, and bundle-factory loaders are untouched.

Verification

New regression suite src/domain/javascript/javascriptStaticAnalysisShadowedRequire.test.ts (16 tests): the issue repro (plain + optional call), destructured param, hoisted function, catch, block and initialized-var shadows emit no reference; unbound, optional-unbound, uninitialized top-level var, createRequire (plain + as-cast + aliased import) keep theirs; loader-before-reassignment, computed-method-key, mixed-file, and call-before-write/call-after-write cases behave as specified. All 16 pass.
Existing suites re-run against the change (node-based runner; see environment note): all src/domain/javascript tests — 1289 passed, 0 failed (one fast-check property suite excluded, see below); tests/boundary/filesystem/referenceSourceShadowedRequire.test.js — pass (reference-source path unaffected); tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.js — 6 passed (full application-graph pipeline).
npm run verify:module-boundaries — pass ("Checked resolved module boundaries in 1758 source files").
tsc --noEmit: 823 errors before and after the change — identical count, all pre-existing (corrupted .d.ts files in node_modules from a disk-full npm ci in this sandbox); zero errors in the touched files.

Environment note: this sandbox cannot spawn vitest workers (SIGBUS, also breaks the repo's own verify:test-discovery script and the oxfmt pre-commit hook), and ESLint is not installed here. Tests were executed by compiling with the repo's TypeScript and running the compiled suites through a small node-based vitest-API shim. CI should run the standard gates.

Relation to #2892

This addresses every review point left on the closed PR #2892:

Whole-file AST walks (Greptile, 2 comments): that PR ran collectSemanticReferences + two full traversals synchronously inside the analysis steps. This fix resolves scope per call site via the already-computed binding index — no new walks, and files without require calls pay nothing beyond a string comparison.
Loader calls before reassignment (Codex): the old file-wide requireLoaderWasWritten flag dropped every call once any write existed. Writes are now compared positionally against each call, so require?.("./dep"); require = localLoader keeps its edge.
Computed method keys treated as deferred (Codex): no deferred/eager ancestor classification exists here at all — the positional write check handles { [require?.("./dep")]() {} } correctly without walking ancestors.
Unwrapping createRequire initializers (Codex): the initializer is passed through unwrapJavaScriptExpression (unwraps TSAsExpression and friends) before matching, so const require = createRequire(import.meta.url) as NodeRequire is recognized.
Filtering semantic references (Codex): no reference collection or copying is added; the classifier reuses the shared lexical analyzer that the findings pass already receives.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2989 **Author:** [@hussnainahmedd](https://github.com/hussnainahmedd) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/2890-shadowed-require` --- ### 📝 Commits (10+) - [`40df65e`](https://github.com/morluto/rea/commit/40df65e2bb0da72eecb4a892baeaac2475c7e80b) fix(graph): skip import edges for lexically shadowed require (#2890) - [`4c4c273`](https://github.com/morluto/rea/commit/4c4c273cbffacffed396a9ae59a242edc3c7d40a) fix(graph): address review findings on shadowed-require detection - [`8ef0dd1`](https://github.com/morluto/rea/commit/8ef0dd1100bdc3050383094c6ac07e3ca318ff8a) Merge branch 'main' into fix/2890-shadowed-require - [`4c7faad`](https://github.com/morluto/rea/commit/4c7faad191f755c946acb79c852e0db6b8c0636d) Merge branch 'main' into fix/2890-shadowed-require - [`3e26716`](https://github.com/morluto/rea/commit/3e26716e63c3482baf6b7f75f2ddb9347d1b5d60) fix(graph): address greptile review findings on loader exemptions - [`92e2783`](https://github.com/morluto/rea/commit/92e27836a17be59774f560a093bb3dd785983dcd) Merge branch 'main' into fix/2890-shadowed-require - [`e8b8bed`](https://github.com/morluto/rea/commit/e8b8bedd5a8ed7a2e7b3ac566d71566580f9d0d9) Merge branch 'main' into fix/2890-shadowed-require - [`3d0f980`](https://github.com/morluto/rea/commit/3d0f980477bbab9ebf1f0cd1f5c749e5c2c02348) fix(graph): verify loader-parameter bindings positionally and accept more createRequire forms - [`4173418`](https://github.com/morluto/rea/commit/4173418b9e8f8f7eca82b0b4910d2041fdddaa26) Merge branch 'main' into fix/2890-shadowed-require - [`84ea39f`](https://github.com/morluto/rea/commit/84ea39f7120d088e40195f4a1beae7d2ab259777) fix(graph): make require-loader checks positional for writes, keys, and rest copies ### 📊 Changes **8 files changed** (+1148 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptSemanticAnalysis.ts` (+18 -1) 📝 `src/domain/javascript/javascriptSemanticIr.ts` (+6 -0) 📝 `src/domain/javascript/javascriptStaticAnalysisBundler.ts` (+6 -0) 📝 `src/domain/javascript/javascriptStaticAnalysisCalls.ts` (+671 -3) 📝 `src/domain/javascript/javascriptStaticAnalysisHelpers.ts` (+1 -1) 📝 `src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts` (+4 -0) ➕ `src/domain/javascript/javascriptStaticAnalysisShadowedRequire.test.ts` (+434 -0) 📝 `src/domain/javascript/javascriptStaticAnalysisState.ts` (+8 -0) </details> ### 📄 Description ## Problem `analyze-javascript-application` (CLI and stdio MCP) emits a resolved, high-confidence `imports` edge for a `require(...)` call even when `require` resolves to a **local binding** — e.g. a function parameter, a hoisted function declaration, a catch binding, or a block-scoped binding. That is a confident-looking false fact in the evidence graph: lexical binding already establishes the call is not the CommonJS loader. Minimal repro from the issue (still failing on current `main`): ```js // main.cjs function run(require) { return require("./ghost.cjs"); } console.log(run(() => "local")); ``` Node prints `local`, but REA returns an `imports` edge (`kind: "require"`, `resolution_status: "resolved"`). Root cause: `inspectCall` already receives the lexical binding index (`JavaScriptSemanticAnalysisState`), but `requireCallSpecifier` in `src/domain/javascript/javascriptStaticAnalysisCalls.ts` recognizes the bare `require` name syntactically without consulting it. `JavaScriptArtifactGraphFindings` then resolves the candidate specifier to an inventoried file and emits the edge. ## Fix `requireCallSpecifier` now consults the binding index at the call site before emitting a `require` reference for a bare `require`/`require?.()` callee: - **Shadowed** (parameter, hoisted function, catch, block binding, initialized `var`/`let`/`const`, import binding) → no reference, no edge. - **Unbound** (Node's wrapper loader) → reference kept. - **Uninitialized top-level `var require`** → reference kept (retains the wrapper loader, per the issue spec). - **Direct `createRequire(import.meta.url)` alias** (`const`/`let`, incl. behind erased TS syntax like `as NodeRequire`, incl. aliased imports) → reference kept. - **Writes are handled positionally, not with a file-wide flag**: a call that runs *before* every write to `require` still sees the loader, so `require?.("./dep"); require = localLoader` and eagerly evaluated computed method keys (`{ [require?.("./dep")]() {} }`) keep their edges. A call at/after a write is treated as shadowed. The check is local to the call site (`resolveSemanticBindingState`, O(scope depth)); no file-wide AST walk is introduced. The gate applies only to the bare `require` name — `__webpack_require__`, module-scoped require names, and bundle-factory loaders are untouched. ## Verification - New regression suite `src/domain/javascript/javascriptStaticAnalysisShadowedRequire.test.ts` (16 tests): the issue repro (plain + optional call), destructured param, hoisted function, catch, block and initialized-var shadows emit no reference; unbound, optional-unbound, uninitialized top-level `var`, `createRequire` (plain + `as`-cast + aliased import) keep theirs; loader-before-reassignment, computed-method-key, mixed-file, and call-before-write/call-after-write cases behave as specified. All 16 pass. - Existing suites re-run against the change (node-based runner; see environment note): all `src/domain/javascript` tests — **1289 passed, 0 failed** (one fast-check property suite excluded, see below); `tests/boundary/filesystem/referenceSourceShadowedRequire.test.js` — pass (reference-source path unaffected); `tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.js` — 6 passed (full application-graph pipeline). - `npm run verify:module-boundaries` — pass ("Checked resolved module boundaries in 1758 source files"). - `tsc --noEmit`: 823 errors before and after the change — identical count, all pre-existing (corrupted `.d.ts` files in `node_modules` from a disk-full `npm ci` in this sandbox); zero errors in the touched files. Environment note: this sandbox cannot spawn vitest workers (SIGBUS, also breaks the repo's own `verify:test-discovery` script and the `oxfmt` pre-commit hook), and ESLint is not installed here. Tests were executed by compiling with the repo's TypeScript and running the compiled suites through a small node-based vitest-API shim. CI should run the standard gates. ## Relation to #2892 This addresses every review point left on the closed PR #2892: 1. **Whole-file AST walks (Greptile, 2 comments)**: that PR ran `collectSemanticReferences` + two full traversals synchronously inside the analysis steps. This fix resolves scope per call site via the already-computed binding index — no new walks, and files without `require` calls pay nothing beyond a string comparison. 2. **Loader calls before reassignment (Codex)**: the old file-wide `requireLoaderWasWritten` flag dropped *every* call once any write existed. Writes are now compared positionally against each call, so `require?.("./dep"); require = localLoader` keeps its edge. 3. **Computed method keys treated as deferred (Codex)**: no deferred/eager ancestor classification exists here at all — the positional write check handles `{ [require?.("./dep")]() {} }` correctly without walking ancestors. 4. **Unwrapping `createRequire` initializers (Codex)**: the initializer is passed through `unwrapJavaScriptExpression` (unwraps `TSAsExpression` and friends) before matching, so `const require = createRequire(import.meta.url) as NodeRequire` is recognized. 5. **Filtering semantic references (Codex)**: no reference collection or copying is added; the classifier reuses the shared lexical analyzer that the findings pass already receives. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:07:27 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:44:24 +00:00

[PR #5573] [CLOSED] fix(native): mark Objective-C declarations partial when the budget cuts their methods #6971

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5543

No description provided.

Rows
Columns