chore(mutants): make structural removal the policy, and close the ifd residue - #575
Merged
Merged
Conversation
Issue #110 asked for the policy to be written down where the mutation-testing rules already live. What this workspace has actually been doing for the last few surveys is now the rule: - A survivor is a question about the code before it is a question about the suite. gamut-tiff removed five shift-or equivalents, gamut-deflate five more, gamut-avif four, and gamut-webp deleted a guard its constructor already made -- each time because the mutant could be made not to exist. Only what survives that gets a test, and only what survives a test gets an exclusion. - Exclusions live in `.cargo/mutants.toml` and nowhere else. There is no `#[mutants::skip]` in this tree today and there should not be one: a single reviewable list is what makes "is this entry still justified?" answerable. `docs/mutation-testing.md` gains what a reader needs to interpret a run rather than just launch one. Exit code 3 is a timeout, not a clean run. The tool's whole vocabulary is a default-valued body, a swapped binary operator, a deleted `!`, a forced match guard and a deleted match arm -- confirmed from `cargo mutants --list` on gamut-ifd -- so a missing arm, a wrong literal and a wrong alternative inside an or-pattern (`delete match arm A(v) | B(v)` removes both) are all things the gate cannot report. And a mutant nothing can kill is often a sign the expression is unobservable, which is a reason to delete it. Refs #110
Three loops in the byte-accounting path advanced a cursor by hand, so whether they terminated was a property of their own arithmetic rather than of the range or slice they walked. A survey of gamut-ifd (652 mutants, four shards) reported no survivors and eight TIMEOUTs, and seven of those eight are these loops: mutate the advance and the loop stops making progress, which cargo-mutants can only report as a timeout at 60s a scenario -- never as a wrong answer a test could catch. - `all_zero` chunked with `pos += n` over `while pos < end`; stepping the range yields the same chunk starts and removes both the advance and the comparison (`+= -> *=`, `< -> <=`). - `ReadLedger::record` merged spans with a hand-advanced `j`; the spans that merge are the run from `i` whose starts still reach `end`, so `take_while` on the sub-slice bounds it (`+= -> *=`). - `write_with`'s pool placement re-ran `pins.iter().find(...)` until nothing overlapped. `pins` is sorted by offset and validated disjoint immediately above, so a jump can never re-expose a pin already behind the cursor and one forward pass settles the placement. Taking the later of the two positions makes a pin the value already clears a no-op, which retires the second comparison entirely (`&& -> ||`, `< -> <=`, `+ -> *`, `+ -> -`). Behaviour is unchanged in each case, and the surviving mutants are ordinary killable ones: the crate re-surveys at 0 missed and 0 timeouts. Refs #110
The eighth and last TIMEOUT in the gamut-ifd survey is `ReadLedger::subtract`'s skip loop, and unlike the other seven it cannot be rewritten away. Dropping the claims that end at or before `pos` is what leaves the covered arm a claim ending *after* `pos`, and so what makes `pos` advance; relax the comparison and the walk spins instead of producing a wrong answer. Bounding the walk by a slice (`partition_point` per span) only moves the problem: it trades the hang for an equivalent mutant at the `r.end() == span.start` boundary, where including or excluding an already-settled claim cannot change the output either way. So the skip becomes its own three-line function. `subtract` has two other `<=` comparisons whose `>` twins are caught, and a name is a stabler anchor than a line for the exclusion that has to document this one. The function returns the surviving claim rather than nothing, which keeps its *body* mutant killable: `with None` says no claim covers anything, the walk then reports every read as unclaimed, and `subtract_reports_exactly_the_unclaimed_reads` fails. Refs #110
One more thing a survivor is not: an argument for changing what the code promises. A bound that exists because the format defines it -- a nesting depth, a size cap, a table length -- is not a knob for test assertability, and lowering one so its boundary comes within reach of the suite can leave an encoder writing a file its own reader refuses. The contract is the deliverable; the gate is the instrument. Refs #110
`ReadLedger::subtract`'s `while pos < end` walks one ledger span. Relaxed to `<=` the body runs once more at `pos == end`, where the span is exhausted and nothing can move `pos`: the settled claims are already dropped, so the covered arm would assign `pos` the value it holds and the fallback arm computes `stop == end`. The walk spins and produces no output any test can check. It also pushes a zero-length range every turn, which is what makes its verdict depend on the runner: under a tight address-space cap the scenario aborts and scores `caught` before the 60s test timeout fires, and under a looser one it is a TIMEOUT. It scored `caught` in a local survey and TIMEOUT on CI from the same tree. An exclusion is what makes the gate say the same thing on both. Refs #110
`ReadLedger::subtract` walked one shared `Peekable` cursor across every ledger span, with a `while pos < end` loop whose progress depended on a settled-claim skip. Two of its operators could then be reported only as a mutation-testing timeout, and the skip's own comparison was worse than that: relaxed, it drops the *live* claims instead of the settled ones, so the walk terminates with a wrong answer far more often than it hangs. Take the claims that can touch a span as a sub-slice instead -- the run that reaches past the span's start, up to the last that begins before its end -- and drive the walk with a `for` over it plus one trailing push. The data bounds the loop, so no arithmetic has to stay right for it to terminate, and both operators become ordinary comparisons a test can pin. Behaviour-identical: over every ledger/claim pair drawn from a 10-cell universe (1048576 pairs, both lists in the normalised form the walk receives) the new walk returns byte-for-byte what the old one returned. Refs #110
Both ledger walks carried a comment claiming that the loop a hand-advanced cursor drives "could then be reported only as a mutation-testing timeout, never as a wrong answer". That is true of the one mutant each comment names -- the one that stalls the cursor -- and false as the general statement it reads as: the skip this branch removed had a comparison whose relaxed twin dropped the *live* claims and terminated with a wrong answer on 230,890 of 426,877 enumerated ledger/claim pairs, hanging on only 59,501 of them. Say which mutants stall and which merely mis-step, and claim unkillability only for the first kind. Refs #110
The list's header called everything in it "provably equivalent". Two other claims were already in there: about forty entries argue that the mutant moves only a choice the format leaves free -- a different but equally valid encoding, which is not equivalence -- and twenty-one argue that the mutant is detected but reportable only as a TIMEOUT, which is not unkillable either. A reader could not tell which claim an entry made without reading its prose. Split the array into two labelled groups, UNKILLABLE and NON-TERMINATING, move every hang entry under the second, and rewrite the header to state both claims and what anchoring each kind needs. No regex changes: the entry set before and after this commit is identical. Add one entry, for `ReadLedger::subtract`'s `take_while` bound. `merged` is coalesced and carries no zero-length claim, so at most one claim can begin exactly at a span's end, and it contributes exactly what the trailing push would have: relaxing `<` to `<=` is byte-identical output. Argued at the entry and checked over all 1048576 ledger/claim pairs of a 10-cell universe, whose output fingerprints match. Line/column-anchored, so the `==` and `>` twins of the same comparison stay in the survey. Refs #110
The verb list was written from three crates and was two verbs short: over the whole tree cargo-mutants also deletes a unary `-` (540 instances) and a field from a struct literal (35). Publish the one-line command that derives all seven from `cargo mutants --list`, with what it returns today, so the next reader re-derives rather than trusts. Narrow the literal claim with it: a literal in an expression or a guard operand is never mutated, but a body that is just a literal is replaced wholesale, so that one is reachable. Give the "never narrow a contract" rule the test it was missing -- whether any conformant input can tell the old bound from the new. A bound no conformant input reaches is arbitrary, and tightening it while citing the clause that says so is a correction; a bound conformant input does reach is the deliverable, and lowering it to bring a boundary within the suite's reach is what makes an encoder emit a file its own reader refuses. Both cases are named. Scope the two documents against each other: policy is stated once, in `AGENTS.md`, and the runner document says how to read a result -- including that an allocation-runaway mutant's verdict currently depends on the cap the runner derives (#613) -- and points at the policy rather than restating it. Refs #110
Regrouping the array moved one comment line one entry too far: the sentence naming `code_length_count`'s trim floor left with the non-terminating entries while the rest of its paragraph stayed behind, so group 1 opened a block mid-sentence and group 2 carried a heading for an entry it does not hold. Comment only; the entry set is untouched. Refs #110
The sentence introducing the derived vocabulary said the written list had been short "twice", which is a claim about other lists. Say what is true of this one: it was taken from three crates and missed two verbs. Refs #110
Four entries argued non-termination from under the UNKILLABLE heading, whose claim is that no test can distinguish the mutant -- and one block heading asserted both at once. Move `build_optimal_table`, both `isqrt` entries and the trellis backtrack terminator to the NON-TERMINATING group, split the `rd` heading so each half states one claim and points at the other, and reunite the `code_length_count` paragraph with its own entry twenty lines below, where it had been reading as an explanation of `decode_image_data`. The header now states the rule the file follows rather than a stricter one it does not: one regex per claim, covering several generated mutants only where one argument covers them all, and the two anchors written out with what each trades -- line:column stops matching silently when code above it moves, a signature survives movement but can reach a mutant nobody argued for. The entry set is byte-for-byte the same 200 regexes; only their order, their grouping and the prose around them change. Refs #110
`.cargo/mutants.toml` holds entries that deliberately cover several generated mutants -- one statement's three identical advances, one site's two operator swaps -- and each says in its own comment why. "One regex per mutant" was therefore a rule its own file contradicted; state it per claim, with the condition that makes a multi-mutant entry legitimate, and point at the file's header for which anchor trades what. Also name the price of the structural removal the bullet above recommends: the rewrite takes the site's killable siblings with it, so a survey that shrinks is not thereby a stronger one. Refs #110
The rule tests a proposed narrowing against conformant inputs, which is the right floor for an encoder. A reader's loose bound is not there for conformant files: it is what carries the malformed ones the world actually holds, so a narrowing that passes the conformance test can still cost real decodes. Require naming what it stops accepting, beside the clause that permits it. Refs #110
The census ended in a catch-all that rewrote anything shaped `replace X with Y` to "operator swap". A future cargo-mutants verb of that shape would have been absorbed into the count silently and the seven-verb claim the paragraph rests on would have gone false with no signal -- the exact failure the derivation replaced a hand-written list to avoid. Each rule now matches one verb's shape (operators against their own alphabet, guards against `with true|false`), and an awk tail prints anything unmatched as UNCLASSIFIED and exits non-zero. Tightening the operator rule surfaced 110 swaps that carry no `in <fn>` tail because they sit outside any function body, which the catch-all had been absorbing; the rule admits them explicitly, so the seven counts are unchanged and the pipeline exits 0 on the tree as it stands. Refs #110
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #110 asks for two things: the mutation-survivor policy written down where the rules
already live, and the enumerated survivor residue closed. This branch does the policy and the
gamut-riff/gamut-ifdresidue; the large-crate surveys are out of scope and filed as#585.
Policy (
AGENTS.md,docs/mutation-testing.md). The rule is now what this workspace hasactually been doing: remove the mutant before you exclude it. A survivor is a question about
the code before it is a question about the suite, a mutant that hangs the suite is a survivor
too, and exclusions live only in
.cargo/mutants.toml(there is no#[mutants::skip]in thistree and there should not be one).
docs/mutation-testing.mdgains what a reader needs tointerpret a run: exit code 3 is a timeout rather than a clean run, an allocation-runaway
mutant's verdict currently depends on the address-space cap the runner derives (filed as #613),
and the tool's whole vocabulary is seven verbs — so a missing match arm, a wrong literal in an
expression and a wrong alternative inside an or-pattern are all invisible to the gate.
The vocabulary is now derived, not listed by hand. The document carries the command and what
it returns, and the command runs verbatim out of the document:
25 215 mutants, seven verbs. The hand-written list this replaces was derived from three crates
and was two verbs short:
delete -(540 workspace-wide) anddelete field(35). Theliteral claim is narrowed with it — a literal in an expression or a guard operand is never
mutated, but a body that is a literal is replaced wholesale, so
replace SrgbIntent::code -> u8 with 0reaches one.The census now fails on a line it cannot classify. Its last rule used to be a catch-all
rewriting anything shaped
replace X with Yto "operator swap", so a future cargo-mutants verbof that shape would have been folded into the count and the seven-verb claim above would have
gone false with no signal — which is the failure a derivation is written to replace. Each rule
matches one verb's exact shape now, and an
awktail prints anything left over asUNCLASSIFIEDand exits non-zero. Tightening the operator rule to its own alphabet surfaced 110swaps the catch-all had been absorbing silently: they carry no
in <fn>tail because they sitoutside any function body (
const/staticinitialisers, and one array length in a signature).The rule admits them explicitly, so the seven counts are unchanged and the pipeline exits 0 on
the tree as it stands.
One rule is added on this pass: never narrow a contract to make a mutant assertable, and it
now carries the test that decides a case — whether any conformant input can tell the old bound
from the new. A bound no conformant input reaches is arbitrary, and tightening it while citing
the clause that says so is a correction; a bound conformant input does reach is the deliverable.
Under that test PR #520's decision 14 stands: it lowered a reader's EXIF depth bound from
16 to 2 citing EXIF 2.3 §4.6.3, under which no conformant tree is deeper, so no input that was
accepted is now refused. The forbidden case is the other one, and both are named in the rule.
Residue. Measured fresh on this base rather than taken from the (months-old) issue text.
gamut-riff — 165 mutants, 157 caught, 8 unviable. No survivors, no timeouts, nothing to
change.
gamut-ifd — 652 mutants, 587 caught, 57 unviable: no survivors and eight TIMEOUTs.
Every one of the eight is now removed structurally rather than excluded. Seven are
hand-advanced cursors whose termination was a property of their own arithmetic (
all_zero,ReadLedger::record,write_with's pool placement). The eighth and ninth both live inReadLedger::subtract, and an earlier revision of this branch excluded them; that isreversed. The walk is now driven by a
forover the sub-slice of claims that can touch thespan, so the data bounds it, and both former hangs are ordinary comparisons the suite kills.
The rewrite introduces exactly one mutant no test kills, and it is provably equivalent:
relaxing the sub-slice bound
r.start < endto<=admits at most one extra claim — the onebeginning exactly at the span's end, since
mergedis coalesced and carries no zero-lengthclaim — and that claim pushes the same trailing gap the loop's tail would have pushed and
leaves
pos == end. Argued at the entry and checked over every ledger/claim pair drawn from a10-cell universe (1 048 576 pairs), whose output fingerprints are identical. It is excluded
line/column-anchored, so the
==and>twins of the same comparison stay in the survey andare caught.
The crate re-surveys at 637 mutants, 580 caught, 57 unviable, 0 missed and 0 TIMEOUTs,
measured at this branch's head across all four round-robin shards.
.cargo/mutants.tomlis split into two labelled groups. Its header called everything in it"provably equivalent"; two other claims were already in there — a body of entries arguing that
the mutant moves only a choice the format leaves free, and a smaller one arguing that the mutant
is detected but reportable only as a TIMEOUT. Neither is equivalence. The array is now GROUP 1
(unkillable: equivalent, or a correctness-preserving free choice) and GROUP 2 (non-terminating).
The membership is derived, not asserted. Parsing the array with each entry attached to the
comment block directly above it gives 176 entries in GROUP 1, 24 in GROUP 2, 200 in total.
Four entries whose comments argue non-termination had been left under the UNKILLABLE heading —
build_optimal_table, bothisqrtentries, and the trellis backtrack terminator — and are nowunder the one that says so; the
rdheading that had claimed "non-terminating andprovably-equivalent" at once is split, so each half states one claim and points at the other.
Two comments that had drifted from their entries are back beside them:
ProgCoder::ac_refine's,which sat 39 lines above the entry it explains, and
code_length_count's, which sat overdecode_image_data's entry and read as an explanation of it.No regex changed at any point in this branch's regrouping. The entry set before and after is
identical as a multiset, and
cargo mutants --listselects the same 24 353 of 25 215generated mutants at the previous head and at this one, compared line by line.
The header also states the rule the file follows rather than a stricter one it does not. "One
regex per mutant" was contradicted by the file it governs: ten entries carry an explicit
alternation over sibling mutants, four of them arguing in their own comment why one entry
deliberately covers several. The rule is stated per claim, with the condition that makes such
an entry legitimate — one statement, or one site's operator class, and a single argument that
covers all of them. And the anchoring trade is written out, since the file argues both sides of
it:
file:line:columnpins one mutant exactly but stops matching silently when code above itmoves, while a function signature survives movement but can reach a mutant nobody argued for.
Correction to the previously published measurement
The "after" row in an earlier revision of this body — 638 mutants, 0 missed, 0 timeouts — was
measured at
2834bce6, one commit beforea425fc09added the second exclusion, and sonever described the head it was published against (
cargo mutants --listgives 638 there and637 at the head). The row below is measured at this branch's head and is the only survey this
PR claims.
Validation
Run in the worktree, whole-workspace commands inside a memory-capped scope
(
MemoryMax=16G,MemorySwapMax=0,CARGO_BUILD_JOBS=2,ulimit -v 12000000):cargo test -p gamut-ifd --all-featurescargo clippy -p gamut-ifd --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkmise run check-testsmise run check-commitsmise run lintmise run testmise run mutants-crate gamut-ifd --shard 0/4(at this head)mise run mutants-crate gamut-ifd --shard 1/4(at this head)mise run mutants-crate gamut-ifd --shard 2/4(at this head)mise run mutants-crate gamut-ifd --shard 3/4(at this head)mise run mutants-crate gamut-ifd --shard i/4(×4, on the base)mise run mutants-crate gamut-riff --shard i/2(×2, on the base)Incremental (PR diff)shards 0-3 (the--in-diffgate)Format & Metadata,Clippy & DoctestsRe-run for the third round, which touches
.cargo/mutants.toml,AGENTS.mdanddocs/mutation-testing.mdonly:__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkmise run check-testsmise run check-commitscargo mutants --list --no-config | wc -lreplace call foo with bar in bazline appendedUNCLASSIFIED: 1 replace call foo with bar in baz, exit 1cargo mutants --listat10505d9fvs at this headpython3 -covertomllib: the file parses, every regex compiles10505d9fvs this headexclude_reonorigin/mastervs this headNo crate source changed in this round, so no mutation survey is claimed for it; the two
cargo mutants --listruns are the evidence that the exclusion file's reorganisation selectsexactly the same mutants as before.
Every shard exited 0. The four
--shard k/4selections are round-robin and disjoint, and theircounts sum to the 637 that
cargo mutants -p gamut-ifd --listreports at this head — one fewerthan the 638 it reports without the single exclusion this branch adds.
The
__CARGO_TEST_ROOTprefix is a known nested-worktree artefact: cargo otherwise walks pastthe worktree root when loading the
tooling/*manifests. No manifest was changed for it.Risks and rollout
Documentation plus a bounded set of local rewrites in
gamut-ifd. No public API change. Eachrewrite is behaviour-preserving by an argument stated at the site: stepping a range yields the
same chunk starts as a hand-advanced cursor; the spans that merge are the run whose starts still
reach
end; the claims that touch a span are the run that reaches past its start; andpinsissorted and validated disjoint immediately above the pool placement, so one forward pass settles
it. The existing byte-accounting tests and the
invariantsproperty cover all of them, and theclaim-walk rewrite is additionally pinned by an exhaustive differential over a 10-cell universe.
Against
origin/masterthe exclusion file grows by one entry, 199 to 200. Earlier revisionsof this body read that the other way round, and this section is corrected in place because it is
the current description rather than a dated entry. The two entries described there as "removed"
were never on the default branch: this branch added them itself (
d8db951a,a425fc09) andreverted them in
2505280donce the rewrite made them unnecessary, so they net to zero againstthe base. What a reviewer reads in the diff is one entry added — the equivalence claim on
ReadLedger::subtract'stake_whilebound, carrying both an argument and an exhaustive check.This round changes no file under
crates/, so it claims no mutation survey. The evidence thatreorganising the exclusion file is inert is the identical
cargo mutants --listselection eitherside of it.
The residual risk is the line/column anchor on the new entry: if code above it moves and nobody
re-runs the crate's survey, the regex stops matching. That fails loudly — the mutant
reappears and the gate goes red — rather than silently suppressing something killable.
Issue
Refs #110. The remainder — mutation surveys of gamut-jpeg, gamut-png, gamut-webp, gamut-av1,
gamut-avif, gamut-cmm, gamut-tiff, gamut-dng and gamut-heic — is filed as #585. The runner's
machine-dependent address-space cap, which makes an allocation-runaway mutant score
caughtonone machine and
TIMEOUTon another, is filed as #613.Decisions taken
No human approved this plan. This is an unattended run; the record above is what a human reads
afterwards.
Unresolved review notes
None.