Refactor handling of output files and add diff files (#328)

NEWS.md

@@ -4,6 +4,8 @@
 
 - New: Mutate `proc_macro` targets and functions.
 
+- New: Write diffs to dedicated files under `mutants.out/diff/`. The filename is included in the mutant json output.
+
 ## 24.9.0
 
 - Fixed: Avoid generating empty string elements in `ENCODED_RUSTFLAGS` when `--cap-lints` is set. In some situations these could cause a compiler error complaining about the empty argument.

book/src/mutants-out.md

@@ -19,6 +19,9 @@ The output directory contains:
 * An `outcomes.json` file describing the results of all tests,
   and summary counts of each outcome.
 
+* A `diff/` directory, containing a diff file for each mutation, relative to the unmutated baseline.
+  `mutants.json` includes for each mutant the name of the diff file.
+
 * A `logs/` directory, with one log file for each mutation plus the baseline
   unmutated case. The log contains the diff of the mutation plus the output from
   cargo. `outcomes.json` includes for each mutant the name of the log file.
@@ -31,4 +34,4 @@ The contents of the directory and the format of these files is subject to change
 
 These files are incrementally updated while cargo-mutants runs, so other programs can read them to follow progress.
 
-There is generally no reason to include this directory in version control, so it is recommended that you add `/mutants.out*` to your `.gitignore` file. This will exclude both `mutants.out` and `mutants.out.old`.
+There is generally no reason to include this directory in version control, so it is recommended that you add `/mutants.out*` to your `.gitignore` file or equivalent. This will exclude both `mutants.out` and `mutants.out.old`.

src/build_dir.rs

@@ -3,6 +3,7 @@
 //! A directory containing mutated source to run cargo builds and tests.
 
 use std::fmt::{self, Debug};
+use std::fs::write;
 
 use tempfile::TempDir;
 use tracing::info;
@@ -77,6 +78,14 @@ impl BuildDir {
     pub fn path(&self) -> &Utf8Path {
         self.path.as_path()
     }
+
+    pub fn overwrite_file(&self, relative_path: &Utf8Path, code: &str) -> Result<()> {
+        let full_path = self.path.join(relative_path);
+        // for safety, don't follow symlinks
+        ensure!(full_path.is_file(), "{full_path:?} is not a file");
+        write(&full_path, code.as_bytes())
+            .with_context(|| format!("failed to write code to {full_path:?}"))
+    }
 }
 
 #[cfg(test)]

src/cargo.rs

@@ -9,6 +9,7 @@ use itertools::Itertools;
 use tracing::{debug, debug_span, warn};
 
 use crate::outcome::PhaseResult;
+use crate::output::ScenarioOutput;
 use crate::package::Package;
 use crate::process::{Process, ProcessStatus};
 use crate::*;
@@ -21,7 +22,7 @@ pub fn run_cargo(
     packages: Option<&[&Package]>,
     phase: Phase,
     timeout: Option<Duration>,
-    log_file: &mut LogFile,
+    scenario_output: &mut ScenarioOutput,
     options: &Options,
     console: &Console,
 ) -> Result<PhaseResult> {
@@ -45,7 +46,7 @@ pub fn run_cargo(
         build_dir.path(),
         timeout,
         jobserver,
-        log_file,
+        scenario_output,
         console,
     )?;
     check_interrupted()?;
@@ -161,6 +162,9 @@ fn cargo_argv(
 
 /// Return adjusted CARGO_ENCODED_RUSTFLAGS, including any changes to cap-lints.
 ///
+/// It seems we have to set this in the environment because Cargo doesn't expose
+/// a way to pass it in as an option from all commands?
+///
 /// This does not currently read config files; it's too complicated.
 ///
 /// See <https://doc.rust-lang.org/cargo/reference/environment-variables.html>

src/console.rs

@@ -6,12 +6,11 @@ use std::borrow::Cow;
 use std::fmt::Write;
 use std::fs::File;
 use std::io;
-use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 use std::time::{Duration, Instant};
 
 use anyhow::Context;
-use camino::Utf8Path;
+use camino::{Utf8Path, Utf8PathBuf};
 use console::{style, StyledObject};
 use humantime::format_duration;
 use nutmeg::Destination;
@@ -73,9 +72,9 @@ impl Console {
     /// Update that a cargo task is starting.
     pub fn scenario_started(
         &self,
-        dir: &Path,
+        dir: &Utf8Path,
         scenario: &Scenario,
-        log_file: &Utf8Path,
+        log_file: File,
     ) -> Result<()> {
         let start = Instant::now();
         let scenario_model = ScenarioModel::new(dir, scenario, start, log_file)?;
@@ -88,7 +87,7 @@ impl Console {
     /// Update that cargo finished.
     pub fn scenario_finished(
         &self,
-        dir: &Path,
+        dir: &Utf8Path,
         scenario: &Scenario,
         outcome: &ScenarioOutcome,
         options: &Options,
@@ -149,13 +148,13 @@ impl Console {
         self.message(&s);
     }
 
-    pub fn start_copy(&self, dir: &Path) {
+    pub fn start_copy(&self, dir: &Utf8Path) {
         self.view.update(|model| {
             model.copy_models.push(CopyModel::new(dir.to_owned()));
         });
     }
 
-    pub fn finish_copy(&self, dir: &Path) {
+    pub fn finish_copy(&self, dir: &Utf8Path) {
         self.view.update(|model| {
             let idx = model
                 .copy_models
@@ -166,7 +165,7 @@ impl Console {
         });
     }
 
-    pub fn copy_progress(&self, dest: &Path, total_bytes: u64) {
+    pub fn copy_progress(&self, dest: &Utf8Path, total_bytes: u64) {
         self.view.update(|model| {
             model
                 .copy_models
@@ -197,13 +196,13 @@ impl Console {
     }
 
     /// A new phase of this scenario started.
-    pub fn scenario_phase_started(&self, dir: &Path, phase: Phase) {
+    pub fn scenario_phase_started(&self, dir: &Utf8Path, phase: Phase) {
         self.view.update(|model| {
             model.find_scenario_mut(dir).phase_started(phase);
         })
     }
 
-    pub fn scenario_phase_finished(&self, dir: &Path, phase: Phase) {
+    pub fn scenario_phase_finished(&self, dir: &Utf8Path, phase: Phase) {
         self.view.update(|model| {
             model.find_scenario_mut(dir).phase_finished(phase);
         })
@@ -451,14 +450,14 @@ impl nutmeg::Model for LabModel {
 }
 
 impl LabModel {
-    fn find_scenario_mut(&mut self, dir: &Path) -> &mut ScenarioModel {
+    fn find_scenario_mut(&mut self, dir: &Utf8Path) -> &mut ScenarioModel {
         self.scenario_models
             .iter_mut()
             .find(|sm| sm.dir == *dir)
             .expect("scenario directory not found")
     }
 
-    fn remove_scenario(&mut self, dir: &Path) {
+    fn remove_scenario(&mut self, dir: &Utf8Path) {
         self.scenario_models.retain(|sm| sm.dir != *dir);
     }
 }
@@ -488,7 +487,7 @@ impl nutmeg::Model for WalkModel {
 /// It draws the command and some description of what scenario is being tested.
 struct ScenarioModel {
     /// The directory where this is being built: unique across all models.
-    dir: PathBuf,
+    dir: Utf8PathBuf,
     name: Cow<'static, str>,
     phase_start: Instant,
     phase: Option<Phase>,
@@ -499,10 +498,10 @@ struct ScenarioModel {
 
 impl ScenarioModel {
     fn new(
-        dir: &Path,
+        dir: &Utf8Path,
         scenario: &Scenario,
         start: Instant,
-        log_file: &Utf8Path,
+        log_file: File,
     ) -> Result<ScenarioModel> {
         let log_tail = TailFile::new(log_file).context("Failed to open log file")?;
         Ok(ScenarioModel {
@@ -556,13 +555,13 @@ impl nutmeg::Model for ScenarioModel {
 
 /// A Nutmeg model for progress in copying a tree.
 struct CopyModel {
-    dest: PathBuf,
+    dest: Utf8PathBuf,
     bytes_copied: u64,
     start: Instant,
 }
 
 impl CopyModel {
-    fn new(dest: PathBuf) -> CopyModel {
+    fn new(dest: Utf8PathBuf) -> CopyModel {
         CopyModel {
             dest,
             start: Instant::now(),

src/copy_tree.rs

@@ -46,7 +46,10 @@ pub fn copy_tree(
         .suffix(".tmp")
         .tempdir()
         .context("create temp dir")?;
-    let dest = temp_dir.path();
+    let dest = temp_dir
+        .path()
+        .try_into()
+        .context("Convert path to UTF-8")?;
     console.start_copy(dest);
     for entry in WalkBuilder::new(from_path)
         .standard_filters(gitignore)

src/lab.rs

@@ -5,7 +5,6 @@
 
 use std::cmp::{max, min};
 use std::panic::resume_unwind;
-use std::path::Path;
 use std::sync::Mutex;
 use std::thread;
 use std::time::Instant;
@@ -209,53 +208,62 @@ fn test_scenario(
     options: &Options,
     console: &Console,
 ) -> Result<ScenarioOutcome> {
-    let mut log_file = output_mutex
+    let mut scenario_output = output_mutex
         .lock()
-        .expect("lock output_dir to create log")
-        .create_log(scenario)?;
-    log_file.message(&scenario.to_string());
+        .expect("lock output_dir to start scenario")
+        .start_scenario(scenario)?;
+    let dir = build_dir.path();
+    console.scenario_started(dir, scenario, scenario_output.open_log_read()?)?;
+
     let phases: &[Phase] = if options.check_only {
         &[Phase::Check]
     } else {
         &[Phase::Build, Phase::Test]
     };
-    let applied = scenario
-        .mutant()
-        .map(|mutant| {
-            // TODO: This is slightly inefficient as it computes the mutated source twice,
-            // once for the diff and once to write it out.
-            log_file.message(&format!("mutation diff:\n{}", mutant.diff()));
-            mutant.apply(build_dir)
-        })
-        .transpose()?;
-    let dir: &Path = build_dir.path().as_ref();
-    console.scenario_started(dir, scenario, log_file.path())?;
+    if let Some(mutant) = scenario.mutant() {
+        let mutated_code = mutant.mutated_code();
+        let diff = scenario.mutant().unwrap().diff(&mutated_code);
+        scenario_output.write_diff(&diff)?;
+        mutant.apply(build_dir, &mutated_code)?;
+    }
 
-    let mut outcome = ScenarioOutcome::new(&log_file, scenario.clone());
+    let mut outcome = ScenarioOutcome::new(&scenario_output, scenario.clone());
     for &phase in phases {
         console.scenario_phase_started(dir, phase);
         let timeout = match phase {
             Phase::Test => timeouts.test,
             Phase::Build | Phase::Check => timeouts.build,
         };
-        let phase_result = run_cargo(
+        match run_cargo(
             build_dir,
             jobserver,
             Some(test_packages),
             phase,
             timeout,
-            &mut log_file,
+            &mut scenario_output,
             options,
             console,
-        )?;
-        let success = phase_result.is_success(); // so we can move it away
-        outcome.add_phase_result(phase_result);
-        console.scenario_phase_finished(dir, phase);
-        if !success {
-            break;
+        ) {
+            Ok(phase_result) => {
+                let success = phase_result.is_success(); // so we can move it away
+                outcome.add_phase_result(phase_result);
+                console.scenario_phase_finished(dir, phase);
+                if !success {
+                    break;
+                }
+            }
+            Err(err) => {
+                // Some unexpected internal error that stops the program.
+                if let Some(mutant) = scenario.mutant() {
+                    mutant.revert(build_dir)?;
+                    return Err(err);
+                }
+            }
         }
     }
-    drop(applied);
+    if let Some(mutant) = scenario.mutant() {
+        mutant.revert(build_dir)?;
+    }
     output_mutex
         .lock()
         .expect("lock output dir to add outcome")

src/list.rs

@@ -37,9 +37,10 @@ pub(crate) fn list_mutants<W: fmt::Write>(
         for mutant in mutants {
             let mut obj = serde_json::to_value(mutant)?;
             if options.emit_diffs {
-                obj.as_object_mut()
-                    .unwrap()
-                    .insert("diff".to_owned(), json!(mutant.diff()));
+                obj.as_object_mut().unwrap().insert(
+                    "diff".to_owned(),
+                    json!(mutant.diff(&mutant.mutated_code())),
+                );
             }
             list.push(obj);
         }
@@ -51,7 +52,7 @@ pub(crate) fn list_mutants<W: fmt::Write>(
         for mutant in mutants {
             writeln!(out, "{}", mutant.name(options.show_line_col, colors))?;
             if options.emit_diffs {
-                writeln!(out, "{}", mutant.diff())?;
+                writeln!(out, "{}", mutant.diff(&mutant.mutated_code()))?;
             }
         }
     }