From bcbae8a497d2d68f7b18b8930e89392e0510f7ec Mon Sep 17 00:00:00 2001 From: Chris Tate Date: Sun, 25 Oct 2026 16:36:06 -0500 Subject: [PATCH] Address review feedback --- .../shape-inference/static-objects.ts | 206 ++++++++++++++---- .../test/static-object-guards.test.ts | 148 +++++++++++++ 2 files changed, 309 insertions(+), 45 deletions(-) create mode 100644 packages/compiler/test/static-object-guards.test.ts diff --git a/packages/compiler/src/frontend/shape-inference/static-objects.ts b/packages/compiler/src/frontend/shape-inference/static-objects.ts index ecaf664aa..640140e61 100644 --- a/packages/compiler/src/frontend/shape-inference/static-objects.ts +++ b/packages/compiler/src/frontend/shape-inference/static-objects.ts @@ -238,38 +238,142 @@ function ownLiteralMember(literal: Node, key: string): string | null { return found; } -/** Whether a top-level statement runs no code but its own: no call, `new`, - * tagged template, class (static blocks and field initializers run), getter - * or setter, `await`, `yield` or `delete`. Function bodies do not run. */ -function runsNoCode(stmt: ts.Statement): boolean { +/** Whether a top-level statement runs no code but its own. Its expressions + * are literals, identifiers, functions (not run), object and array literals + * without spreads or computed keys, `=` stores, `var` bindings without + * patterns, `&&`, `||`, `??`, `?:`, `,`, `!`, `typeof`, `void`, `===` and + * `!==`, none of which calls a function, and member reads and stores that + * `plainMember` accepts (no getter or setter runs). Every other form can + * run code: a call, `new` or tagged template, a class (static blocks and + * field initializers run), coercion (arithmetic, `+`, templates with + * substitutions, relational and loose comparisons, `in`, `instanceof`, + * computed keys), iteration (spreads, destructuring), `delete`, `await` and + * `yield`. */ +function runsNoCode(stmt: ts.Statement, plainMember: (n: Node) => boolean): boolean { if (ts.isFunctionDeclaration(stmt) || ts.isEmptyStatement(stmt)) return true; - if (!ts.isVariableStatement(stmt) && !ts.isExpressionStatement(stmt)) return false; - let quiet = true; - const visit = (n: Node): void => { - if (!quiet) return; - if (ts.isFunctionExpression(n) || ts.isArrowFunction(n)) return; - if ( - ts.isCallExpression(n) || - ts.isNewExpression(n) || - ts.isTaggedTemplateExpression(n) || - ts.isClassExpression(n) || - ts.isGetAccessorDeclaration(n) || - ts.isSetAccessorDeclaration(n) || - ts.isAwaitExpression(n) || - ts.isYieldExpression(n) || - ts.isDeleteExpression(n) || - ts.isMethodDeclaration(n) - ) { - quiet = false; - return; - } - ts.forEachChild(n, (c) => { - visit(c); - return undefined; + if (ts.isExpressionStatement(stmt)) return quietExpression(stmt.expression, plainMember); + if (!ts.isVariableStatement(stmt)) return false; + return stmt.declarationList.declarations.nodes.every( + (d) => + ts.isIdentifier(d.name) && + (d.initializer === undefined || quietExpression(d.initializer, plainMember)), + ); +} + +function quietExpression(e: ts.Expression, plainMember: (n: Node) => boolean): boolean { + const quiet = (x: ts.Expression): boolean => quietExpression(x, plainMember); + if (ts.isParenthesizedExpression(e)) return quiet(e.expression); + if ( + ts.isIdentifier(e) || + ts.isNumericLiteral(e) || + ts.isBigIntLiteral(e) || + ts.isStringLiteral(e) || + ts.isNoSubstitutionTemplateLiteral(e) || + ts.isRegularExpressionLiteral(e) || + ts.isFunctionExpression(e) || + ts.isArrowFunction(e) || + e.kind === ts.SyntaxKind.TrueKeyword || + e.kind === ts.SyntaxKind.FalseKeyword || + e.kind === ts.SyntaxKind.NullKeyword || + e.kind === ts.SyntaxKind.ThisKeyword + ) + return true; + if (ts.isObjectLiteralExpression(e)) + return e.properties.nodes.every((m) => { + if (ts.isShorthandPropertyAssignment(m)) return m.objectAssignmentInitializer === undefined; + if (!ts.isPropertyAssignment(m)) return false; + const name = m.name; + if (!ts.isIdentifier(name) && !ts.isStringLiteral(name) && !ts.isNumericLiteral(name)) + return false; + return quiet(m.initializer); }); - }; - visit(stmt); - return quiet; + if (ts.isArrayLiteralExpression(e)) + return e.elements.nodes.every( + (x) => ts.isOmittedExpression(x) || (!ts.isSpreadElement(x) && quiet(x)), + ); + if (ts.isTypeOfExpression(e) || ts.isVoidExpression(e)) return quiet(e.expression); + if (ts.isPrefixUnaryExpression(e)) + return e.operator === ts.SyntaxKind.ExclamationToken && quiet(e.operand); + if (ts.isConditionalExpression(e)) + return quiet(e.condition) && quiet(e.whenTrue) && quiet(e.whenFalse); + if (ts.isPropertyAccessExpression(e) || ts.isElementAccessExpression(e)) + return quietMember(e, plainMember); + if (!ts.isBinaryExpression(e)) return false; + switch (e.operatorToken.kind) { + case ts.SyntaxKind.EqualsToken: + if (ts.isIdentifier(e.left)) return quiet(e.right); + return ( + (ts.isPropertyAccessExpression(e.left) || ts.isElementAccessExpression(e.left)) && + quietMember(e.left, plainMember) && + quiet(e.right) + ); + case ts.SyntaxKind.AmpersandAmpersandToken: + case ts.SyntaxKind.BarBarToken: + case ts.SyntaxKind.QuestionQuestionToken: + case ts.SyntaxKind.CommaToken: + case ts.SyntaxKind.EqualsEqualsEqualsToken: + case ts.SyntaxKind.ExclamationEqualsEqualsToken: + return quiet(e.left) && quiet(e.right); + default: + return false; + } +} + +/** A member read or store target with a literal key on a receiver that + * runs no code, which `plainMember` accepts. */ +function quietMember( + e: ts.PropertyAccessExpression | ts.ElementAccessExpression, + plainMember: (n: Node) => boolean, +): boolean { + if (e.questionDotToken !== undefined) return false; + if ( + ts.isElementAccessExpression(e) && + !ts.isStringLiteral(e.argumentExpression) && + !ts.isNumericLiteral(e.argumentExpression) + ) + return false; + return plainMember(e) && quietExpression(e.expression, plainMember); +} + +/** Whether an identifier is written where it appears: an assignment or + * update target, a destructuring assignment or loop target (through array + * and object patterns, defaults and rests), or a `var`, `let`, `const` or + * parameter binding of the name (a nested binding shadows it, which only + * makes the answer conservative). */ +function writesBinding(id: ts.Identifier): boolean { + let n: Node = id; + for (let p = n.parent; p !== undefined; n = p, p = p.parent) { + if (ts.isPrefixUnaryExpression(p) || ts.isPostfixUnaryExpression(p)) + return ( + p.operator === ts.SyntaxKind.PlusPlusToken || p.operator === ts.SyntaxKind.MinusMinusToken + ); + if (ts.isBinaryExpression(p)) { + const op = p.operatorToken.kind; + if (op >= ts.SyntaxKind.FirstAssignment && op <= ts.SyntaxKind.LastAssignment) + return p.left === n; + return false; + } + if (ts.isForOfStatement(p) || ts.isForInStatement(p)) return p.initializer === n; + if (ts.isVariableDeclaration(p) || ts.isParameter(p) || ts.isBindingElement(p)) + return p.name === n; + if ( + ts.isParenthesizedExpression(p) || + ts.isArrayLiteralExpression(p) || + ts.isObjectLiteralExpression(p) || + ts.isSpreadElement(p) || + ts.isSpreadAssignment(p) || + ts.isShorthandPropertyAssignment(p) || + ts.isArrayBindingPattern(p) || + ts.isObjectBindingPattern(p) + ) + continue; + if (ts.isPropertyAssignment(p)) { + if (p.initializer !== n) return false; + continue; + } + return false; + } + return false; } function enclosingFunctionLike(n: Node): Node | null { @@ -1345,6 +1449,24 @@ export function planStaticObjectRewrites( if (!l.includes(op.node)) l.push(op.node); } } + // Member reads and stores whose receiver can only be constant objects or + // primitives: neither has accessors of its own, so no getter or setter + // runs (a primitive receiver of a store throws, or reads its prototype). + const memberReceivers = new Map>(); + for (const op of analysis.ops) { + if ((op.kind !== "read" && op.kind !== "write") || op.name === null) continue; + let set = memberReceivers.get(op.node); + if (set === undefined) memberReceivers.set(op.node, (set = new Set())); + for (const a of analysis.nodes[op.recv]!.atoms) set.add(a); + } + const plainMember = (n: Node): boolean => { + const atoms = memberReceivers.get(n); + return ( + atoms !== undefined && + atoms.size > 0 && + [...atoms].every((a) => a.kind === "prim" || constantAtoms.has(a)) + ); + }; const quietPrefix = new Map(); const storedLiteral = ( atom: Atom, @@ -1371,7 +1493,9 @@ export function planStaticObjectRewrites( const statements = stmt.parent.statements.nodes; let quiet = quietPrefix.get(stmt); if (quiet === undefined) { - quiet = statements.slice(0, statements.indexOf(stmt)).every(runsNoCode); + quiet = statements + .slice(0, statements.indexOf(stmt)) + .every((s) => runsNoCode(s, plainMember)); quietPrefix.set(stmt, quiet); } return quiet ? { literal, stmt, statements } : null; @@ -1472,29 +1596,21 @@ export function planStaticObjectRewrites( if (a.kind === "function" && a.node === fn && analysis.escaped.has(a)) return false; const index = (fn as ts.FunctionLikeDeclaration).parameters.nodes.indexOf(keyDecl); const name = keyDecl.name.text; + // Any write of the name in the function, its other parameters included, + // or a direct eval or `arguments` (which aliases parameters in sloppy + // code) can give the key another value. let touched = false; const visit = (n: Node): void => { if (touched) return; - if (ts.isIdentifier(n) && n.text === "arguments") touched = true; - else if ( - ts.isBinaryExpression(n) && - n.operatorToken.kind >= ts.SyntaxKind.FirstAssignment && - n.operatorToken.kind <= ts.SyntaxKind.LastAssignment && - ts.isIdentifier(n.left) && - n.left.text === name - ) - touched = true; - else if ( - (ts.isPrefixUnaryExpression(n) || ts.isPostfixUnaryExpression(n)) && - ts.isIdentifier(n.operand) && - n.operand.text === name - ) + if (ts.isIdentifier(n) && (n.text === "arguments" || n.text === "eval")) touched = true; + else if (ts.isIdentifier(n) && n.text === name && n !== keyDecl.name && writesBinding(n)) touched = true; ts.forEachChild(n, (c) => { visit(c); return undefined; }); }; + for (const p of (fn as ts.FunctionLikeDeclaration).parameters.nodes) visit(p); const body = (fn as { body?: Node }).body; if (body === undefined) return false; visit(body); diff --git a/packages/compiler/test/static-object-guards.test.ts b/packages/compiler/test/static-object-guards.test.ts new file mode 100644 index 000000000..2aba8dfd6 --- /dev/null +++ b/packages/compiler/test/static-object-guards.test.ts @@ -0,0 +1,148 @@ +/* Static-object specialization (shape-inference/static-objects.ts) keeps two + * reads dynamic when the facts it would fold on can change: + * - a member stored after a statement that can run code (a coercion such as + * `+` can call toString or valueOf) keeps its read in functions, which + * that code could call before the store (the statements checked are all + * the file's top-level statements before the store); + * - a table call whose key parameter the function rebinds (destructuring, + * a loop target, another parameter's default or a `var` redeclaration) + * keeps reading the table for other keys, though every caller passes one + * of the table's keys. + * The checks read the rewritten package text SCRIPTC_DEBUG_SHAPE_OVERLAY + * prints. */ +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, expect, test, vi } from "vitest"; +import { compile } from "../src/index.js"; + +const packageSource = `var quiet = quiet || {}; +quiet.cfg = { U: 1 }; +quiet.read = function () { + return quiet.cfg.V; +}; +quiet.label = quiet.cfg.U; +quiet.cfg.V = 6; + +var late = late || {}; +late.cfg = { U: 1 }; +late.read = function () { + return late.cfg.V; +}; +late.label = "v" + late.cfg.U; +late.cfg.V = 5; + +var t = t || {}; +t.ops = { + a: function (v) { + return v + 1; + }, + b: function (v) { + return v * 2; + }, +}; +t.plain = function (op, v) { + return t.ops[op](v); +}; +t.pattern = function (op, v) { + [op] = ["c"]; + return t.ops[op](v); +}; +t.loop = function (op, v) { + for (op of ["c"]); + return t.ops[op](v); +}; +t.initializer = function (op, v, w = (op = "c")) { + return t.ops[op](v); +}; +t.redeclared = function (op, v) { + var op = "c"; + return t.ops[op](v); +}; +module.exports = { late, quiet, t }; +`; + +const consumer = `const { late, quiet, t } = require("guard-fixture"); +console.log(late.read(), late.label, quiet.read(), quiet.label); +const tryCall = (call) => { + try { + return call(); + } catch (e) { + return e.name; + } +}; +console.log(t.plain("a", 1), t.plain("b", 2)); +console.log(tryCall(() => t.pattern("a", 1)), tryCall(() => t.loop("b", 2))); +console.log(tryCall(() => t.initializer("a", 1)), tryCall(() => t.redeclared("b", 2))); +`; + +let dir = ""; +const saved: Record = {}; +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "scriptc-static-guards-")); + const pkg = join(dir, "node_modules/guard-fixture"); + await mkdir(pkg, { recursive: true }); + await writeFile( + join(pkg, "package.json"), + JSON.stringify({ name: "guard-fixture", version: "1.0.0", main: "index.js" }), + ); + await writeFile(join(pkg, "index.js"), packageSource); + await writeFile( + join(dir, "package.json"), + JSON.stringify({ dependencies: { "guard-fixture": "1.0.0" } }), + ); + await writeFile(join(dir, "main.cjs"), consumer); + for (const k of ["SCRIPTC_SHAPE_OVERLAY", "SCRIPTC_DEBUG_SHAPE_OVERLAY", "SCRIPTC_NO_CACHE"]) + saved[k] = process.env[k]; +}); +afterEach(async () => { + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + vi.restoreAllMocks(); + await rm(dir, { recursive: true, force: true }); +}); + +/** The package text after the structural rewrites. */ +async function rewrittenPackage(): Promise { + process.env["SCRIPTC_SHAPE_OVERLAY"] = "1"; + process.env["SCRIPTC_DEBUG_SHAPE_OVERLAY"] = "1"; + process.env["SCRIPTC_NO_CACHE"] = "1"; + let text = ""; + vi.spyOn(process.stderr, "write").mockImplementation((chunk: string | Uint8Array) => { + text += typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8"); + return true; + }); + const result = await compile(join(dir, "main.cjs"), { + outDir: dir, + outPath: join(dir, "main.ir.json"), + outputKind: "ir", + npmStatic: ["guard-fixture"], + }); + vi.restoreAllMocks(); + if (!result.ok) throw new Error(result.diagnostics.map((d) => d.message).join("\n")); + const start = text.indexOf("[static-objects] "); + const end = text.indexOf("[shape-overlay] ", start); + return start < 0 ? "" : text.slice(start, end < 0 ? undefined : end); +} + +/** The body of the function the rewritten text assigns to or hoists as `name`. */ +function body(text: string, name: string): string { + const at = text.search(new RegExp(`function [\\w$]*${name}\\b|${name} = function`)); + return at < 0 ? "" : text.slice(at, text.indexOf("\n}", at)); +} + +test("a store after code that can run keeps the read; a quiet prefix folds it", async () => { + const text = await rewrittenPackage(); + expect(body(text, "quiet_read")).toMatch(/return \(.*\b6\);/); + expect(body(text, "late_read")).toContain("return late.cfg.V"); +}); + +test("a table call whose key parameter is rebound keeps reading the table", async () => { + const text = await rewrittenPackage(); + // Every caller passes "a" or "b": the plain call needs no arm for other keys. + expect(body(text, "t_plain")).not.toContain("t.ops[op](v)"); + for (const name of ["t_pattern", "t_loop", "t_initializer", "t_redeclared"]) + expect(body(text, name), name).toContain("t.ops[op](v)"); +});