From b85593e6bcc33f60e9696d10b3db15e14556b6bb Mon Sep 17 00:00:00 2001 From: vilano93 <244097767+vilano93@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:56:11 +0200 Subject: [PATCH] fix(pregate): parse Cargo manifests as TOML; follow workspace membership The line scan of Cargo.toml missed dependents (a false green) and picked the wrong workspace: - an inherited renamed dependency (`base = { workspace = true }` with `[workspace.dependencies] base = { package = "alpha", path = ... }`); - dotted keys (`alpha.path = "../a"`), `[dependencies.]` tables and target-specific tables, matched by key name only; - a crate the workspace excludes ran `cargo test -p` from the workspace root. Manifests are now parsed with Bun.TOML. A dependency resolves by path (relative to the manifest declaring it, or to the workspace root when inherited), else by package name. A crate runs from the workspace it is a member of under Cargo's rules (package.workspace, the nearest [workspace], members globs, exclude, literal members, path dependencies of members), else from its own directory. Nothing runs cargo. Co-Authored-By: Claude Opus 5.5 --- lib/pregate/checks/lanes.ts | 12 +-- lib/pregate/rust.ts | 207 ++++++++++++++++++++++-------------- test/pregate-rust.test.ts | 55 ++++++++++ 3 files changed, 190 insertions(+), 84 deletions(-) diff --git a/lib/pregate/checks/lanes.ts b/lib/pregate/checks/lanes.ts index 1029f40a2..04745a169 100644 --- a/lib/pregate/checks/lanes.ts +++ b/lib/pregate/checks/lanes.ts @@ -16,10 +16,10 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { baseInputs, sha256 } from '../context'; import { PREGATE_CONFIG_PATH } from '../config'; -import { isDeviceDart, readRepoFile } from '../import-graph'; +import { isDeviceDart, listRepoFiles, readRepoFile } from '../import-graph'; import { selectLanes, type Selection } from '../lane-selection'; import { launchRuntime } from '../runtime'; -import { cargoWorkspaceRoot, crateOf, type ManifestCache } from '../rust'; +import { cargoModel, type CargoModel } from '../rust'; import { adapterFor, readRange, supervise, type Counter, type Supervised } from '../supervise'; import { matchesAny } from '../../ship-policy'; import type { CheckDef, CheckResult, PregateContext, RemoteObligation } from '../types'; @@ -160,8 +160,7 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan const packages = new Map(); const seen = new Map(); const seenPackage = new Map(); - const manifests: ManifestCache = new Map(); - const read = (rel: string) => readRepoFile(ctx.repoRoot, rel); + let model: CargoModel | null = null; const pkgRunners = new Map>(); const rest: string[] = []; const noPubspec: string[] = []; @@ -171,9 +170,10 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan const under = (root: string, f: string) => (root ? f.slice(root.length + 1) : f); for (const f of files) { if (f.endsWith('.rs')) { - const crate = crateOf(read, f, manifests); + model ??= cargoModel(listRepoFiles(ctx.repoRoot), rel => readRepoFile(ctx.repoRoot, rel)); + const crate = model.crateOf(f); if (!crate) { unclaimed.push(f); continue; } - const cwd = cargoWorkspaceRoot(read, crate.root, manifests); + const cwd = crate.workspace; const key = `${cwd}\0${crate.name}`; if (!cargo.has(key)) cargo.set(key, { name: crate.name, cwd, files: [] }); cargo.get(key)!.files.push(under(cwd, f)); diff --git a/lib/pregate/rust.ts b/lib/pregate/rust.ts index 8aa399662..357e92d6b 100644 --- a/lib/pregate/rust.ts +++ b/lib/pregate/rust.ts @@ -5,43 +5,137 @@ * a `#[cfg(test)]` (in-source unit tests). Cargo runs tests per crate, never * per file, so the graph maps at crate level: every `.rs` file of a crate, * and its Cargo.toml, select that crate's tests and the tests of every crate - * that depends on it (transitively, through the manifests' dependency keys). - * Uncertainty widens: a renamed dependency counts under both names, and any - * cfg predicate naming `test` makes a file a test. + * that depends on it, transitively. + * Manifests are parsed as TOML (Bun.TOML, as lib/model-policy.ts does), so + * dotted keys, `[dependencies.]` tables, target-specific tables and + * `{ workspace = true }` inheritance read as Cargo reads them. A dependency + * resolves by `path` (relative to the manifest declaring it) to a tracked + * crate, else by package name (`package = ` or the key); a name match only + * widens. The workspace a crate runs in follows Cargo's membership rules: + * `package.workspace`, else the nearest `[workspace]` above it, when that + * workspace is the crate's own manifest, lists it in `members` or reaches it + * by a path dependency, and does not `exclude` it; otherwise the crate + * directory. Nothing here runs cargo: pregate reads files only. */ import * as path from 'node:path'; +import { matchesAny } from '../ship-policy'; -export interface CargoManifest { name: string | null; workspace: boolean; deps: string[] } -export interface Crate { root: string; name: string } +/** Reads a repo-relative file's text, or null when it is absent or unreadable. */ +export type ReadFile = (rel: string) => string | null; -const DEP_TABLE = /^(?:target\..+\.)?(?:dependencies|dev-dependencies|build-dependencies)$/; -const DEP_SUBTABLE = /^(?:target\..+\.)?(?:dependencies|dev-dependencies|build-dependencies)\.["']?([A-Za-z0-9_-]+)["']?$/; -const PACKAGE_KEY = /\bpackage\s*=\s*["']([A-Za-z0-9_-]+)["']/; +type Table = Record; +const isTable = (v: unknown): v is Table => typeof v === 'object' && v !== null && !Array.isArray(v); +const strings = (v: unknown): string[] => (Array.isArray(v) ? v.filter((x): x is string => typeof x === 'string') : []); +const DEP_TABLES = ['dependencies', 'dev-dependencies', 'build-dependencies'] as const; -/** A Cargo.toml's `[package]` name, whether it declares a `[workspace]`, and every dependency key and `package =` rename, in a line scan (no TOML parser; comments dropped). */ -export function parseCargoManifest(text: string): CargoManifest { - let section = ''; - let name: string | null = null; - let workspace = false; - const deps = new Set(); - for (const raw of text.replace(/^/, '').split('\n')) { - const line = raw.replace(/(^|\s)#.*$/, '$1').trim(); - const header = /^\[\[?\s*([^\]]+?)\s*\]\]?$/.exec(line); - if (header) { - section = header[1]!.replace(/\s+/g, ''); - if (section === 'workspace' || section.startsWith('workspace.')) workspace = true; - const sub = DEP_SUBTABLE.exec(section); - if (sub) deps.add(sub[1]!); - continue; +/** One dependency: the crate directory its `path` names (repo-relative, normalized) and the package name it asks for. */ +interface DepSpec { path: string | null; name: string } + +export interface Crate { + root: string; + name: string; + /** Where `cargo test -p ` runs: the root of the workspace it is a member of, else the crate directory. */ + workspace: string; + /** Roots of the tracked crates it depends on. */ + deps: string[]; +} + +export interface CargoModel { crates: Map; crateOf(file: string): Crate | null } + +interface Manifest { dir: string; toml: Table } + +const dirOf = (manifest: string) => (path.posix.dirname(manifest) === '.' ? '' : path.posix.dirname(manifest)); +/** `dir` joined with a manifest-relative path, normalized; null when it leaves the repository. */ +function joinRel(dir: string, rel: string): string | null { + const joined = path.posix.normalize(path.posix.join(dir || '.', rel.replace(/\\/g, '/'))).replace(/\/+$/, ''); + if (joined === '..' || joined.startsWith('../') || path.posix.isAbsolute(joined)) return null; + return joined === '.' ? '' : joined; +} +const inside = (dir: string, root: string) => root === '' || dir === root || dir.startsWith(`${root}/`); +const relTo = (dir: string, root: string) => (root === '' ? dir : dir === root ? '' : dir.slice(root.length + 1)); + +/** Every dependency of a manifest: `[*dependencies]` and `[target..*dependencies]`, `{ workspace = true }` read from `ws`'s `[workspace.dependencies]` (its paths relative to the workspace root). */ +function depSpecs(m: Manifest, ws: Manifest | null): DepSpec[] { + const tables: Table[] = []; + for (const key of DEP_TABLES) if (isTable(m.toml[key])) tables.push(m.toml[key] as Table); + if (isTable(m.toml.target)) for (const cfg of Object.values(m.toml.target)) if (isTable(cfg)) for (const key of DEP_TABLES) if (isTable(cfg[key])) tables.push(cfg[key] as Table); + const inherited = ws && isTable(ws.toml.workspace) && isTable(ws.toml.workspace.dependencies) ? ws.toml.workspace.dependencies : {}; + const out: DepSpec[] = []; + for (const table of tables) { + for (const [key, raw] of Object.entries(table)) { + const fromWorkspace = isTable(raw) && raw.workspace === true; + const spec = fromWorkspace ? inherited[key] : raw; + const renamed = [spec, raw].find(s => isTable(s) && typeof s.package === 'string') as Table | undefined; + const p = isTable(spec) && typeof spec.path === 'string' ? joinRel(fromWorkspace ? ws!.dir : m.dir, spec.path) : null; + out.push({ path: p, name: (renamed?.package as string | undefined) ?? key }); } - if (section === 'package') { const m = /^name\s*=\s*["']([A-Za-z0-9_-]+)["']/.exec(line); if (m) name = m[1]!; continue; } - if (DEP_TABLE.test(section)) { - const key = /^["']?([A-Za-z0-9_-]+)["']?\s*=/.exec(line); - if (key) deps.add(key[1]!); - } - if (DEP_TABLE.test(section) || DEP_SUBTABLE.test(section)) { const renamed = PACKAGE_KEY.exec(line); if (renamed) deps.add(renamed[1]!); } } - return { name, workspace, deps: [...deps] }; + return out; +} + +/** The tracked crates among `files` (the paths `git ls-files` reports), what each depends on, and the workspace each runs in. */ +export function cargoModel(files: readonly string[], read: ReadFile): CargoModel { + const manifests = new Map(); + for (const f of files) { + if (path.posix.basename(f) !== 'Cargo.toml') continue; + const text = read(f); + if (text === null) continue; + try { const toml = Bun.TOML.parse(text); if (isTable(toml)) manifests.set(dirOf(f), { dir: dirOf(f), toml }); } catch { /* an unparsable manifest names no crate */ } + } + const nameOf = (dir: string) => { const pkg = manifests.get(dir)?.toml.package; return isTable(pkg) && typeof pkg.name === 'string' ? pkg.name : null; }; + const roots = [...manifests.keys()].filter(d => nameOf(d) !== null); + const workspaces = [...manifests.values()].filter(m => isTable(m.toml.workspace)); + const byName = new Map(); + for (const r of roots) byName.set(nameOf(r)!, [...(byName.get(nameOf(r)!) ?? []), r]); + + // `package.workspace` names the workspace; otherwise Cargo consults only the nearest [workspace] at or above the crate. + const candidate = (root: string): Manifest | null => { + const pkg = manifests.get(root)!.toml.package as Table; + if (typeof pkg.workspace === 'string') { const w = manifests.get(joinRel(root, pkg.workspace) ?? '\0'); return w && isTable(w.toml.workspace) ? w : null; } + return workspaces.filter(w => inside(root, w.dir)).sort((a, b) => b.dir.length - a.dir.length)[0] ?? null; + }; + const memberCache = new Map>(); + const membersOf = (ws: Manifest): Set => { + const cached = memberCache.get(ws.dir); + if (cached) return cached; + const conf = ws.toml.workspace as Table; + const norm = (g: string) => joinRel('', g) ?? '\0'; + const globs = strings(conf.members).map(norm); + const exclude = strings(conf.exclude).map(norm); + const own = new Set(roots.filter(r => candidate(r)?.dir === ws.dir)); + const excluded = (r: string) => exclude.some(e => { const rel = relTo(r, ws.dir); return rel === e || rel.startsWith(`${e}/`); }); + // Cargo's rules: the root package and a literally listed member always belong; a glob match or a path dependency of a + // member inside the workspace belongs unless `exclude` names it or a directory above it. + const literal = (r: string) => globs.some(g => !/[*?[]/.test(g) && relTo(r, ws.dir) === g); + const members = new Set([...own].filter(r => r === ws.dir || literal(r) || (matchesAny(globs, relTo(r, ws.dir)) && !excluded(r)))); + memberCache.set(ws.dir, members); + const queue = [...members]; + while (queue.length) { + for (const dep of depSpecs(manifests.get(queue.shift()!)!, ws)) { + if (dep.path === null || members.has(dep.path) || !own.has(dep.path) || !inside(dep.path, ws.dir) || excluded(dep.path)) continue; + members.add(dep.path); + queue.push(dep.path); + } + } + return members; + }; + + // A path names one crate; without a path (or when it names no tracked crate) every crate of that package name is a candidate. + const resolve = (specs: DepSpec[]) => [...new Set(specs.flatMap(s => (s.path !== null && nameOf(s.path) !== null ? [s.path] : byName.get(s.name) ?? [])))]; + const crates = new Map(); + for (const root of roots) { + const ws = candidate(root); + const member = ws !== null && membersOf(ws).has(root) ? ws : null; + crates.set(root, { root, name: nameOf(root)!, workspace: member?.dir ?? root, deps: resolve(depSpecs(manifests.get(root)!, member)).filter(d => d !== root) }); + } + const crateOf = (file: string): Crate | null => { + for (let d = path.posix.dirname(file); ; d = path.posix.dirname(d)) { + const c = crates.get(d === '.' ? '' : d); + if (c) return c; + if (d === '.' || d === '/' || d === '') return null; + } + }; + return { crates, crateOf }; } /** Whether Rust source holds a `#[cfg(...)]` whose predicate names `test` outside a string (`cfg(test)`, `cfg(all(test, unix))`; never `cfg(feature = "test")`). */ @@ -50,40 +144,6 @@ export function hasCfgTest(source: string): boolean { return false; } -/** Reads a repo-relative file's text, or null when it is absent or unreadable. */ -export type ReadFile = (rel: string) => string | null; - -/** A memo of manifests by repo-relative directory (`''` for the repository root). */ -export type ManifestCache = Map; - -function manifestAt(read: ReadFile, dir: string, cache: ManifestCache): CargoManifest | null { - if (!cache.has(dir)) { const text = read(path.posix.join(dir, 'Cargo.toml')); cache.set(dir, text === null ? null : parseCargoManifest(text)); } - return cache.get(dir)!; -} - -function ancestors(file: string): string[] { - const out: string[] = []; - for (let d = path.posix.dirname(file); ; d = path.posix.dirname(d)) { - out.push(d === '.' ? '' : d); - if (d === '.' || d === '/' || d === '') return out; - } -} - -/** The crate owning `file`: the nearest directory at or above it whose Cargo.toml names a `[package]`; null outside any crate. */ -export function crateOf(read: ReadFile, file: string, cache: ManifestCache = new Map()): Crate | null { - for (const dir of ancestors(file)) { const m = manifestAt(read, dir, cache); if (m?.name) return { root: dir, name: m.name }; } - return null; -} - -/** Where `cargo test -p` runs for a crate: the nearest directory at or above its root whose Cargo.toml declares `[workspace]`, else the crate root. */ -export function cargoWorkspaceRoot(read: ReadFile, crateRoot: string, cache: ManifestCache = new Map()): string { - for (const dir of ancestors(path.posix.join(crateRoot, 'Cargo.toml'))) if (manifestAt(read, dir, cache)?.workspace) return dir; - return crateRoot; -} - -/** Whether a crate file is a test by its path alone: under `/tests/`. */ -function inTestsDir(crate: Crate, file: string): boolean { return file.startsWith(crate.root ? `${crate.root}/tests/` : 'tests/'); } - export interface RustGraph { tests: string[]; edges: Array<{ dependency: string; dependent: string; via: 'import' | 'convention' }>; @@ -96,29 +156,20 @@ export interface RustGraph { * (`convention`) and of every crate depending on it, transitively (`import`). */ export function rustEdges(files: readonly string[], read: ReadFile): RustGraph { - const cache: ManifestCache = new Map(); - const manifests = files.filter(f => path.posix.basename(f) === 'Cargo.toml'); - const crates = new Map(); - for (const f of manifests) { - const dir = path.posix.dirname(f) === '.' ? '' : path.posix.dirname(f); - const m = manifestAt(read, dir, cache); - if (m?.name) crates.set(dir, { root: dir, name: m.name, deps: m.deps }); - } + const { crates, crateOf } = cargoModel(files, read); const out: RustGraph = { tests: [], edges: [] }; if (!crates.size) return out; const testsOf = new Map([...crates.keys()].map(k => [k, []])); const manifestOf = (root: string) => (root ? `${root}/Cargo.toml` : 'Cargo.toml'); for (const f of files) { if (!f.endsWith('.rs')) continue; - const crate = crateOf(read, f, cache); - if (!crate || !crates.has(crate.root)) continue; + const crate = crateOf(f); + if (!crate) continue; out.edges.push({ dependency: f, dependent: manifestOf(crate.root), via: 'convention' }); - if (inTestsDir(crate, f) || hasCfgTest(read(f) ?? '')) { out.tests.push(f); testsOf.get(crate.root)!.push(f); } + if (f.startsWith(crate.root ? `${crate.root}/tests/` : 'tests/') || hasCfgTest(read(f) ?? '')) { out.tests.push(f); testsOf.get(crate.root)!.push(f); } } - const byName = new Map(); - for (const c of crates.values()) byName.set(c.name, [...(byName.get(c.name) ?? []), c.root]); const dependents = new Map>(); - for (const c of crates.values()) for (const d of c.deps) for (const root of byName.get(d) ?? []) if (root !== c.root) dependents.set(root, (dependents.get(root) ?? new Set()).add(c.root)); + for (const c of crates.values()) for (const d of c.deps) dependents.set(d, (dependents.get(d) ?? new Set()).add(c.root)); for (const root of crates.keys()) { const reach = new Set([root]); const queue = [root]; diff --git a/test/pregate-rust.test.ts b/test/pregate-rust.test.ts index ab04cf108..ebd163ffe 100644 --- a/test/pregate-rust.test.ts +++ b/test/pregate-rust.test.ts @@ -149,6 +149,61 @@ describe('Rust runner and summary (pure)', () => { }); }); +/** + * Manifest shapes a line scan misreads (review of the first cut): a renamed + * dependency inherited from `[workspace.dependencies]`, TOML dotted keys, the + * `[dependencies.]` table form, target-specific tables, and a crate the + * workspace excludes. Dependencies resolve by path first, then by package name. + */ +const MANIFEST_FILES: Record = { + 'Cargo.toml': '[workspace]\nmembers = ["crates/*"]\nexclude = ["crates/helper"]\n\n[workspace.dependencies]\nbase = { package = "alpha", path = "crates/a" }\n', + 'crates/a/Cargo.toml': '[package]\nname = "alpha"\nversion = "0.1.0"\n', + 'crates/a/src/lib.rs': 'pub fn a() {}\n', + 'crates/a/tests/a.rs': '#[test]\nfn a() {}\n', + 'crates/beta/Cargo.toml': '[package]\nname = "beta"\nversion = "0.1.0"\n\n[dependencies]\nbase = { workspace = true }\n', + 'crates/beta/tests/b.rs': '#[test]\nfn b() {}\n', + 'crates/dotted/Cargo.toml': '[package]\nname = "dotted"\nversion = "0.1.0"\n\n[dependencies]\nfoo.path = "../a"\n', + 'crates/dotted/tests/d.rs': '#[test]\nfn d() {}\n', + 'crates/table/Cargo.toml': '[package]\nname = "table"\nversion = "0.1.0"\n\n[dev-dependencies.whatever]\npath = "../a"\n', + 'crates/table/tests/t.rs': '#[test]\nfn t() {}\n', + 'crates/targeted/Cargo.toml': '[package]\nname = "targeted"\nversion = "0.1.0"\n\n[target.\'cfg(unix)\'.build-dependencies]\nrenamed = { path = "../a" }\n', + 'crates/targeted/tests/x.rs': '#[test]\nfn x() {}\n', + 'crates/plain/Cargo.toml': '[package]\nname = "plain"\nversion = "0.1.0"\n', + 'crates/plain/tests/p.rs': '#[test]\nfn p() {}\n', + 'crates/helper/Cargo.toml': '[package]\nname = "helper"\nversion = "0.1.0"\n', + 'crates/helper/src/lib.rs': '#[cfg(test)]\nmod tests {\n #[test]\n fn h() {}\n}\n', +}; + +function manifestRepo(): FixtureRepo { + const repo = makeFixtureRepo({ pregate: null }); + repos.push(repo); + commit(repo, MANIFEST_FILES, 'cargo manifests'); + return repo; +} + +describe('Cargo manifests are parsed as TOML', () => { + // Value: protects=every dependency spelling Cargo accepts links a crate to its dependents: an inherited renamed workspace dependency, + // a dotted `foo.path` key, a `[dev-dependencies.]` table and a target-specific build dependency, all resolved by path; + // fails_when=the manifest is read by a line scan again and alpha's change misses these dependents (a false green); + // why_new=the first cut matched dependency keys against package names only; seam=none; no_claim=cargo's own resolver + test('renamed, inherited, dotted, table-form and target-specific dependencies all select the dependent crates', () => { + const g = buildGraph(manifestRepo().work); + expect(testsFor(g, 'crates/a/src/lib.rs').tests).toEqual(['crates/a/tests/a.rs', 'crates/beta/tests/b.rs', 'crates/dotted/tests/d.rs', 'crates/table/tests/t.rs', 'crates/targeted/tests/x.rs']); + }); + + // Value: protects=a crate the workspace excludes runs from its own directory, a member from the workspace root; fails_when=the + // nearest [workspace] is taken without checking members/exclude and `cargo test -p helper` runs where helper is no member + // (cargo fails, or tests a same-named member); why_new=membership was not read; seam=none; no_claim=a real cargo (plan only) + test('an excluded crate runs from its own directory; a member runs from the workspace root', () => { + const repo = manifestRepo(); + const ctx = { repoRoot: repo.work, config: DEFAULT_CONFIG, env: cargoShim(repo) } as unknown as PregateContext; + expect(planRuns(ctx, ['crates/beta/tests/b.rs', 'crates/helper/src/lib.rs']).groups).toEqual([ + { runner: 'cargo test -p beta', cwd: '', files: ['crates/beta/tests/b.rs'] }, + { runner: 'cargo test -p helper', cwd: 'crates/helper', files: ['src/lib.rs'] }, + ]); + }); +}); + describe('gstack-pregate on a Rust workspace', () => { const run = (repo: FixtureRepo, env: Record) => runBin(repo, 'gstack-pregate', ['--base', 'main', '--allow-repo-commands', '--stage', 'tests', '--only', 'lanes'], env); const log = (out: string) => fs.readFileSync(/log: (\S+)/.exec(out)![1]!, 'utf8');