-
-
Notifications
You must be signed in to change notification settings - Fork 33
Include mutant name in JSON output (issue #480) #573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include mutant name in JSON output (issue #480) #573
Conversation
src/visit.rs
Outdated
| /// Record that we generated some mutants. | ||
| fn collect_mutant(&mut self, span: Span, replacement: &TokenStream, genre: Genre) { | ||
| self.mutants.push(Mutant { | ||
| name: String::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems sub-optimal to set it to "" in the constructor and fill it in later...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you and actually had the same thoughts while implementing this.
Initializing name as an empty string and then filling it later felt a bit off to me as well.
I tried to think through the options and currently see three possible directions — maybe you see more, and I’d really appreciate your input before I change the PR.
1. What the PR currently does (compute name in walk_tree)
Right now, I construct all Mutant values with name: String::new() in the visitor, and then later in walk_tree I do:
for mutant in &mut mutants {
mutant.name = mutant.name(true);
}This has the advantage that there is a single place where the canonical name (with line/col) is set, without duplicating the naming logic. But I agree with you that it’s a bit sub‑optimal to have an invalid “empty” name during the lifetime of the Mutant and then patch it up later.
2. Compute name immediately at each construction site
An alternative would be to compute the canonical name right after constructing each Mutant in the visitor, for example in collect_mutant:
/// Record that we generated some mutants.
fn collect_mutant(&mut self, span: Span, replacement: &TokenStream, genre: Genre) {
let mut mutant = Mutant {
name: String::new(),
source_file: self.source_file.clone(),
function: self.fn_stack.last().cloned(),
span,
short_replaced: None,
replacement: replacement.to_pretty_string(),
genre,
target: None,
};
// Needs a fully-initialized `Mutant`, so done as a second step.
mutant.name = mutant.name(true);
self.mutants.push(mutant);
}and apply the same pattern to the two other places where we construct a Mutant (for match arms and struct fields).
Because Mutant::name(true) needs &self and uses several fields (source_file, span, function, etc.), it can’t be called inside the struct literal itself — we need a constructed Mutant first. So this two‑step (“construct then compute name”) seems unavoidable somewhere.
With this approach:
- Every
Mutanthas a validnamefrom the moment it’s stored inself.mutants. - We can remove the loop in
walk_treethat recomputes names. - The
nameformatting logic stays centralized inMutant::name(no duplication of the string‑assembly code).
This is my current favourite because it’s relatively small and keeps the semantics clear: “discovery creates fully‑named mutants.”
3. A helper constructor like Mutant::new_discovered
A slightly more structured variant of (2) would be to introduce a helper constructor that encapsulates the two‑step process:
impl Mutant {
/// Construct a mutant discovered while walking source code.
///
/// This computes and stores a canonical human‑readable name that includes
/// the file path, line/column, and change description. The name is used
/// for filtering, reporting, and JSON output.
pub fn new_discovered(
source_file: SourceFile,
function: Option<Arc<Function>>,
span: Span,
short_replaced: Option<String>,
replacement: String,
genre: Genre,
target: Option<MutationTarget>,
) -> Self {
let mut mutant = Mutant {
name: String::new(),
source_file,
function,
span,
short_replaced,
replacement,
genre,
target,
};
mutant.name = mutant.name(true);
mutant
}
}Then the visitor sites would call:
self.mutants.push(Mutant::new_discovered(
self.source_file.clone(),
self.fn_stack.last().cloned(),
span,
None,
replacement.to_pretty_string(),
genre,
None,
));and similar for match arms and struct fields (with appropriate short_replaced / target).
This avoids repeating the “construct then compute name” pattern at each call site, at the cost of one more constructor‑like function on Mutant. I’m not sure if you’d consider that over‑abstracted for this codebase or acceptable.
Question / next step
Given these options, I’d be happy to rework the PR accordingly. My slight preference is option 2 (compute name immediately at the construction sites and remove the loop in walk_tree), but I’m also happy to go with option 3 if you’d rather keep the construction logic inside Mutant itself.
If you have a different pattern in mind that fits better with how you’d like Mutant to evolve, I’d be very interested to hear it and adjust the PR. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It seems the third option is the best for now, because this confines it inside the Mutant.
Perhaps later the code in Mutant::name should be refactored to run at construction time and not need an instance, but I guess that's a little complex because of the variations for generating styled text and optionally including the line/column. Let's leave that out of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the quick feedback and the clear direction.
I’ve updated the PR to follow option 3 as you suggested:
- Introduced
Mutant::new_discovered(...)which:- Takes the fields needed to construct a
Mutant. - Builds the struct.
- Immediately computes and stores the canonical human-readable
nameusingname(true).
- Takes the fields needed to construct a
- Scoped the constructor as
pub(crate)so it’s only used within the crate and remains internal implementation detail.
For consistency and readability, I also updated all three discovery sites to use the same pattern:
let mutant = Mutant::new_discovered(
self.source_file.clone(),
self.fn_stack.last().cloned(),
span,
short_replaced,
replacement,
genre,
target,
);
self.mutants.push(mutant);Concretely, this now applies to:
- The generic
collect_mutanthelper (binary/unary ops, guards, etc.). - Match arm deletion mutants.
- Struct field deletion mutants (when there is a base expression like
..Default::default()).
As a result:
- Every
Mutantdiscovered by the visitor has a fully initializednameas soon as it’s created. - There is no longer a post-processing loop in
walk_treethat mutates thenamefield afterwards. - The naming logic is still centralized in
Mutant::name, and the JSON serialization simply includes the storedname.
Verification
After the refactor I re-ran:
cargo test(full test suite) – all tests pass.- Manual checks on a small sample project:
cargo mutants --list --jsonto confirm thenamefield appears in the list JSON with the same human-readable string as the CLI output.cargo mutantsand inspection ofmutants.jsonandoutcomes.jsonto confirm both contain thenamefield and that the value matches the printed mutant name.
If you’d like me to tweak the constructor signature, visibility, or documentation style, I’m happy to adjust. Thanks again for the guidance – this was a fun one to work on!
sourcefrog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from this one thing.
Fixes #480.
Summary
This change adds the same human-readable mutant name string that
cargo mutants --listprints to the JSON output:cargo mutants --list --jsonmutants.out/mutants.jsonmutants.out/outcomes.jsonThe goal is to make it easier for tools to work with a stable,
human-readable identifier for each mutant, without needing to reconstruct
it from other fields.
Implementation details
Add a
name: Stringfield toMutant:After discovery, compute the canonical name once, including line/col,
reusing the existing
Mutant::name(true)formatting:Extend the custom
Serialize for Mutantimplementation to include thenamefield so it appears in:--list --jsonmutants.out/mutants.jsonMake the same
nameavailable inoutcomes.jsonentries so tools canjoin outcomes back to mutants by this human-readable identifier.
CLI behavior is unchanged:
Mutant::name(show_line_col: bool)andMutant::to_styled_string(show_line_col: bool)are kept as-is andstill used for textual output.
namefield follows the same format as the existingname(true)output (file path, line, column, description).This is an additive JSON schema change: existing fields are preserved and
a new
namestring is added.Testing
cargo test(workspace) – all tests pass.Updated insta snapshot tests to account for the new
namefield inJSON output:
list_mutants_in_cfg_attr_test_skip_jsonlist_mutants_in_factorial_jsonlist_mutants_json_well_testedlist_mutants_regex_filters_jsonuncaught_mutant_in_factorialManual verification on a small test project:
cargo mutants --output mutants.outproducesmutants.out/mutants.jsonwith anamefor each mutant, matchingthe
--listoutput.mutants.out/outcomes.jsonalso includes the samenamefor eachoutcome entry.