From e1229131295049785b23c924b98db4b5e2f9b0b0 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Thu, 14 Dec 2023 13:12:42 -0800 Subject: [PATCH 1/5] Remove Arc Just SourceFile is enough; it holds the big code buffer in an internal arc --- src/in_diff.rs | 7 +++---- src/list.rs | 3 +-- src/mutate.rs | 10 +++++----- src/source.rs | 23 ++++++++++++++++------- src/visit.rs | 29 +++++++++++++---------------- src/workspace.rs | 6 +++--- 6 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/in_diff.rs b/src/in_diff.rs index 62a8f2ec..f57e9f5a 100644 --- a/src/in_diff.rs +++ b/src/in_diff.rs @@ -4,7 +4,6 @@ //! for example from uncommitted or unmerged changes. use std::collections::HashMap; -use std::sync::Arc; use anyhow::{anyhow, bail}; use camino::Utf8Path; @@ -63,17 +62,17 @@ pub fn diff_filter(mutants: Vec, diff_text: &str) -> Result> /// Error if the new text from the diffs doesn't match the source files. fn check_diff_new_text_matches(patches: &[Patch], mutants: &[Mutant]) -> Result<()> { - let mut source_by_name: HashMap<&Utf8Path, Arc> = HashMap::new(); + let mut source_by_name: HashMap<&Utf8Path, &SourceFile> = HashMap::new(); for mutant in mutants { source_by_name .entry(mutant.source_file.path()) - .or_insert_with(|| Arc::clone(&mutant.source_file)); + .or_insert_with(|| &mutant.source_file); } for patch in patches { let path = strip_patch_path(&patch.new.path); if let Some(source_file) = source_by_name.get(&path) { let reconstructed = partial_new_file(patch); - let lines = source_file.code.lines().collect_vec(); + let lines = source_file.code().lines().collect_vec(); for (lineno, diff_content) in reconstructed { let source_content = lines.get(lineno - 1).unwrap_or(&""); if diff_content != *source_content { diff --git a/src/list.rs b/src/list.rs index 58ef71c9..1e8193fc 100644 --- a/src/list.rs +++ b/src/list.rs @@ -4,7 +4,6 @@ use std::fmt; use std::io; -use std::sync::Arc; use serde_json::{json, Value}; @@ -62,7 +61,7 @@ pub(crate) fn list_mutants( pub(crate) fn list_files( mut out: W, - source_files: &[Arc], + source_files: &[SourceFile], options: &Options, ) -> Result<()> { if options.emit_json { diff --git a/src/mutate.rs b/src/mutate.rs index 52f371dd..9fef9eb6 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -31,7 +31,7 @@ pub enum Genre { #[derive(Clone, Eq, PartialEq)] pub struct Mutant { /// Which file is being mutated. - pub source_file: Arc, + pub source_file: SourceFile, /// The function that's being mutated: the nearest enclosing function, if they are nested. /// @@ -71,7 +71,7 @@ impl Mutant { /// Return text of the whole file with the mutation applied. pub fn mutated_code(&self) -> String { self.span.replace( - &self.source_file.code, + self.source_file.code(), &format!("{} {}", &self.replacement, MUTATION_MARKER_COMMENT), ) } @@ -143,7 +143,7 @@ impl Mutant { } pub fn original_text(&self) -> String { - self.span.extract(&self.source_file.code) + self.span.extract(self.source_file.code()) } /// Return the text inserted for this mutation. @@ -165,7 +165,7 @@ impl Mutant { let old_label = self.source_file.tree_relative_slashes(); // There shouldn't be any newlines, but just in case... let new_label = self.describe_change().replace('\n', " "); - TextDiff::from_lines(&self.source_file.code, &self.mutated_code()) + TextDiff::from_lines(self.source_file.code(), &self.mutated_code()) .unified_diff() .context_radius(8) .header(&old_label, &new_label) @@ -178,7 +178,7 @@ impl Mutant { } pub fn unapply(&self, build_dir: &mut BuildDir) -> Result<()> { - self.write_in_dir(build_dir, &self.source_file.code) + self.write_in_dir(build_dir, self.source_file.code()) } #[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] diff --git a/src/source.rs b/src/source.rs index d1866442..571bfd85 100644 --- a/src/source.rs +++ b/src/source.rs @@ -19,7 +19,7 @@ use crate::path::Utf8PathSlashes; /// /// Code is normalized to Unix line endings as it's read in, and modified /// files are written with Unix line endings. -#[derive(Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq)] pub struct SourceFile { /// Package within the workspace. pub package: Arc, @@ -27,8 +27,11 @@ pub struct SourceFile { /// Path of this source file relative to workspace. pub tree_relative_path: Utf8PathBuf, - /// Full copy of the source. - pub code: String, + /// Full copy of the unmodified source. + /// + /// This is held in an [Arc] so that SourceFiles can be cloned without using excessive + /// amounts of memory. + code: Arc, } impl SourceFile { @@ -41,9 +44,11 @@ impl SourceFile { package: &Arc, ) -> Result { let full_path = tree_path.join(&tree_relative_path); - let code = std::fs::read_to_string(&full_path) - .with_context(|| format!("failed to read source of {full_path:?}"))? - .replace("\r\n", "\n"); + let code = Arc::new( + std::fs::read_to_string(&full_path) + .with_context(|| format!("failed to read source of {full_path:?}"))? + .replace("\r\n", "\n"), + ); Ok(SourceFile { tree_relative_path, code, @@ -59,6 +64,10 @@ impl SourceFile { pub fn path(&self) -> &Utf8Path { self.tree_relative_path.as_path() } + + pub fn code(&self) -> &str { + self.code.as_str() + } } #[cfg(test)] @@ -89,6 +98,6 @@ mod test { }), ) .unwrap(); - assert_eq!(source_file.code, "fn main() {\n 640 << 10;\n}\n"); + assert_eq!(source_file.code(), "fn main() {\n 640 << 10;\n}\n"); } } diff --git a/src/visit.rs b/src/visit.rs index bc2e136f..6e720617 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -33,7 +33,7 @@ use crate::*; /// were visited but that produced no mutants. pub struct Discovered { pub mutants: Vec, - pub files: Vec>, + pub files: Vec, } /// Discover all mutants and all source files. @@ -42,7 +42,7 @@ pub struct Discovered { /// pub fn walk_tree( workspace_dir: &Utf8Path, - top_source_files: &[Arc], + top_source_files: &[SourceFile], options: &Options, console: &Console, ) -> Result { @@ -52,24 +52,24 @@ pub fn walk_tree( .map(|e| syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}"))) .collect::>>()?; console.walk_tree_start(); - let mut file_queue: VecDeque> = top_source_files.iter().cloned().collect(); + let mut file_queue: VecDeque = top_source_files.iter().cloned().collect(); let mut mutants = Vec::new(); - let mut files: Vec> = Vec::new(); + let mut files: Vec = Vec::new(); while let Some(source_file) = file_queue.pop_front() { console.walk_tree_update(files.len(), mutants.len()); check_interrupted()?; - let (mut file_mutants, external_mods) = walk_file(Arc::clone(&source_file), &error_exprs)?; + let (mut file_mutants, external_mods) = walk_file(&source_file, &error_exprs)?; // 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 mod_name in &external_mods { if let Some(mod_path) = find_mod_source(workspace_dir, &source_file, mod_name)? { - file_queue.push_back(Arc::new(SourceFile::new( + file_queue.push_back(SourceFile::new( workspace_dir, mod_path, &source_file.package, - )?)) + )?) } } let path = &source_file.tree_relative_path; @@ -101,13 +101,10 @@ pub fn walk_tree( /// /// Returns the mutants found, and the names of modules referenced by `mod` statements /// that should be visited later. -fn walk_file( - source_file: Arc, - error_exprs: &[Expr], -) -> Result<(Vec, Vec)> { +fn walk_file(source_file: &SourceFile, error_exprs: &[Expr]) -> Result<(Vec, Vec)> { let _span = debug_span!("source_file", path = source_file.tree_relative_slashes()).entered(); debug!("visit source file"); - let syn_file = syn::parse_str::(&source_file.code) + let syn_file = syn::parse_str::(source_file.code()) .with_context(|| format!("failed to parse {}", source_file.tree_relative_slashes()))?; let mut visitor = DiscoveryVisitor { error_exprs, @@ -115,7 +112,7 @@ fn walk_file( mutants: Vec::new(), namespace_stack: Vec::new(), fn_stack: Vec::new(), - source_file: Arc::clone(&source_file), + source_file: source_file.clone(), }; visitor.visit_file(&syn_file); Ok((visitor.mutants, visitor.external_mods)) @@ -131,7 +128,7 @@ struct DiscoveryVisitor<'o> { mutants: Vec, /// The file being visited. - source_file: Arc, + source_file: SourceFile, /// The stack of namespaces we're currently inside. namespace_stack: Vec, @@ -180,7 +177,7 @@ impl<'o> DiscoveryVisitor<'o> { let body_span = function_body_span(block).expect("Empty function body"); let mut new_mutants = return_type_replacements(&sig.output, self.error_exprs) .map(|rep| Mutant { - source_file: Arc::clone(&self.source_file), + source_file: self.source_file.clone(), function: Some(Arc::clone(function)), span: body_span, replacement: rep.to_pretty_string(), @@ -355,7 +352,7 @@ impl<'ast> Visit<'ast> for DiscoveryVisitor<'_> { let mut new_mutants = replacements .into_iter() .map(|rep| Mutant { - source_file: Arc::clone(&self.source_file), + source_file: self.source_file.clone(), function: self.fn_stack.last().map(Arc::clone), replacement: rep.to_pretty_string(), span: i.op.span().into(), diff --git a/src/workspace.rs b/src/workspace.rs index bf2d834e..69fc8628 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -182,7 +182,7 @@ impl Workspace { } /// Find all the top source files for selected packages. - fn top_sources(&self, package_filter: &PackageFilter) -> Result>> { + fn top_sources(&self, package_filter: &PackageFilter) -> Result> { let mut sources = Vec::new(); for PackageTop { package, @@ -190,11 +190,11 @@ impl Workspace { } in self.package_tops(package_filter)? { for source_path in top_sources { - sources.push(Arc::new(SourceFile::new( + sources.push(SourceFile::new( &self.dir, source_path.to_owned(), &package, - )?)); + )?); } } Ok(sources) From 2df806da0d8c18a02a4ed7b1c22741cd81af8868 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 15 Dec 2023 09:12:20 +1000 Subject: [PATCH 2/5] clean up --- src/visit.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/visit.rs b/src/visit.rs index 6e720617..5dc5de3d 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -172,6 +172,13 @@ impl<'o> DiscoveryVisitor<'o> { ); } + // /// Record that we generated some mutants. + // fn record_mutant(&mut self, span: &Span, replacement: String, genre: Genre) { + // self.mutants.push(Mutant { + // source_file: self.source_file.clone(), + // function: Some(Arc::clone(se + // } + fn collect_fn_mutants(&mut self, sig: &Signature, block: &Block) { if let Some(function) = self.fn_stack.last() { let body_span = function_body_span(block).expect("Empty function body"); @@ -347,7 +354,13 @@ impl<'ast> Visit<'ast> for DiscoveryVisitor<'_> { BinOp::Gt(_) => vec![quote! { == }, quote! {<}], BinOp::Le(_) => vec![quote! {>}], BinOp::Ge(_) => vec![quote! {<}], - _ => Vec::new(), + _ => { + trace!( + op = i.op.to_pretty_string(), + "No mutants generated for this binary operator" + ); + Vec::new() + } }; let mut new_mutants = replacements .into_iter() @@ -359,14 +372,7 @@ impl<'ast> Visit<'ast> for DiscoveryVisitor<'_> { genre: Genre::BinaryOperator, }) .collect_vec(); - if new_mutants.is_empty() { - debug!( - op = i.op.to_pretty_string(), - "No mutants generated for this binary operator" - ); - } else { - self.mutants.append(&mut new_mutants); - } + self.mutants.append(&mut new_mutants); syn::visit::visit_expr_binary(self, i); } } From 4e5fb9727232bc5b4eb5ed78849257b5ce4142f9 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 15 Dec 2023 10:20:42 +1000 Subject: [PATCH 3/5] Factor out collect_mutant --- src/fnvalue.rs | 4 +- ..._expected_mutants_for_own_source_tree.snap | 13 +++-- src/visit.rs | 49 +++++++------------ 3 files changed, 29 insertions(+), 37 deletions(-) diff --git a/src/fnvalue.rs b/src/fnvalue.rs index 5d708194..3ac2ebe0 100644 --- a/src/fnvalue.rs +++ b/src/fnvalue.rs @@ -17,12 +17,11 @@ use tracing::trace; pub(crate) fn return_type_replacements( return_type: &ReturnType, error_exprs: &[Expr], -) -> impl Iterator { +) -> Vec { match return_type { ReturnType::Default => vec![quote! { () }], ReturnType::Type(_rarrow, type_) => type_replacements(type_, error_exprs).collect_vec(), } - .into_iter() } /// Generate some values that we hope are reasonable replacements for a type. @@ -696,6 +695,7 @@ mod test { fn check_replacements(return_type: ReturnType, error_exprs: &[Expr], expected: &[&str]) { assert_eq!( return_type_replacements(&return_type, error_exprs) + .into_iter() .map(|t| t.to_pretty_string()) .collect_vec(), expected diff --git a/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap b/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap index 16d33f22..313bde19 100644 --- a/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap +++ b/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap @@ -140,8 +140,8 @@ src/copy_tree.rs: replace copy_tree -> Result with Ok(Default::default( src/copy_tree.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) src/copy_tree.rs: replace copy_symlink -> Result<()> with Ok(()) src/copy_tree.rs: replace copy_symlink -> Result<()> with Err(::anyhow::anyhow!("mutated!")) -src/fnvalue.rs: replace return_type_replacements -> impl Iterator with ::std::iter::empty() -src/fnvalue.rs: replace return_type_replacements -> impl Iterator with ::std::iter::once(Default::default()) +src/fnvalue.rs: replace return_type_replacements -> Vec with vec![] +src/fnvalue.rs: replace return_type_replacements -> Vec with vec![Default::default()] src/fnvalue.rs: replace type_replacements -> impl Iterator with ::std::iter::empty() src/fnvalue.rs: replace type_replacements -> impl Iterator with ::std::iter::once(Default::default()) src/fnvalue.rs: replace path_ends_with -> bool with true @@ -466,6 +466,8 @@ src/scenario.rs: replace Scenario::log_file_name_base -> String with "xyzzy".int src/source.rs: replace SourceFile::tree_relative_slashes -> String with String::new() src/source.rs: replace SourceFile::tree_relative_slashes -> String with "xyzzy".into() src/source.rs: replace SourceFile::path -> &Utf8Path with &Default::default() +src/source.rs: replace SourceFile::code -> &str with "" +src/source.rs: replace SourceFile::code -> &str with "xyzzy" src/span.rs: replace ::from -> Self with Default::default() src/span.rs: replace ::fmt -> fmt::Result with Ok(Default::default()) src/span.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) @@ -568,6 +570,7 @@ src/visit.rs: replace walk_file -> Result<(Vec, Vec)> with Ok((v src/visit.rs: replace walk_file -> Result<(Vec, Vec)> with Err(::anyhow::anyhow!("mutated!")) src/visit.rs: replace DiscoveryVisitor<'o>::enter_function -> Arc with Arc::new(Default::default()) src/visit.rs: replace DiscoveryVisitor<'o>::leave_function with () +src/visit.rs: replace DiscoveryVisitor<'o>::collect_mutant with () src/visit.rs: replace DiscoveryVisitor<'o>::collect_fn_mutants with () src/visit.rs: replace DiscoveryVisitor<'o>::in_namespace -> T with Default::default() src/visit.rs: replace >::visit_item_fn with () @@ -645,9 +648,9 @@ src/workspace.rs: replace Workspace::package_tops -> Result> wit src/workspace.rs: replace Workspace::package_tops -> Result> with Ok(vec![Default::default()]) src/workspace.rs: replace Workspace::package_tops -> Result> with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace == with != in Workspace::package_tops -src/workspace.rs: replace Workspace::top_sources -> Result>> with Ok(vec![]) -src/workspace.rs: replace Workspace::top_sources -> Result>> with Ok(vec![Arc::new(Default::default())]) -src/workspace.rs: replace Workspace::top_sources -> Result>> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::top_sources -> Result> with Ok(vec![]) +src/workspace.rs: replace Workspace::top_sources -> Result> with Ok(vec![Default::default()]) +src/workspace.rs: replace Workspace::top_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace Workspace::discover -> Result with Ok(Default::default()) src/workspace.rs: replace Workspace::discover -> Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace Workspace::mutants -> Result> with Ok(vec![]) diff --git a/src/visit.rs b/src/visit.rs index 5dc5de3d..3bef7fc9 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -11,8 +11,7 @@ use std::collections::VecDeque; use std::sync::Arc; use anyhow::Context; -use itertools::Itertools; -use proc_macro2::Ident; +use proc_macro2::{Ident, TokenStream}; use quote::quote; use syn::ext::IdentExt; use syn::spanned::Spanned; @@ -172,33 +171,31 @@ impl<'o> DiscoveryVisitor<'o> { ); } - // /// Record that we generated some mutants. - // fn record_mutant(&mut self, span: &Span, replacement: String, genre: Genre) { - // self.mutants.push(Mutant { - // source_file: self.source_file.clone(), - // function: Some(Arc::clone(se - // } + /// Record that we generated some mutants. + fn collect_mutant(&mut self, span: Span, replacement: TokenStream, genre: Genre) { + self.mutants.push(Mutant { + source_file: self.source_file.clone(), + function: self.fn_stack.last().map(Arc::clone), + span, + replacement: replacement.to_pretty_string(), + genre, + }) + } fn collect_fn_mutants(&mut self, sig: &Signature, block: &Block) { - if let Some(function) = self.fn_stack.last() { + if let Some(function) = self.fn_stack.last().map(Arc::clone) { let body_span = function_body_span(block).expect("Empty function body"); - let mut new_mutants = return_type_replacements(&sig.output, self.error_exprs) - .map(|rep| Mutant { - source_file: self.source_file.clone(), - function: Some(Arc::clone(function)), - span: body_span, - replacement: rep.to_pretty_string(), - genre: Genre::FnValue, - }) - .collect_vec(); - if new_mutants.is_empty() { + let repls = return_type_replacements(&sig.output, self.error_exprs); + if repls.is_empty() { debug!( function_name = function.function_name, return_type = function.return_type, "No mutants generated for this return type" ); } else { - self.mutants.append(&mut new_mutants); + repls.into_iter().for_each(|replacement| { + self.collect_mutant(body_span, replacement, Genre::FnValue) + }) } } else { warn!("collect_fn_mutants called while not in a function?"); @@ -362,17 +359,9 @@ impl<'ast> Visit<'ast> for DiscoveryVisitor<'_> { Vec::new() } }; - let mut new_mutants = replacements + replacements .into_iter() - .map(|rep| Mutant { - source_file: self.source_file.clone(), - function: self.fn_stack.last().map(Arc::clone), - replacement: rep.to_pretty_string(), - span: i.op.span().into(), - genre: Genre::BinaryOperator, - }) - .collect_vec(); - self.mutants.append(&mut new_mutants); + .for_each(|rep| self.collect_mutant(i.op.span().into(), rep, Genre::BinaryOperator)); syn::visit::visit_expr_binary(self, i); } } From 948f823531c1950a1c9d7c688ac89d1025540eba Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 15 Dec 2023 10:55:23 +1000 Subject: [PATCH 4/5] Don't generate mutants the same as the original --- NEWS.md | 4 ++ ..._expected_mutants_for_own_source_tree.snap | 2 +- src/source.rs | 2 +- src/visit.rs | 59 ++++++++++++++++--- 4 files changed, 57 insertions(+), 10 deletions(-) diff --git a/NEWS.md b/NEWS.md index e8e456bf..753ac5a5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,9 @@ # cargo-mutants changelog +## Unreleased + +- Improved: Don't generate function mutants that have the same AST as the code they're replacing. + ## 23.12.0 An exciting step forward: cargo-mutants can now generate mutations smaller than a whole function. To start with, several binary operators are mutated. diff --git a/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap b/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap index 313bde19..8ed356d2 100644 --- a/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap +++ b/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap @@ -68,7 +68,6 @@ src/console.rs: replace ::make_writer -> Sel src/console.rs: replace ::write -> std::io::Result with Ok(0) src/console.rs: replace ::write -> std::io::Result with Ok(1) src/console.rs: replace ::write -> std::io::Result with Err(::anyhow::anyhow!("mutated!")) -src/console.rs: replace ::flush -> std::io::Result<()> with Ok(()) src/console.rs: replace ::flush -> std::io::Result<()> with Err(::anyhow::anyhow!("mutated!")) src/console.rs: replace ::make_writer -> Self::Writer with Default::default() src/console.rs: replace ::write -> io::Result with Ok(0) @@ -572,6 +571,7 @@ src/visit.rs: replace DiscoveryVisitor<'o>::enter_function -> Arc with src/visit.rs: replace DiscoveryVisitor<'o>::leave_function with () src/visit.rs: replace DiscoveryVisitor<'o>::collect_mutant with () src/visit.rs: replace DiscoveryVisitor<'o>::collect_fn_mutants with () +src/visit.rs: replace == with != in DiscoveryVisitor<'o>::collect_fn_mutants src/visit.rs: replace DiscoveryVisitor<'o>::in_namespace -> T with Default::default() src/visit.rs: replace >::visit_item_fn with () src/visit.rs: replace || with && in >::visit_item_fn diff --git a/src/source.rs b/src/source.rs index 571bfd85..c3f31b3c 100644 --- a/src/source.rs +++ b/src/source.rs @@ -31,7 +31,7 @@ pub struct SourceFile { /// /// This is held in an [Arc] so that SourceFiles can be cloned without using excessive /// amounts of memory. - code: Arc, + pub code: Arc, } impl SourceFile { diff --git a/src/visit.rs b/src/visit.rs index 3bef7fc9..29611eeb 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -12,7 +12,7 @@ use std::sync::Arc; use anyhow::Context; use proc_macro2::{Ident, TokenStream}; -use quote::quote; +use quote::{quote, ToTokens}; use syn::ext::IdentExt; use syn::spanned::Spanned; use syn::visit::Visit; @@ -193,9 +193,24 @@ impl<'o> DiscoveryVisitor<'o> { "No mutants generated for this return type" ); } else { - repls.into_iter().for_each(|replacement| { - self.collect_mutant(body_span, replacement, Genre::FnValue) - }) + let orig_block = block.to_token_stream().to_pretty_string(); + for rep in repls { + // Comparing strings is a kludge for proc_macro2 not (yet) apparently + // exposing any way to compare token streams... + // + // TODO: Maybe this should move into collect_mutant, but at the moment + // FnValue is the only genre that seems able to generate no-ops. + // + // The original block has braces and the replacements don't, so put + // them back for the comparison... + let new_block = quote!( { #rep } ).to_token_stream().to_pretty_string(); + dbg!(&orig_block, &new_block); + if orig_block == new_block { + debug!("Replacement is the same as the function body; skipping"); + } else { + self.collect_mutant(body_span, rep, Genre::FnValue); + } + } } } else { warn!("collect_fn_mutants called while not in a function?"); @@ -368,11 +383,9 @@ impl<'ast> Visit<'ast> for DiscoveryVisitor<'_> { // Get the span of the block excluding the braces, or None if it is empty. fn function_body_span(block: &Block) -> Option { - let start = block.stmts.first()?.span().start(); - let end = block.stmts.last()?.span().end(); Some(Span { - start: start.into(), - end: end.into(), + start: block.stmts.first()?.span().start().into(), + end: block.stmts.last()?.span().end().into(), }) } @@ -507,7 +520,37 @@ fn attr_is_mutants_skip(attr: &Attribute) -> bool { #[cfg(test)] mod test { + use indoc::indoc; + use itertools::Itertools; + use super::*; + use crate::package::Package; + use crate::source::SourceFile; + + /// We should not generate mutants that produce the same tokens as the + /// source. + #[test] + fn no_mutants_equivalent_to_source() { + let code = indoc! { " + fn always_true() -> bool { true } + "}; + let source_file = SourceFile { + code: Arc::new(code.to_owned()), + package: Arc::new(Package { + name: "unimportant".to_owned(), + relative_manifest_path: "Cargo.toml".into(), + }), + tree_relative_path: Utf8PathBuf::from("src/lib.rs"), + }; + let (mutants, _files) = walk_file(&source_file, &[]).expect("walk_file"); + let mutant_names = mutants.iter().map(|m| m.name(false, false)).collect_vec(); + // It would be good to suggest replacing this with 'false', breaking a key behavior, + // but bad to replace it with 'true', changing nothing. + assert_eq!( + mutant_names, + ["src/lib.rs: replace always_true -> bool with false"] + ); + } /// As a generic protection against regressions in discovery, the the mutants /// generated from `cargo-mutants` own tree against a checked-in list. From e047f3efb2a61d5725533cb6cf0bee45c5859d0b Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 15 Dec 2023 19:13:02 +1000 Subject: [PATCH 5/5] Update tests: no dupes! --- src/visit.rs | 2 +- ...li__list_mutants_in_all_trees_as_json.snap | 90 ------------------- ...li__list_mutants_in_all_trees_as_text.snap | 3 - ...ing_so_later_missed_mutants_are_found.snap | 7 +- 4 files changed, 3 insertions(+), 99 deletions(-) diff --git a/src/visit.rs b/src/visit.rs index 29611eeb..95978a4c 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -204,7 +204,7 @@ impl<'o> DiscoveryVisitor<'o> { // The original block has braces and the replacements don't, so put // them back for the comparison... let new_block = quote!( { #rep } ).to_token_stream().to_pretty_string(); - dbg!(&orig_block, &new_block); + // dbg!(&orig_block, &new_block); if orig_block == new_block { debug!("Replacement is the same as the function body; skipping"); } else { diff --git a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap index f0af0799..fb7ed6b4 100644 --- a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap +++ b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap @@ -2440,36 +2440,6 @@ expression: buf } } }, - { - "file": "src/a.rs", - "function": { - "function_name": "one", - "return_type": "-> i32", - "span": { - "end": { - "column": 2, - "line": 3 - }, - "start": { - "column": 1, - "line": 1 - } - } - }, - "genre": "FnValue", - "package": "cargo-mutants-testdata-unapply", - "replacement": "1", - "span": { - "end": { - "column": 6, - "line": 2 - }, - "start": { - "column": 5, - "line": 2 - } - } - }, { "file": "src/a.rs", "function": { @@ -2530,36 +2500,6 @@ expression: buf } } }, - { - "file": "src/b.rs", - "function": { - "function_name": "one_untested", - "return_type": "-> i32", - "span": { - "end": { - "column": 2, - "line": 3 - }, - "start": { - "column": 1, - "line": 1 - } - } - }, - "genre": "FnValue", - "package": "cargo-mutants-testdata-unapply", - "replacement": "1", - "span": { - "end": { - "column": 6, - "line": 2 - }, - "start": { - "column": 5, - "line": 2 - } - } - }, { "file": "src/b.rs", "function": { @@ -2620,36 +2560,6 @@ expression: buf } } }, - { - "file": "src/c.rs", - "function": { - "function_name": "one", - "return_type": "-> i32", - "span": { - "end": { - "column": 2, - "line": 3 - }, - "start": { - "column": 1, - "line": 1 - } - } - }, - "genre": "FnValue", - "package": "cargo-mutants-testdata-unapply", - "replacement": "1", - "span": { - "end": { - "column": 6, - "line": 2 - }, - "start": { - "column": 5, - "line": 2 - } - } - }, { "file": "src/c.rs", "function": { diff --git a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap index 78c8d26d..72bcdbc2 100644 --- a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap +++ b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap @@ -234,13 +234,10 @@ src/lib.rs:6:5: replace try_value_coercion -> String with "xyzzy".into() ``` src/a.rs:2:5: replace one -> i32 with 0 -src/a.rs:2:5: replace one -> i32 with 1 src/a.rs:2:5: replace one -> i32 with -1 src/b.rs:2:5: replace one_untested -> i32 with 0 -src/b.rs:2:5: replace one_untested -> i32 with 1 src/b.rs:2:5: replace one_untested -> i32 with -1 src/c.rs:2:5: replace one -> i32 with 0 -src/c.rs:2:5: replace one -> i32 with 1 src/c.rs:2:5: replace one -> i32 with -1 ``` diff --git a/tests/cli/snapshots/cli__mutants_are_unapplied_after_testing_so_later_missed_mutants_are_found.snap b/tests/cli/snapshots/cli__mutants_are_unapplied_after_testing_so_later_missed_mutants_are_found.snap index 66c4ae69..a07a3545 100644 --- a/tests/cli/snapshots/cli__mutants_are_unapplied_after_testing_so_later_missed_mutants_are_found.snap +++ b/tests/cli/snapshots/cli__mutants_are_unapplied_after_testing_so_later_missed_mutants_are_found.snap @@ -2,12 +2,9 @@ source: tests/cli/main.rs expression: stdout --- -Found 9 mutants to test +Found 6 mutants to test Unmutated baseline ... ok -src/a.rs:2:5: replace one -> i32 with 1 ... NOT CAUGHT src/b.rs:2:5: replace one_untested -> i32 with 0 ... NOT CAUGHT -src/b.rs:2:5: replace one_untested -> i32 with 1 ... NOT CAUGHT src/b.rs:2:5: replace one_untested -> i32 with -1 ... NOT CAUGHT -src/c.rs:2:5: replace one -> i32 with 1 ... NOT CAUGHT -9 mutants tested: 5 missed, 4 caught +6 mutants tested: 2 missed, 4 caught