From 35f2d8f791022b779972e54a55d2b369ab657fef Mon Sep 17 00:00:00 2001 From: Chris Tate Date: Thu, 22 Oct 2026 13:07:47 -0500 Subject: [PATCH] Report dictionary copies of `{}` views, and keep method calls direct after writes into dictionaries of dictionaries Fixes #1408 Co-authored-by: Chris Ainsley --- .../record-identity-and-property-order.mdx | 1 + .../compiler/src/diagnostics/diagnostic.ts | 10 +- .../lowering/class-method-mutations.ts | 16 ++- .../src/frontend/lowering/divergences.ts | 110 +++++++++++++++++- .../test/class-method-mutations.test.ts | 42 +++++++ ...onary-of-dictionaries-keep-methods.ts.json | 6 + ...dictionary-of-dictionaries-keep-methods.ts | 38 ++++++ .../dictionary-view-copies.ts | 29 +++++ tests/coverage-fixtures/no-divergences.ts | 11 +- .../coverage-dictionary-view-copies.txt | 21 ++++ ...onary-of-dictionaries-keep-methods.ts.json | 1 + tests/harness/coverage.test.ts | 14 +++ 12 files changed, 288 insertions(+), 11 deletions(-) create mode 100644 packages/compiler/test/ts7/baselines/order-parity/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts.json create mode 100644 tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts create mode 100644 tests/coverage-fixtures/dictionary-view-copies.ts create mode 100644 tests/harness/__snapshots__/coverage-dictionary-view-copies.txt create mode 100644 tests/harness/corpus-costs/7010-dictionary-of-dictionaries-keep-methods.ts.json diff --git a/docs/content/limitations/record-identity-and-property-order.mdx b/docs/content/limitations/record-identity-and-property-order.mdx index 0381803f6..224757d52 100644 --- a/docs/content/limitations/record-identity-and-property-order.mdx +++ b/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` 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. diff --git a/packages/compiler/src/diagnostics/diagnostic.ts b/packages/compiler/src/diagnostics/diagnostic.ts index 17cb2efc0..47db2193b 100644 --- a/packages/compiler/src/diagnostics/diagnostic.ts +++ b/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`, }; } diff --git a/packages/compiler/src/frontend/lowering/class-method-mutations.ts b/packages/compiler/src/frontend/lowering/class-method-mutations.ts index abbaf3a75..7629377d4 100644 --- a/packages/compiler/src/frontend/lowering/class-method-mutations.ts +++ b/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, ); diff --git a/packages/compiler/src/frontend/lowering/divergences.ts b/packages/compiler/src/frontend/lowering/divergences.ts index 1c362cb09..f766aba14 100644 --- a/packages/compiler/src/frontend/lowering/divergences.ts +++ b/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`, `{ [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(); diff --git a/packages/compiler/test/class-method-mutations.test.ts b/packages/compiler/test/class-method-mutations.test.ts index b508a3949..3221e44ee 100644 --- a/packages/compiler/test/class-method-mutations.test.ts +++ b/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>"); + 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 | (() => void)>", + "Record>", + ]) { + 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 { groups: Record> }", + ) + .replace("function addGroup(settings: Settings,", "function addGroup(settings: Settings,") + .replace("const settings: Settings =", "const settings: Settings ="); + expect(await callersOf(source, "%dyn.class.properties")).not.toEqual([]); +}); diff --git a/packages/compiler/test/ts7/baselines/order-parity/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts.json b/packages/compiler/test/ts7/baselines/order-parity/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts.json new file mode 100644 index 000000000..cc3c9dabe --- /dev/null +++ b/packages/compiler/test/ts7/baselines/order-parity/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts.json @@ -0,0 +1,6 @@ +{ + "order": [ + "/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts" + ], + "diags": [] +} diff --git a/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts b/tests/corpus/7010-dictionary-of-dictionaries-keep-methods.ts new file mode 100644 index 000000000..a53e82f83 --- /dev/null +++ b/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>; +} +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); diff --git a/tests/coverage-fixtures/dictionary-view-copies.ts b/tests/coverage-fixtures/dictionary-view-copies.ts new file mode 100644 index 000000000..bfcfa61f4 --- /dev/null +++ b/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 = view; +table[key] = "y"; + +const point = { a: "x" }; +const loose: object = point; +const dict = loose as Record; +dict[key] = "y"; + +const held: unknown = { a: "x" }; +const fromUnknown = held as Record; +console.log(fromUnknown === held); + +const empty: {} = {}; +const filled: Record = empty; +filled[key] = "y"; +console.log(JSON.stringify(counter), JSON.stringify(point), JSON.stringify(empty)); diff --git a/tests/coverage-fixtures/no-divergences.ts b/tests/coverage-fixtures/no-divergences.ts index 3739336e3..ca76bcc32 100644 --- a/tests/coverage-fixtures/no-divergences.ts +++ b/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 = { a: "x" }; +const opaque: {} = counts; +const again: Record = opaque; +again[counts["a"] ?? "a"] = "y"; +console.log(JSON.stringify(counts)); diff --git a/tests/harness/__snapshots__/coverage-dictionary-view-copies.txt b/tests/harness/__snapshots__/coverage-dictionary-view-copies.txt new file mode 100644 index 000000000..2359a4ffa --- /dev/null +++ b/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 \ No newline at end of file diff --git a/tests/harness/corpus-costs/7010-dictionary-of-dictionaries-keep-methods.ts.json b/tests/harness/corpus-costs/7010-dictionary-of-dictionaries-keep-methods.ts.json new file mode 100644 index 000000000..cc07613e7 --- /dev/null +++ b/tests/harness/corpus-costs/7010-dictionary-of-dictionaries-keep-methods.ts.json @@ -0,0 +1 @@ +{ "plain": 2184, "san": 1198 } diff --git a/tests/harness/coverage.test.ts b/tests/harness/coverage.test.ts index 86d202b46..f3958c5fe 100644 --- a/tests/harness/coverage.test.ts +++ b/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([]);