Name function expressions by their source names in SC6101 and skip type names and generated helpers (#1749)

docs/content/docs/coverage.mdx

@@ -79,7 +79,7 @@ The program compiled. Adding types to these functions lets scriptc compile them
 
 Without a profile, a dynamic-tier function counts as likely to run often when a dynamic operation runs on every iteration of one of its loops (a `for` loop's initializer and a `for...of` loop's iterable run once), when a loop calls it, or when it calls itself. A function is named only when its source says why its values are untyped: a value typed `any` (for a `for...of` variable, the value it iterates), an untyped `JSON.parse` result, or an untyped JavaScript file. Dynamic code that runs once, such as setup code that reads an `any` configuration, is not reported, and neither are functions that are dynamic for an operation with no static lowering, which more types would not change.
 
-The warning names at most five functions, most likely to run often first, and gives the total. Dynamic-tier code in npm packages is summarized on one line with each package's function count. A build that comes from the cache repeats the warning. `--no-dynamic-tier-warning` leaves it out, and `--fail-on=dynamic` fails a coverage run on it.
+The warning names at most five functions, most likely to run often first, and gives the total. A function expression is named by its own name, else by the variable, property or class field it is assigned to, else as `anonymous function`. Dynamic-tier code in npm packages is summarized on one line with each package's function count. A build that comes from the cache repeats the warning. `--no-dynamic-tier-warning` leaves it out, and `--fail-on=dynamic` fails a coverage run on it.
 
 ## Diagnostic codes
 

packages/compiler/src/frontend/lowering/dynamic-tier.ts

@@ -172,10 +172,20 @@ function isJsonParse(node: ts.Node | undefined): boolean {
   );
 }
 
+/** Bindings that hold a value: variables, parameters and properties (not
+ * types, classes or functions, whose names an operand may also mention). */
+const VALUE_BINDING =
+  ts.SymbolFlags.FunctionScopedVariable |
+  ts.SymbolFlags.BlockScopedVariable |
+  ts.SymbolFlags.Property;
+
 /** Why an identifier holds a value without a static type, or null. */
 function identifierCause(lowerer: Lowerer, id: ts.Identifier, depth = 0): UntypedCause | null {
-  const symbol = lowerer.checker.getSymbolAtLocation(id);
-  const decl = symbol?.declarations?.[0];
+  let symbol = lowerer.checker.getSymbolAtLocation(id);
+  if (symbol !== undefined && (symbol.flags & ts.SymbolFlags.Alias) !== 0)
+    symbol = lowerer.checker.getAliasedSymbol(symbol);
+  if (symbol === undefined || (symbol.flags & VALUE_BINDING) === 0) return null;
+  const decl = symbol.declarations?.[0];
   // An unannotated `for (const item of items)` variable: name `items`.
   if (
     depth < 4 &&
@@ -241,6 +251,9 @@ function causeOf(lowerer: Lowerer, sites: readonly { loc: SrcLoc }[]): UntypedCa
   return null;
 }
 
+/** Generated functions (see scanDynamicTier), after any module prefix. */
+const GENERATED = /^%*(?:m\d+\.)?%*(?:dyn|caught|init)\./;
+
 /** The likely-hot dynamic-tier functions of `module` (see the header). */
 export function scanDynamicTier(lowerer: Lowerer, module: IrModule): DynamicTierReport {
   const facts = new Map<string, FunctionFacts>();
@@ -255,9 +268,9 @@ export function scanDynamicTier(lowerer: Lowerer, module: IrModule): DynamicTier
   const packages = new Map<string, number>();
   for (const fn of module.functions) {
     // Generated `%dyn.*` helpers are dynamic operations lowered out of line,
-    // and `%init.*` is a module's top level: neither is a function to name.
-    if (fn.tier !== "dynamic" || fn.name.startsWith("%dyn.") || fn.name.startsWith("%init."))
-      continue;
+    // `%caught.*` helpers test caught values, and `%init.*` is a module's top
+    // level: none is a function to name.
+    if (fn.tier !== "dynamic" || GENERATED.test(fn.name)) continue;
     if (isNodeModulesPath(fn.loc.file)) {
       const name = packageOf(fn.loc.file);
       packages.set(name, (packages.get(name) ?? 0) + 1);
@@ -300,9 +313,35 @@ export function scanDynamicTier(lowerer: Lowerer, module: IrModule): DynamicTier
   };
 }
 
-/** `%%m144.Session.finish` → `Session.finish`, `m163.find` → `find`. */
-function displayName(name: string): string {
-  return name.replace(/^%+/, "").replace(/^m\d+\./, "");
+/** The name the source binds a function expression to (`totals` in
+ * `const totals = (rows) => ...`), or null. */
+function boundName(lowerer: Lowerer, loc: SrcLoc): string | null {
+  const sf = lowerer.program.getSourceFile(loc.file);
+  if (sf === undefined) return null;
+  const parent = nodeAt(sf, loc.start, loc.end).parent;
+  if (
+    parent !== undefined &&
+    (ts.isVariableDeclaration(parent) ||
+      ts.isPropertyAssignment(parent) ||
+      ts.isPropertyDeclaration(parent)) &&
+    ts.isIdentifier(parent.name)
+  )
+    return parent.name.text;
+  return null;
+}
+
+/** A function's name as the source spells it, without a module prefix
+ * (`%%m144.Session.finish` → `Session.finish`, `m163.find` → `find`). A
+ * function expression (`%fn<n>`) takes its own name, else the name it is
+ * bound to, else `anonymous function`. */
+function displayName(lowerer: Lowerer, f: HotDynamicFunction): string {
+  const plain = f.name
+    .replace(/^%+/, "")
+    .replace(/^m\d+\./, "")
+    .replace(/^%+/, "");
+  const lambda = /^fn\d+(?:_(.+))?$/.exec(plain);
+  if (lambda === null) return plain;
+  return lambda[1] ?? boundName(lowerer, f.loc) ?? "anonymous function";
 }
 
 /** SC6101 for the program lowering produced, or null (see the header).
@@ -322,7 +361,7 @@ export function dynamicTierWarning(
   };
   return dynamicTierDiag(
     report.hot.map((f) => ({
-      name: displayName(f.name),
+      name: displayName(lowerer, f),
       where: where(f.loc),
       cause: f.cause,
       hot: f.hot,

tests/coverage-fixtures/dynamic-tier/function-names.js

@@ -0,0 +1,32 @@
+// Function expressions in an untyped JavaScript file: SC6101 names each one
+// by its own name, else by the class field, property or variable it is
+// assigned to, else as an anonymous function.
+class Totals {
+  sum = (rows) => {
+    let s = 0;
+    for (let i = 0; i < rows.length; i++) s += rows[i].n;
+    return s;
+  };
+}
+
+const ops = {
+  count: (rows) => {
+    let c = 0;
+    for (let i = 0; i < rows.length; i++) if (rows[i].n > 1) c++;
+    return c;
+  },
+};
+
+const named = function scale(rows) {
+  let s = 0;
+  for (let i = 0; i < rows.length; i++) s += rows[i].n * 2;
+  return s;
+};
+
+const rows = [{ n: 1 }, { n: 2 }];
+const results = [rows].map((list) => {
+  let s = 0;
+  for (let i = 0; i < list.length; i++) s += list[i].n;
+  return s;
+});
+console.log(new Totals().sum(rows), ops.count(rows), named(rows), results[0]);

tests/coverage-fixtures/dynamic-tier/typed-hot.ts

@@ -1,5 +1,7 @@
-// TypeScript functions that loop over untyped values: an `any` parameter
-// and an untyped JSON.parse result (SC6101 names both).
+// TypeScript functions that loop over untyped values: an `any` parameter,
+// an untyped JSON.parse result and an arrow function's `any` rows (SC6101
+// names all three, the arrow by the name it is bound to). A cast to an
+// alias of `any` blames the untyped value it produces, not the alias.
 function regionTotals(items: any): Map<string, number> {
   const out = new Map<string, number>();
   for (const item of items) out.set(item.region, (out.get(item.region) ?? 0) + item.amount);
@@ -13,9 +15,23 @@ function countRegions(text: string): number {
   return n;
 }
 
+const weights = (rows: any): number => {
+  let sum = 0;
+  for (let i = 0; i < rows.length; i++) sum += rows[i].amount;
+  return sum;
+};
+
+type Loose = any;
+
+function entryCount(value: unknown): number {
+  let n = 0;
+  for (const key of value as Loose) if (key !== "") n++;
+  return n;
+}
+
 const rows = [
   { amount: 3, region: "eu" },
   { amount: 4, region: "us" },
 ];
 for (const [region, total] of regionTotals(rows)) console.log(region, total);
-console.log(countRegions(JSON.stringify(rows)));
+console.log(countRegions(JSON.stringify(rows)), weights(rows), entryCount(["a", ""]));

tests/coverage-fixtures/dynamic-tier/unnamed.ts

@@ -0,0 +1,41 @@
+// Dynamic loops that SC6101 does not name. `readEntries` loops over
+// `unknown` values: the type name `Record` in its cast is not an untyped
+// value. `readAll` catches into `Error` while a subclass of `Error` crosses
+// into an `any` value, so its catch clause calls a generated check helper,
+// which is not a source function.
+function parse(text: string): Record<string, unknown> | null {
+  const value: unknown = JSON.parse(text);
+  return value !== null && typeof value === "object" ? (value as Record<string, unknown>) : null;
+}
+
+function readEntries(text: string): Map<string, string> {
+  const out = new Map<string, string>();
+  const entries = parse(text)?.["entries"];
+  if (entries === null || typeof entries !== "object") return out;
+  for (const [name, raw] of Object.entries(entries as Record<string, unknown>)) {
+    if (raw === null || typeof raw !== "object") continue;
+    const e = raw as { dir?: unknown; label?: unknown };
+    if (typeof e.dir !== "string") continue;
+    out.set(name, typeof e.label === "string" ? `${e.dir}:${e.label}` : e.dir);
+  }
+  return out;
+}
+
+class ReadError extends Error {}
+
+function readAll(texts: string[]): string[] {
+  const out: string[] = [];
+  for (const text of texts) {
+    try {
+      if (text === "") throw new ReadError("empty");
+      out.push(text);
+    } catch (e) {
+      out.push(`cannot read: ${(e as Error).message}`);
+    }
+  }
+  return out;
+}
+
+const box: any = { error: new ReadError("boxed") };
+console.log(readEntries('{"entries": {"a": {"dir": "x", "label": "y"}}}').size);
+console.log(readAll(["one", ""]).join(", "), box.error.message);

tests/harness/coverage.test.ts

@@ -133,9 +133,11 @@ test("likely-hot functions with untyped values report as a performance warning",
 test("the performance warning names the untyped value of each function", () => {
   const { coverage } = analyze(fixture("dynamic-tier/typed-hot.ts"));
   expect(coverage.performance?.[0]?.message.split("\n")).toEqual([
-    "2 functions that likely run often use untyped values, which runs slower than typed code:",
-    "  regionTotals (typed-hot.ts:3): `items` is `any`, in a loop",
-    "  countRegions (typed-hot.ts:9): `parsed` holds an untyped `JSON.parse` result, in a loop",
+    "4 functions that likely run often use untyped values, which runs slower than typed code:",
+    "  regionTotals (typed-hot.ts:5): `items` is `any`, in a loop",
+    "  entryCount (typed-hot.ts:26): `key` is `any`, in a loop",
+    "  countRegions (typed-hot.ts:11): `parsed` holds an untyped `JSON.parse` result, in a loop",
+    "  weights (typed-hot.ts:18): `rows` is `any`, in a loop",
   ]);
 });
 
@@ -146,6 +148,22 @@ test("untyped setup code that runs once reports no performance warning", () => {
   expect(coveragePasses(coverage, "dynamic")).toBe(true);
 });
 
+test("the performance warning names function expressions as the source does", () => {
+  const { coverage } = analyze(fixture("dynamic-tier/function-names.js"));
+  expect(coverage.performance?.[0]?.message.split("\n").slice(1)).toEqual([
+    "  sum (function-names.js:5): untyped JavaScript file, in a loop",
+    "  count (function-names.js:13): untyped JavaScript file, in a loop",
+    "  scale (function-names.js:20): untyped JavaScript file, in a loop",
+    "  anonymous function (function-names.js:27): untyped JavaScript file, in a loop",
+  ]);
+});
+
+test("the performance warning names neither type names nor generated helpers", () => {
+  const { coverage } = analyze(fixture("dynamic-tier/unnamed.ts"));
+  expect(coverage.diagnostics).toEqual([]);
+  expect(coverage.performance).toBeUndefined();
+});
+
 test("dynamic-tier code in npm packages is summarized by package", () => {
   const { coverage } = analyze(fixture("dynamic-tier/package/main.ts"));
   expect(coverage.performance?.[0]?.message.split("\n")).toEqual([