Report dictionary copies of {} views, and keep method calls direct after writes into dictionaries of dictionaries

Fixes #1408
Co-authored-by: Chris Ainsley <c.b.ainsley@gmail.com>

docs/content/limitations/record-identity-and-property-order.mdx

@@ -1,6 +1,7 @@
 A record converted to a record type with a different set of properties, such as a narrower shape or one that adds optional properties, is copied.
 Mutations through the converted reference do not affect the original, and the copy is not `===` to it.
 Empty record layouts are also snapshots because they have no property storage.
+A class instance or record held as `{}`, `object` or `unknown` and then converted to a dictionary type such as `Record<string, string>` is copied as well; a dictionary of `unknown` values keeps the original object.
 `scriptc coverage` and `scriptc build` report `SC6001` where a copied value is then written or compared, whether the record was passed as an argument, stored in a typed binding, asserted with `as`, assigned to a typed binding later, or stored as an element of a typed array literal.
 
 Typed records with declared fields, indexed records, and arrays retain identity and mutations through native checked storage.

packages/compiler/src/diagnostics/diagnostic.ts

@@ -129,6 +129,7 @@ export function widthCopyDivergenceDiag(
   useText: string,
   site: "call" | "declaration" | "cast" | "element",
   loc: SrcLoc,
+  dictionary = false,
 ): ScrDiagnostic {
   const how =
     site === "call"
@@ -142,11 +143,16 @@ export function widthCopyDivergenceDiag(
     use === "write"
       ? `the write to '${useText}' updates only the copy, while under Node it updates the original object`
       : `'${useText}' compares the copy, which is never the original object, while under Node it is the same object`;
+  const cause = dictionary
+    ? "converting an object held as '{}', 'object' or 'unknown' to a dictionary type"
+    : "converting to a record type with other properties";
   return {
     code: "SC6001",
-    message: `${how} copies the object (converting to a record type with other properties compiles as a field copy): ${effect}`,
+    message: `${how} copies the object (${cause} compiles as a field copy): ${effect}`,
     loc,
-    hint: `declare the parameter or binding with the value's full type ('${sourceType}'), or make the shape a class — class instances keep their identity when passed as a base class`,
+    hint: dictionary
+      ? `keep the value at its own type ('${sourceType}'), or declare the dictionary's values as 'unknown', which keeps the original object`
+      : `declare the parameter or binding with the value's full type ('${sourceType}'), or make the shape a class — class instances keep their identity when passed as a base class`,
   };
 }
 

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

@@ -125,9 +125,12 @@ export function collectClassMethodMutations(
   };
   // Whether no function value can inhabit `type`. Primitives qualify, and so
   // do non-callable object types a function is not assignable to: one that
-  // requires a property functions lack, or a weak (all-optional) type that
+  // requires a property functions lack, a weak (all-optional) type that
   // shares no property with functions (TypeScript's weak-type rule rejects
-  // callable sources too).
+  // callable sources too), or a dictionary whose string index values are
+  // neither `any` nor a type parameter: a function type has no index
+  // signature, and only an `any` index admits a source without one (an
+  // instantiation can make a type parameter `any`).
   const functionFree = (type: ts.Type): boolean => {
     if (type.isUnionType()) return ts.constituentTypes(type).every(functionFree);
     if (
@@ -149,7 +152,14 @@ export function collectClassMethodMutations(
     )
       return false;
     const properties = lowerer.checker.getPropertiesOfType(type);
-    if (properties.length === 0) return false;
+    if (properties.length === 0)
+      return lowerer.checker
+        .getIndexInfosOfType(type)
+        .some(
+          (index) =>
+            (index.keyType.flags & ts.TypeFlags.StringLike) !== 0 &&
+            (index.type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Instantiable)) === 0,
+        );
     const required = properties.filter(
       (property) => (property.flags & ts.SymbolFlags.Optional) === 0,
     );

packages/compiler/src/frontend/lowering/divergences.ts

@@ -180,7 +180,15 @@ export function scanDivergences(
       const copy = safely(() => widthCopyBetween(lowerer, arg, param.type!));
       if (copy === undefined || copy === null) return;
       out.push(
-        widthCopyDivergenceDiag(copy.source, copy.target, use.kind, use.text, "call", locOf(arg)),
+        widthCopyDivergenceDiag(
+          copy.source,
+          copy.target,
+          use.kind,
+          use.text,
+          "call",
+          locOf(arg),
+          copy.dictionary,
+        ),
       );
     });
   }
@@ -200,6 +208,7 @@ export function scanDivergences(
         use.text,
         "declaration",
         locOf(decl.initializer!),
+        copy.dictionary,
       ),
     );
   }
@@ -215,7 +224,15 @@ export function scanDivergences(
     const copy = safely(() => widthCopyBetween(lowerer, source, type));
     if (copy === undefined || copy === null) continue;
     out.push(
-      widthCopyDivergenceDiag(copy.source, copy.target, use.kind, use.text, "cast", locOf(source)),
+      widthCopyDivergenceDiag(
+        copy.source,
+        copy.target,
+        use.kind,
+        use.text,
+        "cast",
+        locOf(source),
+        copy.dictionary,
+      ),
     );
   }
   for (const assignment of assignments) {
@@ -240,6 +257,7 @@ export function scanDivergences(
         use.text,
         "declaration",
         locOf(assignment.right),
+        copy.dictionary,
       ),
     );
   }
@@ -254,7 +272,15 @@ export function scanDivergences(
       const copy = safely(() => widthCopyBetween(lowerer, item, element));
       if (copy === undefined || copy === null) continue;
       out.push(
-        widthCopyDivergenceDiag(copy.source, copy.target, "write", use, "element", locOf(item)),
+        widthCopyDivergenceDiag(
+          copy.source,
+          copy.target,
+          "write",
+          use,
+          "element",
+          locOf(item),
+          copy.dictionary,
+        ),
       );
       break;
     }
@@ -653,14 +679,23 @@ function enclosingScope(node: ts.Node): ts.Node | undefined {
  * instance flowing into a plain record type that does not keep class
  * identity (ClassProtocols marks the targets a class instance visibly
  * flows into; a value reaching another record type is a field copy). */
+/** The types a width copy converts between, and whether the target is a
+ * dictionary. */
+interface WidthCopy {
+  source: string;
+  target: string;
+  dictionary?: boolean;
+}
+
 function widthCopyBetween(
   lowerer: Lowerer,
   source: ts.Expression,
   targetNode: ts.TypeNode,
-): { source: string; target: string } | null {
+): WidthCopy | null {
   const checker = lowerer.checker;
   const s = checker.getTypeAtLocation(source);
   const t = checker.getTypeFromTypeNode(targetNode);
+  if (isDictionaryType(checker, t)) return dictionaryViewCopy(lowerer, source, s, t);
   if (!isPlainRecordType(checker, t)) return null;
   const classSource = isProgramClassType(checker, s);
   if (!classSource && !isPlainRecordType(checker, s)) return null;
@@ -680,6 +715,73 @@ function widthCopyBetween(
   return { source: checker.typeToString(s), target: checker.typeToString(t) };
 }
 
+/** A copy made by converting a checked-dynamic view (`{}`, `object`,
+ * `unknown`) to a dictionary type that compiles to a native record (a
+ * dictionary of `unknown` values stays checked-dynamic and keeps the
+ * object). A dictionary of that type comes back out of the view as itself,
+ * so a copy is reported only when the view is a constant binding
+ * initialized with a class instance or a record: that object is converted
+ * field by field into a new dictionary. */
+function dictionaryViewCopy(
+  lowerer: Lowerer,
+  source: ts.Expression,
+  s: ts.Type,
+  t: ts.Type,
+): WidthCopy | null {
+  const checker = lowerer.checker;
+  if (!isDynamicViewType(checker, s)) return null;
+  if (lowerer.mapTypeOf(t)?.kind !== "record") return null;
+  while (ts.isParenthesizedExpression(source) || ts.isNonNullExpression(source))
+    source = source.expression;
+  if (!ts.isIdentifier(source)) return null;
+  const decl = checker.getSymbolAtLocation(source)?.valueDeclaration;
+  if (
+    decl === undefined ||
+    !ts.isVariableDeclaration(decl) ||
+    decl.initializer === undefined ||
+    !ts.isVariableDeclarationList(decl.parent) ||
+    !(decl.parent.flags & ts.NodeFlags.Const)
+  )
+    return null;
+  let origin = decl.initializer;
+  while (
+    ts.isParenthesizedExpression(origin) ||
+    ts.isNonNullExpression(origin) ||
+    ts.isAsExpression(origin) ||
+    ts.isTypeAssertion(origin)
+  )
+    origin = origin.expression;
+  const o = checker.getTypeAtLocation(origin);
+  if (!isProgramClassType(checker, o) && !isPlainRecordType(checker, o)) return null;
+  return { source: checker.typeToString(o), target: checker.typeToString(t), dictionary: true };
+}
+
+/** `{}`, `object` and `unknown`: types whose values lower to checked
+ * dynamic storage that keeps the original object. */
+function isDynamicViewType(checker: ts.TypeChecker, type: ts.Type): boolean {
+  if (type.flags & (ts.TypeFlags.Unknown | ts.TypeFlags.NonPrimitive)) return true;
+  return (
+    (type.flags & ts.TypeFlags.Object) !== 0 &&
+    checker.getPropertiesOfType(type).length === 0 &&
+    checker.getIndexInfosOfType(type).length === 0 &&
+    checker.getCallSignatures(type).length === 0 &&
+    checker.getConstructSignatures(type).length === 0
+  );
+}
+
+/** A string-keyed dictionary with no declared properties
+ * (`Record<string, string>`, `{ [key: string]: number }`). */
+function isDictionaryType(checker: ts.TypeChecker, type: ts.Type): boolean {
+  if ((type.flags & ts.TypeFlags.Object) === 0) return false;
+  if (checker.isArrayType(type) || checker.isTupleType(type)) return false;
+  if (checker.getCallSignatures(type).length > 0) return false;
+  if (checker.getConstructSignatures(type).length > 0) return false;
+  if (checker.getPropertiesOfType(type).length > 0) return false;
+  return checker
+    .getIndexInfosOfType(type)
+    .some((index) => (index.keyType.flags & ts.TypeFlags.String) !== 0);
+}
+
 function isProgramClassType(checker: ts.TypeChecker, type: ts.Type): boolean {
   if ((type.flags & ts.TypeFlags.Object) === 0) return false;
   const symbol = type.getSymbol();

packages/compiler/test/class-method-mutations.test.ts

@@ -135,3 +135,45 @@ test("a reflection call with a well-known symbol key keeps checked dispatch", as
   const callers = await callersOf(`${range}${write}`, "%Range.sym:iterator");
   expect(callers.filter((name) => !name.startsWith("%method.value:"))).toEqual([]);
 });
+
+const groups = `
+class Tally {
+  n = 0;
+  add(): void { this.n++; }
+}
+interface Settings { groups: VALUE }
+function addGroup(settings: Settings, name: string): void {
+  settings.groups[name] = {};
+}
+const settings: Settings = { groups: {} };
+const tally = new Tally();
+addGroup(settings, String(tally.n));
+tally.add();
+console.log(tally.n, Object.keys(settings.groups));
+`;
+
+test("a computed write into a dictionary of dictionaries leaves method calls direct", async () => {
+  const source = groups.replace("VALUE", "Record<string, Record<string, string>>");
+  expect(await callersOf(source, "%dyn.class.properties")).toEqual([]);
+});
+
+test("a computed write into a dictionary that can hold functions keeps checked dispatch", async () => {
+  for (const value of [
+    "Record<string, Record<string, string> | (() => void)>",
+    "Record<string, Record<string, any>>",
+  ]) {
+    const source = groups.replace("VALUE", value);
+    expect(await callersOf(source, "%dyn.class.properties")).not.toEqual([]);
+  }
+});
+
+test("a computed write into a generic dictionary of dictionaries keeps checked dispatch", async () => {
+  const source = groups
+    .replace(
+      "interface Settings { groups: VALUE }",
+      "interface Settings<T> { groups: Record<string, Record<string, T>> }",
+    )
+    .replace("function addGroup(settings: Settings,", "function addGroup<T>(settings: Settings<T>,")
+    .replace("const settings: Settings =", "const settings: Settings<string> =");
+  expect(await callersOf(source, "%dyn.class.properties")).not.toEqual([]);
+});

packages/compiler/test/ts7/baselines/order-parity/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts.json

@@ -0,0 +1,6 @@
+{
+ "order": [
+  "<repo>/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts"
+ ],
+ "diags": []
+}

tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts

@@ -0,0 +1,38 @@
+// A string-keyed dictionary whose values are dictionaries cannot hold a
+// function, so storing into it under a key known only at runtime leaves
+// class methods in place: their calls stay direct, and a record a method
+// returns is the record the instance holds, so writes through it are kept.
+interface Data {
+  items: string[];
+}
+class Store {
+  data: Data = { items: [] };
+  get(): Data {
+    return this.data;
+  }
+}
+class Counter {
+  count = 0;
+  bump(): number {
+    return ++this.count;
+  }
+}
+interface Settings {
+  groups: Record<string, Record<string, string>>;
+}
+function addGroup(settings: Settings, name: string): void {
+  settings.groups[name] = {};
+}
+
+const settings: Settings = { groups: {} };
+const name = process.argv.length > 99 ? "never" : "colours";
+addGroup(settings, name);
+const group = settings.groups[name];
+if (group !== undefined) group["sky"] = "blue";
+const store = new Store();
+store.get().items.push("a");
+store.get().items.push("b");
+const counter = new Counter();
+let total = 0;
+for (let i = 0; i < 1000; i++) total += counter.bump();
+console.log(JSON.stringify(settings.groups), store.data.items.join(","), total);

tests/coverage-fixtures/dictionary-view-copies.ts

@@ -0,0 +1,29 @@
+// A class instance or record held as `{}`, `object` or `unknown` keeps the
+// original object, and converting it to a dictionary type copies it field
+// by field. Each site below then writes through or compares the copy.
+class Counter {
+  label = "c";
+  add(): number {
+    return this.label.length;
+  }
+}
+const key = process.argv.length > 5 ? "z" : "b";
+
+const counter = new Counter();
+const view: {} = counter;
+const table: Record<string, string> = view;
+table[key] = "y";
+
+const point = { a: "x" };
+const loose: object = point;
+const dict = loose as Record<string, string>;
+dict[key] = "y";
+
+const held: unknown = { a: "x" };
+const fromUnknown = held as Record<string, string>;
+console.log(fromUnknown === held);
+
+const empty: {} = {};
+const filled: Record<string, string> = empty;
+filled[key] = "y";
+console.log(JSON.stringify(counter), JSON.stringify(point), JSON.stringify(empty));

tests/coverage-fixtures/no-divergences.ts

@@ -1,6 +1,6 @@
 // Ordinary code: identity-preserving calls, declaration-ordered literals,
-// Latin-script comparisons, related-class casts, ISO dates and views of the
-// global object report no divergence.
+// Latin-script comparisons, related-class casts, ISO dates, views of the
+// global object and a dictionary held as `{}` report no divergence.
 interface Item {
   name: string;
   qty: number;
@@ -38,3 +38,10 @@ const COUNT = Symbol.for("no-divergences.count");
 const globalState = globalThis as { [COUNT]?: { n: number } };
 globalState[COUNT] = { n: 1 };
 console.log(globalState[COUNT]?.n);
+
+// A dictionary held as `{}` comes back out as itself.
+const counts: Record<string, string> = { a: "x" };
+const opaque: {} = counts;
+const again: Record<string, string> = opaque;
+again[counts["a"] ?? "a"] = "y";
+console.log(JSON.stringify(counts));

tests/harness/__snapshots__/coverage-dictionary-view-copies.txt

@@ -0,0 +1,21 @@
+scriptc coverage tests/coverage-fixtures/dictionary-view-copies.ts
+
+  statements analyzed   18
+  compile statically    18  (100%)
+    in dynamic tier     3  (operate on values typed only at runtime; --explain-dynamic lists them)
+
+  fully static — all analyzed statements compile to native code.
+
+  differs from Node   4 sites (compiles; the native program can behave differently)
+      ×1  SC6001  storing a 'Counter' in a 'Record<…>' binding copies the object (converting an object held as '{}', 'object' or 'unknown' to a dictionary type compiles as a field copy): the write to 'table[key]' upda…
+          at tests/coverage-fixtures/dictionary-view-copies.ts:14:39
+          hint: keep the value at its own type ('Counter'), or declare the dictionary's values as 'unknown', which keeps the original object
+      ×1  SC6001  storing a '{}' in a 'Record<…>' binding copies the object (converting an object held as '{}', 'object' or 'unknown' to a dictionary type compiles as a field copy): the write to 'filled[key]' updates …
+          at tests/coverage-fixtures/dictionary-view-copies.ts:27:40
+          hint: keep the value at its own type ('{}'), or declare the dictionary's values as 'unknown', which keeps the original object
+      ×1  SC6001  asserting a '{ a: string; }' as 'Record<…>' copies the object (converting an object held as '{}', 'object' or 'unknown' to a dictionary type compiles as a field copy): the write to 'dict[key]' update…
+          at tests/coverage-fixtures/dictionary-view-copies.ts:19:14
+          hint: keep the value at its own type ('{ a: string; }'), or declare the dictionary's values as 'unknown', which keeps the original object
+      ×1  SC6001  asserting a '{ a: string; }' as 'Record<…>' copies the object (converting an object held as '{}', 'object' or 'unknown' to a dictionary type compiles as a field copy): 'fromUnknown === held' compares…
+          at tests/coverage-fixtures/dictionary-view-copies.ts:23:21
+          hint: keep the value at its own type ('{ a: string; }'), or declare the dictionary's values as 'unknown', which keeps the original object

tests/harness/corpus-costs/7010-dictionary-of-dictionaries-keep-methods.ts.json

@@ -0,0 +1 @@
+{ "plain": 2184, "san": 1198 }

tests/harness/coverage.test.ts

@@ -107,6 +107,20 @@ test("record width copies report at every storing site", async () => {
   );
 });
 
+test("dictionary conversions of checked-dynamic views report copies", async () => {
+  const { coverage } = analyze(fixture("dictionary-view-copies.ts"));
+  expect(coverage.diagnostics).toEqual([]);
+  expect((coverage.divergences ?? []).map((d) => d.code)).toEqual([
+    "SC6001",
+    "SC6001",
+    "SC6001",
+    "SC6001",
+  ]);
+  await expect(report(fixture("dictionary-view-copies.ts"))).toMatchFileSnapshot(
+    "__snapshots__/coverage-dictionary-view-copies.txt",
+  );
+});
+
 test("ordinary code reports no divergence", () => {
   const { coverage } = analyze(fixture("no-divergences.ts"));
   expect(coverage.diagnostics).toEqual([]);