Repository navigation
fix(javascript): honor require bindings in static graphs #2892
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,294 @@ | ||
| import * as t from "@babel/types"; | ||
|
|
||
| import { analyzeParsedJavaScriptReferences } from "./javascriptSemanticAnalysis.js"; | ||
| import { | ||
| semanticBindingId, | ||
| semanticScopeId, | ||
| } from "./javascriptSemanticState.js"; | ||
| import { | ||
| completeSemanticSteps, | ||
| traverseJavaScriptAstSteps, | ||
| } from "./javascriptSemanticTraversal.js"; | ||
| import type { JavaScriptSemanticReference } from "./javascriptSemanticIr.js"; | ||
| import { | ||
| isExplicitModuleSourcePath, | ||
| type ParsedJavaScriptSource, | ||
| } from "./javascriptSourceParser.js"; | ||
|
|
||
| /** | ||
| * Keep calls that resolve either to Node's CommonJS wrapper loader or to a | ||
| * direct ESM `createRequire(import.meta.url)` binding. The shared lexical | ||
| * analyzer supplies binding ownership; this boundary accounts for the two | ||
| * Node loader bindings without evaluating arbitrary values. | ||
| */ | ||
| export const unshadowedJavaScriptRequireCalls = ( | ||
| file: Pick<ParsedJavaScriptSource, "program">, | ||
| sourcePath: string | undefined, | ||
| ): ReadonlySet<t.Node> => | ||
| completeSemanticSteps( | ||
| unshadowedJavaScriptRequireCallsSteps( | ||
| file, | ||
| sourcePath, | ||
| analyzeParsedJavaScriptReferences(file), | ||
| ), | ||
| ); | ||
|
|
||
| interface RequireCallSite { | ||
| readonly node: t.Node; | ||
| readonly reference: JavaScriptSemanticReference | undefined; | ||
| readonly statement: number; | ||
| readonly deferred: boolean; | ||
| } | ||
|
|
||
| /** Classify loader calls without draining whole-source work inside a visitor. */ | ||
| export function* unshadowedJavaScriptRequireCallsSteps( | ||
| file: Pick<ParsedJavaScriptSource, "program">, | ||
| sourcePath: string | undefined, | ||
| referenceFacts: readonly JavaScriptSemanticReference[], | ||
| ): Generator<void, ReadonlySet<t.Node>> { | ||
| const references = new Map<string, JavaScriptSemanticReference>(); | ||
| const writeLocations = new Set<string>(); | ||
| const programBinding = semanticBindingId( | ||
| semanticScopeId("program", file.program), | ||
| "require", | ||
| ); | ||
| let hasRequire = false; | ||
| for (const reference of referenceFacts) { | ||
| yield; | ||
| const key = locationKey(reference.location.start); | ||
| references.set(key, reference); | ||
| if (reference.name !== "require") continue; | ||
| hasRequire = true; | ||
| if ( | ||
| reference.role === "write" && | ||
| (reference.resolution === "unbound" || | ||
| reference.bindingId === programBinding) | ||
| ) | ||
| writeLocations.add(key); | ||
| } | ||
| if (!hasRequire) return new Set(); | ||
| const allowsUnbound = | ||
| sourcePath === undefined || !isExplicitModuleSourcePath(sourcePath); | ||
| const createRequireBinding = yield* directCreateRequireBindingSteps( | ||
| file.program, | ||
| references, | ||
| ); | ||
| const scan = yield* scanRequireSitesSteps( | ||
| file.program, | ||
| references, | ||
| writeLocations, | ||
| ); | ||
| const calls = new Set<t.Node>(); | ||
| for (const site of scan.sites) { | ||
| yield; | ||
| const reference = site.reference; | ||
| const loaderIntact = | ||
| !scan.unknownWrite && | ||
| (scan.firstWrite === null || | ||
| (!site.deferred && site.statement < scan.firstWrite)); | ||
| if ( | ||
| (allowsUnbound && loaderIntact && reference?.resolution === "unbound") || | ||
| (((allowsUnbound && scan.wrapperVar && loaderIntact) || | ||
| createRequireBinding) && | ||
| reference?.resolution === "resolved" && | ||
| reference.bindingId === programBinding) | ||
| ) | ||
| calls.add(site.node); | ||
| } | ||
| return calls; | ||
| } | ||
|
|
||
| const locationKey = (position: { | ||
| readonly line: number; | ||
| readonly column: number; | ||
| }): string => `${String(position.line)}:${String(position.column)}`; | ||
|
|
||
| function* scanRequireSitesSteps( | ||
| program: t.Program, | ||
| references: ReadonlyMap<string, JavaScriptSemanticReference>, | ||
| writeLocations: ReadonlySet<string>, | ||
| ): Generator< | ||
| void, | ||
| { | ||
| readonly sites: readonly RequireCallSite[]; | ||
| readonly wrapperVar: boolean; | ||
| readonly firstWrite: number | null; | ||
| readonly unknownWrite: boolean; | ||
| } | ||
| > { | ||
| const sites: RequireCallSite[] = []; | ||
| let statement = -1; | ||
| let wrapperVar = false; | ||
| let firstWrite: number | null = null; | ||
| let unknownWrite = false; | ||
| const locatedWrites = new Set<string>(); | ||
| const writeAt = (node: t.Node, ancestors: readonly t.Node[]): void => { | ||
| if (statement < 0 || deferredExecution(node, ancestors)) | ||
| unknownWrite = true; | ||
| else firstWrite = Math.min(firstWrite ?? statement, statement); | ||
| }; | ||
| yield* traverseJavaScriptAstSteps(program, { | ||
| enter: (node, parent, readAncestors) => { | ||
| if (parent === program && t.isStatement(node)) statement += 1; | ||
| if ( | ||
| t.isIdentifier(node, { name: "require" }) && | ||
| node.loc !== null && | ||
| node.loc !== undefined && | ||
| writeLocations.has(locationKey(node.loc.start)) | ||
| ) { | ||
| locatedWrites.add(locationKey(node.loc.start)); | ||
| writeAt(node, readAncestors()); | ||
| } | ||
| if ( | ||
| t.isVariableDeclarator(node) && | ||
| t.isVariableDeclaration(parent, { kind: "var" }) && | ||
| Object.hasOwn(t.getBindingIdentifiers(node.id), "require") | ||
| ) { | ||
| const ancestors = readAncestors(); | ||
| if ( | ||
| !ancestors.some( | ||
| (ancestor) => t.isFunction(ancestor) || t.isStaticBlock(ancestor), | ||
| ) | ||
| ) { | ||
| wrapperVar = true; | ||
| if (node.init !== null) writeAt(node, ancestors); | ||
| } | ||
| } | ||
| if ( | ||
| !t.isCallExpression(node) && | ||
| !t.isOptionalCallExpression(node) && | ||
| !t.isNewExpression(node) | ||
| ) | ||
| return; | ||
| const identifier = requireCalleeIdentifier(node.callee); | ||
| const start = identifier?.loc?.start; | ||
| if (start === undefined) return; | ||
| const ancestors = readAncestors(); | ||
| if ( | ||
| ancestors.some( | ||
| (ancestor) => | ||
| t.isWithStatement(ancestor) && contains(ancestor.body, node), | ||
| ) | ||
| ) | ||
| return; | ||
| sites.push({ | ||
| node, | ||
| reference: references.get(locationKey(start)), | ||
| statement, | ||
| deferred: deferredExecution(node, ancestors), | ||
| }); | ||
| }, | ||
| }); | ||
| return { | ||
| sites, | ||
| wrapperVar, | ||
| firstWrite, | ||
| unknownWrite: unknownWrite || locatedWrites.size !== writeLocations.size, | ||
| }; | ||
| } | ||
|
|
||
| // Textual order does not establish execution order across a callable or an | ||
| // instance initializer. Same-statement writes also remain conservative; only | ||
| // a distinct later program statement proves that a write follows this call. | ||
| const deferredExecution = ( | ||
| node: t.Node, | ||
| ancestors: readonly t.Node[], | ||
| ): boolean => | ||
| ancestors.some( | ||
| (ancestor) => | ||
| t.isFunction(ancestor) || | ||
| ((t.isClassProperty(ancestor) || | ||
| t.isClassPrivateProperty(ancestor) || | ||
| t.isClassAccessorProperty(ancestor)) && | ||
| !ancestor.static && | ||
| ancestor.value != null && | ||
| contains(ancestor.value, node)), | ||
| ); | ||
|
|
||
| const contains = (container: t.Node, node: t.Node): boolean => | ||
| typeof container.start === "number" && | ||
| typeof container.end === "number" && | ||
| typeof node.start === "number" && | ||
| typeof node.end === "number" && | ||
| container.start <= node.start && | ||
| node.end <= container.end; | ||
|
|
||
| function* directCreateRequireBindingSteps( | ||
| program: t.Program, | ||
| references: ReadonlyMap<string, JavaScriptSemanticReference>, | ||
| ): Generator<void, boolean> { | ||
| const programScope = semanticScopeId("program", program); | ||
| const importedNames = new Set<string>(); | ||
| for (const declaration of program.body) { | ||
| yield; | ||
| if ( | ||
| !t.isImportDeclaration(declaration) || | ||
| (declaration.source.value !== "node:module" && | ||
| declaration.source.value !== "module") | ||
| ) | ||
| continue; | ||
| for (const specifier of declaration.specifiers) { | ||
| yield; | ||
| if ( | ||
| t.isImportSpecifier(specifier) && | ||
| t.isIdentifier(specifier.imported, { name: "createRequire" }) | ||
| ) | ||
| importedNames.add(specifier.local.name); | ||
| } | ||
| } | ||
| if (importedNames.size === 0) return false; | ||
| for (const statement of program.body) { | ||
| yield; | ||
| if (!t.isVariableDeclaration(statement, { kind: "const" })) continue; | ||
| for (const declaration of statement.declarations) { | ||
| yield; | ||
| if ( | ||
| !t.isIdentifier(declaration.id, { name: "require" }) || | ||
| !t.isCallExpression(declaration.init) || | ||
|
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 TypeScript source such as AGENTS.md reference: AGENTS.md:L23-L23 Useful? React with 👍 / 👎. |
||
| !t.isIdentifier(declaration.init.callee) || | ||
| !importedNames.has(declaration.init.callee.name) || | ||
| !isImportMetaUrl(declaration.init.arguments[0]) | ||
| ) | ||
| continue; | ||
| const start = declaration.init.callee.loc?.start; | ||
| if (start === undefined) continue; | ||
| const reference = references.get(locationKey(start)); | ||
| if ( | ||
| reference?.resolution === "resolved" && | ||
| reference.bindingId === | ||
| semanticBindingId(programScope, declaration.init.callee.name) | ||
| ) | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| const isImportMetaUrl = (node: t.Node | undefined): boolean => | ||
| t.isMemberExpression(node) && | ||
| !node.computed && | ||
| t.isIdentifier(node.property, { name: "url" }) && | ||
| t.isMetaProperty(node.object) && | ||
| t.isIdentifier(node.object.meta, { name: "import" }) && | ||
| t.isIdentifier(node.object.property, { name: "meta" }); | ||
|
|
||
| const requireCalleeIdentifier = ( | ||
| callee: t.Node | null | undefined, | ||
| ): t.Identifier | undefined => { | ||
| if (t.isIdentifier(callee, { name: "require" })) return callee; | ||
| if ( | ||
| !t.isMemberExpression(callee) || | ||
| !t.isIdentifier(callee.object, { name: "require" }) || | ||
| ((!t.isIdentifier(callee.property) || callee.computed) && | ||
| !t.isStringLiteral(callee.property)) | ||
| ) | ||
| return undefined; | ||
| const propertyName = t.isIdentifier(callee.property) | ||
| ? callee.property.name | ||
| : t.isStringLiteral(callee.property) | ||
| ? callee.property.value | ||
| : undefined; | ||
| return propertyName === "resolve" || propertyName === "main" | ||
| ? callee.object | ||
| : undefined; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import * as t from "@babel/types"; | ||
| import { expect, it } from "vitest"; | ||
|
|
||
| import { parseJavaScriptSource } from "./javascriptSourceParser.js"; | ||
| import { analyzeParsedJavaScriptStaticSourceSteps } from "./javascriptStaticAnalysis.js"; | ||
| import { traverseJavaScriptAst } from "./javascriptSemanticTraversal.js"; | ||
|
|
||
| it("bounds identifier work between static traversal checkpoints", () => { | ||
| const source = Array.from( | ||
| { length: 5000 }, | ||
| (_, i) => `require("./part-${String(i)}.cjs");`, | ||
| ).join("\n"); | ||
| const parsed = parseJavaScriptSource(source); | ||
| if (parsed === null) throw new Error("Fixture failed to parse"); | ||
| const reads = new Set<t.Identifier>(); | ||
| traverseJavaScriptAst(parsed, { | ||
| enter(node) { | ||
| if (!t.isIdentifier(node)) return; | ||
| const name = node.name; | ||
| Object.defineProperty(node, "name", { | ||
| get() { | ||
| reads.add(node); | ||
| return name; | ||
| }, | ||
| }); | ||
| }, | ||
| }); | ||
| const steps = analyzeParsedJavaScriptStaticSourceSteps(source, parsed); | ||
| let maximumReads = 0; | ||
| for (;;) { | ||
| reads.clear(); | ||
| const step = steps.next(); | ||
| if (step.done) { | ||
| expect(step.value.references).toHaveLength(5000); | ||
| break; | ||
| } | ||
| maximumReads = Math.max(maximumReads, reads.size); | ||
| } | ||
| // Allow the tail of one 1,024-node traversal and the next traversal's | ||
| // first chunk. Draining a whole 5,000-call classification violates this | ||
| // bound independently of CPU speed. Final result projection is separate. | ||
| expect(maximumReads).toBeLessThanOrEqual(2048); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,7 +52,11 @@ import { | |
| } from "./javascriptSourceParser.js"; | ||
|
|
||
| import { createJavaScriptRpcScan } from "./javascriptRpcEndpoints.js"; | ||
| import { semanticImportEqualsSpecifier } from "./javascriptSemanticProjection.js"; | ||
| import { | ||
| collectSemanticReferencesSteps, | ||
| semanticImportEqualsSpecifier, | ||
| } from "./javascriptSemanticProjection.js"; | ||
| import { unshadowedJavaScriptRequireCallsSteps } from "./javascriptRequireBindings.js"; | ||
| import { | ||
| hasUncertainTypeOnlyModuleSideEffect, | ||
| isTypeOnlyExportDeclaration, | ||
|
|
@@ -96,13 +100,19 @@ export function* analyzeParsedJavaScriptStaticSourceSteps( | |
| bindings, | ||
| ); | ||
| const clientRoots = yield* classifyParsedJavaScriptRpcClientRootsSteps(file); | ||
| const requireCalls = yield* unshadowedJavaScriptRequireCallsSteps( | ||
| file, | ||
| undefined, | ||
| yield* collectSemanticReferencesSteps(file.program, bindings), | ||
|
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 large generated source with no AGENTS.md reference: AGENTS.md:L46-L50 Useful? React with 👍 / 👎. |
||
| ); | ||
|
Comment on lines
+103
to
+107
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.
This is non-blocking. Keep the yielding steps, but skip this collection when no loader call needs it. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysis.ts
Line: 103-107
Comment:
**Files without loaders do extra work**
`analyzeParsedJavaScriptStaticSourceSteps` now collects and stores every lexical reference before checking whether the source uses `require`. Files with no `require` calls previously skipped this work. Large ESM files now pay for another whole-file walk and a map of unrelated references.
This is non-blocking. Keep the yielding steps, but skip this collection when no loader call needs it.
---
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! |
||
| yield* traverseStaticSourceSteps( | ||
| file, | ||
| { | ||
| source, | ||
| accumulator, | ||
| ...(openReceiverFacts === undefined ? {} : { openReceiverFacts }), | ||
| electronBindings: electronIdentity.bindings, | ||
| isUnshadowedRequireCall: (node) => requireCalls.has(node), | ||
| rpcScan: createJavaScriptRpcScan(source, clientRoots), | ||
| }, | ||
| bindings, | ||
|
|
||
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.
When a computed object/class method key contains
require?.("./dep")and a later top-level statement reassignsrequire, the key runs immediately during object/class creation, before that assignment.ObjectMethodandClassMethodsatisfy this blankett.isFunction(ancestor)predicate, so the call is marked deferred,loaderIntactbecomes false, and optional calls have no semantic module-link fallback, dropping a dependency that actually loaded. The new blanket predicate is fresh evidence beyond the earlier top-level reassignment case; restrict deferred classification to method parameter/body regions, matching the semantic index's outer-scope handling for computed keys.AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.