Address review feedback

packages/compiler/src/frontend/shape-inference/static-objects.ts

@@ -733,6 +733,67 @@ export function planStaticObjectRewrites(
       const call = requireValue(v);
       return call !== null && requiredFiles(sf, call).includes(target);
     });
+  /** Whether `node` sits inside a function of its top-level statement. */
+  const insideFunction = (node: Node): boolean => {
+    const top = topStatement(node);
+    for (let p: Node | undefined = node.parent; p !== undefined && p !== top; p = p.parent)
+      if (ts.isFunctionLike(p)) return true;
+    return false;
+  };
+  /** The nodes whose statements must have run for an effect-free receiver
+   * (effectFree) to be the constant object it reads as: each member read's
+   * stores of that member, and a name's declaration. */
+  const receiverDeps = (e: ts.Expression): Node[] => {
+    while (ts.isParenthesizedExpression(e)) e = e.expression;
+    if (ts.isIdentifier(e)) {
+      const d = declOf(e);
+      return d === undefined ? [] : [d];
+    }
+    if (!ts.isPropertyAccessExpression(e) && !ts.isElementAccessExpression(e)) return [];
+    const op = readOps.get(e);
+    if (op === undefined || op.name === null) return [];
+    const objects = analysis.nodes[op.recv]!.atoms;
+    const stores = analysis.ops
+      .filter(
+        (w) =>
+          w.kind === "write" &&
+          w.name === op.name &&
+          [...analysis.nodes[w.recv]!.atoms].some((a) => objects.has(a)),
+      )
+      .map((w) => w.node);
+    return [...stores, ...receiverDeps(e.expression)];
+  };
+  /** Whether `read` can only run after the statements holding `deps`: what
+   * it reads exists by then, so a rewrite that no longer evaluates the read
+   * cannot drop the TypeError or ReferenceError of an earlier read. In
+   * another file, the defining module has finished loading unless it
+   * requires the reading file. In the same file, a top-level read comes
+   * after them. A read inside a function runs when the function is called:
+   * the statements from the one that creates the function (the first, for
+   * a hoisted declaration) to the last of them must only define things. */
+  const createdBefore = (read: Node, deps: readonly Node[]): boolean => {
+    const sf = read.getSourceFile();
+    const stmts = sf.statements.nodes;
+    let last = -1;
+    for (const d of deps) {
+      const file = d.getSourceFile();
+      if (file !== sf) {
+        if (requiredClosure(file).has(sf)) return false;
+        continue;
+      }
+      const at = stmts.indexOf(topStatement(d)!);
+      if (at < 0) return false;
+      last = Math.max(last, at);
+    }
+    if (last < 0) return true;
+    const top = topStatement(read)!;
+    if (!insideFunction(read)) return stmts.indexOf(top) > last;
+    let fn: Node | undefined;
+    for (let p: Node | undefined = read.parent; p !== undefined && p !== top; p = p.parent)
+      if (ts.isFunctionLike(p)) fn = p;
+    const from = fn !== undefined && ts.isFunctionDeclaration(fn) ? 0 : stmts.indexOf(top);
+    return stmts.slice(from, last + 1).every(definitionOnly);
+  };
   const moduleBindings = new Map<string, string>();
   const exportAdds = new Map<ts.SourceFile, Set<string>>();
   const crossFileReference = (
@@ -759,11 +820,7 @@ export function planStaticObjectRewrites(
     const at = stmts.findIndex((st) => requiresWhenRun(sf, st, defining));
     if (at < 0) return null;
     const top = topStatement(node)!;
-    const inFunction = ((): boolean => {
-      for (let p: Node | undefined = node.parent; p !== undefined && p !== top; p = p.parent)
-        if (ts.isFunctionLike(p)) return true;
-      return false;
-    })();
+    const inFunction = insideFunction(node);
     if (!inFunction && stmts.indexOf(top) <= at) return null;
     // A function may run before the binding below is initialized if the
     // statement that loads the module, or one before it, runs other code
@@ -829,6 +886,10 @@ export function planStaticObjectRewrites(
       kept.push(`reference ${node.getText()}: the receiver may have effects`);
       continue;
     }
+    if (!createdBefore(node, [field.values[0]!.node!, ...receiverDeps(node.expression)])) {
+      kept.push(`reference ${node.getText()}: it may run before the member is stored`);
+      continue;
+    }
     if (node.getSourceFile() !== field.values[0]!.node!.getSourceFile()) {
       const text = crossFileReference(node, field.values[0]!.node!.getSourceFile(), name);
       if (text === null) continue;
@@ -871,6 +932,10 @@ export function planStaticObjectRewrites(
       kept.push(`constant read ${node.getText()}: the receiver may have effects`);
       continue;
     }
+    if (!createdBefore(node, [o.site.node, ...receiverDeps(node.expression)])) {
+      kept.push(`constant read ${node.getText()}: it may run before the object is created`);
+      continue;
+    }
     callEdits.push({
       sf: node.getSourceFile(),
       edit: { start: ts.tokenStartOfNode(node), end: node.end, text: `(${literal})` },
@@ -950,6 +1015,11 @@ export function planStaticObjectRewrites(
       kept.push(`${where}: a member is not a hoisted function of this file`);
       continue;
     }
+    const armNodes = o.fields.map((f) => f.values[0]!.node!);
+    if (!createdBefore(call, [o.site.node, ...armNodes, ...receiverDeps(node.expression)])) {
+      kept.push(`${where}: it may run before the table is created`);
+      continue;
+    }
     tableCalls.push({ sf: call.getSourceFile(), call, arms });
   }
   for (const { sf, call, arms } of tableCalls) {

tests/fixtures/npm/cases/static-objects-order/js-main.cjs

@@ -8,6 +8,7 @@ const early = require("static-objects-order/early-user.js");
 console.log("start");
 console.log(receivers.ns.run("add"), receivers.ns.run("sub"), receivers.ns.reads());
 console.log(receivers.ns.proto());
+console.log(receivers.ns.early());
 console.log(lazy.greet("a"), lazy.greet("b"));
 console.log(vec.len2(3, 4));
 console.log(early.early());

tests/fixtures/npm/cases/static-objects-order/main.ts

@@ -8,6 +8,7 @@ import { early } from "static-objects-order/early-user.js";
 console.log("start");
 console.log(ns.run("add"), ns.run("sub"), ns.reads());
 console.log(ns.proto());
+console.log(ns.early());
 console.log(greet("a"), greet("b"));
 console.log(len2(3, 4));
 console.log(early());

tests/fixtures/npm/cases/static-objects-order/node_modules/static-objects-order/receivers.js

@@ -1,6 +1,28 @@
 // Constant members, a function table and a namespace function read through
 // receivers with side effects: each receiver still runs.
 var ns = ns || {};
+// Reads that run before their members are created: Node throws.
+var early = (function () {
+  var out = [];
+  try {
+    out.push(ns.Late.X);
+  } catch (e) {
+    out.push(e.constructor.name);
+  }
+  try {
+    out.push(ns.later());
+  } catch (e) {
+    out.push(e.constructor.name);
+  }
+  return out.join(" ");
+})();
+ns.Late = { X: 5 };
+ns.later = function () {
+  return "later";
+};
+ns.early = function () {
+  return early + " " + ns.Late.X + " " + ns.later();
+};
 ns.Kind = { A: 1, B: 2 };
 ns.ops = {
   add: function (a, b) {

tests/harness/static-objects-order.test.ts

@@ -2,10 +2,11 @@
  * rewrites keep what the program evaluates and when its modules load. With
  * the shape overlay on, the npm case static-objects-order (through its
  * JavaScript consumer, js-main.cjs) keeps receivers with effects as
- * written, binds a module where the program loads it and only for reads
- * that run after the binding, never adds a hoisted class to an exports
- * literal that runs before the class's declaration, does not fold a
- * literal's `__proto__`, and prints what Node prints. A module in a require cycle through another
+ * written, keeps reads that may run before what they read exists, binds a
+ * module where the program loads it and only for reads that run after the
+ * binding, never adds a hoisted class to an exports literal that runs
+ * before the class's declaration, does not fold a literal's `__proto__`,
+ * and prints what Node prints. A module in a require cycle through another
  * module is not read through its exports from the cycle. The rewritten
  * files are read from the overlay's debug output
  * (SCRIPTC_DEBUG_SHAPE_OVERLAY). */
@@ -83,6 +84,11 @@ test(
       "kept table call (reads++, ns.ops)[op]: the table expression may have effects",
     );
     // vec.js assigns its exports literal before the constructor function.
+    // receivers.js reads ns.Late.X and calls ns.later() before they exist.
+    expect(debug).toContain(
+      "kept constant read ns.Late.X: it may run before the object is created",
+    );
+    expect(debug).toContain("kept reference ns.later: it may run before the member is stored");
     // lazy-user.js's first mention of lazy.js is a conditional require: the
     // binding follows the statement that loads the module each time it runs.
     expect(rewritten(debug, join(pkg, "lazy-user.js"))).toMatch(