From 164725660974677e6b608ae3ffbddd6f37f0898d Mon Sep 17 00:00:00 2001 From: Predrag Gruevski Date: Mon, 26 Aug 2024 04:08:18 +0000 Subject: [PATCH] Add handling of ambiguous package definitions within the same dir. --- src/rustdoc_gen.rs | 95 ++++++++++++++----- src/snapshot_tests.rs | 26 +++++ .../Cargo.toml | 9 ++ .../nested/Cargo.toml | 9 ++ .../nested/src/lib.rs | 14 +++ .../src/lib.rs | 14 +++ ...iguous_package_name_definitions-input.snap | 30 ++++++ ...guous_package_name_definitions-output.snap | 13 +++ 8 files changed, 185 insertions(+), 25 deletions(-) create mode 100644 test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/Cargo.toml create mode 100644 test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/Cargo.toml create mode 100644 test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/src/lib.rs create mode 100644 test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/src/lib.rs create mode 100644 test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-input.snap create mode 100644 test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-output.snap diff --git a/src/rustdoc_gen.rs b/src/rustdoc_gen.rs index f0199fd1..0d50ed0b 100644 --- a/src/rustdoc_gen.rs +++ b/src/rustdoc_gen.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::path::PathBuf; use anyhow::{bail, Context as _}; @@ -426,8 +427,9 @@ impl RustdocGenerator for RustdocFromFile { #[derive(Debug)] pub(crate) struct RustdocFromProjectRoot { project_root: PathBuf, - manifests: std::collections::HashMap, - manifest_errors: std::collections::HashMap, + manifests: HashMap, + manifest_errors: HashMap, + duplicate_packages: HashMap>, target_root: PathBuf, } @@ -439,31 +441,64 @@ impl RustdocFromProjectRoot { project_root: &std::path::Path, target_root: &std::path::Path, ) -> anyhow::Result { - let mut manifests = std::collections::HashMap::new(); - let mut manifest_errors = std::collections::HashMap::new(); + let mut manifests_by_path: HashMap = HashMap::new(); + let mut manifest_errors = HashMap::new(); + + // First, scan the contents of the root directory for `Cargo.toml` files. + // Parse such files' contents into `Manifest` values. for result in ignore::Walk::new(project_root) { let entry = result?; if entry.file_name() == "Cargo.toml" { let path = entry.into_path(); match crate::manifest::Manifest::parse(path.clone()) { - Ok(manifest) => match crate::manifest::get_package_name(&manifest) { - Ok(name) => { - manifests.insert(name.to_string(), manifest); - } - Err(e) => { - manifest_errors.insert(path, e); - } - }, + Ok(manifest) => { + manifests_by_path.insert(path, manifest); + } Err(e) => { manifest_errors.insert(path, e); } } } } + + // Then, figure out which packages are defined by those manifests. + // If some package name is defined by more than one manifest, record an error. + let mut package_manifests: HashMap = HashMap::new(); + let mut duplicate_packages: HashMap> = Default::default(); + for (path, manifest) in manifests_by_path.into_iter() { + let name = match crate::manifest::get_package_name(&manifest) { + Ok(name) => name.to_string(), + Err(e) => { + manifest_errors.insert(path.clone(), e); + continue + } + }; + + if let Some(duplicates) = duplicate_packages.get_mut(&name) { + // This package is defined in multiple manifests already. + // Add to the list of duplicate manifests that define it. + duplicates.push(path); + } else { + if let Some((prev_path, _)) = package_manifests.insert(name.clone(), (path, manifest)) { + // This is the first duplicate entry for this package. + // Remove it from the `package_manifests` and add both + // conflicting manifests to the duplicates list. + let (path, _) = package_manifests.remove(&name).expect("elements we just inserted weren't present"); + duplicate_packages.insert(name, vec![prev_path, path]); + } + } + } + for (_package, paths) in duplicate_packages.iter_mut() { + paths.sort_unstable(); + } + + let manifests = package_manifests.into_iter().map(|(package, (_, manifest))| (package, manifest)).collect(); + Ok(Self { project_root: project_root.to_owned(), manifests, manifest_errors, + duplicate_packages, target_root: target_root.to_owned(), }) } @@ -477,21 +512,31 @@ impl RustdocGenerator for RustdocFromProjectRoot { crate_data: CrateDataForRustdoc, ) -> anyhow::Result { let manifest: &Manifest = self.manifests.get(crate_data.name).ok_or_else(|| { - let err = anyhow::anyhow!( - "package `{}` not found in {}", - crate_data.name, - self.project_root.display(), - ); - if self.manifest_errors.is_empty() { + if let Some(duplicates) = self.duplicate_packages.get(crate_data.name) { + let duplicates = duplicates.into_iter().map(|p| p.display()).join("\n "); + let err = anyhow::anyhow!( + "package `{}` is ambiguous: it is defined by in multiple manifests within the root path {}\n\ndefined in:\n {duplicates}", + crate_data.name, + self.project_root.display(), + ); err } else { - let cause_list = self - .manifest_errors - .values() - .map(|error| format!(" {error:#},")) - .join("\n"); - let possible_causes = format!("possibly due to errors: [\n{cause_list}\n]"); - err.context(possible_causes) + let err = anyhow::anyhow!( + "package `{}` not found in {}", + crate_data.name, + self.project_root.display(), + ); + if self.manifest_errors.is_empty() { + err + } else { + let cause_list = self + .manifest_errors + .values() + .map(|error| format!(" {error:#},")) + .join("\n"); + let possible_causes = format!("possibly due to errors: [\n{cause_list}\n]"); + err.context(possible_causes) + } } })?; generate_rustdoc( diff --git a/src/snapshot_tests.rs b/src/snapshot_tests.rs index 48c182ce..deb6e550 100644 --- a/src/snapshot_tests.rs +++ b/src/snapshot_tests.rs @@ -336,3 +336,29 @@ fn workspace_baseline_compile_error() { ], ); } + +/// Pin down the behavior when running `cargo-semver-checks` on a project that +/// for some reason contains multiple definitions of the same package name +/// in different workspaces in the same directory. +/// +/// The current behavior *is not* necessarily preferable in the long term, and may change. +/// It looks through all `Cargo.toml` files in the directory and accumulates everything they define. +/// +/// In the long run, we may want to use something like `cargo locate-project` to determine +/// which workspace we're currently "inside" and only load its manifests. +/// This approach is described here: +/// +#[test] +fn multiple_ambiguous_package_name_definitions() { + assert_integration_test( + "multiple_ambiguous_package_name_definitions", + &[ + "cargo", + "semver-checks", + "--baseline-root", + "test_crates/manifest_tests/multiple_ambiguous_package_name_definitions", + "--manifest-path", + "test_crates/manifest_tests/multiple_ambiguous_package_name_definitions", + ], + ); +} diff --git a/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/Cargo.toml b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/Cargo.toml new file mode 100644 index 00000000..6d44d163 --- /dev/null +++ b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/Cargo.toml @@ -0,0 +1,9 @@ +[workspace] +resolver = "2" + +[package] +name = "example" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/Cargo.toml b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/Cargo.toml new file mode 100644 index 00000000..6d44d163 --- /dev/null +++ b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/Cargo.toml @@ -0,0 +1,9 @@ +[workspace] +resolver = "2" + +[package] +name = "example" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/src/lib.rs b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/src/lib.rs new file mode 100644 index 00000000..b93cf3ff --- /dev/null +++ b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/src/lib.rs @@ -0,0 +1,14 @@ +pub fn add(left: u64, right: u64) -> u64 { + left + right +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn it_works() { + let result = add(2, 2); + assert_eq!(result, 4); + } +} diff --git a/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/src/lib.rs b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/src/lib.rs new file mode 100644 index 00000000..b93cf3ff --- /dev/null +++ b/test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/src/lib.rs @@ -0,0 +1,14 @@ +pub fn add(left: u64, right: u64) -> u64 { + left + right +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn it_works() { + let result = add(2, 2); + assert_eq!(result, 4); + } +} diff --git a/test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-input.snap b/test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-input.snap new file mode 100644 index 00000000..ef9e300e --- /dev/null +++ b/test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-input.snap @@ -0,0 +1,30 @@ +--- +source: src/snapshot_tests.rs +expression: check +--- +Check( + scope: Scope( + mode: DenyList(PackageSelection( + selection: DefaultMembers, + excluded_packages: [], + )), + ), + current: Rustdoc( + source: Root("test_crates/manifest_tests/multiple_ambiguous_package_name_definitions"), + ), + baseline: Rustdoc( + source: Root("test_crates/manifest_tests/multiple_ambiguous_package_name_definitions"), + ), + release_type: None, + current_feature_config: FeatureConfig( + features_group: Heuristic, + extra_features: [], + is_baseline: false, + ), + baseline_feature_config: FeatureConfig( + features_group: Heuristic, + extra_features: [], + is_baseline: true, + ), + build_target: None, +) diff --git a/test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-output.snap b/test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-output.snap new file mode 100644 index 00000000..5436fa1f --- /dev/null +++ b/test_outputs/snapshot_tests/cargo_semver_checks__snapshot_tests__multiple_ambiguous_package_name_definitions-output.snap @@ -0,0 +1,13 @@ +--- +source: src/snapshot_tests.rs +expression: result +--- +--- error --- +package `example` is ambiguous: it is defined by in multiple manifests within the root path test_crates/manifest_tests/multiple_ambiguous_package_name_definitions + +defined in: + test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/Cargo.toml + test_crates/manifest_tests/multiple_ambiguous_package_name_definitions/nested/Cargo.toml +--- stdout --- + +--- stderr ---