diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index b686507e5..df93899c9 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -11,12 +11,14 @@ import { renamedSince, } from './committed-source-tree.ts'; import { + addedModules, APPROVED_OVER_CEILING, classifyGrowth, classifyNewEntry, describeClosureGrowth, describeClosurePressure, describePlatformOffenders, + describeSharedGrowthHomes, discoverFacadeEntryFiles, eagerClosureEntries, entryCategoryOf, @@ -71,6 +73,40 @@ test('no-growth fails growth with both counts and passes an equal or smaller clo expect(classifyGrowth('x.ts', 42, 43)).toMatch(/evaluates 43 modules.*merge-base evaluated 42/); }); +test('growth advice names both causes and both remedies, not one prescribed fix', () => { + // #2423's review: a message that only ever says "move it behind a dynamic import" is wrong + // advice when the growth is a new module that belongs in an existing one. This pins that the + // verdict states both common causes and leaves the remedy to the reader. + const finding = classifyGrowth('x.ts', 42, 43) ?? ''; + expect(finding).toMatch(/new static edge/); + expect(finding).toMatch(/used to load on demand/); + expect(finding).toMatch(/home in a module the closure already evaluates/); + expect(finding).toMatch(/function-scoped `await import`/); +}); + +test('growth against the merge-base lists every added module, each with its shortest route, bounded', () => { + // The hole: the old diagnostic named only the FIRST added module, which hid the pattern when a + // whole PR's growth was really "everyone reached one new module" (#2423 review) rather than one + // entry's own new edge. This pins that every added module is named (bounded), not just the + // first, and that the route shown is the whole chain, not just the leaf. + const entry = '/repo/entry.ts'; + const graph = new Map([[entry, null]]); + graph.set('/repo/old.ts', entry); // present at the merge-base too: not "added" + graph.set('/repo/new-a.ts', entry); + graph.set('/repo/new-b.ts', '/repo/new-a.ts'); + for (let index = 0; index < 12; index += 1) graph.set(`/repo/new-filler-${index}.ts`, entry); + const base = new Set(['/repo/entry.ts', '/repo/old.ts']); + + const described = describeClosureGrowth(graph, base, entry, '/repo'); + expect(described, 'the route must show the whole chain, not just the leaf').toContain( + 'new-a.ts → new-b.ts', + ); + expect(described, 'output must be capped and say how much it omitted').toMatch( + /\(\+\d+ more newly evaluated module\(s\)\)/, + ); + expect(described.split('\n').length).toBeLessThan(15); +}); + test('a first-introduced entry fits its category ceiling or carries an approval', () => { expect(classifyNewEntry('x.ts', 'vocabulary-facade', 4, false)).toBeNull(); expect(classifyNewEntry('x.ts', 'vocabulary-facade', 5, false)).toMatch( @@ -179,6 +215,146 @@ test('an entry that evaluates only itself is described without pretending to an expect(describeClosurePressure(graph, '/repo/solo.ts', '/repo')).toContain('only itself'); }); +test('growth shared by two entries names the merge-base modules they already have in common', () => { + // The aggregation #2423's review asked for: two entries independently grow by the SAME new + // module. Per-entry diagnostics would each show that same chain with no hint that a shared + // existing home was available -- this names the modules BOTH entries already evaluate, scoped + // to the added module's own package, once. + const addedFile = '/repo/packages/demo/src/new-thing.ts'; + const sharedHome = '/repo/packages/demo/src/shared-home.ts'; + const otherPkgFile = '/repo/packages/other/src/not-a-home.ts'; // different package: excluded + const onlyInOne = '/repo/packages/demo/src/only-in-a.ts'; // not common to both: excluded + + const graphA = new Map([ + ['/repo/entry-a.ts', null], + [sharedHome, '/repo/entry-a.ts'], + [onlyInOne, '/repo/entry-a.ts'], + [otherPkgFile, '/repo/entry-a.ts'], + ]); + const graphB = new Map([ + ['/repo/entry-b.ts', null], + [sharedHome, '/repo/entry-b.ts'], + [otherPkgFile, '/repo/entry-b.ts'], + ]); + + const shared = describeSharedGrowthHomes( + [ + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size + 1 }, + { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, headClosureSize: graphB.size + 1 }, + ], + '/repo', + ); + + expect(shared, 'labeled neutrally, not as a prescribed fix').toMatch(/possible homes/i); + expect(shared).toContain('packages/demo/src/shared-home.ts'); + expect(shared, 'only in one entry: not a shared home').not.toContain('only-in-a.ts'); + expect(shared, 'a different package than the added module: not a candidate home').not.toContain( + 'not-a-home.ts', + ); +}); + +test('growth that no other entry shares produces no shared-homes note', () => { + const graphA = new Map([['/repo/entry-a.ts', null]]); + expect( + describeSharedGrowthHomes( + [{ id: 'entry-a.ts', added: ['/repo/new.ts'], baseGraph: graphA, headClosureSize: 2 }], + '/repo', + ), + 'nothing to aggregate when only one entry grew by this module', + ).toBeNull(); +}); + +test('an equal-size replacement is not growth, so the aggregate stays silent', () => { + // #2471's review: the aggregate used to take every entry with a newly evaluated module, which is + // not the condition the per-entry rule applies. Both entries below swapped one module for + // another -- same closure size, one module the merge-base did not evaluate, and the SAME one, so + // the grouping would fire. `classifyGrowth` passes both, so this must print nothing at all. + const addedFile = '/repo/packages/demo/src/new-thing.ts'; + const sharedHome = '/repo/packages/demo/src/shared-home.ts'; + const graphA = new Map([ + ['/repo/entry-a.ts', null], + [sharedHome, '/repo/entry-a.ts'], + ['/repo/packages/demo/src/dropped-by-a.ts', '/repo/entry-a.ts'], + ]); + const graphB = new Map([ + ['/repo/entry-b.ts', null], + [sharedHome, '/repo/entry-b.ts'], + ['/repo/packages/demo/src/dropped-by-b.ts', '/repo/entry-b.ts'], + ]); + + expect(classifyGrowth('entry-a.ts', graphA.size, graphA.size)).toBeNull(); + expect(classifyGrowth('entry-b.ts', graphB.size, graphB.size)).toBeNull(); + expect( + describeSharedGrowthHomes( + [ + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size }, + { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, headClosureSize: graphB.size }, + ], + '/repo', + ), + 'a swap at equal size is no violation: the aggregate may not fail where the rule passes', + ).toBeNull(); + expect( + describeSharedGrowthHomes( + [ + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size }, + { + id: 'entry-b.ts', + added: [addedFile], + baseGraph: graphB, + headClosureSize: graphB.size - 1, + }, + ], + '/repo', + ), + 'a closure that shrank while adding a module is likewise not growth', + ).toBeNull(); +}); + +test('disjoint growth groups keep their own entries and homes in separate blocks', () => { + // #2471's review: candidate homes from separate added-module groups were unioned into one list + // labelled as common to every failing entry, which is false as soon as two groups exist. Both + // added modules live in the same package here, so only the grouping -- not the package scope -- + // can keep them apart: new-x.ts grew a1/a2, new-y.ts grew b1/b2, and no closure is shared. + const addedX = '/repo/packages/demo/src/new-x.ts'; + const addedY = '/repo/packages/demo/src/new-y.ts'; + const homeX = '/repo/packages/demo/src/home-x.ts'; + const homeY = '/repo/packages/demo/src/home-y.ts'; + const growth = (id: string, added: string, home: string) => { + const entryPath = `/repo/${id}`; + const baseGraph = new Map([ + [entryPath, null], + [home, entryPath], + ]); + return { id, added: [added], baseGraph, headClosureSize: baseGraph.size + 1 }; + }; + + const shared = describeSharedGrowthHomes( + [ + growth('entry-a1.ts', addedX, homeX), + growth('entry-a2.ts', addedX, homeX), + growth('entry-b1.ts', addedY, homeY), + growth('entry-b2.ts', addedY, homeY), + ], + '/repo', + ); + + const blocks = (shared ?? '').split('\n\n'); + expect(blocks, 'one block per added module, never one merged list').toHaveLength(2); + const blockX = blocks.find((block) => block.startsWith('packages/demo/src/new-x.ts')) ?? ''; + const blockY = blocks.find((block) => block.startsWith('packages/demo/src/new-y.ts')) ?? ''; + + expect(blockX, 'each entry is named with how much it grew').toContain('entry-a1.ts (+1)'); + expect(blockX).toContain('entry-a2.ts (+1)'); + expect(blockX).toContain('packages/demo/src/home-x.ts'); + expect(blockX, 'b1/b2 did not grow by new-x.ts').not.toContain('entry-b1.ts'); + expect(blockX, 'no entry that grew by new-x.ts evaluates home-y.ts').not.toContain('home-y.ts'); + expect(blockY).toContain('entry-b1.ts (+1)'); + expect(blockY).toContain('packages/demo/src/home-y.ts'); + expect(blockY, 'a1/a2 did not grow by new-y.ts').not.toContain('entry-a1.ts'); + expect(blockY, 'no entry that grew by new-y.ts evaluates home-x.ts').not.toContain('home-x.ts'); +}); + /** * A committed fixture repository: one workspace package with a manifest export target, a * top-level façade, a nested façade, and a test source. Everything here is TRACKED, so anything a @@ -350,14 +526,14 @@ function basePathOf(entryFile: string): string | null { return baseTree.isFile(absolute(file)) ? file : null; } -const baseClosures = new Map>(); -function baseClosureOf(baseFile: string): ReadonlySet { - let closure = baseClosures.get(baseFile); - if (!closure) { - closure = new Set(eagerClosureGraphOf(absolute(baseFile), baseTree).keys()); - baseClosures.set(baseFile, closure); +const baseGraphs = new Map>(); +function baseGraphOf(baseFile: string): ReadonlyMap { + let graph = baseGraphs.get(baseFile); + if (!graph) { + graph = eagerClosureGraphOf(absolute(baseFile), baseTree); + baseGraphs.set(baseFile, graph); } - return closure; + return graph; } const platformFacades = entries.filter((entry) => entry.category === 'platform-facade'); @@ -368,6 +544,19 @@ const carried = others.flatMap((entry) => { }); const introduced = others.filter((entry) => basePathOf(entry.entryFile) === null); +/** + * Every carried entry's growth data, computed once so the per-entry NO-GROWTH test below and the + * cross-entry shared-homes note after it read the same graphs instead of walking each closure + * twice. + */ +const carriedGrowth = carried.map((entry) => { + const entryPath = absolute(entry.entryFile); + const graph = eagerClosureGraphOf(entryPath); + const baseGraph = baseGraphOf(entry.baseFile); + const base = new Set(baseGraph.keys()); + return { entry, entryPath, graph, baseGraph, base, added: addedModules(graph, base, entryPath) }; +}); + test('every hub exists and is not also a discovered façade', () => { const discovered = new Set(discoverFacadeEntryFiles(repoRoot)); expect(HUB_ENTRY_FILES.filter((file) => !fs.existsSync(absolute(file)))).toEqual([]); @@ -402,20 +591,42 @@ test.for(platformFacades)('$id evaluates exactly one module: itself', (entry) => ).toBe(PLATFORM_FACADE_CLOSURE); }); -test.for(carried)('$id evaluates no more modules than at the merge-base', (entry) => { - const entryPath = absolute(entry.entryFile); - const graph = eagerClosureGraphOf(entryPath); - const base = baseClosureOf(entry.baseFile); +test.for(carriedGrowth)('$entry.id evaluates no more modules than at the merge-base', (growth) => { + const { entry, entryPath, graph, base } = growth; const finding = classifyGrowth(entry.id, base.size, graph.size); expect( finding, finding === null ? '' - : `${finding}\n\nFirst newly evaluated module, by shortest import route:\n` + + : `${finding}\n\nNewly evaluated module(s), each by shortest import route from the entry:\n` + describeClosureGrowth(graph, base, entryPath, repoRoot), ).toBeNull(); }); +test('entries that grow by the same new module report shared merge-base homes once', () => { + // #2423's review: when several entries grow because they all reached the same new module, the + // per-entry test above already fails once per entry -- this adds ONE more diagnostic naming the + // modules they already have in common, instead of leaving a reader to notice the repetition + // across several separate failures by hand. Every carried entry is handed over with its head + // closure size, and the aggregation itself keeps only the ones the no-growth rule fails + // (#2471 review) -- so it can only fail alongside at least two already-red entries above and + // never changes the gate's own pass/fail verdict. + const growths = carriedGrowth.map((growth) => ({ + id: growth.entry.id, + added: growth.added, + baseGraph: growth.baseGraph, + headClosureSize: growth.graph.size, + })); + const shared = describeSharedGrowthHomes(growths, repoRoot); + expect( + shared, + shared === null + ? '' + : 'More than one entry grew by the same new module -- see the shared homes below instead ' + + `of chasing each entry's own diagnostic separately:\n\n${shared}`, + ).toBeNull(); +}); + test.for(introduced)( '$id is first-introduced and fits the $category ceiling or carries an approval', (entry) => { diff --git a/scripts/__tests__/eager-closure-budgets.ts b/scripts/__tests__/eager-closure-budgets.ts index 6f61185b1..337a4b6e5 100644 --- a/scripts/__tests__/eager-closure-budgets.ts +++ b/scripts/__tests__/eager-closure-budgets.ts @@ -187,13 +187,24 @@ export function eagerClosureEntries(repoRoot: string): EagerClosureEntry[] { ]; } -/** The no-growth verdict: `null` unless the head closure is larger than the merge-base one. */ +/** + * The no-growth verdict: `null` unless the head closure is larger than the merge-base one. + * + * The closing sentence deliberately does not prescribe one fix. #2423's review found that a + * generic "move it behind a dynamic import" sent five reviewers toward the wrong change: the + * growth there was a small new module that belonged in a module every affected entry already + * evaluated, not behind a lazy boundary. There are two common causes and two remedies, and which + * applies is exactly what the added-module listing this verdict is always printed alongside + * (`describeClosureGrowth`) is for. + */ export function classifyGrowth(id: string, base: number, head: number): string | null { if (head <= base) return null; return ( - `${id} evaluates ${head} modules on import; the merge-base evaluated ${base}. Something ` + - 'that used to load on demand now loads eagerly, or a new static edge was added: move it ' + - 'behind a function-scoped `await import`.' + `${id} evaluates ${head} modules on import; the merge-base evaluated ${base}. That means ` + + 'either a new static edge was added, or something that used to load on demand now loads ' + + 'eagerly. The fix is either to give the new code a home in a module the closure already ' + + 'evaluates, or to move the new edge behind a function-scoped `await import` -- see the ' + + 'added module(s) below for which one fits.' ); } @@ -358,10 +369,29 @@ export function describeClosurePressure( ); } +/** How many of `added` a growth diagnostic names individually before it just counts the rest. */ +const REPORTED_ADDED_MODULES = 10; + +/** Head-closure files absent from the merge-base closure -- what actually grew, entry excluded. */ +export function addedModules( + graph: ReadonlyMap, + baseClosure: ReadonlySet, + entryPath: string, +): string[] { + return [...graph.keys()].filter((file) => file !== entryPath && !baseClosure.has(file)); +} + /** - * Where an existing entry grew: the shortest import route to the first module the merge-base did - * not evaluate, plus how many more there are. The walk is breadth-first, so the first one in - * closure order is the shallowest, which is where the new edge almost always is. + * Every newly evaluated module against the merge-base (bounded to `REPORTED_ADDED_MODULES`), each + * with the shortest static import route from the entry down to it. + * + * #2423's review is why this names more than one module: the previous version of this diagnostic + * printed only the FIRST added module, which hid the pattern when the real growth was one small + * new module reached from several places. A reader saw one chain, read it as "this one edge + * should be lazy", and proposed a dynamic import for what was actually a shared constant -- five + * times, across five separate CI failures, because nothing in any one entry's message showed that + * the "new" modules were mostly the same one. Listing every added module (still bounded) lets a + * reader see that shape from a single failure. */ export function describeClosureGrowth( graph: ReadonlyMap, @@ -369,11 +399,13 @@ export function describeClosureGrowth( entryPath: string, repoRoot: string, ): string { - const added = [...graph.keys()].filter((file) => file !== entryPath && !baseClosure.has(file)); - const first = added[0]; - if (first === undefined) return ' (no module is new against the merge-base)'; - const more = added.length > 1 ? `\n (+${added.length - 1} more newly evaluated module(s))` : ''; - return ` ${formatImportChain(graph, first, repoRoot)}${more}`; + const added = addedModules(graph, baseClosure, entryPath); + if (added.length === 0) return ' (no module is new against the merge-base)'; + const shown = added.slice(0, REPORTED_ADDED_MODULES); + const lines = shown.map((file) => ` ${formatImportChainArrow(graph, file, repoRoot)}`); + const hidden = added.length - shown.length; + const more = hidden > 0 ? `\n (+${hidden} more newly evaluated module(s))` : ''; + return `${lines.join('\n')}${more}`; } /** @@ -400,24 +432,46 @@ export function describePlatformOffenders( } /** - * The import chain from a closure's entry down to `target`, rendered one edge per line. + * The hops from a closure's entry down to `target`, repo-relative, shallowest first. * * #1960 asks a violation to "name the offending edge chain". A sorted set of evaluated files names * the destination but not the route, which leaves the reader to rediscover by hand which import * actually pulled it in. `eagerClosureGraphOf` records each file's discoverer, so the route is * just a walk back up, and because that walk is breadth-first the route is the shortest one. */ -function formatImportChain( +function chainTo( graph: ReadonlyMap, target: string, repoRoot: string, -): string { +): string[] { const chain: string[] = []; for (let at: string | null | undefined = target; at != null; at = graph.get(at)) { chain.push(path.relative(repoRoot, at)); if (chain.length > 64) break; // defensive: a cycle would otherwise spin here } - return chain.reverse().join('\n -> '); + return chain.reverse(); +} + +/** The import chain from a closure's entry down to `target`, rendered one edge per line. */ +function formatImportChain( + graph: ReadonlyMap, + target: string, + repoRoot: string, +): string { + return chainTo(graph, target, repoRoot).join('\n -> '); +} + +/** + * The same route as `formatImportChain`, arrow-joined on one line -- compact enough to list + * several of them (`describeClosureGrowth`) without the multi-line chain format burying the list + * itself under indentation. + */ +function formatImportChainArrow( + graph: ReadonlyMap, + target: string, + repoRoot: string, +): string { + return chainTo(graph, target, repoRoot).join(' → '); } /** How many hops from the entry down to `target`, bounded so a cycle cannot spin. */ @@ -429,3 +483,210 @@ function chainLength(graph: ReadonlyMap, target: string): } return length; } + +/** A growing entry's contribution to the cross-entry shared-homes aggregation below. */ +export type GrowthForAggregation = { + /** The entry's label, matching `EagerClosureEntry.id`. */ + id: string; + /** This entry's added modules (absolute paths), as `addedModules` returns them. */ + added: readonly string[]; + /** This entry's merge-base closure graph -- gives both membership and forward edges. */ + baseGraph: ReadonlyMap; + /** + * This entry's head closure size, so the aggregation can apply the same condition + * `classifyGrowth` does: an entry only grew when this exceeds `baseGraph.size`. A closure that + * swapped one module for another, or shrank while adding one, has newly evaluated modules and + * no growth -- `classifyGrowth` passes it, so nothing here may report it (#2471 review). + */ + headClosureSize: number; +}; + +/** How many candidate homes one added-module block names before it just counts the rest. */ +const MAX_SHARED_HOMES = 30; + +/** How many added-module blocks the whole note prints before it just counts the rest. */ +const MAX_SHARED_GROUPS = 5; + +/** How many grown entries one block names before it just counts the rest. */ +const MAX_GROUP_ENTRIES = 8; + +/** + * One added module with the entries that grew by it and the merge-base modules exactly those + * entries share. Groups stay separate all the way to the message: a home common to one group's + * entries says nothing about another group's, so unioning them would label modules as common to + * entries that never evaluate them (#2471 review). + */ +type SharedGrowthGroup = { + addedModule: string; + entries: readonly GrowthForAggregation[]; + homes: readonly string[]; +}; + +/** How many modules an entry evaluates beyond its merge-base closure; `<= 0` is not growth. */ +function netGrowth(growth: GrowthForAggregation): number { + return growth.headClosureSize - growth.baseGraph.size; +} + +/** + * The workspace package (or the root `src/` mechanics tree) a repo-relative path lives under -- + * the scope `describeSharedGrowthHomes` searches for an existing home, since a candidate outside + * the added module's own package is not a home the added module could plausibly move into. + */ +function packageOf(relativeFile: string): string { + const match = /^(packages\/[^/]+)\//.exec(relativeFile); + return match ? match[1] : 'src'; +} + +/** True when nothing in `graph` records `candidate` as the direct importer of a same-package file. */ +function hasNoInPackageChild( + candidate: string, + pkg: string, + graph: ReadonlyMap, + repoRoot: string, +): boolean { + for (const [child, parent] of graph) { + if (parent === candidate && packageOf(path.relative(repoRoot, child)) === pkg) return false; + } + return true; +} + +/** + * Every entry that actually GREW, indexed by each of its added modules -- one entry may appear + * under several. Entries whose head closure is no larger than their merge-base one are dropped + * here, which is what keeps the aggregation from reporting a swap the per-entry rule passes. + */ +function growthsByAddedModule( + growths: readonly GrowthForAggregation[], +): Map { + const byAddedModule = new Map(); + for (const growth of growths) { + if (netGrowth(growth) <= 0) continue; + for (const added of growth.added) { + const group = byAddedModule.get(added); + if (group) group.push(growth); + else byAddedModule.set(added, [growth]); + } + } + return byAddedModule; +} + +/** + * Candidate homes with modules that have no in-package eager import of their own first -- + * a leaf module is the safer home, since adding a symbol to it cannot itself grow anyone else's + * closure -- then alphabetically by repo-relative path. + */ +function sortHomesLeavesFirst( + homes: readonly string[], + pkg: string, + graph: ReadonlyMap, + repoRoot: string, +): string[] { + return [...homes].sort((left, right) => { + const leftLeaf = hasNoInPackageChild(left, pkg, graph, repoRoot); + const rightLeaf = hasNoInPackageChild(right, pkg, graph, repoRoot); + if (leftLeaf !== rightLeaf) return leftLeaf ? -1 : 1; + return path.relative(repoRoot, left).localeCompare(path.relative(repoRoot, right)); + }); +} + +/** + * The merge-base modules every entry in `group` already evaluates, scoped to the added module's + * own package -- the candidate homes a shared symbol could plausibly move into, for this group + * only. + */ +function sharedHomesForGroup( + addedModule: string, + group: readonly GrowthForAggregation[], + repoRoot: string, +): string[] { + const [first, ...rest] = group; + if (!first) return []; + const pkg = packageOf(path.relative(repoRoot, addedModule)); + const homes = [...first.baseGraph.keys()].filter( + (candidate) => + packageOf(path.relative(repoRoot, candidate)) === pkg && + rest.every((other) => other.baseGraph.has(candidate)), + ); + return sortHomesLeavesFirst(homes, pkg, first.baseGraph, repoRoot); +} + +/** Every added module two or more grown entries share and that has at least one candidate home. */ +function sharedGrowthGroups( + growths: readonly GrowthForAggregation[], + repoRoot: string, +): SharedGrowthGroup[] { + const groups: SharedGrowthGroup[] = []; + for (const [addedModule, entries] of growthsByAddedModule(growths)) { + if (entries.length < 2) continue; + const homes = sharedHomesForGroup(addedModule, entries, repoRoot); + if (homes.length > 0) groups.push({ addedModule, entries, homes }); + } + return groups; +} + +/** The grown entries of one group with how much each grew, bounded to `MAX_GROUP_ENTRIES`. */ +function formatGrownEntries(entries: readonly GrowthForAggregation[]): string { + const shown = entries + .slice(0, MAX_GROUP_ENTRIES) + .map((entry) => `${entry.id} (+${netGrowth(entry)})`); + const hidden = entries.length - shown.length; + const more = hidden > 0 ? `, and ${hidden} more entry(ies)` : ''; + return `${shown.join(', ')}${more}`; +} + +/** One group's candidate homes, one per indented line, bounded to `MAX_SHARED_HOMES`. */ +function formatCandidateHomes(homes: readonly string[], repoRoot: string): string { + const shown = homes.slice(0, MAX_SHARED_HOMES); + const hidden = homes.length - shown.length; + const more = hidden > 0 ? `\n (+${hidden} more)` : ''; + return `${shown.map((file) => ` ${path.relative(repoRoot, file)}`).join('\n')}${more}`; +} + +/** One added module's block: what grew by it, and the homes those same entries already evaluate. */ +function formatSharedGrowthGroup(group: SharedGrowthGroup, repoRoot: string): string { + const added = path.relative(repoRoot, group.addedModule); + return ( + `${added} -- newly evaluated by ${formatGrownEntries(group.entries)}\n` + + `Entries that grew by ${added} already evaluate these modules at the merge-base (possible ` + + `homes for a shared symbol; not a statement of ownership):\n` + + formatCandidateHomes(group.homes, repoRoot) + ); +} + +/** + * When two or more entries grow by the SAME newly-added module, the per-entry diagnostic above + * cannot show the shape that actually matters: there is no old edge to make lazy, because the + * module is brand new, so "move it behind a dynamic import" is not even coherent advice. The + * useful question is "where does this already have a home", and the answer is scoped to what + * every affected entry already evaluates, under the added module's own package -- a real + * candidate list instead of "somewhere in the repo". + * + * #2423's review: five reviewers each independently proposed a dynamic import for a constant that + * a module already imported eagerly wherever it was needed; the fix was moving the constant + * there. Nothing in any one entry's own message could show that shape, because each entry's + * diagnostic only ever describes that one entry's own closure. This runs once, after every entry + * has been evaluated, and is silent (`null`) unless at least two entries that actually GREW share + * an added module and their merge-base intersection is non-empty. + * + * One block per added module (#2471 review). Two properties the shape has to keep: an entry whose + * closure did not grow contributes nothing however many modules are new to it, and a home is only + * ever printed under the added module whose grown entries all evaluate it -- never unioned across + * groups and labelled as common to every failing entry. + * + * Homes are sorted with modules that have no in-package eager import of their own first -- it is + * cheap here, since a candidate's forward edges are read straight off the base graph that + * produced it, no extra tree read required -- because a leaf module is the safer home: adding a + * symbol to it cannot itself grow anyone else's closure. + */ +export function describeSharedGrowthHomes( + growths: readonly GrowthForAggregation[], + repoRoot: string, +): string | null { + const groups = sharedGrowthGroups(growths, repoRoot); + if (groups.length === 0) return null; + const shown = groups.slice(0, MAX_SHARED_GROUPS); + const hidden = groups.length - shown.length; + const blocks = shown.map((group) => formatSharedGrowthGroup(group, repoRoot)); + if (hidden > 0) blocks.push(`(+${hidden} more added module(s) shared by two or more entries)`); + return blocks.join('\n\n'); +}