# Code Health Review — 2026-07-06
Scope: duplicate code, code length, refactoring/generalization opportunities, structure,
readability. Ordered by expected payoff within each section. Line numbers are as of commit
f5e78e0.
## Status (2026-07-07)
Implemented in the working tree, verified by the full test suite (266 passed; only the two
pre-existing failures remain: `rust_algorithm_change` / `rust_turbopack_module_rule`
`matches_human_solution`) and by `benchmark_optimal_solutions` (178 mismatches / 13 unsolved —
byte-identical to a clean HEAD baseline run):
- **1.1 + 1.2 + 1.3** — `code::metadata::metadata_of` (Cow) replaced all 18 clone stanzas; the
twin passes are now thin wrappers over `diff::hash_tree_matching::solve` (spec = hash accessor +
classifier + reasons); the O(n) `mapping.iter().any` scans became O(1) `before_node_map`
lookups; the identical pass got the structural pass's unclaimed-duplicate fix, and the pinning
test now asserts one-to-one duplicate pairing.
- **1.4** — `NodeCache::build` folded into one `cache_for` helper; the `unsafe` transmute now has
a single home.
- **1.5 + 1.6 + 1.9(blob)** — `stats::filesystem::find_git_repositories` (sorted; `commit_stats`
is now deterministic), generic `stats::sampling::Reservoir<T>`, `stats::git::blob_bytes`.
- **§4 lints** — the four `spf_a` dead-store initializers removed; rustc's definite-assignment
check proves the values were never read. Build is warning-free.
- **§4 typos** — `memo`, `unmatched`, forest/theoretically/mapping/maps/languages/its, plus
`Therefore`/`something`/`fields` in code.rs/metadata.rs.
Not yet done (deliberately): the module splits of `human_solver.rs` / `apted/common.rs` (§2) and
the before/after side-parameterization (1.7/1.8) — the next candidates, per §5's order of attack.
---
## 1. Duplicate code
### 1.1 Metadata-clone boilerplate in every solver pass (also a real perf cost)
Every pass in the pipeline opens with the same 8-line stanza, once per side:
```rust
});
```
Sites (9 files, 18 clones): `solve_identical_trees.rs:37`, `solve_structurally_identical_trees.rs:42`,
`solve_semantically_structural_nodes.rs:115` **and** `:258` (twice in one file),
`solve_similar_flow_control.rs:56`, `solve_identical_diagnostic_statements.rs:62`,
`solve_moved_subtrees.rs:66`, `apted/common.rs:2293`, `test/helper/optimal_iud.rs:160`.
This is not just repetition: `ASTMetadata` holds several whole-tree `HashMap`s
(`node_info`, `node_to_full_hash`, `full_hash_to_node`, structural-hash maps, …), so a single
`Diff::from_code` run deep-copies both sides' metadata ~7 times each. The comment at each site
("We clone to avoid lifetime issues") concedes the clone is incidental.
**Suggestion:** compute/ensure metadata once in `Diff::from_code` and pass `&ASTMetadata` for each
side into every pass (they already all share the `solve(before, after, &node_cache, &mut ast_diff)`
signature — extend it to take the two metadata refs). If a standalone entry point still needs the
fallback, one shared helper `fn metadata_of(code: &Code) -> Cow<'_, ASTMetadata>` replaces all 18
stanzas.
### 1.2 `solve_identical_trees` vs `solve_structurally_identical_trees` are near-clones with drift
The two passes share ~80% of their body line-for-line: same metadata stanza, same
reference-node loop, same "skip already mapped" check, same paired stack-descent matching children
by position and kind, same redundant `{ let after_node_id = matching_after_node.id(); … }` shadow
block. They differ only in (a) which hash map they consult (full vs structural), (b) how the
root/child operation is classified (always `Identical` vs text-compare → `Identical`/`Update`),
and (c) duplicate handling — and (c) is *accidental* drift, not a design difference:
- `solve_identical_trees.rs:91` takes `after_node_ids.iter().next()` — the documented
duplicate-collapse bug (all N before-duplicates map onto one after-node; there's a 40-line
comment plus a pinning test for it).
- `solve_structurally_identical_trees.rs:88` already fixed the same problem with
`.find(|&&id| !diff.after_node_map.contains_key(&id))`.
**Suggestion:** extract one generic pass parameterized by (hash-map accessor, operation
classifier) — the structural pass's claimed-node filter then fixes the identical pass's TODO for
free. If the full extraction feels heavy, at minimum port the `.find(unclaimed)` fix and extract
the shared child-descent loop.
### 1.3 O(n) linear scans over `diff.mapping` inside per-node loops
Both twin passes ask "is this before-node already mapped?" via
```rust