Repository: sourcefrog/cargo-mutants (rust)

Issue:
fails on packages like itertools that recursively depend on themselves
Itertools depends on a package that depends on a different version of itertools. Rust supports this.

But when cargo-mutants runs `cargo test -p itertools`, cargo complains that the package specification is ambiguous: do we mean the top level itertools (we do), or the indirect dependency?

Options:

1. cargo invites us to disambiguate with a file URL, which I've tried in #110, but this seems to be annoyingly brittle across OSes. It might still be possible to make it work.
2. We could use `--manifest-path` instead of `--package` to disambiguate it. Is this enough? 
3. We could omit `-p` when there's only one package that we want to test, which would intuitively match what you do when running the tests by hand, and probably will fix itertools. But this won't help with trees that actually *do* contain multiple packages that we want to mutate, so it's only a partial solution.
4. We can, at least in some cases, disambiguate by including the package version, like `itertools@0.11.0`, which has the advantage of working even when we want to test multiple packages.

Pull request:
Use --manifest-path rather than --package
Fixes #117

Patch:
diff --git a/NEWS.md b/NEWS.md
index d899ae1..bce8b44 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+- `cargo mutants` can now successfully test packages that transitively depend on a
+  different version of themselves, such as `itertools`. Previously, cargo-mutants
+  used the cargo `--package` option, which is ambiguous in this case, and now
+  it uses `--manifest-path` instead.
+
 - Mutate functions returning `&'_ str` (whether a lifetime is named or not) to
   return `"xyzzy"` and `""`.
 
diff --git a/src/cargo.rs b/src/cargo.rs
index 4e6f2d0..b27958b 100644
--- a/src/cargo.rs
+++ b/src/cargo.rs
@@ -9,11 +9,13 @@ use std::sync::Arc;
 use anyhow::{anyhow, bail, Context, Result};
 use camino::{Utf8Path, Utf8PathBuf};
 use serde_json::Value;
+use tracing::debug_span;
 #[allow(unused_imports)]
 use tracing::{debug, error, info, span, trace, warn, Level};
 
 use crate::path::TreeRelativePathBuf;
 use crate::process::get_command_output;
+use crate::source::Package;
 use crate::tool::Tool;
 use crate::*;
 
@@ -51,26 +53,47 @@ impl Tool for CargoTool {
         Ok(root)
     }
 
+    /// Find the root files for each relevant package in the source tree.
+    ///
+    /// A source tree might include multiple packages (e.g. in a Cargo workspace),
+    /// and each package might have multiple targets (e.g. a bin and lib). Test targets
+    /// are excluded here: we run them, but we don't mutate them.
+    ///
+    /// Each target has one root file, typically but not necessarily called `src/lib.rs`
+    /// or `src/main.rs`. This function returns a list of all those files.
+    ///
+    /// After this, there is one more level of discovery, by walking those root files
+    /// to find `mod` statements, and then recursively walking those files to find
+    /// all source files.
     fn root_files(&self, source_root_path: &Utf8Path) -> Result<Vec<Arc<SourceFile>>> {
         let cargo_toml_path = source_root_path.join("Cargo.toml");
-        debug!("cargo_toml_path = {}", cargo_toml_path);
+        debug!(?cargo_toml_path, ?source_root_path, "find root files");
         check_interrupted()?;
         let metadata = cargo_metadata::MetadataCommand::new()
             .manifest_path(&cargo_toml_path)
             .exec()
             .context("run cargo metadata")?;
-        check_interrupted()?;
 
         let mut r = Vec::new();
         for package_metadata in &metadata.workspace_packages() {
-            debug!("walk package {:?}", package_metadata.manifest_path);
-            let package_name = Arc::new(package_metadata.name.to_string());
+            check_interrupted()?;
+            let _span = debug_span!("package", name = %package_metadata.name).entered();
+            debug!(manifest_path = %package_metadata.manifest_path, "walk package");
+            let relative_manifest_path = package_metadata
+                .manifest_path
+                .strip_prefix(source_root_path)
+                .expect("manifest_path is within source_root_path")
+                .to_owned();
+            let package = Arc::new(Package {
+                name: package_metadata.name.clone(),
+                relative_manifest_path,
+            });
             for source_path in direct_package_sources(source_root_path, package_metadata)? {
                 check_interrupted()?;
                 r.push(Arc::new(SourceFile::new(
                     source_root_path,
                     source_path,
-                    package_name.clone(),
+                    &package,
                 )?));
             }
         }
@@ -79,11 +102,17 @@ impl Tool for CargoTool {
 
     fn compose_argv(
         &self,
+        build_dir: &BuildDir,
         scenario: &Scenario,
         phase: Phase,
         options: &Options,
     ) -> Result<Vec<String>> {
-        Ok(cargo_argv(scenario.package_name(), phase, options))
+        Ok(cargo_argv(
+            build_dir.path(),
+            scenario.package(),
+            phase,
+            options,
+        ))
     }
 
     fn compose_env(
@@ -106,14 +135,19 @@ fn cargo_bin() -> String {
 
 /// Make up the argv for a cargo check/build/test invocation, including argv[0] as the
 /// cargo binary itself.
-fn cargo_argv(package_name: Option<&str>, phase: Phase, options: &Options) -> Vec<String> {
+fn cargo_argv(
+    build_dir: &Utf8Path,
+    package: Option<&Package>,
+    phase: Phase,
+    options: &Options,
+) -> Vec<String> {
     let mut cargo_args = vec![cargo_bin(), phase.name().to_string()];
     if phase == Phase::Check || phase == Phase::Build {
         cargo_args.push("--tests".to_string());
     }
-    if let Some(package_name) = package_name {
-        cargo_args.push("--package".to_owned());
-        cargo_args.push(package_name.to_owned());
+    if let Some(package) = package {
+        cargo_args.push("--manifest-path".to_owned());
+        cargo_args.push(build_dir.join(&package.relative_manifest_path).to_string());
     } else {
         cargo_args.push("--workspace".to_string());
     }
@@ -227,44 +261,69 @@ mod test {
     #[test]
     fn generate_cargo_args_for_baseline_with_default_options() {
         let options = Options::default();
+        let build_dir = Utf8Path::new("/tmp/buildXYZ");
         assert_eq!(
-            cargo_argv(None, Phase::Check, &options)[1..],
+            cargo_argv(build_dir, None, Phase::Check, &options)[1..],
             ["check", "--tests", "--workspace"]
         );
         assert_eq!(
-            cargo_argv(None, Phase::Build, &options)[1..],
+            cargo_argv(build_dir, None, Phase::Build, &options)[1..],
             ["build", "--tests", "--workspace"]
         );
         assert_eq!(
-            cargo_argv(None, Phase::Test, &options)[1..],
+            cargo_argv(build_dir, None, Phase::Test, &options)[1..],
             ["test", "--workspace"]
         );
     }
 
     #[test]
-    fn generate_cargo_args_with_additional_cargo_test_args_and_package_name() {
+    fn generate_cargo_args_with_additional_cargo_test_args_and_package() {
         let mut options = Options::default();
         let package_name = "cargo-mutants-testdata-something";
+        let build_dir = Utf8Path::new("/tmp/buildXYZ");
+        let relative_manifest_path = Utf8PathBuf::from("testdata/something/Cargo.toml");
         options
             .additional_cargo_test_args
             .extend(["--lib", "--no-fail-fast"].iter().map(|s| s.to_string()));
+        let package = Arc::new(Package {
+            name: package_name.to_owned(),
+            relative_manifest_path: relative_manifest_path.clone(),
+        });
+        let build_manifest_path = build_dir.join(relative_manifest_path);
         assert_eq!(
-            cargo_argv(Some(package_name), Phase::Check, &options)[1..],
-            ["check", "--tests", "--package", package_name]
+            cargo_argv(build_dir, Some(&package), Phase::Check, &options)[1..],
+            [
+                "check",
+                "--tests",
+                "--manifest-path",
+                build_manifest_path.as_str(),
+            ]
         );
         assert_eq!(
-            cargo_argv(Some(package_name), Phase::Build, &options)[1..],
-            ["build", "--tests", "--package", package_name]
+            cargo_argv(build_dir, Some(&package), Phase::Build, &options)[1..],
+            [
+                "build",
+                "--tests",
+                "--manifest-path",
+                build_manifest_path.as_str(),
+            ]
         );
         assert_eq!(
-            cargo_argv(Some(package_name), Phase::Test, &options)[1..],
-            ["test", "--package", package_name, "--lib", "--no-fail-fast"]
+            cargo_argv(build_dir, Some(&package), Phase::Test, &options)[1..],
+            [
+                "test",
+                "--manifest-path",
+                build_manifest_path.as_str(),
+                "--lib",
+                "--no-fail-fast"
+            ]
         );
     }
 
     #[test]
     fn generate_cargo_args_with_additional_cargo_args_and_test_args() {
         let mut options = Options::default();
+        let build_dir = Utf8Path::new("/tmp/buildXYZ");
         options
             .additional_cargo_test_args
             .extend(["--lib", "--no-fail-fast"].iter().map(|s| s.to_string()));
@@ -272,15 +331,15 @@ mod test {
             .additional_cargo_args
             .extend(["--release".to_owned()]);
         assert_eq!(
-            cargo_argv(None, Phase::Check, &options)[1..],
+            cargo_argv(build_dir, None, Phase::Check, &options)[1..],
             ["check", "--tests", "--workspace", "--release"]
         );
         assert_eq!(
-            cargo_argv(None, Phase::Build, &options)[1..],
+            cargo_argv(build_dir, None, Phase::Build, &options)[1..],
             ["build", "--tests", "--workspace", "--release"]
         );
         assert_eq!(
-            cargo_argv(None, Phase::Test, &options)[1..],
+            cargo_argv(build_dir, None, Phase::Test, &options)[1..],
             [
                 "test",
                 "--workspace",
diff --git a/src/lab.rs b/src/lab.rs
index c50c2e0..67b2e65 100644
--- a/src/lab.rs
+++ b/src/lab.rs
@@ -194,7 +194,7 @@ fn test_scenario(
             Phase::Test => test_timeout,
             _ => Duration::MAX,
         };
-        let argv = tool.compose_argv(scenario, phase, options)?;
+        let argv = tool.compose_argv(build_dir, scenario, phase, options)?;
         let env = tool.compose_env(scenario, phase, options)?;
         let process_status = Process::run(
             &argv,
diff --git a/src/main.rs b/src/main.rs
index ea48936..0e9f98f 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -265,7 +265,7 @@ fn list_files(tool: &dyn Tool, source: &Utf8Path, options: &Options, json: bool)
                     json!({
                         // to_string so that we get it with slashes.
                         "path": source_file.tree_relative_path.to_string(),
-                        "package": source_file.package_name.as_ref(),
+                        "package": source_file.package.name,
                     })
                 })
                 .collect(),
diff --git a/src/mutate.rs b/src/mutate.rs
index 6749ab1..161aa11 100644
--- a/src/mutate.rs
+++ b/src/mutate.rs
@@ -15,6 +15,7 @@ use serde::Serialize;
 use similar::TextDiff;
 
 use crate::build_dir::BuildDir;
+use crate::source::Package;
 use crate::source::SourceFile;
 use crate::textedit::{replace_region, Span};
 
@@ -101,7 +102,11 @@ impl Mutant {
 
     /// Return the cargo package name.
     pub fn package_name(&self) -> &str {
-        &self.source_file.package_name
+        &self.source_file.package.name
+    }
+
+    pub fn package(&self) -> &Package {
+        &self.source_file.package
     }
 
     /// Return a unified diff for the mutant.
diff --git a/src/scenario.rs b/src/scenario.rs
index 3e25804..a9f7f15 100644
--- a/src/scenario.rs
+++ b/src/scenario.rs
@@ -1,8 +1,9 @@
-// Copyright 2021, 2022 Martin Pool
+// Copyright 2021-2023 Martin Pool
 
 use serde::Serialize;
 use std::fmt;
 
+use crate::source::Package;
 use crate::Mutant;
 
 /// A scenario is either a freshening build in the source tree, a baseline test with no mutations, or a mutation test.
@@ -35,12 +36,12 @@ impl Scenario {
         }
     }
 
-    /// Return the package name that should be tested for this scenario,
-    /// or None to test every package.
-    pub fn package_name(&self) -> Option<&str> {
+    /// Return the package tested for this scenario (for mutants), or None to test
+    /// every package (for baselines).
+    pub fn package(&self) -> Option<&Package> {
         match self {
-            Scenario::Mutant(mutant) => Some(mutant.package_name()),
-            _ => None,
+            Scenario::Mutant(mutant) => Some(mutant.package()),
+            Scenario::Baseline => None,
         }
     }
 }
diff --git a/src/source.rs b/src/source.rs
index defb622..535577c 100644
--- a/src/source.rs
+++ b/src/source.rs
@@ -5,7 +5,7 @@
 use std::sync::Arc;
 
 use anyhow::{Context, Result};
-use camino::Utf8Path;
+use camino::{Utf8Path, Utf8PathBuf};
 #[allow(unused_imports)]
 use tracing::{debug, info, warn};
 
@@ -18,10 +18,10 @@ use crate::path::TreeRelativePathBuf;
 ///
 /// Code is normalized to Unix line endings as it's read in, and modified
 /// files are written with Unix line endings.
-#[derive(Clone, PartialEq, Eq, Ord, PartialOrd)]
+#[derive(Debug, Eq, PartialEq)]
 pub struct SourceFile {
     /// Package within the workspace.
-    pub package_name: Arc<String>,
+    pub package: Arc<Package>,
 
     /// Path relative to the root of the tree.
     pub tree_relative_path: TreeRelativePathBuf,
@@ -37,7 +37,7 @@ impl SourceFile {
     pub fn new(
         tree_path: &Utf8Path,
         tree_relative_path: TreeRelativePathBuf,
-        package_name: Arc<String>,
+        package: &Arc<Package>,
     ) -> Result<SourceFile> {
         let full_path = tree_relative_path.within(tree_path);
         let code = std::fs::read_to_string(&full_path)
@@ -46,7 +46,7 @@ impl SourceFile {
         Ok(SourceFile {
             tree_relative_path,
             code: Arc::new(code),
-            package_name,
+            package: Arc::clone(package),
         })
     }
 
@@ -61,6 +61,15 @@ impl SourceFile {
     }
 }
 
+/// A package built and tested as a unit.
+#[derive(Debug, Eq, PartialEq)] // no clone, use Arc
+pub struct Package {
+    /// The short name of the package, like "mutants".
+    pub name: String,
+    /// For Cargo, the path of the `Cargo.toml` manifest file, relative to the top of the tree.
+    pub relative_manifest_path: Utf8PathBuf,
+}
+
 #[cfg(test)]
 mod test {
     use std::fs::File;
@@ -83,7 +92,10 @@ mod test {
         let source_file = SourceFile::new(
             temp_dir_path,
             file_name.parse().unwrap(),
-            Arc::new("imaginary-package".to_owned()),
+            &Arc::new(Package {
+                name: "imaginary-package".to_owned(),
+                relative_manifest_path: "whatever/Cargo.toml".into(),
+            }),
         )
         .unwrap();
         assert_eq!(*source_file.code, "fn main() {\n    640 << 10;\n}\n");
diff --git a/src/tool.rs b/src/tool.rs
index 81b6aaf..97ac9d3 100644
--- a/src/tool.rs
+++ b/src/tool.rs
@@ -17,8 +17,8 @@ use tracing::{debug, debug_span, trace};
 use crate::options::Options;
 use crate::outcome::Phase;
 use crate::scenario::Scenario;
-use crate::Result;
 use crate::SourceFile;
+use crate::{build_dir, Result};
 
 pub trait Tool: Debug + Send + Sync {
     fn name(&self) -> &str;
@@ -38,6 +38,7 @@ pub trait Tool: Debug + Send + Sync {
     /// Compose argv to run one phase in this tool.
     fn compose_argv(
         &self,
+        build_dir: &build_dir::BuildDir,
         scenario: &Scenario,
         phase: Phase,
         options: &Options,
diff --git a/src/visit.rs b/src/visit.rs
index f5c7d61..ef9c159 100644
--- a/src/visit.rs
+++ b/src/visit.rs
@@ -43,14 +43,13 @@ pub fn walk_tree(tool: &dyn Tool, root: &Utf8Path, options: &Options) -> Result<
     let mut file_queue: VecDeque<Arc<SourceFile>> = tool.root_files(root)?.into();
     while let Some(source_file) = file_queue.pop_front() {
         check_interrupted()?;
-        let package_name = source_file.package_name.clone();
         let (mut file_mutants, more_files) = walk_file(root, Arc::clone(&source_file), options)?;
         // We'll still walk down through files that don't match globs, so that
         // we have a chance to find modules underneath them. However, we won't
         // collect any mutants from them, and they don't count as "seen" for
         // `--list-files`.
         for path in more_files {
-            file_queue.push_back(Arc::new(SourceFile::new(root, path, package_name.clone())?));
+            file_queue.push_back(Arc::new(SourceFile::new(root, path, &source_file.package)?));
         }
         let path = &source_file.tree_relative_path;
         if let Some(examine_globset) = &options.examine_globset {


Test patch:
diff --git a/tests/cli/main.rs b/tests/cli/main.rs
index 8063a28..86b6b62 100644
--- a/tests/cli/main.rs
+++ b/tests/cli/main.rs
@@ -504,13 +504,13 @@ fn workspace_tree_is_well_tested() {
         assert_eq!(mutant_phases.len(), 2);
         assert_eq!(mutant_phases[0]["process_status"], "Success");
         assert_eq!(
-            mutant_phases[0]["argv"].as_array().unwrap()[1..],
-            ["build", "--tests", "--package", package_name]
+            mutant_phases[0]["argv"].as_array().unwrap()[1..=3],
+            ["build", "--tests", "--manifest-path"]
         );
         assert_eq!(mutant_phases[1]["process_status"], "Failure");
         assert_eq!(
-            mutant_phases[1]["argv"].as_array().unwrap()[1..],
-            ["test", "--package", package_name],
+            mutant_phases[1]["argv"].as_array().unwrap()[1..=2],
+            ["test", "--manifest-path"],
         );
     }
     {