Keep literal key order through self copies and conversions that build no records (#1982)

packages/compiler/src/frontend/lowering/class-method-mutations.ts

@@ -46,6 +46,13 @@ export function collectClassMethodMutations(
   const typedClasses: (ts.ClassDeclaration | ts.ClassExpression)[] = [];
   const symbolMethods = new Set<string>();
   let unknownKey: ts.Node | undefined;
+  /** Every write with an unknown key: each may install a replacement of any
+   * method name, and the key-order witness asks what each one installs. */
+  const unknownKeySites: ts.Node[] = [];
+  const markUnknownKey = (site: ts.Node): void => {
+    unknownKey = site;
+    unknownKeySites.push(site);
+  };
   let numericKey: ts.Node | undefined;
   let unknownOwnField = false;
   // `sym:iterator` for `Symbol.iterator`: the name a class method declared
@@ -83,7 +90,7 @@ export function collectClassMethodMutations(
   for (const key of computedMethods)
     for (const name of literalValues(lowerer.typeOf(key)) ?? []) methods.add(String(name));
   const markName = (name: string, site: ts.Node, arrow = false): void => {
-    if (name === "prototype" || name === "__proto__") unknownKey = site;
+    if (name === "prototype" || name === "__proto__") markUnknownKey(site);
     if (methods.has(name)) lowerer.noteMethodAccess(name, site);
     if (!arrow) receiverFields.add(name);
   };
@@ -449,11 +456,12 @@ export function collectClassMethodMutations(
       (lowerer.isStdlibGlobal(node, "Object") || lowerer.isStdlibGlobal(node, "Reflect")) &&
       (!ts.isPropertyAccessExpression(node.parent) || node.parent.expression !== node)
     )
-      unknownKey = node;
+      markUnknownKey(node);
   for (const node of reflection) {
     const objectMethod = lowerer.stdlibGlobalMember(node, "Object");
     const reflectMethod = lowerer.stdlibGlobalMember(node, "Reflect");
-    if (objectMethod === "setPrototypeOf" || reflectMethod === "setPrototypeOf") unknownKey = node;
+    if (objectMethod === "setPrototypeOf" || reflectMethod === "setPrototypeOf")
+      markUnknownKey(node);
     const single =
       objectMethod === "defineProperty" ||
       reflectMethod === "defineProperty" ||
@@ -463,7 +471,7 @@ export function collectClassMethodMutations(
     if (!single && !multiple) continue;
     const call = node.parent;
     if (!ts.isCallExpression(call) || call.expression !== node) {
-      unknownKey = node;
+      markUnknownKey(node);
       continue;
     }
     const receiver = call.arguments.nodes[0];
@@ -618,7 +626,7 @@ export function collectClassMethodMutations(
       // Numeric writes can replace numeric names but not ordinary names.
       numericKey = site;
     } else if (!(flags & ts.TypeFlags.ESSymbolLike)) {
-      unknownKey = site;
+      markUnknownKey(site);
     }
   }
   if (numericKey) {
@@ -627,7 +635,10 @@ export function collectClassMethodMutations(
     }
   }
   if (unknownKey) {
-    for (const method of methods) lowerer.noteMethodAccess(method, unknownKey);
+    for (const method of methods) {
+      lowerer.prototypeMethodAccesses.set(method, unknownKey);
+      for (const site of unknownKeySites) lowerer.noteMethodSite(method, site);
+    }
     for (const method of symbolMethods) lowerer.replacedSymbolMethods.add(method);
   }
   if (!unknownKey && !unknownOwnField)

packages/compiler/src/frontend/lowering/key-order-witness.test.ts

@@ -29,7 +29,7 @@ interface Fixture {
   notes: KeyOrderNotes;
   record(fields: [string, IrType][], order: string[]): IrType & { kind: "record" };
   literal(type: IrType, keys: string[], segments?: KeyOrderSegment[]): IrExpr;
-  run(body: IrStmt[]): IrFunction;
+  run(body: IrStmt[], name?: string): IrFunction;
   order(type: IrType & { kind: "record" }): string[] | undefined;
 }
 
@@ -73,8 +73,8 @@ function fixture(): Fixture {
       segments ?? keys.map((name) => ({ kind: "key", name, definite: true })),
     );
   };
-  const run = (body: IrStmt[]): IrFunction => {
-    const fn: IrFunction = { name: "main", params: [], returnType: F64, locals: [], body, loc };
+  const run = (body: IrStmt[], name = "main"): IrFunction => {
+    const fn: IrFunction = { name, params: [], returnType: F64, locals: [], body, loc };
     applyKeyOrderWitnesses(lowerer, [fn]);
     return fn;
   };
@@ -706,7 +706,10 @@ test("a spread reads its source's order right after the literal evaluates the so
 
 /** `{ a, b }` with a declared table `["a", "b"]` and a literal `{ b, a }`
  * plus `{ ...value, a }` of the same shape, beside `extra`. */
-function selfCopy(extra: (f: Fixture, ab: IrType & { kind: "record" }) => IrStmt[]): string[] {
+function selfCopy(
+  extra: (f: Fixture, ab: IrType & { kind: "record" }) => IrStmt[],
+  fnName = "main",
+): string[] {
   const f = fixture();
   const ab = f.record(
     [
@@ -716,11 +719,14 @@ function selfCopy(extra: (f: Fixture, ab: IrType & { kind: "record" }) => IrStmt
     ["a", "b"],
   );
   const self: KeyOrderSegment = { kind: "spread", shapeId: ab.shapeId, maybeAbsent: false };
-  f.run([
-    decl("v", f.literal(ab, ["b", "a"])),
-    observe(f.literal(ab, ["a", "b"], [self, { kind: "key", name: "a", definite: true }])),
-    ...extra(f, ab),
-  ]);
+  f.run(
+    [
+      decl("v", f.literal(ab, ["b", "a"])),
+      observe(f.literal(ab, ["a", "b"], [self, { kind: "key", name: "a", definite: true }])),
+      ...extra(f, ab),
+    ],
+    fnName,
+  );
   return f.order(ab)!;
 }
 
@@ -762,6 +768,31 @@ test("a replaceable method's checked result builds records when a replacement ma
   ).toEqual(["a", "b"]);
 });
 
+/** A native callback taking the shape, boxed for JavaScript. */
+const boxedCallback = (ab: IrType): IrExpr => ({
+  kind: "dynFrom",
+  value: {
+    kind: "closure",
+    fnName: "callback",
+    captures: [],
+    type: { kind: "func", params: [ab], ret: { kind: "void" } },
+    loc,
+  },
+  type: { kind: "dyn" },
+  loc,
+});
+
+test("a native callback boxed for JavaScript receives records JavaScript builds", () => {
+  expect(selfCopy((_, ab) => [observe(boxedCallback(ab))])).toEqual(["a", "b"]);
+});
+
+test("a class's prototype slots box its methods without building their parameters", () => {
+  expect(selfCopy((_, ab) => [observe(boxedCallback(ab))], "%prototype.data.Service")).toEqual([
+    "b",
+    "a",
+  ]);
+});
+
 test("a replaceable method's result keeps its witness through an IR rewrite", () => {
   expect(
     selfCopy((f, ab) => {

packages/compiler/src/frontend/lowering/key-order-witness.ts

@@ -292,6 +292,17 @@ function replacesWithoutRecords(lowerer: Lowerer, site: ts.Node): boolean {
   );
 }
 
+/** A compiler helper that boxes a class's own state for JavaScript's reads:
+ * its prototype data and the property and method dispatch over boxed
+ * instances. */
+function boxesOwnState(fnName: string): boolean {
+  return (
+    fnName.startsWith("%prototype.data.") ||
+    fnName.startsWith("%dyn.class.") ||
+    fnName.startsWith("%class.")
+  );
+}
+
 /** A type whose values are primitives (or void): it holds no record. */
 function primitiveOnly(type: ts.Type): boolean {
   if (type.isUnionType()) return ts.constituentTypes(type).every(primitiveOnly);
@@ -357,8 +368,18 @@ export function applyKeyOrderWitnesses(lowerer: Lowerer, fns: readonly IrFunctio
    * its checked field setters, each a witness of its own. A checked
    * function's parameters receive native values: a record parameter is
    * passed out, never built, though a callback among them receives values
-   * built from JavaScript again. */
-  const shapesIn = (type: IrType, out: Set<string>, seen: Set<string>, produced = false): void => {
+   * built from JavaScript again. `built` false starts the walk at a value
+   * passed out (a native value boxed for JavaScript); `writes` false leaves
+   * out what JavaScript could write into the containers passed out, so the
+   * walk counts only what the functions among them receive. */
+  const shapesIn = (
+    type: IrType,
+    out: Set<string>,
+    seen: Set<string>,
+    produced = false,
+    built = true,
+    writes = true,
+  ): void => {
     const visit = (t: IrType, built: boolean): boolean => {
       const mark = built ? "" : "-";
       // A container JavaScript builds converts into fresh native storage, so
@@ -367,7 +388,7 @@ export function applyKeyOrderWitnesses(lowerer: Lowerer, fns: readonly IrFunctio
       // such as callbacks whose parameters it then builds (children passed
       // out). Those children count in both directions.
       const child = (type: IrType): boolean => {
-        visit(type, true);
+        if (built || writes) visit(type, true);
         if (!built) visit(type, false);
         return true;
       };
@@ -401,7 +422,7 @@ export function applyKeyOrderWitnesses(lowerer: Lowerer, fns: readonly IrFunctio
       }
       return everyTypeChild(t, child);
     };
-    visit(type, true);
+    visit(type, built);
   };
   const opaqueShapes = new Set<string>();
   const opaqueSeen = new Set<string>();
@@ -412,6 +433,7 @@ export function applyKeyOrderWitnesses(lowerer: Lowerer, fns: readonly IrFunctio
    * declaration's order, the order their table holds. */
   const declaredShapes = new Set<string>();
   const declaredSeen = new Set<string>();
+  const boxedSeen = new Set<string>();
   /** The checked result of a replaceable method call while every site that
    * can replace the method installs functions that return no record. */
   const unchangedMethodResult = (node: IrExpr): boolean => {
@@ -518,6 +540,16 @@ export function applyKeyOrderWitnesses(lowerer: Lowerer, fns: readonly IrFunctio
       case "jsonStringify":
       case "dynFrom":
         observedRoots.push(e.value.type);
+        // A native value the program hands to JavaScript: the callbacks it
+        // holds (itself, a field, an element) receive records JavaScript
+        // builds. The helpers that box the program's own state for
+        // JavaScript's reads (a class's prototype slots and property
+        // dispatch) hand out the class's methods, whose arguments the
+        // program's own calls pass as native values, as a replaceable
+        // method's result above. Writes through the boxed value are left
+        // out: reading a boxed value creates no record.
+        if (!boxesOwnState(fnName))
+          shapesIn(e.value.type, runtimeShapes, boxedSeen, true, false, false);
         return;
       case "recordKeyGet":
         observedRoots.push(e.obj.type);

packages/compiler/test/ts7/baselines/order-parity/tests/corpus/7624-key-order-boxed-callback-records/main.ts.json

@@ -0,0 +1,7 @@
+{
+ "order": [
+  "<repo>/tests/corpus/7624-key-order-boxed-callback-records/lib.js",
+  "<repo>/tests/corpus/7624-key-order-boxed-callback-records/main.ts"
+ ],
+ "diags": []
+}

tests/corpus/7623-key-order-javascript-built-records/lib.js

@@ -1,11 +1,18 @@
 // Records built in JavaScript in another key order than the TypeScript
 // literals of the same type: a replacement method installed through a
-// computed key, and a function stored on the global object, read through a
-// typed view, that calls a callback inside its options record.
+// computed key (beside another computed write that installs a function
+// returning a primitive), and a function stored on the global object, read
+// through a typed view, that calls a callback inside its options record.
 export function install(target, name) {
   target[name] = () => ({ a: 1, b: 2 });
 }
 
+// A later computed write of a function that returns a primitive does not
+// hide the record-returning one above.
+export function installLabel(target, name) {
+  target[name] = () => "label";
+}
+
 globalThis.__eachRow = function (options) {
   options.visit({ x: 5, y: 6 });
 };

tests/corpus/7623-key-order-javascript-built-records/main.ts

@@ -1,7 +1,7 @@
 // Copies and enumerations of records JavaScript builds follow the order
 // JavaScript wrote, also when the TypeScript literals of the type write
 // another order and spread their own type. Each case uses its own type.
-import { install } from "./lib.js";
+import { install, installLabel } from "./lib.js";
 
 interface Pair {
   a: number;
@@ -39,6 +39,9 @@ showPair("method", store.get());
 showPair("self copy", { ...store.get(), b: 3 });
 install(store, "get");
 showPair("replaced", store.get());
+const labels: { label(): string } = { label: () => "plain" };
+installLabel(labels, "label");
+console.log(labels.label());
 
 // A JavaScript function, checked into a typed slot, calls a native callback
 // it receives inside its options record with a record it builds.

tests/corpus/7624-key-order-boxed-callback-records/lib.js

@@ -0,0 +1,7 @@
+// A JavaScript function that calls a callback inside its options object
+// with records it builds, in another key order than the TypeScript literals
+// of the callback's parameter type.
+export function each(options) {
+  options.visit({ a: 1, b: 2 });
+  options.visit({ a: 3, b: 4 });
+}

tests/corpus/7624-key-order-boxed-callback-records/main.ts

@@ -0,0 +1,27 @@
+// A native callback handed to JavaScript inside an options record receives
+// records JavaScript builds: copies and enumerations of them follow the
+// order JavaScript wrote, also when the TypeScript literals of the type
+// write another order and spread their own type.
+import { each } from "./lib.js";
+
+interface Row {
+  a: number;
+  b: number;
+}
+
+function show(label: string, row: Row): void {
+  const copy = { ...row };
+  console.log(label, Object.keys(row).join(), JSON.stringify(copy), Object.keys(copy).join());
+}
+
+const literal: Row = { b: 20, a: 10 };
+show("literal", literal);
+show("self copy", { ...literal, b: 21 });
+const seen: Row[] = [];
+each({
+  visit: (row: Row) => {
+    seen.push(row);
+    show("callback", row);
+  },
+});
+console.log(JSON.stringify(seen), JSON.stringify(seen.map((row) => ({ ...row, a: row.a * 2 }))));

tests/harness/corpus-costs/7624-key-order-boxed-callback-records/main.ts.json

@@ -0,0 +1 @@
+{ "plain": 6931, "san": 8003 }