Repository navigation
fix(javascript): ignore a require call that a local binding shadows #2921
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 all commits
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 |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { analyzeJavaScriptStaticSource } from "./javascriptStaticAnalysis.js"; | ||
|
|
||
| /** Every specifier the static analysis recorded as a loader call. */ | ||
| const requireSpecifiers = (source: string): readonly unknown[] => | ||
| analyzeJavaScriptStaticSource(source) | ||
| .references.filter((reference) => reference.kind === "require") | ||
| .map((reference) => (reference as { specifier?: unknown }).specifier); | ||
|
|
||
| describe("static require loader calls", () => { | ||
| it("records a require call that no binding shadows", () => { | ||
| expect(requireSpecifiers('const loaded = require("./real.cjs");')).toEqual([ | ||
| "./real.cjs", | ||
| ]); | ||
| }); | ||
|
|
||
| it("records a member loader call", () => { | ||
| expect( | ||
| requireSpecifiers('const loaded = module.require("./member.cjs");'), | ||
| ).toEqual(["./member.cjs"]); | ||
| }); | ||
|
|
||
| it("records a loader a createRequire binding provides", () => { | ||
| expect( | ||
| requireSpecifiers(` | ||
| import { createRequire } from "node:module"; | ||
| const require = createRequire(import.meta.url); | ||
| const loaded = require("./created.cjs"); | ||
| `), | ||
| ).toEqual(["./created.cjs"]); | ||
| }); | ||
|
|
||
| it("follows an aliased createRequire import from node:module", () => { | ||
| expect( | ||
| requireSpecifiers(` | ||
| import { createRequire as makeRequire } from "node:module"; | ||
| const require = makeRequire(import.meta.url); | ||
| const loaded = require("./aliased.cjs"); | ||
| `), | ||
| ).toEqual(["./aliased.cjs"]); | ||
| }); | ||
|
|
||
| it("does not treat a local createRequire function as Node's factory", () => { | ||
| expect( | ||
| requireSpecifiers(` | ||
| function createRequire() { return () => "local"; } | ||
| const require = createRequire(); | ||
| const loaded = require("./ghost.cjs"); | ||
| `), | ||
| ).toEqual([]); | ||
| }); | ||
|
|
||
| it("keeps the CommonJS wrapper loader separate from globalThis.require", () => { | ||
| expect( | ||
| requireSpecifiers(` | ||
| globalThis.require = shim; | ||
| const loaded = require("./wrapper.cjs"); | ||
| `), | ||
| ).toEqual(["./wrapper.cjs"]); | ||
| }); | ||
|
|
||
| it("keeps an uninitialized top-level var redeclaration of the wrapper loader", () => { | ||
| expect(requireSpecifiers('var require; require("./wrapper.cjs");')).toEqual( | ||
| ["./wrapper.cjs"], | ||
| ); | ||
| }); | ||
|
|
||
| it("ignores a require call that a local binding shadows", () => { | ||
| expect( | ||
| requireSpecifiers(` | ||
| function run(require) { | ||
| return require("./ghost.cjs"); | ||
| } | ||
| console.log(run(() => "local")); | ||
| `), | ||
| ).toEqual([]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| } from "./javascriptStaticAnalysisState.js"; | ||
| import { | ||
| resolveSemanticBindingState, | ||
| semanticStableInitializer, | ||
| type JavaScriptSemanticAnalysisState, | ||
| } from "./javascriptSemanticState.js"; | ||
| import { | ||
|
|
@@ -57,7 +58,12 @@ | |
| 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, | ||
| isRequireLoaderReference(node.callee, bindings, name), | ||
| ); | ||
| if (requireCall !== null) | ||
| addReference(context, { | ||
| node, | ||
|
|
@@ -210,12 +216,98 @@ | |
| accumulator.mediaElementPreloads.add(property); | ||
| }; | ||
|
|
||
| /** | ||
| * A `require` name is the loader unless a lexical binding replaces it. A binding | ||
| * proven to come from `createRequire(...)` is that loader; any other binding, | ||
| * such as a function parameter, is a different value that only shares the name. | ||
| */ | ||
| const isRequireLoaderReference = ( | ||
| callee: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| name: string, | ||
| ): boolean => { | ||
| const binding = resolveSemanticBindingState(state, callee, name); | ||
| if (binding === undefined) { | ||
| // Node's CommonJS wrapper supplies a lexical `require` parameter. Writes | ||
| // to globalThis.require do not replace it; retain that possibility for | ||
| // script/CommonJS source where wrapper mode is possible. | ||
| if (name === "require" && isScriptProgram(callee, state)) return true; | ||
|
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.
The new script shortcut ignores direct assignments to Keep the distinction between Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 234
Comment:
**Replaced loaders invent dependencies**
The new script shortcut ignores direct assignments to `require`. With `require = () => "local"; require("./ghost.cjs");`, no lexical binding is created, so this branch returns `true` even though the source replaced the loader. The previous `isUnmodifiedGlobalReference` check rejected this visible write. The analyzer now records a dependency for a call that loads nothing.
Keep the distinction between `globalThis.require` and the wrapper's `require`, but still reject direct writes to the latter.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| return isUnmodifiedGlobalReference(callee, state, name); | ||
|
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.
In a AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| } | ||
| if ( | ||
| name === "require" && | ||
| isTopLevelCommonJsVarRequire(binding, callee, state) | ||
| ) | ||
| return true; | ||
| const initializer = semanticStableInitializer(binding, state)?.node; | ||
|
Comment on lines
+229
to
+242
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.
For a AGENTS.md reference: AGENTS.md:L21-L21 Useful? React with 👍 / 👎. 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.
In a script/CommonJS file with AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| const expression = | ||
| initializer !== undefined && t.isVariableDeclarator(initializer) | ||
| ? initializer.init | ||
| : initializer; | ||
| return ( | ||
| expression !== null && | ||
| expression !== undefined && | ||
| t.isCallExpression(expression) && | ||
|
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.
When TypeScript writes a loader as AGENTS.md reference: AGENTS.md:L23-L23 Useful? React with 👍 / 👎. |
||
| isNodeCreateRequireFactory(expression.callee, state) | ||
|
Comment on lines
+242
to
+251
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.
Require an empty Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 242-251
Comment:
**Loader properties become imports**
`isRequireLoaderReference` ignores the initializer's `projection`. With `import { createRequire } from 'node:module'; const { resolve: require } = createRequire(import.meta.url); require('./ghost.cjs');`, the local `require` calls the loader's `resolve` method, not the loader. The new check still records `./ghost.cjs` as a dependency even though the call never loads it.
Require an empty `projection` before treating the binding as the returned loader, and add a regression test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+250
to
+251
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.
In a CommonJS module using AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎.
Comment on lines
+250
to
+251
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.
For RequireJS/AMD sources using the simplified CommonJS form AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| ); | ||
| }; | ||
|
|
||
| const isNodeCreateRequireFactory = ( | ||
| callee: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| if (!t.isIdentifier(callee)) return false; | ||
|
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.
The identifier-only check rejects real loaders created through a namespace import. For example, Check the member's module origin rather than rejecting every member call, and add a regression test. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 259
Comment:
**Namespace-created loaders lose dependencies**
The identifier-only check rejects real loaders created through a namespace import. For example, `import * as mod from "node:module"; const require = mod.createRequire(import.meta.url); require("./real.cjs");` was recognized before this change. Now `isNodeCreateRequireFactory` rejects `mod.createRequire`, so `./real.cjs` disappears from the static `references`.
Check the member's module origin rather than rejecting every member call, and add a regression test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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.
When ESM uses AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| const binding = resolveSemanticBindingState(state, callee, callee.name); | ||
| const [origin] = binding?.directOrigins ?? []; | ||
| return ( | ||
| binding?.runtimeImport === true && | ||
| binding.definitions.length === 1 && | ||
| binding.directOrigins.length === 1 && | ||
|
Comment on lines
+263
to
+265
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.
Recover the factory's CommonJS origin instead of requiring an ESM import, and add a regression test. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 263-265
Comment:
**CommonJS loader dependencies disappear**
`isNodeCreateRequireFactory` rejects factories loaded through CommonJS. For example, `const { createRequire } = require("node:module"); function run() { const require = createRequire(__filename); return require("./real.cjs"); } run();` creates a real loader, but the factory binding has `runtimeImport: false` and no `directOrigins`. The new check therefore drops `./real.cjs` from `references` and the application graph.
Recover the factory's CommonJS origin instead of requiring an ESM import, and add a regression test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| origin !== undefined && | ||
| (origin.specifier === "node:module" || origin.specifier === "module") && | ||
| origin.importedPath.length === 1 && | ||
| origin.importedPath[0] === "createRequire" | ||
| ); | ||
| }; | ||
|
|
||
| const isScriptProgram = ( | ||
| node: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| let parent = state.parentsByNode.get(node); | ||
| while (parent !== undefined && !t.isProgram(parent)) | ||
| parent = state.parentsByNode.get(parent); | ||
| return ( | ||
| parent !== undefined && | ||
| (parent.sourceType === "script" || parent.sourceType === "commonjs") | ||
|
Check failure on line 282 in src/domain/javascript/javascriptStaticAnalysisCalls.ts
|
||
| ); | ||
| }; | ||
|
|
||
| const isTopLevelCommonJsVarRequire = ( | ||
| binding: NonNullable<ReturnType<typeof resolveSemanticBindingState>>, | ||
| node: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => | ||
| binding.name === "require" && | ||
| state.scopesById.get(binding.scopeId)?.kind === "program" && | ||
| binding.definitions.length > 0 && | ||
| binding.definitions.every(({ kind }) => kind === "variable") && | ||
| binding.initializers.length === 0 && | ||
|
Comment on lines
+294
to
+295
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.
Check that the actual declaration uses Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 294-295
Comment:
**Let bindings become loaders**
`isTopLevelCommonJsVarRequire` also accepts `let require;`. Both `var` and `let` declarations get the semantic kind `"variable"`, and a top-level declaration without a value has no initializer. Thus `let require; function run() { return require("./ghost.cjs"); }` passes this check even though `let` creates a separate binding, not a wrapper redeclaration. The analyzer records a false loader reference even when `run` is never called.
Check that the actual declaration uses `var`, and add a negative test for `let`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| isScriptProgram(node, state); | ||
|
|
||
| const requireCallSpecifier = ( | ||
| node: JavaScriptCallLike, | ||
| name: string, | ||
| moduleRequireName: string | null, | ||
| isGlobalRequire: boolean, | ||
| ): { readonly specifier: string | undefined } | null => { | ||
| if (name === "require" || name.endsWith(".require")) | ||
| // A name alone does not make a loader call: a local binding named `require` | ||
| // shadows the ambient loader, so only an unshadowed reference to it counts. | ||
| // The module's own factory require name is a real binding, so it keeps its | ||
| // own lane below. | ||
| if (name === "require" && isGlobalRequire) | ||
|
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.
For calls such as AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎.
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| return { specifier: argumentValue(node.arguments[0]) }; | ||
| if (name.endsWith(".require")) | ||
| return { specifier: argumentValue(node.arguments[0]) }; | ||
| if (name === "__webpack_require__" || name === moduleRequireName) | ||
| return { specifier: argumentValue(node.arguments[0]) }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isRequireLoaderReferenceis evaluated eagerly for every call and new expression, althoughrequireCallSpecifierreads the result only for the exact namerequire; ordinary local and global calls now traverse semantic scope chains, sometimes again through initializer import provenance. On large minified bundles where calls dominate nodes, this adds avoidable analysis CPU and time proportional to all call sites, so guard this lookup withname === "require"or make the argument lazy.AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.