diff --git a/packages/compiler/src/frontend/npm-static-bundled-chunks.test.ts b/packages/compiler/src/frontend/npm-static-bundled-chunks.test.ts index fe54681d7..f9463ea1e 100644 --- a/packages/compiler/src/frontend/npm-static-bundled-chunks.test.ts +++ b/packages/compiler/src/frontend/npm-static-bundled-chunks.test.ts @@ -150,11 +150,43 @@ console.log(e.outer, e.indirect, e.wrapped, e.nested, e.seen.join(",")); expect(chunk).toContain("function wrap(n)"); }); +test("a named function expression that only calls itself travels with a chunk", () => { + const answer = expectSameBehavior(`${keyedHelper}var fact = function self(n) { + return n <= 1 ? 1 : n * self(n - 1); +}; +var kind = function self(self) { + return typeof self; +}; +var outer = function self() { + var self = 2; + return self; +}; +var require_g = __commonJS({ + "g.js"(exports2, module2) { + module2.exports = [fact(5), kind(1), outer()]; + } +}); +console.log(require_g().join(",")); +`); + const chunk = answer.chunks[0]!.source; + expect(chunk).toContain("var fact = function self(n)"); +}); + test.each([ ["exports it", `function tally() {}`, `module2.exports = { tally };`], ["reads its properties", `function tally() {}`, `exports2.calls = tally.calls;`], ["constructs it", `function tally() {}`, `exports2.made = new tally();`], ["stores an arrow helper", `var tally = () => 1;`, `exports2.fn = tally;`], + [ + "calls a function that reads its own name as a value", + `var tally = function self() { self.n = (self.n || 0) + 1; return self.n; };`, + `exports2.count = tally();`, + ], + [ + "calls a function whose parameter default reads its own name", + `var tally = function self(box = self) { box.n = (box.n || 0) + 1; return box.n; };`, + `exports2.count = tally();`, + ], ])("a chunk that %s keeps the package refused", (_, declaration, statement) => { const result = split(`${keyedHelper}${declaration} var require_f = __commonJS({ diff --git a/packages/compiler/src/frontend/npm-static-bundled-chunks.ts b/packages/compiler/src/frontend/npm-static-bundled-chunks.ts index abc86bcbd..276079494 100644 --- a/packages/compiler/src/frontend/npm-static-bundled-chunks.ts +++ b/packages/compiler/src/frontend/npm-static-bundled-chunks.ts @@ -209,6 +209,15 @@ class BundleScopes { return false; } + /** True when a scope inside `root` (exclusive) binds the reference's + * name. */ + boundInside(id: ts.Identifier, root: ts.Node): boolean { + for (let n = id.parent; n !== undefined && n !== root; n = n.parent) { + if (this.of(n).has(id.text)) return true; + } + return false; + } + /** True when the reference resolves to a top-level binding of `sf`. */ topLevel(id: ts.Identifier, sf: ts.SourceFile): boolean { for (let n = id.parent; n !== undefined && n !== sf; n = n.parent) { @@ -350,6 +359,25 @@ function isFunctionValued(node: ts.VariableDeclaration | ts.FunctionDeclaration) return init !== undefined && (ts.isArrowFunction(init) || ts.isFunctionExpression(init)); } +/** True when a named function expression initializer (`var f = function + * self() {}`) uses its own name only as a callee: `self` is the copied + * function, so it is as observable as the top-level binding. */ +function selfCalledOnly( + node: ts.VariableDeclaration | ts.FunctionDeclaration, + scopes: BundleScopes, +): boolean { + if (!ts.isVariableDeclaration(node) || node.initializer === undefined) return true; + const fn = unparen(node.initializer); + if (!ts.isFunctionExpression(fn) || fn.name === undefined || fn.body === undefined) return true; + const self = fn.name.text; + // A parameter or `var` of the same name hides the function's own name. + const own = varScopedNames(fn.body); + for (const param of fn.parameters.nodes) addBindingNames(param.name, own); + if (own.has(self)) return true; + // Its parameter initializers see the name too. + return references(fn).every((r) => r.text !== self || scopes.boundInside(r, fn) || isCallee(r)); +} + /** True when a reference is the function a call invokes: `f(...)`, or the * `(0, f)(...)` form that drops the receiver. */ function isCallee(id: ts.Identifier): boolean { @@ -748,7 +776,7 @@ function chunkSource( return why(`reads the bundle's top-level binding '${name}', which it cannot carry along`); // Each chunk gets its own copy, so a function's identity and its // properties must stay unobservable: the bundle only ever calls it. - if (isFunctionValued(node) && !calledOnly(name)) + if (isFunctionValued(node) && (!calledOnly(name) || !selfCalledOnly(node, scopes))) return why( `uses the bundle's top-level function '${name}' as a value, which its chunks cannot share`, ); diff --git a/packages/compiler/src/frontend/program.ts b/packages/compiler/src/frontend/program.ts index 12d9b703a..79978abee 100644 --- a/packages/compiler/src/frontend/program.ts +++ b/packages/compiler/src/frontend/program.ts @@ -3199,11 +3199,66 @@ function cjsCycleRefusal7( return null; } -/** True for a plain read `binding.name` (not a call, not a write) of the - * cycle member `dep` whose export `name` is a top-level function - * declaration nothing reassigns. Every member settles its exports before - * entering the cycle, so Node reads that same function whenever the read - * runs, and the compiled read resolves to the declaration itself. */ +/** True when `use` initializes a top-level const whose every other + * reference is a value of an export statement (`module.exports = { … }` + * or `exports.n = …`). */ +function publishedAlias7(use: ts.Node, decl: ts.Node | undefined): boolean { + if ( + decl === undefined || + !ts.isVariableDeclaration(decl) || + decl.initializer !== use || + !ts.isIdentifier(decl.name) + ) + return false; + const list = decl.parent; + const stmt = list?.parent; + const sf = stmt?.parent; + if ( + list === undefined || + (list.flags & ts.NodeFlags.Const) === 0 || + stmt === undefined || + !ts.isVariableStatement(stmt) || + sf === undefined || + !ts.isSourceFile(sf) + ) + return false; + const alias = decl.name; + const published = new Set(); + for (const st of sf.statements.nodes) { + const cjs = cjsExportAssignmentOf(st); + if (cjs === null) continue; + if (cjs.kind === "member") published.add(cjs.value); + else if (cjs.obj !== null) { + for (const p of cjs.obj.properties.nodes) { + if (ts.isPropertyAssignment(p) && !ts.isComputedPropertyName(p.name)) + published.add(p.initializer); + else if (ts.isShorthandPropertyAssignment(p)) published.add(p.name); + } + } + } + let kept = false; + ts.walkPreorder(sf, (n) => { + if (kept) return "stop"; + if (!ts.isIdentifier(n) || n === alias || n.text !== alias.text) return undefined; + const p = n.parent; + if ( + p !== undefined && + ((ts.isPropertyAccessExpression(p) && p.name === n) || + (ts.isPropertyAssignment(p) && p.name === n)) + ) + return undefined; + if (!published.has(n)) kept = true; + return undefined; + }); + return !kept; +} + +/** True for a plain read `binding.name` (not a call, not a write, not a + * value that is stored or passed on) of the cycle member `dep` whose + * export `name` is a top-level function declaration nothing reassigns. + * Every member settles its exports before entering the cycle, so Node + * reads that same function whenever the read runs, and the compiled read + * resolves to the declaration itself. */ function functionExportRead7(id: ts.Identifier, dep: ts.SourceFile): boolean { const access = id.parent; if (!ts.isPropertyAccessExpression(access) || access.expression !== id) return false; @@ -3222,25 +3277,27 @@ function functionExportRead7(id: ts.Identifier, dep: ts.SourceFile): boolean { else break; } const parent = use.parent; - // A use that can run the function (a call, `new`, a tag, a member of it - // such as `.call`, or an argument another call receives) is not a read. - if ( - (ts.isCallExpression(parent) && parent.expression === use) || - (ts.isCallExpression(parent) && parent.arguments.nodes.includes(use as ts.Expression)) || - ts.isNewExpression(parent) || - (ts.isTaggedTemplateExpression(parent) && parent.tag === use) || - (ts.isPropertyAccessExpression(parent) && parent.expression === use) || - (ts.isElementAccessExpression(parent) && parent.expression === use) || - ts.isDeleteExpression(parent) || - ((ts.isPrefixUnaryExpression(parent) || ts.isPostfixUnaryExpression(parent)) && - (parent.operator === ts.SyntaxKind.PlusPlusToken || - parent.operator === ts.SyntaxKind.MinusMinusToken)) || + // Only a use that neither runs the function nor keeps it is a read: a + // stored alias (`const f = binding.name`, `{ f: binding.name }`) or an + // argument can be called while the cycle initializes. A top-level const + // that only publishes it (`module.exports = { name: f }`, the static + // rewrite's re-export spelling) runs nothing: a module that reads the + // export back checks that read itself. + const observes = + publishedAlias7(use, parent) || + ts.isExpressionStatement(parent) || + ts.isTypeOfExpression(parent) || + ts.isVoidExpression(parent) || + (ts.isPrefixUnaryExpression(parent) && parent.operator === ts.SyntaxKind.ExclamationToken) || (ts.isBinaryExpression(parent) && - parent.left === use && - parent.operatorToken.kind >= ts.SyntaxKind.FirstAssignment && - parent.operatorToken.kind <= ts.SyntaxKind.LastAssignment) - ) - return false; + (parent.operatorToken.kind === ts.SyntaxKind.EqualsEqualsEqualsToken || + parent.operatorToken.kind === ts.SyntaxKind.ExclamationEqualsEqualsToken || + parent.operatorToken.kind === ts.SyntaxKind.EqualsEqualsToken || + parent.operatorToken.kind === ts.SyntaxKind.ExclamationEqualsToken)) || + ((ts.isIfStatement(parent) || ts.isWhileStatement(parent) || ts.isDoStatement(parent)) && + parent.expression === use) || + (ts.isConditionalExpression(parent) && parent.condition === use); + if (!observes) return false; const name = access.name.text; // The exported value's identifier: a table entry or a member export. const values: ts.Expression[] = []; diff --git a/packages/compiler/test/ts7/baselines/order-parity/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts.json b/packages/compiler/test/ts7/baselines/order-parity/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts.json new file mode 100644 index 000000000..c3e0442f9 --- /dev/null +++ b/packages/compiler/test/ts7/baselines/order-parity/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts.json @@ -0,0 +1,6 @@ +{ + "order": [ + "/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts" + ], + "diags": [] +} diff --git a/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts b/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts new file mode 100644 index 000000000..ae2be341a --- /dev/null +++ b/tests/fixtures/npm/cases/2571-cjs-cycle-stored-function/main.ts @@ -0,0 +1,8 @@ +// Require cycles whose partner stores an export of index.js (in a const, +// or in an object literal) and calls it while index is still +// initializing: the export reads a const in its temporal dead zone, so +// Node throws a ReferenceError, and scriptc refuses both cycles. +import { show } from "gtcyclealias"; +import { show as showBox } from "gtcyclebox"; + +console.log(show(), showBox()); diff --git a/tests/fixtures/npm/node_modules/gtcyclealias/dist/index.d.ts b/tests/fixtures/npm/node_modules/gtcyclealias/dist/index.d.ts new file mode 100644 index 000000000..5f4373e2f --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclealias/dist/index.d.ts @@ -0,0 +1,3 @@ +export declare const LIMIT = 3; +export declare function show(): string; +export declare function peek(): number; diff --git a/tests/fixtures/npm/node_modules/gtcyclealias/dist/index.js b/tests/fixtures/npm/node_modules/gtcyclealias/dist/index.js new file mode 100644 index 000000000..77aca3c9b --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclealias/dist/index.js @@ -0,0 +1,37 @@ +var __defProp = Object.defineProperty; +var __getOwnPropDesc = Object.getOwnPropertyDescriptor; +var __getOwnPropNames = Object.getOwnPropertyNames; +var __hasOwnProp = Object.prototype.hasOwnProperty; +var __export = (target, all) => { + for (var name in all) + __defProp(target, name, { get: all[name], enumerable: true }); +}; +var __copyProps = (to, from, except, desc) => { + if (from && typeof from === "object" || typeof from === "function") { + for (let key of __getOwnPropNames(from)) + if (!__hasOwnProp.call(to, key) && key !== except) + __defProp(to, key, { get: () => from[key], enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable }); + } + return to; +}; +var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod); +var index_exports = {}; +__export(index_exports, { + LIMIT: () => LIMIT, + peek: () => peek, + show: () => show +}); +module.exports = __toCommonJS(index_exports); +var import_reader = require("./reader"); +const LIMIT = 3; +function peek() { + return LIMIT; +} +function show() { + return (0, import_reader.describe)(); +} +// Annotate the CommonJS export names for ESM import in node: +0 && (module.exports = { + LIMIT, + show +}); diff --git a/tests/fixtures/npm/node_modules/gtcyclealias/dist/reader.js b/tests/fixtures/npm/node_modules/gtcyclealias/dist/reader.js new file mode 100644 index 000000000..943e61a0a --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclealias/dist/reader.js @@ -0,0 +1,34 @@ +var __defProp = Object.defineProperty; +var __getOwnPropDesc = Object.getOwnPropertyDescriptor; +var __getOwnPropNames = Object.getOwnPropertyNames; +var __hasOwnProp = Object.prototype.hasOwnProperty; +var __export = (target, all) => { + for (var name in all) + __defProp(target, name, { get: all[name], enumerable: true }); +}; +var __copyProps = (to, from, except, desc) => { + if (from && typeof from === "object" || typeof from === "function") { + for (let key of __getOwnPropNames(from)) + if (!__hasOwnProp.call(to, key) && key !== except) + __defProp(to, key, { get: () => from[key], enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable }); + } + return to; +}; +var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod); +var reader_exports = {}; +__export(reader_exports, { + captured: () => captured, + describe: () => describe +}); +module.exports = __toCommonJS(reader_exports); +var import_index = require("./index"); +const read = import_index.peek; +const captured = read(); +function describe() { + return "limit " + captured; +} +// Annotate the CommonJS export names for ESM import in node: +0 && (module.exports = { + captured, + describe +}); diff --git a/tests/fixtures/npm/node_modules/gtcyclealias/package.json b/tests/fixtures/npm/node_modules/gtcyclealias/package.json new file mode 100644 index 000000000..9ee7441e0 --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclealias/package.json @@ -0,0 +1,7 @@ +{ + "name": "gtcyclealias", + "version": "1.0.0", + "description": "esbuild 0.25.10 per-file CommonJS output shape of a cycle whose partner stores an exported function in a const and calls it while that function's const binding is uninitialized", + "main": "./dist/index.js", + "types": "./dist/index.d.ts" +} diff --git a/tests/fixtures/npm/node_modules/gtcyclebox/dist/index.d.ts b/tests/fixtures/npm/node_modules/gtcyclebox/dist/index.d.ts new file mode 100644 index 000000000..5f4373e2f --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclebox/dist/index.d.ts @@ -0,0 +1,3 @@ +export declare const LIMIT = 3; +export declare function show(): string; +export declare function peek(): number; diff --git a/tests/fixtures/npm/node_modules/gtcyclebox/dist/index.js b/tests/fixtures/npm/node_modules/gtcyclebox/dist/index.js new file mode 100644 index 000000000..77aca3c9b --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclebox/dist/index.js @@ -0,0 +1,37 @@ +var __defProp = Object.defineProperty; +var __getOwnPropDesc = Object.getOwnPropertyDescriptor; +var __getOwnPropNames = Object.getOwnPropertyNames; +var __hasOwnProp = Object.prototype.hasOwnProperty; +var __export = (target, all) => { + for (var name in all) + __defProp(target, name, { get: all[name], enumerable: true }); +}; +var __copyProps = (to, from, except, desc) => { + if (from && typeof from === "object" || typeof from === "function") { + for (let key of __getOwnPropNames(from)) + if (!__hasOwnProp.call(to, key) && key !== except) + __defProp(to, key, { get: () => from[key], enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable }); + } + return to; +}; +var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod); +var index_exports = {}; +__export(index_exports, { + LIMIT: () => LIMIT, + peek: () => peek, + show: () => show +}); +module.exports = __toCommonJS(index_exports); +var import_reader = require("./reader"); +const LIMIT = 3; +function peek() { + return LIMIT; +} +function show() { + return (0, import_reader.describe)(); +} +// Annotate the CommonJS export names for ESM import in node: +0 && (module.exports = { + LIMIT, + show +}); diff --git a/tests/fixtures/npm/node_modules/gtcyclebox/dist/reader.js b/tests/fixtures/npm/node_modules/gtcyclebox/dist/reader.js new file mode 100644 index 000000000..b8f8a927e --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclebox/dist/reader.js @@ -0,0 +1,34 @@ +var __defProp = Object.defineProperty; +var __getOwnPropDesc = Object.getOwnPropertyDescriptor; +var __getOwnPropNames = Object.getOwnPropertyNames; +var __hasOwnProp = Object.prototype.hasOwnProperty; +var __export = (target, all) => { + for (var name in all) + __defProp(target, name, { get: all[name], enumerable: true }); +}; +var __copyProps = (to, from, except, desc) => { + if (from && typeof from === "object" || typeof from === "function") { + for (let key of __getOwnPropNames(from)) + if (!__hasOwnProp.call(to, key) && key !== except) + __defProp(to, key, { get: () => from[key], enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable }); + } + return to; +}; +var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod); +var reader_exports = {}; +__export(reader_exports, { + captured: () => captured, + describe: () => describe +}); +module.exports = __toCommonJS(reader_exports); +var import_index = require("./index"); +const box = { read: import_index.peek }; +const captured = box.read(); +function describe() { + return "limit " + captured; +} +// Annotate the CommonJS export names for ESM import in node: +0 && (module.exports = { + captured, + describe +}); diff --git a/tests/fixtures/npm/node_modules/gtcyclebox/package.json b/tests/fixtures/npm/node_modules/gtcyclebox/package.json new file mode 100644 index 000000000..67f943195 --- /dev/null +++ b/tests/fixtures/npm/node_modules/gtcyclebox/package.json @@ -0,0 +1,7 @@ +{ + "name": "gtcyclebox", + "version": "1.0.0", + "description": "esbuild 0.25.10 per-file CommonJS output shape of a cycle whose partner stores an exported function in an object literal and calls it while that function's const binding is uninitialized", + "main": "./dist/index.js", + "types": "./dist/index.d.ts" +} diff --git a/tests/harness/npm-cases.ts b/tests/harness/npm-cases.ts index 451441009..0830d4162 100644 --- a/tests/harness/npm-cases.ts +++ b/tests/harness/npm-cases.ts @@ -19,12 +19,12 @@ export function npmCases(fixturesRoot: string): NpmCase[] { return [ ...globSync(join(fixturesRoot, "npm/cases/*/main.ts")) .sort() - // 2465-2469 and 2556-2566 are the --npm-static bundler-emitted-CJS + // 2465-2469, 2556-2566 and 2571 are the --npm-static bundler-emitted-CJS // cases (npm-static.test.ts drives them with the opt-in): their // consumers exercise the static rewrite's surface (lexer-visible // names the shipped .d.ts never declares, the __toESM interop // family), so they stay out of the flagless island lane by design. - .filter((entry) => !/\/(246[5-9]|255[6-9]|256[0-24-6])-[^/]+\/main\.ts$/.test(entry)) + .filter((entry) => !/\/(246[5-9]|255[6-9]|256[0-24-6]|2571)-[^/]+\/main\.ts$/.test(entry)) .map((entry) => ({ name: entry.split("/").at(-2)!, entry, diff --git a/tests/harness/npm-static.test.ts b/tests/harness/npm-static.test.ts index 4930487e2..8325e2998 100644 --- a/tests/harness/npm-static.test.ts +++ b/tests/harness/npm-static.test.ts @@ -76,7 +76,7 @@ async function buildStatic(entry: string, npmStatic: string[] | "auto"): Promise ...globSync( join(fixturesRoot, "npm/node_modules/cryptozoo/**/*.{js,mjs,cjs,json,d.ts}"), ).sort(), - // the bundler-emitted-CJS mini packages (cases 2465-2469, 2556-2566) + // the bundler-emitted-CJS mini packages (cases 2465-2469, 2556-2566, 2571) ...globSync(join(fixturesRoot, "npm/node_modules/gt*/**/*.{js,cjs,json}")).sort(), ]; for (const f of inputs) hash.update(f).update(readFileSync(f)); @@ -1482,6 +1482,21 @@ describe(`npm-static pilots${sanitize ? " (sanitized)" : ""}`, () => { ]); }, 120_000); + // 2571: the partner stores the export (in a const, or in an object + // literal) and calls it while the cycle initializes; both keep SC1016. + test("a CommonJS cycle that stores an export and calls it stays refused", () => { + const entry = join(fixturesRoot, "npm/cases/2571-cjs-cycle-stored-function/main.ts"); + const { coverage } = analyze(entry, { npmStatic: ["gtcyclealias", "gtcyclebox"] }); + for (const pkg of ["gtcyclealias", "gtcyclebox"]) + expect(coverage.npmStatic).toContainEqual({ + package: pkg, + status: "refused", + detail: expect.stringMatching( + /circular imports .*can use its binding 'import_index' of a module of the cycle/, + ) as string, + }); + }, 120_000); + // 2564: a chunk reads a top-level function whose name a nested scope of // the chunk also declares; the function still travels with the chunk. test("a chunk carries a top-level function its nested scopes shadow", async () => {