Repository navigation
fix(graph): skip import edges for lexically shadowed require (#2890) #2989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
40df65e
4c4c273
8ef0dd1
4c7faad
3e26716
92e2783
e8b8bed
3d0f980
4173418
84ea39f
87049c7
0c1aca4
b3e1522
dd6c1a2
453a31a
9a09003
8659198
a6e6555
e8111ff
5b49524
82859a2
68cb6c6
b633841
f3a4463
c8030ec
6b9f061
c7017dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| argumentValue, | ||
| calleeName, | ||
| endpointArgument as endpointArgumentHelper, | ||
| isImportMetaUrl, | ||
| prefixedArgument, | ||
| range, | ||
| rangeForOffsets, | ||
|
|
@@ -33,7 +34,10 @@ import type { | |
| } from "./javascriptStaticAnalysisState.js"; | ||
| import { | ||
| resolveSemanticBindingState, | ||
| semanticResolutionBlocked, | ||
| semanticStableInitializer, | ||
| type JavaScriptSemanticAnalysisState, | ||
| type JavaScriptSemanticBindingState, | ||
| } from "./javascriptSemanticState.js"; | ||
| import { | ||
| isUnmodifiedGlobalReference, | ||
|
|
@@ -57,7 +61,7 @@ export const inspectCall = ( | |
| const first = stringValue(node.arguments[0]); | ||
| const module = context.accumulator.moduleRangeIndex.find(node.start); | ||
| const moduleRequireName = module?.requireName ?? null; | ||
| const requireCall = requireCallSpecifier(node, name, moduleRequireName); | ||
| const requireCall = requireCallSpecifier(node, name, moduleRequireName, bindings); | ||
| if (requireCall !== null) | ||
| addReference(context, { | ||
| node, | ||
|
|
@@ -214,9 +218,18 @@ const requireCallSpecifier = ( | |
| node: JavaScriptCallLike, | ||
| name: string, | ||
| moduleRequireName: string | null, | ||
| bindings: JavaScriptSemanticAnalysisState, | ||
| ): { readonly specifier: string | undefined } | null => { | ||
| if (name === "require" || name.endsWith(".require")) | ||
| if (name === "require" || name.endsWith(".require")) { | ||
| // A bare `require` is the CommonJS loader only while its callee | ||
| // identifier is lexically unbound. A local parameter, hoisted function, | ||
| // catch binding or block binding shadows the wrapper loader, so the call | ||
| // cannot be a module load. The binding index is consulted at the call | ||
| // site; no file-wide walk is needed. | ||
| if (name === "require" && isShadowedRequireCallee(node, bindings)) | ||
| return null; | ||
| return { specifier: argumentValue(node.arguments[0]) }; | ||
| } | ||
| if (name === "__webpack_require__" || name === moduleRequireName) | ||
| return { specifier: argumentValue(node.arguments[0]) }; | ||
| if ( | ||
|
|
@@ -227,6 +240,95 @@ const requireCallSpecifier = ( | |
| return null; | ||
| }; | ||
|
|
||
| /** | ||
| * Decide whether a bare `require(...)` / `require?.(...)` callee resolves to | ||
| * a local binding that shadows the CommonJS loader. Unbound identifiers keep | ||
| * the loader; only a binding that provably retains or aliases the loader | ||
| * (uninitialized top-level `var require`, direct `createRequire` alias) | ||
| * preserves the candidate. | ||
| */ | ||
| const isShadowedRequireCallee = ( | ||
| node: JavaScriptCallLike, | ||
| bindings: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| const callee = unwrapJavaScriptExpression(node.callee).node; | ||
| if (!t.isIdentifier(callee, { name: "require" })) return false; | ||
| // An incomplete scope cannot prove shadowing; keep the candidate. | ||
| if (semanticResolutionBlocked(bindings, callee, "require")) return false; | ||
| const binding = resolveSemanticBindingState(bindings, callee, "require"); | ||
| if (binding === undefined) return false; | ||
| return !isRetainedRequireLoader(binding, node, bindings); | ||
| }; | ||
|
|
||
| const isRetainedRequireLoader = ( | ||
| binding: JavaScriptSemanticBindingState, | ||
| node: JavaScriptCallLike, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => | ||
| isDirectCreateRequireBinding(binding, state) || | ||
| isUninitializedTopLevelVarRequire(binding, state, node); | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| /** | ||
| * `const require = createRequire(import.meta.url)` aliases the ESM loader. | ||
| * Erased TypeScript syntax around the initializer (e.g. `as NodeRequire`) | ||
| * is unwrapped before matching, mirroring the reference-source classifier. | ||
| */ | ||
| const isDirectCreateRequireBinding = ( | ||
| binding: JavaScriptSemanticBindingState, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| const initializer = semanticStableInitializer(binding, state); | ||
| if (initializer === undefined) return false; | ||
| const init = unwrapJavaScriptExpression(initializer.node).node; | ||
| if (!t.isCallExpression(init)) return false; | ||
| const callee = unwrapJavaScriptExpression(init.callee).node; | ||
| if (!t.isIdentifier(callee)) return false; | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| const calleeBinding = resolveSemanticBindingState(state, callee, callee.name); | ||
| const [origin] = calleeBinding?.directOrigins ?? []; | ||
| return ( | ||
| calleeBinding?.runtimeImport === true && | ||
| origin !== undefined && | ||
| (origin.specifier === "node:module" || origin.specifier === "module") && | ||
| origin.importedPath.length === 1 && | ||
| origin.importedPath[0] === "createRequire" && | ||
| isImportMetaUrl(init.arguments[0]) | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * An uninitialized top-level `var require` retains Node's wrapper loader, but | ||
| * a later write replaces it. Write handling is positional: a call that runs | ||
| * before every write still sees the loader (this covers loader calls before | ||
| * reassignment and eagerly evaluated computed method keys without classifying | ||
| * enclosing functions as deferred). A call at or after a write is shadowed. | ||
| */ | ||
| const isUninitializedTopLevelVarRequire = ( | ||
| binding: JavaScriptSemanticBindingState, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| node: JavaScriptCallLike, | ||
| ): boolean => { | ||
| const assignmentCount = binding.definitions.filter( | ||
| ({ kind }) => kind === "assignment", | ||
| ).length; | ||
| if ( | ||
| binding.kind !== "variable" || | ||
| !binding.definitions.every( | ||
| ({ kind }) => kind === "variable" || kind === "assignment", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every bare Cache the binding’s declaration facts and index its write positions once instead of rescanning them. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 310-316
Comment:
**Repeated writes slow analysis**
Every bare `require` call now scans the binding’s full `definitions` array with `filter` and `every`, then potentially scans it again with `some`. Each assignment adds another definition, so files with many calls and writes pay calls × writes work, plus a temporary array per call.
Cache the binding’s declaration facts and index its write positions once instead of rescanning them.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| ) || | ||
| // A declaration initializer means the loader was replaced at declaration. | ||
| binding.initializers.length !== assignmentCount || | ||
| state.scopesById.get(binding.scopeId)?.kind !== "program" | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| ) | ||
| return false; | ||
| const { line, column } = range(node).start; | ||
| return !binding.definitions.some( | ||
| ({ kind, location }) => | ||
| kind === "assignment" && | ||
| (location.start.line < line || | ||
| (location.start.line === line && location.start.column <= column)), | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| ); | ||
| }; | ||
|
|
||
| const chunkLoadSpecifier = ( | ||
| node: JavaScriptCallLike, | ||
| name: string, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { analyzeJavaScriptStaticSource } from "./javascriptStaticAnalysis.js"; | ||
|
|
||
| /** | ||
| * Regression tests for https://github.com/morluto/rea/issues/2890: the | ||
| * application graph must not invent a CommonJS `imports` edge for a `require` | ||
| * call whose callee resolves to a lexically shadowing local binding. The | ||
| * static findings are the producing boundary, so the tests assert on the | ||
| * recovered `require` references directly. | ||
| */ | ||
| const requireSpecifiers = (source: string): readonly (string | null)[] => | ||
| analyzeJavaScriptStaticSource(source) | ||
| .references.filter((reference) => reference.kind === "require") | ||
| .map((reference) => reference.specifier); | ||
|
|
||
| describe("lexically shadowed require calls", () => { | ||
| it.each([ | ||
| [ | ||
| "function parameter", | ||
| `function run(require) {\n return require("./ghost.cjs");\n}\nconsole.log(run(() => "local"));`, | ||
| ], | ||
| [ | ||
| "optional call with function parameter", | ||
| `function run(require) {\n return require?.("./ghost.cjs");\n}`, | ||
| ], | ||
| [ | ||
| "destructured parameter", | ||
| `function load({ require }) { return require("./throwing.cjs"); }\nconsole.log(load({ require: () => "local" }));`, | ||
| ], | ||
| [ | ||
| "hoisted function declaration", | ||
| `function load() {\n console.log(require("./throwing.cjs"));\n function require() { return "local"; }\n}\nload();`, | ||
| ], | ||
| [ | ||
| "catch binding", | ||
| `try { throw () => "local"; }\ncatch (require) { console.log(require("./throwing.cjs")); }`, | ||
| ], | ||
| [ | ||
| "block-scoped binding", | ||
| `{ const require = () => "local";\n console.log(require("./throwing.cjs")); }`, | ||
| ], | ||
| [ | ||
| "initialized top-level var", | ||
| `var require = () => "local";\nconsole.log(require("./throwing.cjs"));`, | ||
| ], | ||
| ])("emits no require reference for a %s shadow", (_label, source) => { | ||
| expect(requireSpecifiers(source)).toEqual([]); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [ | ||
| "ordinary unbound call", | ||
| `const value = require("./real.cjs");`, | ||
| ["./real.cjs"], | ||
| ], | ||
| [ | ||
| "optional unbound call", | ||
| `const value = require?.("./real.cjs");`, | ||
| ["./real.cjs"], | ||
| ], | ||
| [ | ||
| "uninitialized top-level var retains the wrapper loader", | ||
| `var require;\nconsole.log(require("./loaded.cjs"));`, | ||
| ["./loaded.cjs"], | ||
| ], | ||
| [ | ||
| "direct createRequire binding", | ||
| `import { createRequire } from "node:module";\nconst require = createRequire(import.meta.url);\nconst value = require("./dep.cjs");`, | ||
| ["./dep.cjs"], | ||
| ], | ||
| [ | ||
| "createRequire binding behind erased TypeScript syntax", | ||
| `import { createRequire } from "node:module";\nconst require = createRequire(import.meta.url) as NodeRequire;\nconst value = require("./dep.cjs");`, | ||
| ["./dep.cjs"], | ||
| ], | ||
| [ | ||
| "loader call before reassignment", | ||
| `require?.("./dep");\nrequire = localLoader;`, | ||
| ["./dep"], | ||
| ], | ||
| [ | ||
| "eager computed method key before reassignment", | ||
| `const routes = { [require?.("./dep")]() {} };\nrequire = localLoader;`, | ||
| ["./dep"], | ||
| ], | ||
| ])("keeps the require reference for %s", (_label, source, expected) => { | ||
| expect(requireSpecifiers(source)).toEqual(expected); | ||
| }); | ||
|
|
||
| it("keeps only the genuine call in a mixed file", () => { | ||
| expect( | ||
| requireSpecifiers( | ||
| `console.log(require("./loaded.cjs"));\nfunction load(require) { return require("./throwing.cjs"); }\nconsole.log(load(() => "local"));`, | ||
| ), | ||
| ).toEqual(["./loaded.cjs"]); | ||
| }); | ||
|
|
||
| it("keeps the loader call before a write but not the call after it", () => { | ||
| expect( | ||
| requireSpecifiers( | ||
| `var require;\nrequire("./before.cjs");\nrequire = localLoader;\nrequire("./after.cjs");`, | ||
| ), | ||
| ).toEqual(["./before.cjs"]); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.