Find files by walking mod statements

Fixes #90

Cargo.toml

@@ -37,7 +37,6 @@ toml = "0.5"
 tracing = "0.1"
 tracing-appender = "0.2"
 tracing-subscriber = "0.3"
-walkdir = "2.3"
 whoami = "1.2"
 
 [dependencies.regex]
@@ -69,6 +68,7 @@ lazy_static = "1.4"
 predicates = "2"
 pretty_assertions = "1"
 regex = "1.5"
+walkdir = "2.3"
 
 [workspace]
 members = ["mutants_attrs"]

NEWS.md

@@ -2,7 +2,9 @@
 
 ## Unreleased
 
-- Improved: cargo-mutants appends to `RUSTFLAGS` instead of overwriting it, and reads `CARGO_ENCODED_RUSTFLAGS`. This makes it possible to pass flags to the code under test, for example to use the Mold linker.
+- Fixed support for the Mold linker, or for other options passed via `RUSTFLAGS` or `CARGO_ENCODED_RUSTFLAGS`. (See the instructions in README.md).
+
+- Source trees are walked by following `mod` statements rather than globbing the directory. This is more correct if there are files that are not referenced by `mod` statements. Once attributes on modules are stable in Rust (<https://github.com/rust-lang/rust/issues/54727>) this opens a path to skip mods using attributes.
 
 ## 1.1.0
 

README.md

@@ -142,7 +142,7 @@ To mark functions so they are not mutated:
    crate, version "0.0.3" or later. (This must be a regular `dependency` not a
    `dev-dependency`, because the annotation will be on non-test code.)
 
-2. Mark functions with `#[mutants::skip]` or other attributes containing `mutants::skip` (e.g. `#[cfg_attr(test, mutants::skip)`).
+2. Mark functions with `#[mutants::skip]` or other attributes containing `mutants::skip` (e.g. `#[cfg_attr(test, mutants::skip)]`).
 
 See `testdata/tree/hang_avoided_by_attr/` for an example.
 

src/cargo.rs

@@ -2,7 +2,6 @@
 
 //! Run Cargo as a subprocess, including timeouts and propagating signals.
 
-use std::collections::BTreeSet;
 use std::env;
 use std::sync::Arc;
 use std::thread::sleep;
@@ -11,7 +10,6 @@ use std::time::{Duration, Instant};
 #[allow(unused_imports)]
 use anyhow::{anyhow, bail, Context, Result};
 use camino::{Utf8Path, Utf8PathBuf};
-use globset::GlobSet;
 use serde_json::Value;
 #[allow(unused_imports)]
 use tracing::{debug, error, info, span, trace, warn, Level};
@@ -180,8 +178,7 @@ impl SourceTree for CargoSourceTree {
         &self.root
     }
 
-    /// Find all source files that can be mutated within a tree, including their cargo packages.
-    fn source_files(&self, options: &Options) -> Result<Vec<SourceFile>> {
+    fn root_files(&self, _options: &Options) -> Result<Vec<Arc<SourceFile>>> {
         debug!("cargo_toml_path = {}", self.cargo_toml_path);
         check_interrupted()?;
         let metadata = cargo_metadata::MetadataCommand::new()
@@ -193,73 +190,20 @@ impl SourceTree for CargoSourceTree {
         let mut r = Vec::new();
         for package_metadata in &metadata.workspace_packages() {
             debug!("walk package {:?}", package_metadata.manifest_path);
-            let top_sources = direct_package_sources(&self.root, package_metadata)?;
-            let source_paths = indirect_source_paths(
-                &self.root,
-                top_sources,
-                &options.examine_globset,
-                &options.exclude_globset,
-            )?;
             let package_name = Arc::new(package_metadata.name.to_string());
-            for source_path in source_paths {
+            for source_path in direct_package_sources(&self.root, package_metadata)? {
                 check_interrupted()?;
-                r.push(SourceFile::new(
+                r.push(Arc::new(SourceFile::new(
                     &self.root,
                     source_path,
-                    Arc::clone(&package_name),
-                )?);
+                    package_name.clone(),
+                )?));
             }
         }
         Ok(r)
     }
 }
 
-/// Find all the `.rs` files, by starting from the sources identified by the manifest
-/// and walking down.
-///
-/// This just walks the directory tree rather than following `mod` statements (for now)
-/// so it may pick up some files that are not actually linked in.
-fn indirect_source_paths(
-    root: &Utf8Path,
-    top_sources: impl IntoIterator<Item = TreeRelativePathBuf>,
-    examine_globset: &Option<GlobSet>,
-    exclude_globset: &Option<GlobSet>,
-) -> Result<BTreeSet<TreeRelativePathBuf>> {
-    let dirs: BTreeSet<TreeRelativePathBuf> = top_sources.into_iter().map(|p| p.parent()).collect();
-    let mut files: BTreeSet<TreeRelativePathBuf> = BTreeSet::new();
-    for top_dir in dirs {
-        for p in walkdir::WalkDir::new(top_dir.within(root))
-            .sort_by_file_name()
-            .into_iter()
-        {
-            let p = p.with_context(|| "error walking source tree {top_dir}")?;
-            if !p.file_type().is_file() {
-                continue;
-            }
-            let path = p.into_path();
-            if !path
-                .extension()
-                .map_or(false, |p| p.eq_ignore_ascii_case("rs"))
-            {
-                continue;
-            }
-            let relative_path = path.strip_prefix(root).expect("strip prefix").to_owned();
-            if let Some(examine_globset) = examine_globset {
-                if !examine_globset.is_match(&relative_path) {
-                    continue;
-                }
-            }
-            if let Some(exclude_globset) = exclude_globset {
-                if exclude_globset.is_match(&relative_path) {
-                    continue;
-                }
-            }
-            files.insert(relative_path.into());
-        }
-    }
-    Ok(files)
-}
-
 /// Find all the files that are named in the `path` of targets in a Cargo manifest that should be tested.
 ///
 /// These are the starting points for discovering source files.
@@ -379,19 +323,6 @@ mod test {
         CargoSourceTree::open(Utf8Path::new("/")).unwrap_err();
     }
 
-    #[test]
-    fn source_files_in_testdata_factorial() {
-        let source_paths = CargoSourceTree::open(Utf8Path::new("testdata/tree/factorial"))
-            .unwrap()
-            .source_files(&Options::default())
-            .unwrap();
-        assert_eq!(source_paths.len(), 1);
-        assert_eq!(
-            source_paths[0].tree_relative_path().to_string(),
-            "src/bin/factorial.rs",
-        );
-    }
-
     #[test]
     fn open_subdirectory_of_crate_opens_the_crate() {
         let source_tree = CargoSourceTree::open(Utf8Path::new("testdata/tree/factorial/src"))

src/lab.rs

@@ -16,6 +16,7 @@ use crate::cargo::{cargo_argv, run_cargo, rustflags, CargoSourceTree};
 use crate::console::Console;
 use crate::outcome::{LabOutcome, Phase, ScenarioOutcome};
 use crate::output::OutputDir;
+use crate::visit::discover_mutants;
 use crate::*;
 
 /// Run all possible mutation experiments.
@@ -37,8 +38,7 @@ pub fn test_unmutated_then_all_mutants(
     console.set_debug_log(output_dir.open_debug_log()?);
 
     let rustflags = rustflags();
-
-    let mut mutants = source_tree.mutants(&options)?;
+    let mut mutants = discover_mutants(source_tree, &options)?;
     if options.shuffle {
         mutants.shuffle(&mut rand::thread_rng());
     }

src/main.rs

@@ -23,7 +23,7 @@ mod visit;
 
 use std::convert::TryFrom;
 use std::env;
-use std::io;
+use std::io::{self, Write};
 use std::process::exit;
 use std::time::Duration;
 
@@ -34,8 +34,7 @@ use clap::CommandFactory;
 use clap::Parser;
 use clap_complete::{generate, Shell};
 use path_slash::PathExt;
-use serde_json::json;
-use serde_json::Value;
+use serde_json::{json, Value};
 
 // Imports of public names from this crate.
 use crate::build_dir::BuildDir;
@@ -50,7 +49,7 @@ use crate::outcome::{Phase, ScenarioOutcome};
 use crate::path::Utf8PathSlashes;
 use crate::scenario::Scenario;
 use crate::source::{SourceFile, SourceTree};
-use crate::visit::discover_mutants;
+use crate::visit::{discover_files, discover_mutants};
 
 const VERSION: &str = env!("CARGO_PKG_VERSION");
 const NAME: &str = env!("CARGO_PKG_NAME");
@@ -196,16 +195,9 @@ fn main() -> Result<()> {
     } else if let Some(shell) = args.completions {
         generate(shell, &mut Cargo::command(), "cargo", &mut io::stdout());
     } else if args.list_files {
-        if args.json {
-            list_files_as_json(&source_tree, &options)?;
-        } else {
-            let source_files = source_tree.source_files(&options)?;
-            for f in source_files {
-                println!("{}", f.tree_relative_slashes());
-            }
-        }
+        list_files(&source_tree, &options, args.json)?;
     } else if args.list {
-        let mutants = source_tree.mutants(&options)?;
+        let mutants = discover_mutants(&source_tree, &options)?;
         if args.json {
             if args.diff {
                 eprintln!("--list --diff --json is not (yet) supported");
@@ -222,20 +214,27 @@ fn main() -> Result<()> {
     Ok(())
 }
 
-fn list_files_as_json(source_tree: &dyn SourceTree, options: &Options) -> Result<()> {
-    let list = Value::Array(
-        source_tree
-            .source_files(options)?
-            .iter()
-            .map(|source_file| {
-                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(),
+fn list_files(source_tree: &CargoSourceTree, options: &Options, json: bool) -> Result<()> {
+    let files = discover_files(source_tree, options)?;
+    let mut out = io::BufWriter::new(io::stdout());
+    if json {
+        let json_list = Value::Array(
+            files
+                .iter()
+                .map(|source_file| {
+                    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(),
+                    })
                 })
-            })
-            .collect(),
-    );
-    serde_json::to_writer_pretty(io::BufWriter::new(io::stdout()), &list)?;
+                .collect(),
+        );
+        serde_json::to_writer_pretty(out, &json_list)?;
+    } else {
+        for file in files {
+            writeln!(out, "{}", file.tree_relative_path)?;
+        }
+    }
     Ok(())
 }

src/mutate.rs

@@ -61,7 +61,7 @@ impl MutationOp {
 #[derive(Clone, Eq, PartialEq)]
 pub struct Mutant {
     /// Which file is being mutated.
-    source_file: Arc<SourceFile>,
+    pub source_file: Arc<SourceFile>,
 
     /// The function that's being mutated.
     function_name: Arc<String>,
@@ -242,8 +242,6 @@ impl Serialize for Mutant {
 
 #[cfg(test)]
 mod test {
-    use std::sync::Arc;
-
     use camino::Utf8Path;
     use itertools::Itertools;
     use pretty_assertions::assert_eq;
@@ -252,41 +250,34 @@ mod test {
 
     #[test]
     fn discover_factorial_mutants() {
-        let source_file = SourceFile::new(
-            Utf8Path::new("testdata/tree/factorial"),
-            "src/bin/factorial.rs".parse().unwrap(),
-            Arc::new("cargo-mutants-testdata-factorial".to_string()),
-        )
-        .unwrap();
-        let muts = discover_mutants(source_file.into()).unwrap();
-        assert_eq!(muts.len(), 2);
+        let tree_path = Utf8Path::new("testdata/tree/factorial");
+        let source_tree = CargoSourceTree::open(&tree_path).unwrap();
+        let options = Options::default();
+        let mutants = discover_mutants(&source_tree, &options).unwrap();
+        assert_eq!(mutants.len(), 2);
         assert_eq!(
-            format!("{:?}", muts[0]),
+            format!("{:?}", mutants[0]),
             r#"Mutant { op: Unit, function_name: "main", return_type: "", start: (1, 11), end: (5, 2), package_name: "cargo-mutants-testdata-factorial" }"#
         );
         assert_eq!(
-            muts[0].to_string(),
+            mutants[0].to_string(),
             "src/bin/factorial.rs:1: replace main with ()"
         );
         assert_eq!(
-            format!("{:?}", muts[1]),
+            format!("{:?}", mutants[1]),
             r#"Mutant { op: Default, function_name: "factorial", return_type: "-> u32", start: (7, 29), end: (13, 2), package_name: "cargo-mutants-testdata-factorial" }"#
         );
         assert_eq!(
-            muts[1].to_string(),
+            mutants[1].to_string(),
             "src/bin/factorial.rs:7: replace factorial with Default::default()"
         );
     }
 
     #[test]
     fn filter_by_attributes() {
-        let source_file = SourceFile::new(
-            Utf8Path::new("testdata/tree/hang_avoided_by_attr"),
-            "src/lib.rs".parse().unwrap(),
-            Arc::new("cargo-mutants-testdata-hang-avoided".to_string()),
-        )
-        .unwrap();
-        let mutants = discover_mutants(source_file.into()).unwrap();
+        let tree_path = Utf8Path::new("testdata/tree/hang_avoided_by_attr");
+        let source_tree = CargoSourceTree::open(&tree_path).unwrap();
+        let mutants = discover_mutants(&source_tree, &Options::default()).unwrap();
         let descriptions = mutants.iter().map(Mutant::describe_change).collect_vec();
         insta::assert_snapshot!(
             descriptions.join("\n"),
@@ -296,13 +287,9 @@ mod test {
 
     #[test]
     fn mutate_factorial() {
-        let source_file = SourceFile::new(
-            Utf8Path::new("testdata/tree/factorial"),
-            "src/bin/factorial.rs".parse().unwrap(),
-            Arc::new("cargo-mutants-testdata-factorial".to_string()),
-        )
-        .unwrap();
-        let mutants = discover_mutants(source_file.into()).unwrap();
+        let tree_path = Utf8Path::new("testdata/tree/factorial");
+        let source_tree = CargoSourceTree::open(&tree_path).unwrap();
+        let mutants = discover_mutants(&source_tree, &Options::default()).unwrap();
         assert_eq!(mutants.len(), 2);
 
         let mut mutated_code = mutants[0].mutated_code();

src/path.rs

@@ -95,9 +95,14 @@ impl TreeRelativePathBuf {
         tree_path.join(&self.0)
     }
 
+    pub fn join(&self, p: impl AsRef<Utf8Path>) -> Self {
+        TreeRelativePathBuf(self.0.join(p))
+    }
+
     /// Return the tree-relative path of the containing directory.
     ///
     /// Panics if there is no parent, i.e. if self is already the tree root.
+    #[allow(dead_code)]
     pub fn parent(&self) -> TreeRelativePathBuf {
         self.0
             .parent()
@@ -107,6 +112,12 @@ impl TreeRelativePathBuf {
     }
 }
 
+impl AsRef<Utf8Path> for TreeRelativePathBuf {
+    fn as_ref(&self) -> &Utf8Path {
+        &self.0
+    }
+}
+
 impl From<&Utf8Path> for TreeRelativePathBuf {
     fn from(path_buf: &Utf8Path) -> Self {
         TreeRelativePathBuf::new(path_buf.to_owned())