Mutate match arms #485

zaneduffield · 2025-01-18T09:40:20Z

Implements #484.

I created just one new genre for the two new kinds of mutations here, but maybe it should be two new genres?

Another thing I wasn't sure about was whether and how to change Mutant::styled_parts to more simply render the new mutations. If the match arm or guard being mutated is long then the output could become quite hard to read.

With this new mutation genre, there are some uncovered mutations in the cargo-mutants source tree that I've added a commit to address. A couple of things about that are worth noting

I removed a match arm in src/fnvalue.rs that I think was redundant. It's possible that it deserves to be there as an optimisation though, which would make it hard to cover.

I wasn't able to add a test to cover the mutation on a match arm relating to Cargo `default-members.

cargo-mutants/src/workspace.rs

Lines 204 to 207 in 437a36b

    
           Ok(default_packages) if default_packages.is_empty() => { 
        
               debug!("manifest has no explicit default packages"); 
        
               PackageSelection::All 
        
           }

I think this might be redundant as well. The docs say that the default value for this property is non-empty, which would mean that for it to be empty would require it to actually be empty:

When unspecified, the root package will be used. In the case of a virtual workspace, all members will be used (as if --workspace were specified on the command-line).

sourcefrog

Implements #484.

Thanks, this looks good! We should also document the new patterns in https://github.com/sourcefrog/cargo-mutants/blob/main/book/src/mutants.md

I created just one new genre for the two new kinds of mutations here, but maybe it should be two new genres?

I think two distinct new genres is better.

Another thing I wasn't sure about was whether and how to change Mutant::styled_parts to more simply render the new mutations. If the match arm or guard being mutated is long then the output could become quite hard to read.

Probably the rendering code should know to abbreviate long replacements, and just say "replace match arm with xxx". In a sense it has this special coded for functions now, since it doesn't quote the whole of the function body that's being replaced... But, let's not do that in this PR.

With this new mutation genre, there are some uncovered mutations in the cargo-mutants source tree that I've added a commit to address. A couple of things about that are worth noting

I removed a match arm in src/fnvalue.rs that I think was redundant. It's possible that it deserves to be there as an optimisation though, which would make it hard to cover.

That's great that it found this! I think letting it hit the general non-empty case is fine: the iterator will be empty and that should finish quickly. I think the code you removed is just a relic of a time when it didn't know how to expand tuples, only to replace unit types.

I wasn't able to add a test to cover the mutation on a match arm relating to Cargo `default-members.

cargo-mutants/src/workspace.rs

Lines 204 to 207 in 437a36b

Ok(default_packages) if default_packages.is_empty() => {

debug!("manifest has no explicit default packages");

PackageSelection::All

}

I think this might be redundant as well. The docs say that the default value for this property is non-empty, which would mean that for it to be empty would require it to actually be empty:

When unspecified, the root package will be used. In the case of a virtual workspace, all members will be used (as if --workspace were specified on the command-line).

Yes I guess we could just delete that arm.

sourcefrog · 2025-01-21T15:45:01Z

testdata/everything_skipped/src/bin/everything-skipped.rs

+}
+
+impl Foo {
+    #[mutants::skip]


I think this addition to the testdata is harmless but we don't exactly need it... it's just proving again that mutants::skip works...?

Or was there a coverage gap without it?

I think I started adding this when I thought it was possible to add #[mutants::skip] to a statement. I'll delete it.

sourcefrog · 2025-01-21T15:48:10Z

testdata/well_tested/src/lib.rs

@@ -23,3 +23,4 @@ mod slices;
 mod static_item;
 mod struct_with_lifetime;
 mod traits;
+mod matches;


FYI I now feel it's probably better to add mutation patterns as unit tests in visit.rs rather than here, because actually testing this tree is contributing to mutants' own test suite becoming slower, and the big Insta snapshots are unwieldy.

However you don't necessarily need to rewrite this now. I'm not settled on converting them to unit tests.

I was following the existing testing pattern, but I'm happy to rewrite as unit tests.

sourcefrog · 2025-01-21T15:50:08Z

testdata/well_tested/src/matches.rs

+        }
+    }
+
+    // Don't use the value of the match expression, because we don't expect it to be mutated.


Suggested change

// Don't use the value of the match expression, because we don't expect it to be mutated.

// This match expression has no default so won't be mutated.

// This doesn't count as a missed mutant because the function

// return value doesn't depend on the match expression.

sourcefrog · 2025-01-21T15:53:55Z

tests/workspace.rs

+fn in_workspace_only_default_members_are_mutated_if_present() {
+    let tmp = copy_of_testdata("workspace_default_members");
+    run()
+        .args(["mutants", "--no-shuffle", "-d"])


Maybe this should just run with --list which will be a bit faster: we've probably sufficiently tested elsewhere that running tests from packages works.

sourcefrog · 2025-01-21T15:53:57Z

tests/workspace.rs

+fn in_workspace_only_default_members_are_mutated_if_present() {
+    let tmp = copy_of_testdata("workspace_default_members");
+    run()
+        .args(["mutants", "--no-shuffle", "-d"])


Maybe this should just run with --list which will be a bit faster: we've probably sufficiently tested elsewhere that running tests from packages works.

sourcefrog · 2025-01-21T16:17:29Z

I see some mutants come out like this:

src/process.rs:187:17: delete ' ' | '\\' | '\'' | '"' => {
                    r.push('\\');
                    r.push(c);
                } in quote_argv

src/fnvalue.rs:186:9: delete Type::Reference(syn::TypeReference {
            mutability: Some(_),
            elem,
            ..
        }) => match &**elem {
            Type::Slice(TypeSlice { elem, .. }) => iter::once(quote! { Vec::leak(Vec::new()) })
                .chain(
                    type_replacements(elem, error_exprs).map(|r| quote! { Vec::leak(vec![ #r ]) }),
                )
                .collect_vec(),
            _ => {
                // Make &mut with static lifetime by leaking them on the heap.
                type_replacements(elem, error_exprs)
                    .map(|rep| {
                        quote! { Box::leak(Box::new(#rep)) }
                    })
                    .collect_vec()
            }
        }, in type_replacements

and others are even longer.

Maybe we should just change this to "delete match arm" or "replace match guard with true".

zaneduffield · 2025-01-22T12:31:03Z

Thanks, this looks good! We should also document the new patterns in https://github.com/sourcefrog/cargo-mutants/blob/main/book/src/mutants.md

I've addressed all the comments but this. I won't have any time to work on this until next week, though.

sourcefrog self-assigned this Jan 21, 2025

sourcefrog requested changes Jan 21, 2025

View reviewed changes

zaneduffield added 3 commits January 22, 2025 23:24

Mutate match arms by deletion when _ pattern is present

06eb438

Mutate guards on match arms by replacement with true/false

3d757cc

Add tests to cover new mutations in cargo-mutants

96ab9a3

zaneduffield force-pushed the mutate-match branch from 437a36b to 96ab9a3 Compare January 22, 2025 12:29

zaneduffield requested a review from sourcefrog January 22, 2025 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutate match arms #485

Mutate match arms #485

zaneduffield commented Jan 18, 2025

sourcefrog left a comment

sourcefrog Jan 21, 2025

zaneduffield Jan 22, 2025

sourcefrog Jan 21, 2025

zaneduffield Jan 22, 2025

sourcefrog Jan 21, 2025

zaneduffield Jan 22, 2025

sourcefrog Jan 21, 2025

sourcefrog Jan 21, 2025

sourcefrog commented Jan 21, 2025

zaneduffield commented Jan 22, 2025

	Ok(default_packages) if default_packages.is_empty() => {
	debug!("manifest has no explicit default packages");
	PackageSelection::All
	}

-    // Don't use the value of the match expression, because we don't expect it to be mutated.
+    // This match expression has no default so won't be mutated.
+    // This doesn't count as a missed mutant because the function
+    // return value doesn't depend on the match expression.

Mutate match arms #485

Are you sure you want to change the base?

Mutate match arms #485

Conversation

zaneduffield commented Jan 18, 2025

sourcefrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcefrog commented Jan 21, 2025

zaneduffield commented Jan 22, 2025