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;