diff --git a/Cargo.lock b/Cargo.lock index e1eb3f4..4acd44d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -556,6 +556,7 @@ dependencies = [ "owo-colors", "serde", "serde_json", + "tempfile", "tokio", "toml_edit", "tracing", diff --git a/crates/dependable-core/src/manifest.rs b/crates/dependable-core/src/manifest.rs index a93a35d..8307bd7 100644 --- a/crates/dependable-core/src/manifest.rs +++ b/crates/dependable-core/src/manifest.rs @@ -33,7 +33,10 @@ pub struct AlternateRegistryDecl { /// Distinguishes manifest files. Every variant has a parser; the mapping to /// [`Ecosystem`] is many-to-one, since several manifest formats can belong to one /// registry. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// +/// `Hash` because a kind is part of a cache key: a path alone does not say which parser +/// read it, and two kinds can name the same file. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum ManifestKind { CargoToml, @@ -138,8 +141,7 @@ impl ManifestKind { pub fn workspace_roots(self) -> Option { match self { ManifestKind::CargoToml => Some(WorkspaceRoots { - root_names: &["Cargo.toml"], - root_kind: ManifestKind::CargoToml, + root_names: &[("Cargo.toml", ManifestKind::CargoToml)], self_governing: true, }), _ => None, @@ -203,18 +205,42 @@ impl ManifestKind { #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[non_exhaustive] pub struct WorkspaceRoots { - /// Candidate file names for a root, tried in order within each directory — - /// the same precedence rule [`ManifestKind::lockfiles`] uses, so a root beside - /// the manifest always beats one further up whichever name it goes by. - pub root_names: &'static [&'static str], - /// The kind a located root is parsed as. Not necessarily the kind that went - /// looking: an ecosystem may keep its central declarations in a different file - /// format from the manifests that inherit them. - pub root_kind: ManifestKind, + /// Candidate roots, each a path relative to a directory being searched, paired + /// with the kind that candidate is recognized and parsed as. + /// + /// They are tried in order within each directory — the same precedence rule + /// [`ManifestKind::lockfiles`] uses, so a root beside the manifest always beats + /// one further up whichever name it goes by. + /// + /// Each name carries its **own** kind rather than the list sharing one, because a + /// single ecosystem's roots need not share a file format: a pnpm member roots at + /// `pnpm-workspace.yaml`, a Bun member at the workspace root's own `package.json`, + /// and one kind over both names would run the wrong parser over one of them. The + /// paired kind is also not necessarily the kind that went looking — an ecosystem may + /// keep its central declarations in a format none of its members are written in. + /// + /// A candidate is a *path*, not just a file name, and the `dir.join()` the walk + /// performs already resolves the separator. A Gradle descriptor would name + /// `gradle/libs.versions.toml` — illustrative of a descriptor a future ecosystem + /// would write, not one that exists, since [`ManifestKind::GradleVersionCatalog`] + /// declares no roots today and no shipped candidate has more than one segment. + /// + /// Only the directory a candidate is joined onto is symlink-resolved; the + /// candidate's own segments are not, so two members reaching one physical root by + /// two spellings of a symlinked segment would parse it twice rather than share it. + pub root_names: &'static [(&'static str, ManifestKind)], /// Whether a manifest may be its own root. Cargo's root-that-is-also-a-package /// writes `serde.workspace = true` against its own table, so walking past /// itself would leave exactly those entries unresolved; a kind whose central /// declarations always live in a separate file sets this `false`. + /// + /// A self-governing manifest is recognized **and** parsed as its own kind, so this + /// may only be set on a kind that appears among its own [`root_names`] kinds. + /// Setting it on a kind that is only ever read by a different parser would accept a + /// manifest as its own root with one parser and then read it with another, which + /// answers `Err`, and an unparseable root silently declares nothing. + /// + /// [`root_names`]: WorkspaceRoots::root_names pub self_governing: bool, } @@ -468,6 +494,68 @@ mod tests { assert!(!ManifestKind::GoMod.has_lockfile_support()); } + /// Every manifest kind, so an invariant that has to hold for *all* of them can be + /// asserted over the whole set rather than over whichever ones a test remembered. + const ALL_KINDS: [ManifestKind; 12] = [ + ManifestKind::CargoToml, + ManifestKind::GoMod, + ManifestKind::PackageJson, + ManifestKind::DenoJson, + ManifestKind::PnpmWorkspaceYaml, + ManifestKind::ComposerJson, + ManifestKind::RequirementsTxt, + ManifestKind::PyprojectToml, + ManifestKind::PubspecYaml, + ManifestKind::MixExs, + ManifestKind::Csproj, + ManifestKind::GradleVersionCatalog, + ]; + + /// The match is what makes the compiler care that [`ALL_KINDS`] is complete: adding a + /// [`ManifestKind`] variant makes it non-exhaustive. It maps each kind to its + /// *position* in the array and the round trip is asserted, so the arm a new variant + /// needs has no position it can honestly name — every index the array has is already + /// claimed by another kind — and `ALL_KINDS` and its length annotation have to grow + /// with the enum before one is free. + /// + /// This narrows the door rather than closing it: the loop visits what the array + /// holds, so an arm naming a position the array does not have is never evaluated. + /// Nothing in the language forces a variant into the array without either an + /// unstable `variant_count` or a derive dependency, which `dependable-core` does not + /// take. + #[test] + fn all_kinds_lists_every_variant_once() { + let mut seen = Vec::new(); + for kind in ALL_KINDS { + let position = match kind { + ManifestKind::CargoToml => 0, + ManifestKind::GoMod => 1, + ManifestKind::PackageJson => 2, + ManifestKind::DenoJson => 3, + ManifestKind::PnpmWorkspaceYaml => 4, + ManifestKind::ComposerJson => 5, + ManifestKind::RequirementsTxt => 6, + ManifestKind::PyprojectToml => 7, + ManifestKind::PubspecYaml => 8, + ManifestKind::MixExs => 9, + ManifestKind::Csproj => 10, + ManifestKind::GradleVersionCatalog => 11, + }; + assert_eq!( + ALL_KINDS.get(position), + Some(&kind), + "{kind:?} claims position {position} of ALL_KINDS" + ); + assert!(!seen.contains(&kind), "{kind:?} listed twice"); + seen.push(kind); + } + assert_eq!( + seen.len(), + ALL_KINDS.len(), + "every position is claimed once" + ); + } + /// Every kind but Cargo declares its versions in place, so the upward walk is /// skipped for all of them — the behaviour the boolean gate this replaced had. #[test] @@ -475,23 +563,13 @@ mod tests { let cargo = ManifestKind::CargoToml .workspace_roots() .expect("Cargo inherits"); - assert_eq!(cargo.root_names, ["Cargo.toml"]); - assert_eq!(cargo.root_kind, ManifestKind::CargoToml); + assert_eq!(cargo.root_names, [("Cargo.toml", ManifestKind::CargoToml)]); assert!(cargo.self_governing, "a Cargo root may be a package too"); - for kind in [ - ManifestKind::GoMod, - ManifestKind::PackageJson, - ManifestKind::DenoJson, - ManifestKind::PnpmWorkspaceYaml, - ManifestKind::ComposerJson, - ManifestKind::RequirementsTxt, - ManifestKind::PyprojectToml, - ManifestKind::PubspecYaml, - ManifestKind::MixExs, - ManifestKind::Csproj, - ManifestKind::GradleVersionCatalog, - ] { + for kind in ALL_KINDS { + if kind == ManifestKind::CargoToml { + continue; + } assert!(kind.workspace_roots().is_none(), "{kind:?}"); assert!( !kind.declares_workspace("[workspace]\nmembers = []\n"), @@ -500,6 +578,29 @@ mod tests { } } + /// The invariant [`WorkspaceRoots::self_governing`] states, checked against every + /// descriptor that exists. + /// + /// A self-governing manifest is accepted as its own root by its own kind's + /// `declares_workspace` and then read by its own kind's parser. A kind that claimed + /// to govern itself while only ever appearing under a *different* root kind would be + /// recognised with one parser and read with another: the read fails, the root + /// declares nothing, and every entry inheriting from it is silently reported as + /// unresolved rather than as wrong. + #[test] + fn a_self_governing_kind_is_one_of_its_own_root_kinds() { + for kind in ALL_KINDS { + let Some(roots) = kind.workspace_roots() else { + continue; + }; + assert!( + !roots.self_governing || roots.root_names.iter().any(|(_, root)| *root == kind), + "{kind:?} governs itself but is not among its own root kinds: {:?}", + roots.root_names + ); + } + } + /// Recognition has to stay the same predicate the walk used before, or a root /// that used to govern a member would be walked straight past. #[test] diff --git a/crates/dependable-fetch/src/cache.rs b/crates/dependable-fetch/src/cache.rs index ee53443..6e76408 100644 --- a/crates/dependable-fetch/src/cache.rs +++ b/crates/dependable-fetch/src/cache.rs @@ -8,7 +8,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::time::{Duration, SystemTime, UNIX_EPOCH}; -use dependable_core::Item; +use dependable_core::{Item, ManifestKind}; use moka::future::Cache; use crate::registries::PackageMetadata; @@ -56,13 +56,21 @@ pub fn metadata_cache() -> MetadataCache { .build() } -/// Caches a workspace root's `[workspace.dependencies]`, keyed by the root's path. +/// Caches a workspace root's central declarations, keyed by the root's path **and** the +/// kind it was parsed as. /// /// A monorepo asks the same question once per member — "what does the root declare" — /// and the answer is one file read plus a full `toml_edit` parse of bytes that have not /// changed. Without this, a 500-crate workspace pays for both 500 times over. The `Arc` /// is what makes a hit free rather than a clone of every declaration. -pub type WorkspaceCache = Cache>>; +/// +/// The kind is in the key because the path alone does not determine the parser: a name +/// can be a candidate root for more than one kind +/// ([`WorkspaceRoots::root_names`](dependable_core::WorkspaceRoots::root_names) pairs +/// each name with its own), and a root cached under one member's kind would then be +/// served to a member of the other. That failure is order-dependent — whichever kind was +/// checked first wins — and so shows up intermittently rather than in a test. +pub type WorkspaceCache = Cache<(PathBuf, ManifestKind), Arc>>; /// A fresh workspace-declaration cache with a 5-minute TTL, matching the versions cache: /// a root edited mid-run is a rare enough thing to be worth one stale answer, and a @@ -276,6 +284,38 @@ mod tests { ); } + /// A root's path does not say which parser read it: one name can be a candidate root + /// for two kinds, and serving one kind's parse to a member of the other is a wrong + /// answer that depends on which member was checked first. Keying on the pair is what + /// keeps that from being possible. + #[tokio::test] + async fn a_root_cached_under_one_kind_is_not_served_to_another() { + let cache = workspace_cache(); + let root = PathBuf::from("/repo/package.json"); + + cache + .insert( + (root.clone(), ManifestKind::PackageJson), + Arc::new(Vec::new()), + ) + .await; + + assert!( + cache + .get(&(root.clone(), ManifestKind::CargoToml)) + .await + .is_none(), + "a different kind read the same path and must parse it itself" + ); + assert!( + cache + .get(&(root, ManifestKind::PackageJson)) + .await + .is_some(), + "the kind that wrote the entry still hits" + ); + } + #[test] fn cache_root_prefers_xdg_then_localappdata_then_home() { // `$XDG_CACHE_HOME` wins outright. diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index 8446710..8d8ee62 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -505,22 +505,27 @@ impl Checker { /// difference that the answer is on local disk, so it only ever showed up as CPU. A /// 500-crate workspace parsed one root manifest 500 times. /// - /// Locating the root is still done per manifest: it is the walk that produces the key - /// this is cached on, and it is the cheap half. + /// Locating the root is still done per manifest: it is the walk that produces the + /// `(path, kind)` key this is cached on, and it is the cheap half. async fn workspace_source( &self, path: &Path, kind: ManifestKind, manifest: &str, ) -> Option<(PathBuf, Arc>)> { - let (root, root_content) = crate::discover::workspace_root_of(path, kind, manifest)?; - if let Some(hit) = self.workspace_cache.get(&root).await { + let (root, root_kind, root_content) = + crate::discover::workspace_root_of(path, kind, manifest)?; + // Keyed on the kind as well as the path: the same file can be a candidate root + // for two kinds, and the declarations are whatever that kind's parser saw. + let key = (root.clone(), root_kind); + if let Some(hit) = self.workspace_cache.get(&key).await { return Some((root, hit)); } - let declarations = Arc::new(crate::discover::workspace_declarations(kind, &root_content)); - self.workspace_cache - .insert(root.clone(), declarations.clone()) - .await; + let declarations = Arc::new(crate::discover::workspace_declarations( + root_kind, + &root_content, + )); + self.workspace_cache.insert(key, declarations.clone()).await; Some((root, declarations)) } diff --git a/crates/dependable-fetch/src/discover.rs b/crates/dependable-fetch/src/discover.rs index 1a7aa1f..5dbf1a6 100644 --- a/crates/dependable-fetch/src/discover.rs +++ b/crates/dependable-fetch/src/discover.rs @@ -273,7 +273,7 @@ pub fn find_lockfile(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, Lo } /// The nearest manifest above `manifest` offering central dependency declarations, with -/// its path and content. +/// its path, the kind it is to be parsed as, and its content. /// /// Which names to try in each directory, and what recognizes one as a root, come from /// `kind` ([`ManifestKind::workspace_roots`]); a kind with no such indirection is @@ -281,6 +281,12 @@ pub fn find_lockfile(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, Lo /// candidate name before moving up, so a root beside the manifest always beats one /// further away — the same precedence [`find_lockfile`] gives lockfiles. /// +/// The returned kind is the one the *matched name* is paired with, not `kind`: a member +/// and the root governing it need not share a file format, and within one ecosystem two +/// candidate names need not share one either. It is what the root's content must be +/// parsed with, so a caller reading the returned text passes it to +/// [`workspace_declarations`] unchanged. +/// /// `manifest` itself is excluded, so a member always resolves against a root *above* it. /// A root that is also a package declares its own values literally, and /// [`workspace_root_of`] is what handles it inheriting from its own table. The walk stops @@ -295,23 +301,25 @@ pub fn find_lockfile(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, Lo /// otherwise hand `../other` that workspace's `[workspace.dependencies]`, and no `.git` /// check catches it, because the boundary is tested against the current directory too. /// -/// The returned path is therefore absolute and symlink-resolved, whatever `manifest` was -/// spelled as. A manifest that cannot be canonicalized (it was deleted between discovery -/// and here) belongs to no workspace. +/// The returned path is therefore absolute whatever `manifest` was spelled as, and the +/// *directory* it was found in is symlink-resolved. A multi-segment candidate's own +/// segments are not: the walk joins the candidate onto an already-canonical directory +/// without resolving it again, so two members reaching one physical root through two +/// spellings of a symlinked segment would key a cache twice — a duplicate parse, not a +/// wrong answer, and no shipped candidate has more than one segment today. A manifest +/// that cannot be canonicalized (it was deleted between discovery and here) belongs to +/// no workspace. #[must_use] -pub fn nearest_workspace_root(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, String)> { +pub fn nearest_workspace_root( + manifest: &Path, + kind: ManifestKind, +) -> Option<(PathBuf, ManifestKind, String)> { let roots = kind.workspace_roots()?; let manifest = std::fs::canonicalize(manifest).ok()?; let mut dir = manifest.parent()?; loop { - for name in roots.root_names { - let candidate = dir.join(name); - if !same_file(&candidate, &manifest) - && let Ok(content) = std::fs::read_to_string(&candidate) - && roots.root_kind.declares_workspace(&content) - { - return Some((simplified(candidate), content)); - } + if let Some(found) = root_in_dir(dir, roots.root_names, &manifest) { + return Some(found); } if dir.join(".git").exists() { return None; @@ -320,7 +328,38 @@ pub fn nearest_workspace_root(manifest: &Path, kind: ManifestKind) -> Option<(Pa } } -/// The manifest whose central declarations govern `manifest`, and its text. +/// The first of `candidates` present in `dir` that its own paired kind recognizes as a +/// root, with the path, that kind, and the content. +/// +/// Each candidate is recognized by **its own** kind rather than by one kind shared across +/// the list, which is what lets an ecosystem name roots of more than one file format: a +/// pnpm member roots at `pnpm-workspace.yaml`, a Bun member at the workspace root's own +/// `package.json`, and a shared kind would run the YAML parser over one or the JSON +/// parser over the other. A candidate its paired kind does not recognize is walked past, +/// not treated as the end of the search, so a name that exists but declares nothing never +/// hides a real root further down the list. +/// +/// `exclude` is the manifest doing the asking: a member always resolves against a root +/// *other than* itself, and [`workspace_root_of`] is what handles the self case. +fn root_in_dir( + dir: &Path, + candidates: &[(&str, ManifestKind)], + exclude: &Path, +) -> Option<(PathBuf, ManifestKind, String)> { + for (name, root_kind) in candidates { + let candidate = dir.join(name); + if !same_file(&candidate, exclude) + && let Ok(content) = std::fs::read_to_string(&candidate) + && root_kind.declares_workspace(&content) + { + return Some((simplified(candidate), *root_kind, content)); + } + } + None +} + +/// The manifest whose central declarations govern `manifest`, the kind it is to be +/// parsed as, and its text. /// /// A manifest that declares a root itself governs **itself**, where its kind allows that /// ([`WorkspaceRoots::self_governing`](dependable_core::WorkspaceRoots)): Cargo lets a @@ -329,6 +368,12 @@ pub fn nearest_workspace_root(manifest: &Path, kind: ManifestKind) -> Option<(Pa /// root governs. `content` is the manifest's own text, already in the caller's hand, so /// the self case costs no extra read. /// +/// The self case reports `kind` as the root kind, because it is `kind`'s own +/// `declares_workspace` that accepted the text — recognising a root with one parser and +/// then reading it with another is exactly the mismatch +/// [`WorkspaceRoots::self_governing`](dependable_core::WorkspaceRoots) is documented to +/// forbid, and the `debug_assert` below is what catches a descriptor that breaks it. +/// /// Separate from [`workspace_declarations`] so a caller checking many members of one /// workspace can key a cache on the root's path and parse it only once; use /// [`workspace_source`] when that does not matter. @@ -340,14 +385,19 @@ pub fn workspace_root_of( manifest: &Path, kind: ManifestKind, content: &str, -) -> Option<(PathBuf, String)> { +) -> Option<(PathBuf, ManifestKind, String)> { let roots = kind.workspace_roots()?; + debug_assert!( + !roots.self_governing || roots.root_names.iter().any(|(_, root)| *root == kind), + "{kind:?} governs itself but is not among its own root kinds: {:?}", + roots.root_names + ); if roots.self_governing && kind.declares_workspace(content) { // Canonical, to match the shape [`nearest_workspace_root`] returns. let root = std::fs::canonicalize(manifest) .map(simplified) .unwrap_or_else(|_| manifest.to_path_buf()); - return Some((root, content.to_owned())); + return Some((root, kind, content.to_owned())); } nearest_workspace_root(manifest, kind) } @@ -362,25 +412,33 @@ pub fn workspace_source( kind: ManifestKind, content: &str, ) -> Option<(PathBuf, Vec)> { - let (root, root_content) = workspace_root_of(manifest, kind, content)?; - Some((root, workspace_declarations(kind, &root_content))) + let (root, root_kind, root_content) = workspace_root_of(manifest, kind, content)?; + Some((root, workspace_declarations(root_kind, &root_content))) } -/// The central declarations offered by `content`, a root governing a manifest of kind -/// `kind`. +/// The central declarations offered by `content`, the text of a root of kind `root_kind`. /// -/// The root is parsed as its own kind, not as `kind`, since an ecosystem may keep its -/// central declarations in a different file format from the manifests inheriting them. +/// The kind taken is the **root's own**, not that of the member inheriting from it, since +/// an ecosystem may keep its central declarations in a different file format from the +/// manifests inheriting them. It is the kind [`workspace_root_of`] and +/// [`nearest_workspace_root`] return alongside the root, so a caller holding nothing but +/// a located root and its text can call this — which a caller taking the member's kind +/// could not. /// /// A manifest that will not parse declares nothing, which is the same answer as a -/// manifest with no such table — neither is worth failing a whole check over, and neither -/// is a kind that has no central declarations to offer. +/// manifest with no such table — neither is worth failing a whole check over. +/// +/// `content` is parsed as `root_kind` unconditionally. There is no short-circuit for a +/// kind whose [`ManifestKind::workspace_roots`] is `None`: such a kind has no workspace +/// *indirection*, which is a fact about members of that kind looking upward, not about +/// whether the file in hand holds central declarations — a `package.json` carrying a +/// `catalog` block holds them and answers with them. Passing a member's kind rather than +/// the root's therefore reads the wrong file's shape instead of answering nothing, and +/// the compiler cannot tell the two apart. Take `root_kind` from [`workspace_root_of`] +/// or [`nearest_workspace_root`], which return it beside the root they located. #[must_use] -pub fn workspace_declarations(kind: ManifestKind, content: &str) -> Vec { - let Some(roots) = kind.workspace_roots() else { - return Vec::new(); - }; - parse(roots.root_kind, content) +pub fn workspace_declarations(root_kind: ManifestKind, content: &str) -> Vec { + parse(root_kind, content) .map(|parsed| { parsed .items @@ -764,6 +822,100 @@ mod tests { assert_eq!(declarations[0].name, "serde"); } + /// The trap a single `root_kind` over a whole name list sets: the paired kind, not + /// the list's, decides which parser reads a candidate. + /// + /// The real case is a JavaScript member, which roots either at a + /// `pnpm-workspace.yaml` or at the workspace root's own `package.json`. One kind + /// across both names has to mis-read one of them. Cargo is the only kind that + /// recognises anything today, so the heterogeneity is staged with a name whose usual + /// kind is JSON: paired with `PackageJson` the file is not a root, paired with + /// `CargoToml` the same bytes are. + #[test] + fn a_candidate_is_read_with_the_kind_it_is_paired_with() { + let dir = tempfile::tempdir().expect("tempdir"); + let dir = dir.path().canonicalize().expect("canonical"); + let central = + "[workspace]\nmembers = []\n\n[workspace.dependencies]\nserde = \"1.0.200\"\n"; + write(&dir.join("package.json"), central); + let nobody = dir.join("member/Cargo.toml"); + + // Paired with `PackageJson`, the candidate is not a root. No JSON parser runs to + // decide that: `declares_workspace` inspects content for `CargoToml` alone and is + // `false` for every other kind, so it is the pairing rather than a parse that + // rejects this. The assertion holds for any bytes and any non-Cargo kind today, + // and becomes load-bearing the day a second kind recognises roots of its own. + assert!( + root_in_dir( + &dir, + &[("package.json", ManifestKind::PackageJson)], + &nobody + ) + .is_none() + ); + + // Paired with `CargoToml`, the very same bytes are a root — and the kind reported + // back is the pair's, so the caller parses it with the parser that recognised it. + let (path, root_kind, content) = + root_in_dir(&dir, &[("package.json", ManifestKind::CargoToml)], &nobody) + .expect("a root"); + assert_eq!(path, simplified(dir.join("package.json"))); + assert_eq!(root_kind, ManifestKind::CargoToml); + let declarations = workspace_declarations(ManifestKind::CargoToml, &content); + assert_eq!(declarations.len(), 1, "{declarations:?}"); + assert_eq!(declarations[0].name, "serde"); + } + + /// A candidate its own kind cannot recognise is walked past, not taken as the answer: + /// with one kind over a heterogeneous list, the unreadable name would silently hide + /// the real root later in the list. + #[test] + fn an_unrecognised_candidate_does_not_hide_a_later_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let dir = dir.path().canonicalize().expect("canonical"); + write(&dir.join("package.json"), "{\"name\": \"web\"}"); + write( + &dir.join("Cargo.toml"), + "[workspace]\nmembers = []\n\n[workspace.dependencies]\nserde = \"1\"\n", + ); + let nobody = dir.join("member/Cargo.toml"); + + let (path, root_kind, _) = root_in_dir( + &dir, + &[ + ("package.json", ManifestKind::PackageJson), + ("Cargo.toml", ManifestKind::CargoToml), + ], + &nobody, + ) + .expect("the second candidate"); + + assert_eq!(path, simplified(dir.join("Cargo.toml"))); + assert_eq!(root_kind, ManifestKind::CargoToml); + } + + /// A root that governs itself is reported as its own kind, which is what + /// `self_governing` promises: the parser that accepted the text is the parser that + /// then reads it. Reporting the descriptor's root kind instead would, for a kind + /// whose ancestors are a different format, hand the caller a parser that returns + /// `Err` — and an unparseable root declares nothing, so every inherited entry comes + /// back unresolved rather than wrong. + #[test] + fn a_self_governing_root_is_reported_as_its_own_kind() { + let dir = tempfile::tempdir().expect("tempdir"); + let manifest = dir + .path() + .canonicalize() + .expect("canonical") + .join("Cargo.toml"); + let content = "[workspace]\nmembers = []\n\n[workspace.dependencies]\nserde = \"1\"\n"; + write(&manifest, content); + + let (_, root_kind, _) = + workspace_root_of(&manifest, ManifestKind::CargoToml, content).expect("itself"); + assert_eq!(root_kind, ManifestKind::CargoToml); + } + /// The same boundary the lockfile search respects: an unrelated checkout above the /// repository must never lend its `[workspace.dependencies]`. #[test] diff --git a/crates/dependable/Cargo.toml b/crates/dependable/Cargo.toml index 7922248..e2dd111 100644 --- a/crates/dependable/Cargo.toml +++ b/crates/dependable/Cargo.toml @@ -34,3 +34,6 @@ tracing-subscriber.workspace = true anyhow.workspace = true serde.workspace = true serde_json.workspace = true + +[dev-dependencies] +tempfile.workspace = true diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index 73ba766..919f15b 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -19,7 +19,7 @@ use dependable_fetch::{ Item, JsrFetcher, ManifestKind, MavenCentralFetcher, NpmFetcher, NuGetFetcher, PackageSource, PackagistFetcher, ParseError, ProgressEvent, PubDevFetcher, PyPiFetcher, ScopedRegistry, TreeOptions, UnstableFilter, WorkspaceGraphOptions, build_client, build_workspace_graph, - nearest_workspace_root, workspace_source, + workspace_root_of, workspace_source, }; use dependable_tui::TuiOptions; use globset::{GlobBuilder, GlobSet, GlobSetBuilder}; @@ -639,7 +639,7 @@ pub async fn run_list(args: ListArgs) -> anyhow::Result { .then(|| apply_nearest_lockfile(manifest, kind, &root, &mut parsed.items)) .flatten(); let meta = parse_project(kind, &content); - let (version, version_inherited) = resolve_version(manifest, kind, &meta); + let (version, version_inherited) = resolve_version(manifest, kind, &content, &meta); reports.push(ProjectReport { relative: relative_to(&root, manifest), @@ -690,18 +690,19 @@ fn apply_nearest_lockfile( } /// The manifest's version, resolving a Cargo `version.workspace = true` against the -/// nearest ancestor `[workspace.package]` table. Returns the version and whether it was -/// inherited. +/// `[workspace.package]` table governing it — its own, when the manifest is itself the +/// root. Returns the version and whether it was inherited. fn resolve_version( manifest: &Path, kind: ManifestKind, + content: &str, meta: &ProjectMeta, ) -> (Option, bool) { match &meta.version { None => (None, false), Some(PackageField::Literal(version)) => (Some(version.clone()), false), Some(PackageField::Workspace) => { - let inherited = workspace_package_defaults(manifest, kind) + let inherited = workspace_package_defaults(manifest, kind, content) .and_then(|defaults| defaults.get("version").cloned()); (inherited, true) } @@ -710,7 +711,14 @@ fn resolve_version( } } -/// `[workspace.package]` from the nearest ancestor `Cargo.toml` declaring a workspace. +/// `[workspace.package]` from the `Cargo.toml` governing `manifest`. +/// +/// [`workspace_root_of`] rather than `nearest_workspace_root`, because a Cargo root that +/// is also a package inherits from its **own** table: a single `Cargo.toml` holding both +/// `[workspace.package] version` and `[package] version.workspace = true` is legal, and a +/// walk that excludes the asking manifest never finds the table sitting in it. That is +/// already how the dependency inheritance a few lines up resolves, via `workspace_source`; +/// the scalar axis had no reason to disagree. /// /// Reading the located root as a Cargo `[workspace]` table is this function's own /// business: scalar inheritance (`version.workspace = true`) is a different axis from the @@ -718,11 +726,22 @@ fn resolve_version( fn workspace_package_defaults( manifest: &Path, kind: ManifestKind, + content: &str, ) -> Option> { if kind != ManifestKind::CargoToml { return None; } - let (_, content) = nearest_workspace_root(manifest, kind)?; + let (_, root_kind, content) = workspace_root_of(manifest, kind, content)?; + // Checked rather than assumed. Cargo's descriptor names one candidate today, so + // this always holds — but pairing a kind with each candidate name is precisely + // what makes a second candidate of another kind possible, and reading someone + // else's file as a `[workspace.package]` table would answer with its contents + // instead of declining. Every other caller now uses the kind the walk returned; + // this one hard-codes Cargo because scalar inheritance is Cargo-only, so the + // guard is how it says so. + if root_kind != ManifestKind::CargoToml { + return None; + } Some(parse_workspace(&content)?.package_defaults) } diff --git a/crates/dependable/tests/cli_list.rs b/crates/dependable/tests/cli_list.rs index 7a368f7..197c76d 100644 --- a/crates/dependable/tests/cli_list.rs +++ b/crates/dependable/tests/cli_list.rs @@ -280,6 +280,40 @@ fn a_path_override_is_not_treated_as_inherited() { assert_eq!(util["constraint"], Value::Null); } +/// A Cargo root that is also a package inherits its scalars from its **own** +/// `[workspace.package]` table — the table is legal there, and the crate declaring +/// `version.workspace = true` is the same file that holds it. A walk that excludes the +/// asking manifest never finds it, and reports the version as unknown while still +/// flagging it inherited: the inventory says both "there is no version" and "the version +/// came from somewhere else". Dependency inheritance already resolves this case; the +/// scalar axis has to agree. +#[test] +fn a_root_that_is_also_a_package_inherits_its_version_from_itself() { + let tmp = tempfile::TempDir::new().expect("temp dir"); + std::fs::write( + tmp.path().join("Cargo.toml"), + r#" +[workspace] + +[workspace.package] +version = "9.9.9" + +[package] +name = "selfroot" +version.workspace = true + +[dependencies] +serde = "1" +"#, + ) + .expect("write manifest"); + + let doc = list_json(tmp.path(), &[]); + let root = project(&doc, "selfroot"); + assert_eq!(root["version"], "9.9.9"); + assert_eq!(root["version_inherited"], true); +} + /// The root's `[workspace.dependencies]` are central declarations, not dependencies of /// the root — and it inherits nothing, because it is what everything else inherits from. #[test]