From 906b2416dd1e0dc0ef90891cd3f6635c22d61fd5 Mon Sep 17 00:00:00 2001 From: Thomas Dickson Date: Tue, 11 Aug 2026 10:22:21 +0100 Subject: [PATCH] fix(checks): detect clones that differ only in literal values (CD-331) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Refactor.DuplicateBlock` missed structurally identical blocks whose literals differ. The ticket blamed unmodified identifiers; that was wrong — identifiers were already canonicalised to window-relative positional indices, verified with a fixture that renames every one and still fires. Literals were hashed by value, so three worker files differing in three strings produced nothing. Literals are now normalised to positional placeholders too, behind `normalize_literals` (default on). Two hashes are computed per window in one pass so a group can be told apart as verbatim or merely near. Validating against five real repos surfaced two consequences: - Normalising module specifiers made every run of `import` statements match every other one (one repo went 8 -> 66 findings). Imports and re-exports are now excluded from statement windows outright. - The remaining rise was dominated by test setup blocks differing in their fixture values. Since `--fail-on` defaults to medium, reporting those under the existing check would turn CI red for duplication nobody chose to gate on. Severity is stamped per check id, not per issue, so near-clones move to a new `Refactor.NearDuplicateBlock` at low severity. Medium-and-above counts now sit at or below baseline on four of five repos. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019afugU2bTYCtXn6Vy9dn1v --- CHANGELOG.md | 4 +- .../docs/Refactor.DuplicateBlock.md | 10 +- .../docs/Refactor.NearDuplicateBlock.md | 38 ++ crates/cofferdam-checks/src/lib.rs | 1 + .../src/refactor/duplicate_block.rs | 472 +++++++++++++----- crates/cofferdam-checks/src/refactor/mod.rs | 394 ++++++++++++++- docs/.vitepress/sidebar-checks.ts | 1 + docs/checks/Refactor.DuplicateBlock.md | 10 +- docs/checks/Refactor.NearDuplicateBlock.md | 43 ++ docs/checks/index.md | 1 + docs/public/checks.json | 55 +- examples/duplicate_block_literals.ts | 69 +++ 12 files changed, 973 insertions(+), 125 deletions(-) create mode 100644 crates/cofferdam-checks/docs/Refactor.NearDuplicateBlock.md create mode 100644 docs/checks/Refactor.NearDuplicateBlock.md create mode 100644 examples/duplicate_block_literals.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 83818d52..4499e3c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A check declares a set of languages rather than one, and `Consistency.SpellingDialect` now reads Markdown as well as TypeScript (CD-316). The check shipped against the codebase but not against the corpus that motivated it: a docs tree split between "analyz*" and "analys*", which it could not reach because `Check::language()` returned a single `Language` and the engine dispatched on equality. `Check::languages()` returns `&'static [Language]`, defaulting to `&[Language::TypeScript]`, so every check that does not care compiles unchanged; the four that do — three Rust checks and one HTML — name their own. Registering a second check id for Markdown was the cheaper option and was rejected: two entries in the catalogue for one convention is a worse surface than the gap. In Markdown the whole document is prose, so what gets excluded is the code a page quotes — fenced blocks, inline code spans, link destinations and YAML frontmatter — but not indented blocks, since four spaces is also a nested list continuation. Markdown discovery remains opt-in behind `[engine] extra_extensions`, so no project sees new findings without asking. ### Fixed -- `Design.LayerViolation`'s catalogue page no longer recommends `.cofferdamignore` for excluding tests. That is the exact configuration that makes `Design.OrphanExport` claim a symbol is "never imported in the project" when its only importers are the ignored tests — `.cofferdamignore` prunes files before discovery, so they contribute no import edges either (CD-325). The page now points at an `[[overrides]]` block with `disabled = true`, which turns off the one check without removing the files from the graph. The underlying behaviour is unchanged and tracked separately. +- `Refactor.DuplicateBlock` missed clones that differ only in their literal values (CD-331). The report blamed unmodified identifiers, which was wrong: identifiers were already canonicalised to window-relative positional indices, confirmed with a fixture that renamed every one and still fired. String and number literals were the gap — they were hashed by value, so three worker files differing in three strings produced nothing. Literals are now normalised to positional placeholders too, behind a `normalize_literals` option that is on by default. +- Import blocks are no longer eligible to be duplicates. Normalising module specifiers made every run of `import` statements match every other one — one repo went from 8 findings to 66, almost all on line 1 — and an import list cannot be extracted into a shared helper, so the finding was never actionable. `import`, `export * from` and `export { x } from` are excluded from statement windows outright; a plain `export { x }` or `export function` still participates. **On upgrade this can move a finding's primary span further down a file.** Suppression targets a comment's next non-blank line rather than a range, so a `cofferdam-ignore` anchored to an import may no longer cover the block it was written for, surfacing a duplicate that had been suppressed. Move the comment to the first statement of the block itself. +- `Refactor.NearDuplicateBlock` is new, and reports the near-clones that literal normalisation newly finds. They are worth seeing — a copy that has drifted is where bugs hide — but on real projects they are mostly test setup blocks differing in their fixture values, and `--fail-on` defaults to `medium`, so reporting them under the existing check would turn CI red for duplication nobody chose to gate on. The engine stamps severity per check id rather than per issue, so a second id is the only way to separate them. `Refactor.DuplicateBlock` keeps `medium` and now reports verbatim clones only; `NearDuplicateBlock` defaults to `low`, printing without failing a build. Raise it with `[checks."Refactor.NearDuplicateBlock"] severity = "medium"` to gate on it deliberately. Both read one corpus slot and share a single overlap-claim pass, so they never report the same region twice.- `Design.LayerViolation`'s catalogue page no longer recommends `.cofferdamignore` for excluding tests. That is the exact configuration that makes `Design.OrphanExport` claim a symbol is "never imported in the project" when its only importers are the ignored tests — `.cofferdamignore` prunes files before discovery, so they contribute no import edges either (CD-325). The page now points at an `[[overrides]]` block with `disabled = true`, which turns off the one check without removing the files from the graph. The underlying behaviour is unchanged and tracked separately. - `Refactor.PurityHeuristic` can be switched on (CD-324). Its one option is called `enabled`, and the config loader stripped that key out of every check's option bag before validation as a forward-compatible placeholder — so the check's own catalogue page told the user to write `enabled = true`, and the check never saw it. A registered check no user could reach is worse than an unrecognised key, so `enabled` now flows through to the option bag like any other option, and is dropped again only for the checks that do not declare it. Configs carrying a stray `enabled` therefore keep loading rather than failing on upgrade, which is why the fix is silent rather than a warning: making the inert case loud needs a warning sink `options_for_raw` does not have, and is not worth a breaking change to reach. - `scripts/version.mjs set X.Y.Z --regen` could stamp the previous version into `docs/public/checks.json` and `docs/public/llms.txt` (CD-317). A stale cargo fingerprint after a `Cargo.toml` revert-then-rebump left `cargo build --workspace` reporting `Finished` without recompiling `cofferdam-cli`, so the subsequent `gen-docs` ran the old binary and wrote the old version, and `node scripts/version.mjs check` then failed on locations that regen was meant to fix. `--regen` now runs `cargo clean -p cofferdam-cli` before the build to force that crate's recompile, checks the built binary's own `--version` against the target before trusting it to run `gen-docs`, and re-reads `checks.json`/`llms.txt` afterwards to confirm they landed on the target version — failing loudly and naming both versions if not. - `--hide-baselined` now applies to every output format, not text alone (CD-315). The flag filtered findings inside the text formatter, so `--format=json`, `compact` and `sarif` received the untouched list and printed baselined findings anyway, marked `"baselined": true`. Nothing in the help text said so, and a CI pipeline reading JSON was quietly counting pre-existing findings as new work. The filter now runs once in the CLI, ahead of the per-format branch, so all four formats render the same list. Summary counts are deliberately left whole — `total`, `new` and `baselined` still describe the full run, matching what the text formatter has always done — so the gate count stays visible when the entries are hidden. diff --git a/crates/cofferdam-checks/docs/Refactor.DuplicateBlock.md b/crates/cofferdam-checks/docs/Refactor.DuplicateBlock.md index b6f8d2ec..85999fac 100644 --- a/crates/cofferdam-checks/docs/Refactor.DuplicateBlock.md +++ b/crates/cofferdam-checks/docs/Refactor.DuplicateBlock.md @@ -3,10 +3,10 @@ id: Refactor.DuplicateBlock category: Refactor base_priority: 12 default_severity: Medium -options: [min_statements, min_chars, include_tokens, include_ast] +options: [min_statements, min_chars, include_tokens, include_ast, normalize_literals] --- -Runs of statements that recur (after rename canonicalisation) in multiple files. Likely copy-paste — extract a shared helper. Canonicalisation maps identifier tokens to per-window local indices so renamed copies still match. Minimum window is `6` consecutive statements (and `80` characters) to keep noise low. Cross-file: per-file `run` writes fingerprints into the shared corpus; `finalize` groups by hash and emits one `Issue` per duplicate set with `related` spans pointing at every other occurrence. +Runs of statements that recur, verbatim, in multiple files. Likely copy-paste — extract a shared helper. Canonicalisation maps identifier tokens to per-window local indices so renamed copies still match; a block that is otherwise identical but carries different string or number literal values is not this check's concern — it is reported separately, at low severity, by [`Refactor.NearDuplicateBlock`](Refactor.NearDuplicateBlock.md), since a near-clone is a weaker and noisier signal than a byte-identical one and should not trip a default CI gate on its own. The `normalize_literals` option controls which of the two checks a given block lands on (see that page), not whether it is reported at all. `import` declarations and re-exports (`export { x } from './y'`) are never windowed at all — an import block can't be extracted into a shared helper, so treating one run of imports as a duplicate of another is never actionable, and normalizing their module-specifier string literals would otherwise make every same-length import block in a project match every other. A plain `export { x }` or `export function f() {}` (no `source`) is ordinary code and is unaffected. Minimum window is `6` consecutive statements (and `80` characters) to keep noise low. Cross-file: per-file `run` writes fingerprints into a corpus slot shared with `Refactor.NearDuplicateBlock`; `finalize` groups by hash, runs one overlap-claim pass across both checks' candidates together (so the two never report overlapping spans), and emits one `Issue` per verbatim-duplicate set with `related` spans pointing at every other occurrence. ```ts // src/orders.ts:42 @@ -44,4 +44,8 @@ occurrence — one primary location plus `related` spans for the rest. A `cofferdam-ignore: Refactor.DuplicateBlock` comment placed at *any* occurrence (the primary one or any related one) suppresses the whole finding, not just that copy. You don't need to find and suppress every occurrence individually — one ignore -comment on either side of a duplicated pair is enough. +comment on either side of a duplicated pair is enough. Suppression targets the +comment's next non-blank line, not a range, so anchor it to the first *statement* +of the duplicated run — not to an import or other declaration preceding it. Import +and re-export statements are excluded from windows entirely, so a comment sitting +above one no longer covers the block that follows. diff --git a/crates/cofferdam-checks/docs/Refactor.NearDuplicateBlock.md b/crates/cofferdam-checks/docs/Refactor.NearDuplicateBlock.md new file mode 100644 index 00000000..a5ca91f8 --- /dev/null +++ b/crates/cofferdam-checks/docs/Refactor.NearDuplicateBlock.md @@ -0,0 +1,38 @@ +--- +id: Refactor.NearDuplicateBlock +category: Refactor +base_priority: 10 +default_severity: Low +options: [min_statements, min_chars, include_tokens, include_ast, normalize_literals] +--- + +Runs of statements that are structurally identical to a block elsewhere in the project but differ in a string or number literal value — a near-clone rather than a verbatim one. Identifier tokens were already canonicalised to per-window local indices before this check existed, so a renamed copy has always matched [`Refactor.DuplicateBlock`](Refactor.DuplicateBlock.md); this check is specifically about the case where a literal, not a name, is the only thing that changed. That is usually the interesting half of the finding: two blocks drift apart because someone copied one and edited the values, and the edit — which fields moved, which threshold changed — is worth a look even though the shape underneath is unchanged. + +```ts +// src/billing/charge-gold.ts +const productId = "gold-membership"; +const amountCents = 4999; +const invoice = createInvoice(account, productId, amountCents); +const receipt = submitInvoice(invoice, "Gold membership"); +return receipt; +``` + +```ts +// src/billing/charge-silver.ts — same shape, different literals: flagged as related +const productId = "silver-membership"; +const amountCents = 2999; +const invoice = createInvoice(account, productId, amountCents); +const receipt = submitInvoice(invoice, "Silver membership"); +return receipt; +``` + +Both blocks share one `finalize` pass and one corpus slot (`Refactor.DuplicateBlock.fingerprints`) with `Refactor.DuplicateBlock` — only that check's `run` writes to it, this one reads the same data back and reports the other half of the same grouping: groups whose members are structurally identical (same `hash`) but not byte-identical (differing `exact_hash`). The two checks never report overlapping spans, because the shared overlap-claim pass runs once across both checks' candidates before either is filtered out. + +**Severity:** near-clones default to `low`, unlike `Refactor.DuplicateBlock`'s `medium`, and print without tripping the default `--fail-on medium` gate. That is deliberate — severity is set per check id, not per finding, so splitting verbatim clones from literal-drift ones into two ids was the only way to keep a noisier, less actionable signal from failing a build that only meant to gate on real copy-paste. To gate on this check too, raise its severity in `cofferdam.toml`: + +```toml +[checks."Refactor.NearDuplicateBlock"] +severity = "medium" +``` + +**Suppressing:** as with `Refactor.DuplicateBlock`, each group is one `Issue` with `related` spans for every other occurrence, and a `cofferdam-ignore: Refactor.NearDuplicateBlock` comment at any occurrence suppresses the whole finding. diff --git a/crates/cofferdam-checks/src/lib.rs b/crates/cofferdam-checks/src/lib.rs index bcba0f06..b34d238d 100644 --- a/crates/cofferdam-checks/src/lib.rs +++ b/crates/cofferdam-checks/src/lib.rs @@ -86,6 +86,7 @@ pub fn all_builtins() -> Vec> { Box::new(refactor::CognitiveComplexity::new(15)), Box::new(refactor::LongAndComplex::new(75, 15)), Box::new(refactor::DuplicateBlock::default()), + Box::new(refactor::NearDuplicateBlock::default()), Box::new(refactor::PreferOptionalChain), Box::new(refactor::DeadExport), Box::new(refactor::PreferNullishCoalescing), diff --git a/crates/cofferdam-checks/src/refactor/duplicate_block.rs b/crates/cofferdam-checks/src/refactor/duplicate_block.rs index ff6ba04a..e118591b 100644 --- a/crates/cofferdam-checks/src/refactor/duplicate_block.rs +++ b/crates/cofferdam-checks/src/refactor/duplicate_block.rs @@ -61,6 +61,12 @@ const DUPLICATE_BLOCK_MIN_CHARS: usize = 80; #[derive(Clone)] struct Fingerprint { hash: u64, + /// Hash with string/number literal *values* included (never + /// normalized to positional placeholders), regardless of the + /// `normalize_literals` option. Used at finalize to tell whether a + /// group of blocks sharing `hash` are truly identical or only + /// identical in structure (CD-331). + exact_hash: u64, kind: FingerprintKind, file: PathBuf, span: Span, @@ -99,6 +105,11 @@ pub struct DuplicateBlock { /// AST-mode enabled (default: true). Can be disabled to run /// token-mode only. Configurable via cofferdam.toml. include_ast: bool, + /// Treat string/number literals as positional placeholders (like + /// identifiers) when computing the grouping hash, so blocks that + /// differ only in their literal values are still reported as + /// duplicates (CD-331). Default: true. + normalize_literals: bool, } pub const DUP_BLOCK_OPTIONS: &[OptionSpec] = &[ @@ -126,6 +137,12 @@ pub const DUP_BLOCK_OPTIONS: &[OptionSpec] = &[ default: OptionDefault::Bool(true), doc: "run the AST statement-window pass (disable to use token-mode only)", }, + OptionSpec { + name: "normalize_literals", + kind: OptionKind::Bool, + default: OptionDefault::Bool(true), + doc: "treat string and number literals as positional placeholders, so blocks differing only in their literal values are still reported as duplicates", + }, ]; impl Default for DuplicateBlock { @@ -136,6 +153,7 @@ impl Default for DuplicateBlock { min_tokens: DUPLICATE_BLOCK_MIN_TOKENS, include_tokens: false, include_ast: true, + normalize_literals: true, } } } @@ -172,6 +190,35 @@ const DUP_META: CheckMeta = CheckMeta { pure_run: false, }; +/// `Refactor.NearDuplicateBlock` — CD-331 split. Shares `DUPLICATE_BLOCKS` +/// with `DuplicateBlock` (same corpus slot, populated only by +/// `DuplicateBlock::run`) but reports the other half of the same +/// grouping pass: AST-mode groups whose members are structurally +/// identical yet differ in a string/number literal value. +/// +/// A separate check id exists because severity is stamped per check id +/// by the engine's post-pass (a check cannot vary severity per issue — +/// see `Engine::finalize_and_filter`'s severity post-pass), and +/// `Severity::Medium` on `Refactor.DuplicateBlock` trips the default +/// `--fail-on medium` gate. Near-clones are a real signal (the +/// divergence between two copies is often exactly what is worth +/// looking at) but are noisier and less actionable than a verbatim +/// clone, so they default to `Severity::Low` and print without gating +/// CI — see `docs/Refactor.NearDuplicateBlock.md`. +const DUP_NEAR_META: CheckMeta = CheckMeta { + id: "Refactor.NearDuplicateBlock", + category: Category::Refactor, + base_priority: 10, + default_severity: Severity::Low, + explanation: "Runs of statements that are structurally identical but differ in their string or number literals — often the same logic copied and then partially edited, where the edit is the thing worth looking at.", + body: include_str!("../../docs/Refactor.NearDuplicateBlock.md"), + requires_types: false, + consistency: false, + options: DUP_BLOCK_OPTIONS, + autofix: false, + pure_run: false, +}; + impl Check for DuplicateBlock { fn meta(&self) -> &'static CheckMeta { &DUP_META @@ -207,6 +254,10 @@ impl Check for DuplicateBlock { // field can also turn it off (no existing constructor does this, but // the option is there for completeness). let include_ast = self.include_ast && ctx.options.get_bool("include_ast").unwrap_or(true); + let normalize_literals = ctx + .options + .get_bool("normalize_literals") + .unwrap_or(self.normalize_literals); let mut collected = Vec::new(); // Built once per file: `span_from_bytes` is O(start_byte), and a @@ -223,6 +274,7 @@ impl Check for DuplicateBlock { line_index: &line_index, min_statements, min_chars, + normalize_literals, collected: Vec::new(), }; visitor.visit_program(parsed.program); @@ -246,111 +298,215 @@ impl Check for DuplicateBlock { } fn finalize(&self, ctx: &mut FinalizeContext<'_>) -> Vec { - let mut by_hash: BTreeMap> = BTreeMap::new(); - // Read-only (cd-32): a draining read would empty the slot as a - // side effect of finalize, which is fine for a one-shot analyze - // but corrupts `Engine::analyze_incremental`'s persistent - // `AnalysisState` — the next incremental call would finalize - // over an empty slot for every file that didn't just change. - ctx.corpus.with_slot(&DUPLICATE_BLOCKS, |slot| { - for fp in slot.iter().cloned() { - by_hash.entry(fp.hash).or_default().push(fp); - } - }); - - // Build candidate findings and dedupe overlapping windows in the - // same file. Sort groups by their primary's (file, start_byte) so - // earlier-in-the-source windows claim the territory first. - let mut candidates: Vec> = by_hash - .into_values() - .map(|mut fps| { - fps.sort_by(|a, b| { - a.file - .cmp(&b.file) - .then_with(|| a.span.start_byte.cmp(&b.span.start_byte)) - }); - dedupe_self_overlaps(fps) + // Only the exact half (verbatim clones, plus all token-mode + // groups — token mode's exact_hash always equals hash, see + // `collect_token_fingerprints`) belongs to this check id; the + // literal-drift half is `Refactor.NearDuplicateBlock`'s (CD-331 + // follow-up split, see `DUP_NEAR_META`'s doc comment for why). + let (exact_groups, _near_groups) = partition_claimed_groups(ctx); + exact_groups + .into_iter() + .map(|group| { + let message = match group[0].kind { + FingerprintKind::Ast => format!( + "duplicate {}-statement block, also at {} other location(s)", + self.min_statements, + group.len() - 1 + ), + FingerprintKind::Token => format!( + "duplicate {}-token window (cross-statement), also at {} other location(s)", + self.min_tokens, + group.len() - 1 + ), + }; + build_issue(DUP_META.id, DUP_META.base_priority, message, &group) }) - // Self-overlap dedup can shrink a group below 2 (a single - // real block whose sliding windows all hashed identically), - // so re-check the size floor after it, not before. - .filter(|fps| fps.len() >= 2) - .collect(); - // AST candidates run first so they claim territory before token - // candidates compete. Inside a kind, sort by primary's location. - candidates.sort_by(|a, b| { - a[0].kind - .cmp(&b[0].kind) - .then_with(|| a[0].file.cmp(&b[0].file)) - .then_with(|| a[0].span.start_byte.cmp(&b[0].span.start_byte)) - }); + .collect() + } +} - // (file, start, end) of every primary span we've already emitted. - // A new candidate whose primary OR any related span overlaps an - // already-emitted region in the same file is dropped. - let mut claimed: Vec<(PathBuf, u32, u32)> = Vec::new(); - let mut issues = Vec::new(); +/// `Refactor.NearDuplicateBlock` — see `DUP_NEAR_META`'s doc comment. +/// Only holds `min_statements`, the one field its message text needs; +/// it never collects fingerprints itself (`run` is a no-op — see the +/// `Check` impl below), so it has no use for the rest of +/// `DuplicateBlock`'s configuration. +pub struct NearDuplicateBlock { + min_statements: usize, +} - for group in candidates { - let primary = &group[0]; - let overlaps = |c: &(PathBuf, u32, u32), file: &PathBuf, s: u32, e: u32| { - &c.0 == file && c.1 < e && s < c.2 - }; - if claimed.iter().any(|c| { - overlaps( - c, - &primary.file, - primary.span.start_byte, - primary.span.end_byte, - ) - }) { - continue; - } - // Also drop if any related span overlaps an already-claimed - // region — same logical duplicate, viewed from the other side. - if group[1..].iter().any(|fp| { - claimed - .iter() - .any(|c| overlaps(c, &fp.file, fp.span.start_byte, fp.span.end_byte)) - }) { - continue; - } +impl Default for NearDuplicateBlock { + fn default() -> Self { + Self { + min_statements: DUPLICATE_BLOCK_MIN_STATEMENTS, + } + } +} - for fp in &group { - claimed.push((fp.file.clone(), fp.span.start_byte, fp.span.end_byte)); - } +impl Check for NearDuplicateBlock { + fn meta(&self) -> &'static CheckMeta { + &DUP_NEAR_META + } - let related: Vec = group[1..] - .iter() - .map(|fp| RelatedSpan { - location: Location::from_span(&fp.file, fp.span), - file: fp.file.clone(), - }) - .collect(); - let message = match primary.kind { - FingerprintKind::Ast => format!( - "duplicate {}-statement block, also at {} other location(s)", + fn run(&self, _file: &SourceFile, _ctx: &mut CheckContext<'_>) -> Vec { + // Deliberately a no-op: `DuplicateBlock::run` is the sole writer + // of the shared `DUPLICATE_BLOCKS` corpus slot. Writing here too + // would double every fingerprint (CD-331 follow-up spec). + Vec::new() + } + + fn finalize(&self, ctx: &mut FinalizeContext<'_>) -> Vec { + let (_exact_groups, near_groups) = partition_claimed_groups(ctx); + near_groups + .into_iter() + .map(|group| { + let message = format!( + "duplicate {}-statement block differing only in literal values, also at {} other location(s)", self.min_statements, - related.len() - ), - FingerprintKind::Token => format!( - "duplicate {}-token window (cross-statement), also at {} other location(s)", - self.min_tokens, - related.len() - ), - }; - issues.push(Issue { - check_id: DUP_META.id.to_string(), - message, - file: primary.file.clone(), - location: Location::from_span(&primary.file, primary.span), - priority: Priority(DUP_META.base_priority), - severity: Severity::Medium, - related, + group.len() - 1 + ); + build_issue(DUP_NEAR_META.id, DUP_NEAR_META.base_priority, message, &group) + }) + .collect() + } +} + +/// Common `Issue` construction for both `DuplicateBlock` and +/// `NearDuplicateBlock`: same fields, different check id / priority / +/// message. `severity` is always the `Severity::Medium` placeholder — +/// the engine's severity post-pass stamps the real value from each +/// check id's registered `CheckMeta::default_severity` (or config +/// override), so which severity a check "has" lives in exactly one +/// place regardless of which of these two check ids built the issue. +fn build_issue( + check_id: &'static str, + base_priority: i8, + message: String, + group: &[Fingerprint], +) -> Issue { + let primary = &group[0]; + let related: Vec = group[1..] + .iter() + .map(|fp| RelatedSpan { + location: Location::from_span(&fp.file, fp.span), + file: fp.file.clone(), + }) + .collect(); + Issue { + check_id: check_id.to_string(), + message, + file: primary.file.clone(), + location: Location::from_span(&primary.file, primary.span), + priority: Priority(base_priority), + severity: Severity::Medium, + related, + } +} + +/// Shared finalize core for `DuplicateBlock` and `NearDuplicateBlock`: +/// reads `DUPLICATE_BLOCKS`, groups by `hash`, dedupes self-overlaps, +/// and runs the cross-group overlap-claim pass exactly once — then +/// partitions the *emitted* groups into "exact" (every member shares +/// the primary's `exact_hash` — verbatim clones, and all token-mode +/// groups) and "near" (an AST-mode group whose members differ in a +/// literal value). +/// +/// Splitting after the claim pass, not before, is what keeps the two +/// checks from ever reporting overlapping spans: the claim pass is +/// identical to (and, called once per check, reproduces) the single +/// pass this file ran before the CD-331 split, so `exact_groups ∪ +/// near_groups` is exactly the set of groups the one check used to +/// emit, and the two checks between them cover it once each rather +/// than only recomputing the same deterministic partition twice. +fn partition_claimed_groups( + ctx: &mut FinalizeContext<'_>, +) -> (Vec>, Vec>) { + let mut by_hash: BTreeMap> = BTreeMap::new(); + // Read-only (cd-32): a draining read would empty the slot as a + // side effect of finalize, which is fine for a one-shot analyze + // but corrupts `Engine::analyze_incremental`'s persistent + // `AnalysisState` — the next incremental call would finalize + // over an empty slot for every file that didn't just change. + ctx.corpus.with_slot(&DUPLICATE_BLOCKS, |slot| { + for fp in slot.iter().cloned() { + by_hash.entry(fp.hash).or_default().push(fp); + } + }); + + // Build candidate findings and dedupe overlapping windows in the + // same file. Sort groups by their primary's (file, start_byte) so + // earlier-in-the-source windows claim the territory first. + let mut candidates: Vec> = by_hash + .into_values() + .map(|mut fps| { + fps.sort_by(|a, b| { + a.file + .cmp(&b.file) + .then_with(|| a.span.start_byte.cmp(&b.span.start_byte)) }); + dedupe_self_overlaps(fps) + }) + // Self-overlap dedup can shrink a group below 2 (a single + // real block whose sliding windows all hashed identically), + // so re-check the size floor after it, not before. + .filter(|fps| fps.len() >= 2) + .collect(); + // AST candidates run first so they claim territory before token + // candidates compete. Inside a kind, sort by primary's location. + candidates.sort_by(|a, b| { + a[0].kind + .cmp(&b[0].kind) + .then_with(|| a[0].file.cmp(&b[0].file)) + .then_with(|| a[0].span.start_byte.cmp(&b[0].span.start_byte)) + }); + + // (file, start, end) of every primary span we've already emitted. + // A new candidate whose primary OR any related span overlaps an + // already-emitted region in the same file is dropped. + let mut claimed: Vec<(PathBuf, u32, u32)> = Vec::new(); + let mut exact_groups = Vec::new(); + let mut near_groups = Vec::new(); + + for group in candidates { + let primary = &group[0]; + let overlaps = |c: &(PathBuf, u32, u32), file: &PathBuf, s: u32, e: u32| { + &c.0 == file && c.1 < e && s < c.2 + }; + if claimed.iter().any(|c| { + overlaps( + c, + &primary.file, + primary.span.start_byte, + primary.span.end_byte, + ) + }) { + continue; + } + // Also drop if any related span overlaps an already-claimed + // region — same logical duplicate, viewed from the other side. + if group[1..].iter().any(|fp| { + claimed + .iter() + .any(|c| overlaps(c, &fp.file, fp.span.start_byte, fp.span.end_byte)) + }) { + continue; + } + + for fp in &group { + claimed.push((fp.file.clone(), fp.span.start_byte, fp.span.end_byte)); + } + + // Grouping happens on `hash` (normalized under + // `normalize_literals`); check whether the group is also + // identical in its exact (literal-sensitive) hash to decide + // which check id owns it (CD-331 / CD-331 follow-up). + let all_exact = group.iter().all(|fp| fp.exact_hash == primary.exact_hash); + if all_exact { + exact_groups.push(group); + } else { + near_groups.push(group); } - issues } + (exact_groups, near_groups) } /// Collapse windows *within one hash group* that overlap each other in @@ -388,11 +544,26 @@ struct DupCollector<'a> { line_index: &'a LineIndex, min_statements: usize, min_chars: usize, + normalize_literals: bool, collected: Vec, } impl<'a> DupCollector<'a> { fn scan(&mut self, stmts: &[Statement<'a>]) { + // Module-level import/re-export declarations are excluded from + // windowing entirely (CD-331 follow-up), not just as window + // starts — otherwise a window could straddle the import block + // into real code. A run of import statements differing only in + // their module specifiers is never actionable (you cannot + // extract a shared helper for an import block), and once + // literal normalization treats those specifiers as positional + // placeholders, every same-length import block in a project + // hashes identically. Plain re-exports (`export { x } from + // './y'`) are excluded the same way; a bare `export { x }` or + // `export function f() {}` (no `source`) is ordinary code and + // still participates. + let stmts: Vec<&Statement<'a>> = + stmts.iter().filter(|s| !is_import_or_reexport(s)).collect(); if stmts.len() < self.min_statements { return; } @@ -405,8 +576,8 @@ impl<'a> DupCollector<'a> { // don't pay for op collection they'll never use. let mut stmt_ops: Vec>>> = (0..stmts.len()).map(|_| None).collect(); for i in 0..=stmts.len() - self.min_statements { - let first = &stmts[i]; - let last = &stmts[i + self.min_statements - 1]; + let first = stmts[i]; + let last = stmts[i + self.min_statements - 1]; let start = first.span().start as usize; let end = last.span().end as usize; if start >= end || end > self.file.text.len() { @@ -419,18 +590,27 @@ impl<'a> DupCollector<'a> { } for j in i..i + self.min_statements { if stmt_ops[j].is_none() { - stmt_ops[j] = Some(collect_stmt_ops(&stmts[j])); + stmt_ops[j] = Some(collect_stmt_ops(stmts[j])); } } let window = &stmt_ops[i..i + self.min_statements]; - let hash = hash_ops( + let (normalized, exact) = hash_ops( window .iter() .flat_map(|ops| ops.as_ref().expect("just populated above").iter()), ); + // When normalization is off, the grouping hash IS the exact + // hash — that reproduces pre-CD-331 grouping behaviour and + // keeps the "is this group exact?" check trivially true. + let hash = if self.normalize_literals { + normalized + } else { + exact + }; let span = self.line_index.span_from_bytes(start as u32, end as u32); self.collected.push(Fingerprint { hash, + exact_hash: exact, kind: FingerprintKind::Ast, file: self.file.path.clone(), span, @@ -439,6 +619,20 @@ impl<'a> DupCollector<'a> { } } +/// True for statements that must never enter an AST-mode window +/// (CD-331 follow-up): plain `import` declarations, `export * from`, +/// and re-exports (`export { x } from './y'`). A bare `export { x }` +/// or `export function f() {}` — an `ExportNamedDeclaration` with no +/// `source` — is ordinary code and is NOT excluded. +fn is_import_or_reexport(stmt: &Statement<'_>) -> bool { + match stmt { + Statement::ImportDeclaration(_) => true, + Statement::ExportAllDeclaration(_) => true, + Statement::ExportNamedDeclaration(decl) => decl.source.is_some(), + _ => false, + } +} + impl<'a> Visit<'a> for DupCollector<'a> { fn visit_program(&mut self, node: &oxc_ast::ast::Program<'a>) { self.scan(&node.body); @@ -676,19 +870,32 @@ enum HashOp<'a> { /// state), so concatenating precomputed per-statement op lists in /// statement order reproduces the original single-pass sequence /// exactly. -fn hash_ops<'a, 'i>(ops: impl Iterator>) -> u64 +/// Combines a window's `HashOp`s into two hashes in a single pass: +/// `(normalized, exact)`. `normalized` treats string/number literals as +/// positional placeholders (same idea as identifier canonicalisation); +/// `exact` hashes literal values verbatim, as `hash_ops` always did +/// before CD-331. Computed together — not via two calls — because this +/// is a hot path (CD-173). +fn hash_ops<'a, 'i>(ops: impl Iterator>) -> (u64, u64) where 'a: 'i, { use std::hash::Hasher; - let mut hasher = std::collections::hash_map::DefaultHasher::new(); + let mut norm_hasher = std::collections::hash_map::DefaultHasher::new(); + let mut exact_hasher = std::collections::hash_map::DefaultHasher::new(); let mut locals: HashMap<&str, u32> = HashMap::new(); let mut next_local: u32 = 0; + let mut str_locals: HashMap<&str, u32> = HashMap::new(); + let mut next_str: u32 = 0; + let mut num_locals: HashMap = HashMap::new(); + let mut next_num: u32 = 0; for op in ops { match *op { HashOp::Bytes(bytes) => { - hasher.write(bytes); - hasher.write_u8(HASH_SEPARATOR); + norm_hasher.write(bytes); + norm_hasher.write_u8(HASH_SEPARATOR); + exact_hasher.write(bytes); + exact_hasher.write_u8(HASH_SEPARATOR); } HashOp::Ident { prefix, name } => { let idx = match locals.get(name) { @@ -700,23 +907,53 @@ where i } }; - hasher.write(prefix); - hasher.write_u32(idx); - hasher.write_u8(HASH_SEPARATOR); + norm_hasher.write(prefix); + norm_hasher.write_u32(idx); + norm_hasher.write_u8(HASH_SEPARATOR); + exact_hasher.write(prefix); + exact_hasher.write_u32(idx); + exact_hasher.write_u8(HASH_SEPARATOR); } HashOp::Str(value) => { - hasher.write(b"Str:"); - hasher.write(value.as_bytes()); - hasher.write_u8(HASH_SEPARATOR); + let idx = match str_locals.get(value) { + Some(&i) => i, + None => { + let i = next_str; + next_str += 1; + str_locals.insert(value, i); + i + } + }; + norm_hasher.write(b"Str#"); + norm_hasher.write_u32(idx); + norm_hasher.write_u8(HASH_SEPARATOR); + + exact_hasher.write(b"Str:"); + exact_hasher.write(value.as_bytes()); + exact_hasher.write_u8(HASH_SEPARATOR); } HashOp::Num(value) => { - hasher.write(b"Num:"); - hasher.write(&value.to_le_bytes()); - hasher.write_u8(HASH_SEPARATOR); + let bits = value.to_bits(); + let idx = match num_locals.get(&bits) { + Some(&i) => i, + None => { + let i = next_num; + next_num += 1; + num_locals.insert(bits, i); + i + } + }; + norm_hasher.write(b"Num#"); + norm_hasher.write_u32(idx); + norm_hasher.write_u8(HASH_SEPARATOR); + + exact_hasher.write(b"Num:"); + exact_hasher.write(&value.to_le_bytes()); + exact_hasher.write_u8(HASH_SEPARATOR); } } } - hasher.finish() + (norm_hasher.finish(), exact_hasher.finish()) } pub struct AstHashWalker<'a> { @@ -737,7 +974,7 @@ impl<'a> AstHashWalker<'a> { } #[allow(dead_code)] // exercised by refactor::tests' AST-hash canonicalisation sanity checks - pub fn finish(self) -> u64 { + pub fn finish(self) -> (u64, u64) { hash_ops(self.ops.iter()) } } @@ -1106,6 +1343,11 @@ fn collect_token_fingerprints( let span = line_index.span_from_bytes(start, end); out.push(Fingerprint { hash, + // Token mode doesn't go through `hash_ops` and is out of + // scope for CD-331's literal-normalization; exact == hash + // here so the finalize "is this group exact?" check stays + // trivially true for token-mode findings. + exact_hash: hash, kind: FingerprintKind::Token, file: file.path.clone(), span, diff --git a/crates/cofferdam-checks/src/refactor/mod.rs b/crates/cofferdam-checks/src/refactor/mod.rs index b7b2b23e..5d88382d 100644 --- a/crates/cofferdam-checks/src/refactor/mod.rs +++ b/crates/cofferdam-checks/src/refactor/mod.rs @@ -31,7 +31,7 @@ pub use cyclomatic_complexity::{ max_in_file as max_cyclomatic_complexity_in_file, CyclomaticComplexity, }; pub use dead_export::DeadExport; -pub use duplicate_block::DuplicateBlock; +pub use duplicate_block::{DuplicateBlock, NearDuplicateBlock}; pub use long_and_complex::LongAndComplex; pub use mixed_throw_and_return_error::MixedThrowAndReturnError; pub use mutated_parameter::MutatedParameter; @@ -132,7 +132,7 @@ mod tests { // Run a quick parse on a synthetic source, then compare hashes to verify // structural canonicalisation does what we expect. - fn ast_hash_first_n_stmts(text: &str, n: usize) -> u64 { + fn ast_hash_first_n_stmts(text: &str, n: usize) -> (u64, u64) { let file = SourceFile::new(PathBuf::from("test.ts"), text.to_string()); let alloc = Allocator::default(); let parsed = parse_into(&alloc, &file); @@ -417,6 +417,396 @@ mod tests { let _ = run_duplicate_block_with_options(&check, &make_duplicate_source(), &opts); } + // ─── DuplicateBlock literal normalization (CD-331) ───────────────────── + // + // The ticket claimed identifiers were missed by canonicalisation; that + // was already handled (window-relative local indices). The real gap: + // `HashOp::Str`/`HashOp::Num` were hashed by value, so blocks differing + // only in literals never grouped. These tests pin the fix at both the + // `hash_ops` level (via `AstHashWalker::finish`) and the full + // run+finalize level. + + #[test] + fn ast_hash_string_literal_normalizes_but_exact_differs() { + let a = r#"const label = "hello"; return label;"#; + let b = r#"const label = "goodbye"; return label;"#; + let (norm_a, exact_a) = ast_hash_first_n_stmts(a, 2); + let (norm_b, exact_b) = ast_hash_first_n_stmts(b, 2); + assert_eq!( + norm_a, norm_b, + "normalized hash should treat string literals as positional placeholders" + ); + assert_ne!( + exact_a, exact_b, + "exact hash must still distinguish different string literal values" + ); + } + + #[test] + fn ast_hash_numeric_literal_normalizes_but_exact_differs() { + let a = "const n = 1; return n;"; + let b = "const n = 2; return n;"; + let (norm_a, exact_a) = ast_hash_first_n_stmts(a, 2); + let (norm_b, exact_b) = ast_hash_first_n_stmts(b, 2); + assert_eq!( + norm_a, norm_b, + "normalized hash should treat numeric literals as positional placeholders" + ); + assert_ne!( + exact_a, exact_b, + "exact hash must still distinguish different numeric literal values" + ); + } + + #[test] + fn ast_hash_normalization_does_not_collapse_genuinely_different_structure() { + // Guard against over-normalising: differing operators are a + // structural difference (`Bin:` tag payload), not a literal, and + // must still hash differently even with literal normalization on. + let a = "if (x === 1) return;"; + let b = "if (x !== 1) return;"; + let (norm_a, _) = ast_hash_first_n_stmts(a, 1); + let (norm_b, _) = ast_hash_first_n_stmts(b, 1); + assert_ne!( + norm_a, norm_b, + "different operators must still hash differently under literal normalization" + ); + } + + #[test] + fn ast_hash_string_placeholder_does_not_collide_with_numeric_placeholder() { + // Both literals occupy local placeholder index 0 within their + // window; distinct `Str#`/`Num#` prefixes must keep them from + // aliasing each other. + let with_string = r#"const first = "x";"#; + let with_number = "const first = 1;"; + let (norm_str, _) = ast_hash_first_n_stmts(with_string, 1); + let (norm_num, _) = ast_hash_first_n_stmts(with_number, 1); + assert_ne!( + norm_str, norm_num, + "a string literal placeholder at index 0 must not hash the same as \ + a numeric literal placeholder at index 0" + ); + } + + /// Run DuplicateBlock on two *different* sources (one per file) and + /// return the issues emitted by finalize. Unlike + /// `run_duplicate_block_with_options`, the two files needn't be + /// byte-identical — used to test literal-only differences. + fn run_duplicate_block_two_sources( + check: &DuplicateBlock, + source_a: &str, + source_b: &str, + options: &CheckOptions, + ) -> Vec { + let corpus = CorpusIndex::default(); + + let alloc_a = Allocator::default(); + let file_a = SourceFile::new(PathBuf::from("a.ts"), source_a.to_string()); + let ret_a = parse_into(&alloc_a, &file_a); + let view_a = ParsedView { + program: &ret_a.program, + diagnostics: &ret_a.errors, + }; + let mut ctx_a = CheckContext::new(&file_a) + .with_parsed(&view_a) + .with_options(options) + .with_corpus(&corpus); + check.run(&file_a, &mut ctx_a); + + let alloc_b = Allocator::default(); + let file_b = SourceFile::new(PathBuf::from("b.ts"), source_b.to_string()); + let ret_b = parse_into(&alloc_b, &file_b); + let view_b = ParsedView { + program: &ret_b.program, + diagnostics: &ret_b.errors, + }; + let mut ctx_b = CheckContext::new(&file_b) + .with_parsed(&view_b) + .with_options(options) + .with_corpus(&corpus); + check.run(&file_b, &mut ctx_b); + + let mut finalize_ctx = FinalizeContext::new(&corpus); + check.finalize(&mut finalize_ctx) + } + + /// Six structurally identical statements whose literal values are + /// parameterised, large enough to clear the default thresholds. + fn make_duplicate_source_with_literals(product: &str, amount_cents: i64) -> String { + format!( + "const productId = \"{product}\";\n\ + const amountCents = {amount_cents};\n\ + const description = \"description for {product}\";\n\ + const invoice = createInvoice(productId, amountCents);\n\ + const receipt = submitInvoice(invoice, description);\n\ + return receipt;" + ) + } + + /// Runs `DuplicateBlock::run` (the sole corpus writer) on two files, + /// then finalizes both `dup` and `near` against the same corpus and + /// returns `(dup_issues, near_issues)`. Used everywhere the CD-331 + /// split needs pinning: which check a group lands on, and that the + /// two never claim overlapping territory. + fn run_both_checks_two_sources( + dup: &DuplicateBlock, + near: &NearDuplicateBlock, + source_a: &str, + source_b: &str, + options: &CheckOptions, + ) -> (Vec, Vec) { + let corpus = CorpusIndex::default(); + + let alloc_a = Allocator::default(); + let file_a = SourceFile::new(PathBuf::from("a.ts"), source_a.to_string()); + let ret_a = parse_into(&alloc_a, &file_a); + let view_a = ParsedView { + program: &ret_a.program, + diagnostics: &ret_a.errors, + }; + let mut ctx_a = CheckContext::new(&file_a) + .with_parsed(&view_a) + .with_options(options) + .with_corpus(&corpus); + dup.run(&file_a, &mut ctx_a); + + let alloc_b = Allocator::default(); + let file_b = SourceFile::new(PathBuf::from("b.ts"), source_b.to_string()); + let ret_b = parse_into(&alloc_b, &file_b); + let view_b = ParsedView { + program: &ret_b.program, + diagnostics: &ret_b.errors, + }; + let mut ctx_b = CheckContext::new(&file_b) + .with_parsed(&view_b) + .with_options(options) + .with_corpus(&corpus); + dup.run(&file_b, &mut ctx_b); + + let mut finalize_ctx_dup = FinalizeContext::new(&corpus); + let dup_issues = dup.finalize(&mut finalize_ctx_dup); + let mut finalize_ctx_near = FinalizeContext::new(&corpus); + let near_issues = near.finalize(&mut finalize_ctx_near); + (dup_issues, near_issues) + } + + #[test] + fn duplicate_block_exact_clone_lands_on_duplicate_block_not_near() { + let dup = DuplicateBlock::default(); + let near = NearDuplicateBlock::default(); + let opts = CheckOptions::defaults_from(DUP_BLOCK_OPTIONS); + let source = make_duplicate_source(); + let (dup_issues, near_issues) = + run_both_checks_two_sources(&dup, &near, &source, &source, &opts); + assert_eq!( + dup_issues.len(), + 1, + "expected exactly one Refactor.DuplicateBlock finding for a verbatim clone, got {:?}", + dup_issues.iter().map(|i| &i.message).collect::>() + ); + assert_eq!(dup_issues[0].check_id, "Refactor.DuplicateBlock"); + assert!( + near_issues.is_empty(), + "a verbatim clone must not also be reported by Refactor.NearDuplicateBlock, got {:?}", + near_issues.iter().map(|i| &i.message).collect::>() + ); + } + + #[test] + fn duplicate_block_literal_drift_lands_on_near_duplicate_block_not_duplicate_block() { + let dup = DuplicateBlock::default(); + let near = NearDuplicateBlock::default(); + let opts = CheckOptions::defaults_from(DUP_BLOCK_OPTIONS); + let source_a = make_duplicate_source_with_literals("gold", 4999); + let source_b = make_duplicate_source_with_literals("silver", 2999); + let (dup_issues, near_issues) = + run_both_checks_two_sources(&dup, &near, &source_a, &source_b, &opts); + assert!( + dup_issues.is_empty(), + "blocks differing only in literal values must not be reported by \ + Refactor.DuplicateBlock, got {:?}", + dup_issues.iter().map(|i| &i.message).collect::>() + ); + assert_eq!( + near_issues.len(), + 1, + "expected exactly one Refactor.NearDuplicateBlock finding for blocks differing \ + only in literals, got {:?}", + near_issues.iter().map(|i| &i.message).collect::>() + ); + assert_eq!(near_issues[0].check_id, "Refactor.NearDuplicateBlock"); + assert!( + near_issues[0] + .message + .contains("differing only in literal values"), + "message should call out that the blocks differ only in literal values, got {:?}", + near_issues[0].message + ); + } + + #[test] + fn duplicate_block_normalize_literals_false_reproduces_old_grouping() { + let dup = DuplicateBlock::default(); + let near = NearDuplicateBlock::default(); + let mut raw: BTreeMap = BTreeMap::new(); + raw.insert( + "normalize_literals".to_string(), + RawOptionValue::Bool(false), + ); + let opts = validate_options("Refactor.DuplicateBlock", DUP_BLOCK_OPTIONS, &raw).unwrap(); + let source_a = make_duplicate_source_with_literals("gold", 4999); + let source_b = make_duplicate_source_with_literals("silver", 2999); + let (dup_issues, near_issues) = + run_both_checks_two_sources(&dup, &near, &source_a, &source_b, &opts); + assert!( + dup_issues.is_empty() && near_issues.is_empty(), + "with normalize_literals=false, blocks differing only in literal values must not \ + be grouped by either check (pre-CD-331 behaviour), got dup={:?} near={:?}", + dup_issues.iter().map(|i| &i.message).collect::>(), + near_issues.iter().map(|i| &i.message).collect::>() + ); + } + + #[test] + fn duplicate_block_and_near_duplicate_block_never_overlap_spans() { + // Mix a verbatim-clone pair with a literal-drift pair across the + // same two files and assert that no span (primary or related) + // reported by either check overlaps a span reported by the + // other, for the same file — pinning that `partition_claimed_groups` + // runs its overlap-claim pass once, across both checks' candidates + // together, rather than each check claiming independently. + let dup = DuplicateBlock::default(); + let near = NearDuplicateBlock::default(); + let opts = CheckOptions::defaults_from(DUP_BLOCK_OPTIONS); + let exact = make_duplicate_source(); + let near_a = make_duplicate_source_with_literals("gold", 4999); + let near_b = make_duplicate_source_with_literals("silver", 2999); + let source_a = format!("{exact}\n\n{near_a}"); + let source_b = format!("{exact}\n\n{near_b}"); + let (dup_issues, near_issues) = + run_both_checks_two_sources(&dup, &near, &source_a, &source_b, &opts); + assert_eq!( + dup_issues.len(), + 1, + "expected the verbatim clone, got {dup_issues:?}" + ); + assert_eq!( + near_issues.len(), + 1, + "expected the literal-drift clone, got {near_issues:?}" + ); + + let spans_of = |issues: &[CoreIssue]| -> Vec<(PathBuf, u32, u32)> { + issues + .iter() + .flat_map(|i| { + std::iter::once((i.file.clone(), i.location.line(), i.location.line())).chain( + i.related + .iter() + .map(|r| (r.file.clone(), r.location.line(), r.location.line())), + ) + }) + .collect() + }; + let dup_spans = spans_of(&dup_issues); + let near_spans = spans_of(&near_issues); + for (dfile, dline, _) in &dup_spans { + for (nfile, nline, _) in &near_spans { + assert!( + !(dfile == nfile && dline == nline), + "Refactor.DuplicateBlock and Refactor.NearDuplicateBlock reported the same \ + location {dfile:?}:{dline}" + ); + } + } + } + + // ─── DuplicateBlock: import/re-export windows excluded (CD-331 follow-up) ── + // + // Real-repo validation of the literal-normalization fix above turned up a + // false-positive class it did not anticipate: normalizing string literals + // also normalizes module specifiers, so any two same-length runs of + // `import` statements now hash equal — an import block can never be + // "extracted into a shared helper", so the finding is never actionable. + // Import declarations and re-exports (`export { x } from './y'`) are now + // excluded from AST-mode windows entirely; a plain `export { x }` or + // `export function f() {}` (no `source`) is ordinary code and still + // participates. + + fn make_import_block(prefix: &str) -> String { + (0..6) + .map(|i| format!("import {{ value{i} }} from \"{prefix}/mod{i}\";")) + .collect::>() + .join("\n") + } + + fn make_reexport_block(prefix: &str) -> String { + (0..6) + .map(|i| format!("export {{ value{i} }} from \"{prefix}/mod{i}\";")) + .collect::>() + .join("\n") + } + + fn make_plain_export_function_block(offset: i64) -> String { + (0..6) + .map(|i| format!("export function step{i}() {{ return {}; }}", offset + i)) + .collect::>() + .join("\n") + } + + #[test] + fn duplicate_block_import_run_produces_no_finding() { + let check = DuplicateBlock::default(); + let opts = CheckOptions::defaults_from(DUP_BLOCK_OPTIONS); + let source_a = make_import_block("./a"); + let source_b = make_import_block("./b"); + let issues = run_duplicate_block_two_sources(&check, &source_a, &source_b, &opts); + assert!( + issues.is_empty(), + "a run of six or more import declarations differing only in module \ + specifiers must not be flagged as a duplicate block, got {:?}", + issues.iter().map(|i| &i.message).collect::>() + ); + } + + #[test] + fn duplicate_block_reexport_run_produces_no_finding() { + let check = DuplicateBlock::default(); + let opts = CheckOptions::defaults_from(DUP_BLOCK_OPTIONS); + let source_a = make_reexport_block("./a"); + let source_b = make_reexport_block("./b"); + let issues = run_duplicate_block_two_sources(&check, &source_a, &source_b, &opts); + assert!( + issues.is_empty(), + "a run of re-exports (`export {{ x }} from './y'`) differing only in \ + module specifiers must not be flagged as a duplicate block, got {:?}", + issues.iter().map(|i| &i.message).collect::>() + ); + } + + #[test] + fn duplicate_block_plain_export_function_still_participates_in_windows() { + // Sanity guard against over-excluding: an `ExportNamedDeclaration` + // with no `source` (a plain `export function`) is ordinary code and + // must still be windowed and flagged like any other duplicate. + let check = DuplicateBlock::default(); + let opts = CheckOptions::defaults_from(DUP_BLOCK_OPTIONS); + // Same offset on both sides — a verbatim clone, so this stays a + // pure windowing sanity check rather than exercising the + // literal-drift split (that's covered separately, above). + let source_a = make_plain_export_function_block(100); + let source_b = make_plain_export_function_block(100); + let issues = run_duplicate_block_two_sources(&check, &source_a, &source_b, &opts); + assert_eq!( + issues.len(), + 1, + "a plain `export function` block with no `source` must still be \ + windowed and flagged, got {:?}", + issues.iter().map(|i| &i.message).collect::>() + ); + } + // ─── UnusedVariable: TS parameter properties (cd-sh72 / gh #44) ───────── // // A parameter property (`constructor(private ctx: T)`) is both a diff --git a/docs/.vitepress/sidebar-checks.ts b/docs/.vitepress/sidebar-checks.ts index 1b0e24f5..8c743fb9 100644 --- a/docs/.vitepress/sidebar-checks.ts +++ b/docs/.vitepress/sidebar-checks.ts @@ -60,6 +60,7 @@ export const checksItems = [ { text: 'LongAndComplex', link: '/checks/Refactor.LongAndComplex' }, { text: 'MixedThrowAndReturnError', link: '/checks/Refactor.MixedThrowAndReturnError' }, { text: 'MutatedParameter', link: '/checks/Refactor.MutatedParameter' }, + { text: 'NearDuplicateBlock', link: '/checks/Refactor.NearDuplicateBlock' }, { text: 'PreferArrayMethodOverLoop', link: '/checks/Refactor.PreferArrayMethodOverLoop' }, { text: 'PreferConstOverLet', link: '/checks/Refactor.PreferConstOverLet' }, { text: 'PreferNullishCoalescing', link: '/checks/Refactor.PreferNullishCoalescing' }, diff --git a/docs/checks/Refactor.DuplicateBlock.md b/docs/checks/Refactor.DuplicateBlock.md index 08db012d..ad8b3d13 100644 --- a/docs/checks/Refactor.DuplicateBlock.md +++ b/docs/checks/Refactor.DuplicateBlock.md @@ -4,14 +4,14 @@ title: Refactor.DuplicateBlock category: Refactor base_priority: 12 default_severity: Medium -options: [min_statements, min_chars, include_tokens, include_ast] +options: [min_statements, min_chars, include_tokens, include_ast, normalize_literals] autofix: false --- -Runs of statements that recur (after rename canonicalisation) in multiple files. Likely copy-paste — extract a shared helper. Canonicalisation maps identifier tokens to per-window local indices so renamed copies still match. Minimum window is `6` consecutive statements (and `80` characters) to keep noise low. Cross-file: per-file `run` writes fingerprints into the shared corpus; `finalize` groups by hash and emits one `Issue` per duplicate set with `related` spans pointing at every other occurrence. +Runs of statements that recur, verbatim, in multiple files. Likely copy-paste — extract a shared helper. Canonicalisation maps identifier tokens to per-window local indices so renamed copies still match; a block that is otherwise identical but carries different string or number literal values is not this check's concern — it is reported separately, at low severity, by [`Refactor.NearDuplicateBlock`](Refactor.NearDuplicateBlock.md), since a near-clone is a weaker and noisier signal than a byte-identical one and should not trip a default CI gate on its own. The `normalize_literals` option controls which of the two checks a given block lands on (see that page), not whether it is reported at all. `import` declarations and re-exports (`export { x } from './y'`) are never windowed at all — an import block can't be extracted into a shared helper, so treating one run of imports as a duplicate of another is never actionable, and normalizing their module-specifier string literals would otherwise make every same-length import block in a project match every other. A plain `export { x }` or `export function f() {}` (no `source`) is ordinary code and is unaffected. Minimum window is `6` consecutive statements (and `80` characters) to keep noise low. Cross-file: per-file `run` writes fingerprints into a corpus slot shared with `Refactor.NearDuplicateBlock`; `finalize` groups by hash, runs one overlap-claim pass across both checks' candidates together (so the two never report overlapping spans), and emits one `Issue` per verbatim-duplicate set with `related` spans pointing at every other occurrence. ```ts // src/orders.ts:42 @@ -49,4 +49,8 @@ occurrence — one primary location plus `related` spans for the rest. A `cofferdam-ignore: Refactor.DuplicateBlock` comment placed at *any* occurrence (the primary one or any related one) suppresses the whole finding, not just that copy. You don't need to find and suppress every occurrence individually — one ignore -comment on either side of a duplicated pair is enough. +comment on either side of a duplicated pair is enough. Suppression targets the +comment's next non-blank line, not a range, so anchor it to the first *statement* +of the duplicated run — not to an import or other declaration preceding it. Import +and re-export statements are excluded from windows entirely, so a comment sitting +above one no longer covers the block that follows. diff --git a/docs/checks/Refactor.NearDuplicateBlock.md b/docs/checks/Refactor.NearDuplicateBlock.md new file mode 100644 index 00000000..433690a3 --- /dev/null +++ b/docs/checks/Refactor.NearDuplicateBlock.md @@ -0,0 +1,43 @@ +--- +id: Refactor.NearDuplicateBlock +title: Refactor.NearDuplicateBlock +category: Refactor +base_priority: 10 +default_severity: Low +options: [min_statements, min_chars, include_tokens, include_ast, normalize_literals] +autofix: false +--- + + + + +Runs of statements that are structurally identical to a block elsewhere in the project but differ in a string or number literal value — a near-clone rather than a verbatim one. Identifier tokens were already canonicalised to per-window local indices before this check existed, so a renamed copy has always matched [`Refactor.DuplicateBlock`](Refactor.DuplicateBlock.md); this check is specifically about the case where a literal, not a name, is the only thing that changed. That is usually the interesting half of the finding: two blocks drift apart because someone copied one and edited the values, and the edit — which fields moved, which threshold changed — is worth a look even though the shape underneath is unchanged. + +```ts +// src/billing/charge-gold.ts +const productId = "gold-membership"; +const amountCents = 4999; +const invoice = createInvoice(account, productId, amountCents); +const receipt = submitInvoice(invoice, "Gold membership"); +return receipt; +``` + +```ts +// src/billing/charge-silver.ts — same shape, different literals: flagged as related +const productId = "silver-membership"; +const amountCents = 2999; +const invoice = createInvoice(account, productId, amountCents); +const receipt = submitInvoice(invoice, "Silver membership"); +return receipt; +``` + +Both blocks share one `finalize` pass and one corpus slot (`Refactor.DuplicateBlock.fingerprints`) with `Refactor.DuplicateBlock` — only that check's `run` writes to it, this one reads the same data back and reports the other half of the same grouping: groups whose members are structurally identical (same `hash`) but not byte-identical (differing `exact_hash`). The two checks never report overlapping spans, because the shared overlap-claim pass runs once across both checks' candidates before either is filtered out. + +**Severity:** near-clones default to `low`, unlike `Refactor.DuplicateBlock`'s `medium`, and print without tripping the default `--fail-on medium` gate. That is deliberate — severity is set per check id, not per finding, so splitting verbatim clones from literal-drift ones into two ids was the only way to keep a noisier, less actionable signal from failing a build that only meant to gate on real copy-paste. To gate on this check too, raise its severity in `cofferdam.toml`: + +```toml +[checks."Refactor.NearDuplicateBlock"] +severity = "medium" +``` + +**Suppressing:** as with `Refactor.DuplicateBlock`, each group is one `Issue` with `related` spans for every other occurrence, and a `cofferdam-ignore: Refactor.NearDuplicateBlock` comment at any occurrence suppresses the whole finding. diff --git a/docs/checks/index.md b/docs/checks/index.md index 949dcc17..edd20bac 100644 --- a/docs/checks/index.md +++ b/docs/checks/index.md @@ -52,6 +52,7 @@ This catalog is generated from `CheckMeta` in the cofferdam source — every che - [`Refactor.LongAndComplex`](Refactor.LongAndComplex.md) `file` `advisable` — Functions that are both long and complex are the strongest refactor candidates. Length alone catches flat config tables; complexity alone catches deeply-branching short helpers. The intersection is almost always a real refactor target. - [`Refactor.MixedThrowAndReturnError`](Refactor.MixedThrowAndReturnError.md) `file` — A function that both throws and returns an error-shaped object for what looks like the same class of failure mixes two error-handling idioms, hurting composability of error paths for callers. - [`Refactor.MutatedParameter`](Refactor.MutatedParameter.md) `file` — Reassigning or mutating a function parameter breaks pure input→output semantics, making the function harder to test and reason about in isolation. +- [`Refactor.NearDuplicateBlock`](Refactor.NearDuplicateBlock.md) `file` `advisable` — Runs of statements that are structurally identical but differ in their string or number literals — often the same logic copied and then partially edited, where the edit is the thing worth looking at. - [`Refactor.PreferArrayMethodOverLoop`](Refactor.PreferArrayMethodOverLoop.md) `file` — A loop whose entire body pushes one computed value (optionally gated by a single `if`) onto an accumulator array is more clearly expressed as `.map()`/`.filter()`. - [`Refactor.PreferConstOverLet`](Refactor.PreferConstOverLet.md) `file` — A `let` binding that's never reassigned should be `const` — it signals the value doesn't change and rules out reassignment bugs at compile time. - [`Refactor.PreferNullishCoalescing`](Refactor.PreferNullishCoalescing.md) `file` — `x || default` falls through on every falsy value (`0`, `""`, `false`). Use `??` to fall through only on `null`/`undefined`. diff --git a/docs/public/checks.json b/docs/public/checks.json index 25f00650..3027d4f0 100644 --- a/docs/public/checks.json +++ b/docs/public/checks.json @@ -669,7 +669,7 @@ "base_priority": 12, "default_severity": "Medium", "explanation": "Runs of statements that recur (after rename canonicalisation) in multiple files. Likely copy-paste — extract a shared helper.", - "body": "---\nid: Refactor.DuplicateBlock\ncategory: Refactor\nbase_priority: 12\ndefault_severity: Medium\noptions: [min_statements, min_chars, include_tokens, include_ast]\n---\n\nRuns of statements that recur (after rename canonicalisation) in multiple files. Likely copy-paste — extract a shared helper. Canonicalisation maps identifier tokens to per-window local indices so renamed copies still match. Minimum window is `6` consecutive statements (and `80` characters) to keep noise low. Cross-file: per-file `run` writes fingerprints into the shared corpus; `finalize` groups by hash and emits one `Issue` per duplicate set with `related` spans pointing at every other occurrence.\n\n```ts\n// src/orders.ts:42\nconst items = parseItems(input);\nconst validated = validateItems(items);\nconst priced = priceItems(validated, currency);\nconst taxed = applyTax(priced, region);\nconst total = sumItems(taxed);\nreturn { items: taxed, total };\n```\n\n```ts\n// src/quotes.ts:88 — same shape, renamed: flagged as related\nconst products = parseItems(input);\nconst checkedProducts = validateItems(products);\nconst pricedProducts = priceItems(checkedProducts, currency);\nconst taxedProducts = applyTax(pricedProducts, region);\nconst total = sumItems(taxedProducts);\nreturn { items: taxedProducts, total };\n```\n\n```ts\n// fix: extract once\nexport function pipeline(input: RawInput, currency: Currency, region: Region) {\n const items = parseItems(input);\n const validated = validateItems(items);\n const priced = priceItems(validated, currency);\n const taxed = applyTax(priced, region);\n return { items: taxed, total: sumItems(taxed) };\n}\n```\n\n**Suppressing:** each duplicate group is emitted as a single `Issue` covering every\noccurrence — one primary location plus `related` spans for the rest. A\n`cofferdam-ignore: Refactor.DuplicateBlock` comment placed at *any* occurrence (the\nprimary one or any related one) suppresses the whole finding, not just that copy.\nYou don't need to find and suppress every occurrence individually — one ignore\ncomment on either side of a duplicated pair is enough.\n", + "body": "---\nid: Refactor.DuplicateBlock\ncategory: Refactor\nbase_priority: 12\ndefault_severity: Medium\noptions: [min_statements, min_chars, include_tokens, include_ast, normalize_literals]\n---\n\nRuns of statements that recur, verbatim, in multiple files. Likely copy-paste — extract a shared helper. Canonicalisation maps identifier tokens to per-window local indices so renamed copies still match; a block that is otherwise identical but carries different string or number literal values is not this check's concern — it is reported separately, at low severity, by [`Refactor.NearDuplicateBlock`](Refactor.NearDuplicateBlock.md), since a near-clone is a weaker and noisier signal than a byte-identical one and should not trip a default CI gate on its own. The `normalize_literals` option controls which of the two checks a given block lands on (see that page), not whether it is reported at all. `import` declarations and re-exports (`export { x } from './y'`) are never windowed at all — an import block can't be extracted into a shared helper, so treating one run of imports as a duplicate of another is never actionable, and normalizing their module-specifier string literals would otherwise make every same-length import block in a project match every other. A plain `export { x }` or `export function f() {}` (no `source`) is ordinary code and is unaffected. Minimum window is `6` consecutive statements (and `80` characters) to keep noise low. Cross-file: per-file `run` writes fingerprints into a corpus slot shared with `Refactor.NearDuplicateBlock`; `finalize` groups by hash, runs one overlap-claim pass across both checks' candidates together (so the two never report overlapping spans), and emits one `Issue` per verbatim-duplicate set with `related` spans pointing at every other occurrence.\n\n```ts\n// src/orders.ts:42\nconst items = parseItems(input);\nconst validated = validateItems(items);\nconst priced = priceItems(validated, currency);\nconst taxed = applyTax(priced, region);\nconst total = sumItems(taxed);\nreturn { items: taxed, total };\n```\n\n```ts\n// src/quotes.ts:88 — same shape, renamed: flagged as related\nconst products = parseItems(input);\nconst checkedProducts = validateItems(products);\nconst pricedProducts = priceItems(checkedProducts, currency);\nconst taxedProducts = applyTax(pricedProducts, region);\nconst total = sumItems(taxedProducts);\nreturn { items: taxedProducts, total };\n```\n\n```ts\n// fix: extract once\nexport function pipeline(input: RawInput, currency: Currency, region: Region) {\n const items = parseItems(input);\n const validated = validateItems(items);\n const priced = priceItems(validated, currency);\n const taxed = applyTax(priced, region);\n return { items: taxed, total: sumItems(taxed) };\n}\n```\n\n**Suppressing:** each duplicate group is emitted as a single `Issue` covering every\noccurrence — one primary location plus `related` spans for the rest. A\n`cofferdam-ignore: Refactor.DuplicateBlock` comment placed at *any* occurrence (the\nprimary one or any related one) suppresses the whole finding, not just that copy.\nYou don't need to find and suppress every occurrence individually — one ignore\ncomment on either side of a duplicated pair is enough. Suppression targets the\ncomment's next non-blank line, not a range, so anchor it to the first *statement*\nof the duplicated run — not to an import or other declaration preceding it. Import\nand re-export statements are excluded from windows entirely, so a comment sitting\nabove one no longer covers the block that follows.\n", "requires_types": false, "consistency": false, "autofix": false, @@ -697,6 +697,12 @@ "kind": "boolean", "doc": "run the AST statement-window pass (disable to use token-mode only)", "default": true + }, + { + "id": "normalize_literals", + "kind": "boolean", + "doc": "treat string and number literals as positional placeholders, so blocks differing only in their literal values are still reported as duplicates", + "default": true } ], "badges": [ @@ -763,6 +769,53 @@ "file" ] }, + { + "id": "Refactor.NearDuplicateBlock", + "category": "Refactor", + "base_priority": 10, + "default_severity": "Low", + "explanation": "Runs of statements that are structurally identical but differ in their string or number literals — often the same logic copied and then partially edited, where the edit is the thing worth looking at.", + "body": "---\nid: Refactor.NearDuplicateBlock\ncategory: Refactor\nbase_priority: 10\ndefault_severity: Low\noptions: [min_statements, min_chars, include_tokens, include_ast, normalize_literals]\n---\n\nRuns of statements that are structurally identical to a block elsewhere in the project but differ in a string or number literal value — a near-clone rather than a verbatim one. Identifier tokens were already canonicalised to per-window local indices before this check existed, so a renamed copy has always matched [`Refactor.DuplicateBlock`](Refactor.DuplicateBlock.md); this check is specifically about the case where a literal, not a name, is the only thing that changed. That is usually the interesting half of the finding: two blocks drift apart because someone copied one and edited the values, and the edit — which fields moved, which threshold changed — is worth a look even though the shape underneath is unchanged.\n\n```ts\n// src/billing/charge-gold.ts\nconst productId = \"gold-membership\";\nconst amountCents = 4999;\nconst invoice = createInvoice(account, productId, amountCents);\nconst receipt = submitInvoice(invoice, \"Gold membership\");\nreturn receipt;\n```\n\n```ts\n// src/billing/charge-silver.ts — same shape, different literals: flagged as related\nconst productId = \"silver-membership\";\nconst amountCents = 2999;\nconst invoice = createInvoice(account, productId, amountCents);\nconst receipt = submitInvoice(invoice, \"Silver membership\");\nreturn receipt;\n```\n\nBoth blocks share one `finalize` pass and one corpus slot (`Refactor.DuplicateBlock.fingerprints`) with `Refactor.DuplicateBlock` — only that check's `run` writes to it, this one reads the same data back and reports the other half of the same grouping: groups whose members are structurally identical (same `hash`) but not byte-identical (differing `exact_hash`). The two checks never report overlapping spans, because the shared overlap-claim pass runs once across both checks' candidates before either is filtered out.\n\n**Severity:** near-clones default to `low`, unlike `Refactor.DuplicateBlock`'s `medium`, and print without tripping the default `--fail-on medium` gate. That is deliberate — severity is set per check id, not per finding, so splitting verbatim clones from literal-drift ones into two ids was the only way to keep a noisier, less actionable signal from failing a build that only meant to gate on real copy-paste. To gate on this check too, raise its severity in `cofferdam.toml`:\n\n```toml\n[checks.\"Refactor.NearDuplicateBlock\"]\nseverity = \"medium\"\n```\n\n**Suppressing:** as with `Refactor.DuplicateBlock`, each group is one `Issue` with `related` spans for every other occurrence, and a `cofferdam-ignore: Refactor.NearDuplicateBlock` comment at any occurrence suppresses the whole finding.\n", + "requires_types": false, + "consistency": false, + "autofix": false, + "options": [ + { + "id": "min_statements", + "kind": "integer", + "doc": "minimum number of consecutive statements required to flag a duplicate block", + "default": 6 + }, + { + "id": "min_chars", + "kind": "integer", + "doc": "minimum raw byte length of a duplicate window; prevents trivial single-liner runs from firing", + "default": 80 + }, + { + "id": "include_tokens", + "kind": "boolean", + "doc": "also run a sliding token-window pass that catches duplicates spanning non-statement boundaries", + "default": false + }, + { + "id": "include_ast", + "kind": "boolean", + "doc": "run the AST statement-window pass (disable to use token-mode only)", + "default": true + }, + { + "id": "normalize_literals", + "kind": "boolean", + "doc": "treat string and number literals as positional placeholders, so blocks differing only in their literal values are still reported as duplicates", + "default": true + } + ], + "badges": [ + "file", + "advisable" + ] + }, { "id": "Refactor.PreferArrayMethodOverLoop", "category": "Refactor", diff --git a/examples/duplicate_block_literals.ts b/examples/duplicate_block_literals.ts new file mode 100644 index 00000000..59e9f7e7 --- /dev/null +++ b/examples/duplicate_block_literals.ts @@ -0,0 +1,69 @@ +// CD-331: type-2 clones — blocks with identical structure that differ +// only in their literal values. Refactor.DuplicateBlock should flag the +// pair below (with the "differing only in literal values" wording) even +// though every string/number literal is different. The unrelated block +// at the bottom has different structure and must NOT be flagged. +// +// The two 6-statement import runs below (CD-331 follow-up) are a second, +// separate hazard: under literal normalization, module specifiers +// collapse just like any other string literal, so two same-length import +// blocks would otherwise hash equal too — an unactionable false positive, +// since you cannot extract a shared helper out of an import list. Import +// declarations are excluded from windowing entirely, so this pair must +// NOT be flagged. +import { helperOne } from "./helpers/helper-one"; +import { helperTwo } from "./helpers/helper-two"; +import { helperThree } from "./helpers/helper-three"; +import { helperFour } from "./helpers/helper-four"; +import { helperFive } from "./helpers/helper-five"; +import { helperSix } from "./helpers/helper-six"; + +export function chargeGold(account: Account): Receipt { + const productId = "gold-membership"; + const amountCents = 4999; + const description = "Gold membership"; + const invoice = createInvoice(account, productId, amountCents); + const receipt = submitInvoice(invoice, description); + return receipt; +} + +export function chargeSilver(account: Account): Receipt { + const productId = "silver-membership"; + const amountCents = 2999; + const description = "Silver membership"; + const invoice = createInvoice(account, productId, amountCents); + const receipt = submitInvoice(invoice, description); + return receipt; +} + +export function renderSummaryPanel(items: Item[]): string { + const rows = items.map((item) => `${item.name}: ${item.count}`); + const header = "Summary"; + const totalCount = items.reduce((sum, item) => sum + item.count, 0); + const footer = `Total: ${totalCount}`; + return [header, ...rows, footer].join("\n"); +} + +import { utilOne } from "./util/util-one"; +import { utilTwo } from "./util/util-two"; +import { utilThree } from "./util/util-three"; +import { utilFour } from "./util/util-four"; +import { utilFive } from "./util/util-five"; +import { utilSix } from "./util/util-six"; + +declare interface Account { + id: string; +} +declare interface Receipt { + id: string; +} +declare interface Item { + name: string; + count: number; +} +declare function createInvoice( + account: Account, + productId: string, + amountCents: number +): unknown; +declare function submitInvoice(invoice: unknown, description: string): Receipt;