Feat(cli): Attribute a named export list hunk to its names (#1758)

Author: daniboomerang

.changeset/selector-named-exports.md

@@ -0,0 +1,5 @@
+---
+'@attalabs/vinaya': patch
+---
+
+The pre-push test selection attributes a change inside a named export list (`export { a, b as c } from '…'`) to the names on the changed lines, so adding one name to a package barrel no longer selects every test that reaches the barrel. A change to the list's braces, its `from` clause, a one-line list or an `export *` still makes the whole file affected.

apps/cli/specs/self-hosting.md

@@ -108,7 +108,7 @@ An ordinary modification selects none of them. `apps/cli/tests/lib/pre-push-scan
 
 **Resolution is the TypeScript compiler's, not a text scan.** A regular expression can read the specifier `'./demo.js'`; it cannot know the file on disk is `demo.ts`. This repository writes 482 of its 1275 relative import specifiers with the extension the EMIT will have, which is what TypeScript's own ESM guidance says to do — and every one of those edges was absent from the regex graph, across 144 importer files and 104 distinct targets. The visible symptom: a change to `apps/cli/src/commands/demo.ts` selected no test at all, though `apps/cli/tests/demo.test.ts` imports it by name. A selector that drops edges does not over-select; it silently under-selects, and its reported reductions are lost edges wearing precision's clothes. `lib/ts-module-graph.ts` now resolves every specifier with `ts.resolveModuleName` and follows each imported name to its defining file along the checker's own alias chain, hop by hop, so a re-export chain and a cross-package barrel both resolve to the file that actually defines the name. The compiler is resolved at runtime from the repository under analysis (`createRequire`), never bundled into the published artifact and never added to an adopter's install; a repository without it falls back to the text-scan graph, which is a strict over-approximation.
 
-**Selection follows the changed exported NAMES, not the changed file.** `lib/changed-names.ts` attributes each diff hunk to the top-level declarations it touches — on both sides of the diff, so a deleted declaration is attributed too — and closes that set backwards over intra-file references, since a change to a local helper affects every exported declaration that uses it. A test is selected when its import closure reaches a USE of an affected name. Any hunk that cannot be attributed to exactly one declaration — module-scope code, an `import`/`export` statement, an `export *`, `export default`, a destructuring binding, an unparseable region, a range outside every declaration — makes the whole file affected, which is the file-level answer; so is a changed file whose names were never computed. Narrowing is something the selector has to earn, every time.
+**Selection follows the changed exported NAMES, not the changed file.** `lib/changed-names.ts` attributes each diff hunk to the top-level declarations it touches — on both sides of the diff, so a deleted declaration is attributed too — and closes that set backwards over intra-file references, since a change to a local helper affects every exported declaration that uses it. A test is selected when its import closure reaches a USE of an affected name. A hunk inside a named export list — `export { a, b as c } from '…'` or `export { a, b }` — is attributed to the specifiers written on its lines, by the name each EXPORTS (`c`, the `as` alias, not `b`), so a one-name addition to a package barrel selects only the tests that import that name, which before the change is none; a local list's exported name is also affected whenever the declaration it names is. Only the lines strictly between the list's braces narrow: a hunk on the `export {` line, the `}` line, the `from` clause or the module specifier re-points every name the statement exports, and so does a one-line list, whose single line holds all of them. Any other hunk that cannot be attributed to exactly one declaration — module-scope code, an `import` statement, an `export` statement outside a named list's specifier lines, an `export *`, `export default`, a destructuring binding, an unparseable region, a range outside every declaration — makes the whole file affected, which is the file-level answer; so is a changed file whose names were never computed. Narrowing is something the selector has to earn, every time.
 
 A bare workspace-package import is not treated as a dependency on the whole package: the imported NAMES are resolved through the target package's declared entrypoint (`exports`/`main`, handed to the compiler as `paths` so the answer does not depend on the install layout) and its named, aliased, star and cross-package re-export graph, to the files that actually define them. Every shape the compiler cannot prove keeps the whole-package edge — a namespace, default, side-effect or dynamic import, an `export *` supplying one name from two sources, `import x = require(...)`, an alias chain that does not terminate in a declaration, or a package whose entrypoint cannot be derived from its manifest — so the refinement can over-select but never omits a truly affected test.
 

apps/cli/src/lib/changed-names.ts

@@ -29,15 +29,29 @@
  * over-approximates (a shadowing local of the same name creates an edge that
  * isn't real) — the safe direction.
  *
+ * ## A named export list is attributed line by line
+ *
+ * A hunk inside `export { a, b as c } from '…'` or `export { a, b }` is
+ * attributed to the specifiers written on its lines, by the name each one
+ * EXPORTS (`c`, not `b`) — the name a test imports. That is what keeps a
+ * one-name addition to a package barrel from selecting every test that reaches
+ * the barrel. Only the lines strictly between the list's braces narrow: a hunk
+ * on the `export {` line, the `}` line, the `from` clause or the module
+ * specifier re-points every name the statement exports, so it widens, as does a
+ * line inside the braces that holds no specifier. A local list's exported names
+ * join the intra-file closure through the declarations they name, so a change
+ * to `b` above also affects `c`.
+ *
  * ## When it gives up, it gives up completely
  *
  * {@link ALL_NAMES} — the whole file is affected — is the answer for every hunk
  * that cannot be attributed to exactly one declaration: module-scope executable
- * code, an `import` or `export` statement (including `export *`, which can
- * supply any name, and `export default`), a destructuring or computed binding,
- * a region that parses to nothing, and any range falling outside every
- * declaration. This is never a narrowing: it is the file-level answer, which is
- * what the selector does when it is told nothing at all.
+ * code, an `import` statement, an `export` statement outside a named export
+ * list's specifier lines (including `export *`, which can supply any name, and
+ * `export default`), a destructuring or computed binding, a region that parses
+ * to nothing, and any range falling outside every declaration. This is never a
+ * narrowing: it is the file-level answer, which is what the selector does when
+ * it is told nothing at all.
  */
 import type { TypeScriptApi } from './ts-module-graph.js'
 
@@ -81,8 +95,27 @@ function namesForRanges(
   const sourceFile = ts.createSourceFile(fileName, source, ts.ScriptTarget.ES2022, true)
   const lineOf = (pos: number): number => sourceFile.getLineAndCharacterOfPosition(pos).line + 1
 
-  /** Every top-level declaration, with the line span it owns and the name it declares. A `null` name is a statement no hunk can be attributed to. */
-  const declarations: Array<{ name: string | null; start: number; end: number; node: import('typescript').Node }> = []
+  /**
+   * A named export list's own layout: the lines of its `{` and `}`, and each
+   * specifier's exported name, the local name it re-exports when the statement
+   * has no module specifier, and its line span.
+   */
+  type ExportList = {
+    open: number
+    close: number
+    specifiers: Array<{ exported: string; local: string | null; start: number; end: number }>
+  }
+  /**
+   * Every top-level declaration, with the line span it owns and the name it declares. A `null` name is a statement no
+   * hunk can be attributed to — unless it carries an `exportList`, whose specifier lines are attributed one by one.
+   */
+  const declarations: Array<{
+    name: string | null
+    start: number
+    end: number
+    node: import('typescript').Node
+    exportList?: ExportList
+  }> = []
   /**
    * Where a declaration's OWN text begins: its first leading comment when it has
    * one (a doc comment is part of the declaration for attribution — editing it
@@ -109,6 +142,28 @@ function namesForRanges(
       }
       continue
     }
+    // `export { … }` and `export { … } from '…'`; an `export *` (with or
+    // without `as ns`) has no list and stays unattributable.
+    if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) {
+      const clause = statement.exportClause
+      const local = statement.moduleSpecifier === undefined
+      declarations.push({
+        name: null,
+        ...span,
+        node: statement,
+        exportList: {
+          open: lineOf(clause.getStart(sourceFile)),
+          close: lineOf(clause.getEnd()),
+          specifiers: clause.elements.map((element) => ({
+            exported: element.name.text,
+            local: local ? (element.propertyName ?? element.name).text : null,
+            start: lineOf(element.getStart(sourceFile)),
+            end: lineOf(element.getEnd())
+          }))
+        }
+      })
+      continue
+    }
     const named =
       ts.isFunctionDeclaration(statement) ||
       ts.isClassDeclaration(statement) ||
@@ -133,6 +188,18 @@ function namesForRanges(
     // can follow — either way the file is wholly affected.
     if (overlapping.length === 0) return null
     for (const d of overlapping) {
+      if (d.exportList) {
+        const list = d.exportList
+        for (let line = Math.max(range.start, d.start); line <= Math.min(range.end, d.end); line++) {
+          // The braces' own lines, and everything outside them — a leading
+          // comment, `export {`, `} from '…'` — re-point every name.
+          if (line <= list.open || line >= list.close) return null
+          const onLine = list.specifiers.filter((s) => s.start <= line && line <= s.end)
+          if (onLine.length === 0) return null
+          for (const s of onLine) seeded.add(s.exported)
+        }
+        continue
+      }
       if (d.name === null) return null
       seeded.add(d.name)
     }
@@ -151,6 +218,16 @@ function namesForRanges(
     ts.forEachChild(d.node, visit)
     references.set(d.name, used)
   }
+  // A local export list's exported name is affected whenever the declaration
+  // it names is.
+  for (const d of declarations) {
+    for (const s of d.exportList?.specifiers ?? []) {
+      if (s.local === null || s.local === s.exported || !topLevelNames.has(s.local)) continue
+      const used = references.get(s.exported) ?? new Set<string>()
+      used.add(s.local)
+      references.set(s.exported, used)
+    }
+  }
   const affected = new Set(seeded)
   for (let grew = true; grew; ) {
     grew = false

apps/cli/tests/lib/test-selector.test.ts

@@ -22,6 +22,7 @@ import {
 import { cliSpawnEdgeOf } from '../../src/lib/cli-spawn-tests'
 import { scannedRootsOf } from '../../src/lib/repo-scanner-tests'
 import { loadTypeScript } from '../../src/lib/ts-module-graph'
+import { fixtureDir } from './test-temp-fixture'
 
 function fixtureRepo(): string {
   const root = mkdtempSync(join(tmpdir(), 'vinaya-selector-'))
@@ -403,8 +404,10 @@ describe('a second real CI failure in the SAME task-files-v1/1 change set (Issue
 type PkgSpec = { name: string; main?: string; exports?: unknown; test?: string; files: Record<string, string> }
 
 /** Writes a throwaway multi-package workspace to a temp dir and returns its root plus a `dir(name)` locator. */
-function mkWorkspace(pkgs: PkgSpec[]): { root: string; dir: (name: string) => string } {
-  const root = mkdtempSync(join(tmpdir(), 'vinaya-sym-'))
+function mkWorkspace(
+  pkgs: PkgSpec[],
+  root = mkdtempSync(join(tmpdir(), 'vinaya-sym-'))
+): { root: string; dir: (name: string) => string } {
   writeFileSync(join(root, 'package.json'), JSON.stringify({ workspaces: ['packages/*'] }))
   const dirs = new Map<string, string>()
   pkgs.forEach((p, i) => {
@@ -1542,6 +1545,71 @@ describe('changed-name attribution (O3)', () => {
     })
   }
 
+  const EXPORT_LISTS = [
+    'export {', // 1
+    '  alpha,', // 2
+    '  beta as renamed,', // 3
+    "} from './a.js'", // 4
+    "export { solo } from './b.js'", // 5
+    "export * from './c.js'", // 6
+    'function local() { return 1 }', // 7
+    'export {', // 8
+    '  local as publicLocal', // 9
+    '}', // 10
+    'export {', // 11
+    '  moved', // 12
+    '}', // 13
+    "  from './d.js'" // 14
+  ].join('\n')
+
+  it('a hunk on a specifier line of a named export list attributes to that exported name', () => {
+    expect(modified(EXPORT_LISTS, [{ start: 2, end: 2 }])).toEqual(new Set(['alpha']))
+  })
+
+  it('an aliased specifier attributes to the name it exports, not the one it re-exports', () => {
+    expect(modified(EXPORT_LISTS, [{ start: 3, end: 3 }])).toEqual(new Set(['renamed']))
+    expect(modified(EXPORT_LISTS, [{ start: 9, end: 9 }])).toEqual(new Set(['publicLocal']))
+  })
+
+  it('a hunk across several specifier lines attributes to each of them', () => {
+    expect(modified(EXPORT_LISTS, [{ start: 2, end: 3 }])).toEqual(new Set(['alpha', 'renamed']))
+  })
+
+  it('a local declaration exported under an alias affects the exported name too', () => {
+    expect(modified(EXPORT_LISTS, [{ start: 7, end: 7 }])).toEqual(new Set(['local', 'publicLocal']))
+  })
+
+  for (const [label, line] of [
+    ['the `export {` line', 1],
+    ['the `} from` line', 4],
+    ['a one-line list, whose line holds its braces and module specifier', 5],
+    ['an `export *` statement', 6],
+    ['a local list’s closing brace', 10],
+    ['a module specifier on its own line', 14]
+  ] as const) {
+    it(`a hunk on ${label} widens to the whole file`, () => {
+      expect(modified(EXPORT_LISTS, [{ start: line, end: line }])).toBe(ALL_NAMES)
+    })
+  }
+
+  it('a hunk spanning a specifier line and the statement’s brace widens to the whole file', () => {
+    expect(modified(EXPORT_LISTS, [{ start: 3, end: 4 }])).toBe(ALL_NAMES)
+  })
+
+  it('a specifier removed by the diff is attributed from the OLD side', () => {
+    const before = "export {\n  kept,\n  gone,\n} from './a.js'\n"
+    const after = "export {\n  kept,\n} from './a.js'\n"
+    expect(
+      affectedNamesForFile(ts, {
+        file: '/w/mod.ts',
+        after,
+        before,
+        afterRanges: [{ start: 3, end: 2 }],
+        beforeRanges: [{ start: 3, end: 3 }]
+      })
+    ).toEqual(new Set(['gone']))
+  })
+
   it('a declaration deleted by the diff is attributed from the OLD side', () => {
     const before = 'export function gone() { return 1 }\nexport function kept() { return 2 }\n'
     const after = 'export function kept() { return 2 }\n'
@@ -1638,6 +1706,57 @@ describe('selection follows the changed names, and widens to file level when it
     }
   })
 
+  it('a barrel that gains one name selects no test through the barrel, only what the diff adds and alwaysRun', () => {
+    const ts = loadTypeScript(REPO_ROOT) as NonNullable<ReturnType<typeof loadTypeScript>>
+    const before = "export {\n  hot,\n} from './hot.js'\nexport { cold } from './cold.js'\n"
+    const after = "export {\n  hot,\n  fresh,\n} from './hot.js'\nexport { cold } from './cold.js'\n"
+    const { root, dir } = mkWorkspace(
+      [
+        {
+          name: '@o2/lib',
+          main: INDEX_MAIN,
+          files: {
+            'src/index.ts': after,
+            'src/hot.ts': 'export function hot() { return 1 }\nexport function fresh() { return 3 }\n',
+            'src/cold.ts': 'export function cold() { return 2 }\n',
+            'src/index.test.ts': "import { hot, cold } from './index.js'\ntest('both', () => hot() + cold())\n"
+          }
+        },
+        {
+          name: '@o2/app',
+          files: {
+            'src/uses-hot.test.ts': "import { hot } from '@o2/lib'\ntest('hot', () => hot())\n",
+            'src/uses-cold.test.ts': "import { cold } from '@o2/lib'\ntest('cold', () => cold())\n",
+            'src/uses-fresh.test.ts': "import { fresh } from '@o2/lib'\ntest('fresh', () => fresh())\n",
+            'src/unrelated.test.ts': "test('unrelated', () => 1)\n"
+          }
+        }
+      ],
+      fixtureDir('vinaya-barrel-')
+    )
+    const index = join(dir('@o2/lib'), 'src/index.ts')
+    const added = join(dir('@o2/app'), 'src/uses-fresh.test.ts')
+    const names = affectedNamesForFile(ts, {
+      file: index,
+      after,
+      before,
+      afterRanges: [{ start: 3, end: 3 }],
+      beforeRanges: [{ start: 2, end: 1 }]
+    })
+    expect(names).toEqual(new Set(['fresh']))
+    const alwaysRun = 'packages/p1/src/unrelated.test.ts'
+    const selected = selectAffectedTestFiles(root, [index, added], {
+      affectedNames: new Map([[index, names]]),
+      alwaysRun: [alwaysRun]
+    }).selected
+    expect(new Set(selected)).toEqual(new Set([added, join(root, alwaysRun)]))
+    // The barrel is reached: at file level the same change selects every test through it.
+    const fileLevel = new Set(selectAffectedTestFiles(root, [index]).selected)
+    expect(fileLevel).toContain(join(dir('@o2/app'), 'src/uses-hot.test.ts'))
+    expect(fileLevel).toContain(join(dir('@o2/app'), 'src/uses-cold.test.ts'))
+    expect(fileLevel).toContain(join(dir('@o2/lib'), 'src/index.test.ts'))
+  })
+
   // Two whole-repository selections, each walking every workspace package and
   // parsing its sources — seconds of real work apiece on this repository's
   // current size, and several times that when the suite runs it alongside