From 168d1f117681031a33f6bc06891d315e015140cd Mon Sep 17 00:00:00 2001 From: Vaiz <4908982+Vaiz@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:07:19 +0100 Subject: [PATCH 1/3] fix(cargo-anvil): stop writing an unparsable TOML host when a table is hand-written Introducing a managed region that declares a whole table beside a hand-written copy of that table produced two identical headers, which TOML rejects outright. The generator had already rewritten the file and recorded the region by the time anything noticed, so a repository onboarding with a customised `deny.toml` got one that `cargo deny` cannot read. Adoption now classifies each hand-written entry against the region body instead of accepting or declining the table as a whole. An entry the body also declares with the same value is dropped, because the region re-emits it. An entry the body does not declare is kept as residue and re-emitted after the region's closing sentinel, where it continues the table the region opens -- carried across as its original source slice, so the comments written around it survive. An entry both declare with different values has no safe output at all, and is refused. Whatever adoption concludes, the spliced result is parsed before it is planned. If it would not parse, the region is refused: the host is left alone, a diagnostic names it, and every other artifact is still planned. Other managed regions are masked out of that check, because two regions may legitimately declare the same key while a migration is in flight. Table location moves off the line-oriented scanner and onto the parsed document, which is what makes the above possible and removes two long-standing limitations. A bracketed line inside a `"""` value is a value to the parser, so the guard that declined adoption for any host merely containing a multi-line string is gone. The rewrite is now a copy of the gaps between non-overlapping deletion ranges, so the `in_managed`/`dropping` streaming flag pair -- the source of two defects found in review of #140 -- is gone with it. The fixtures that write TOML hosts now assert the output parses rather than that it contains an expected fragment, which is what let this survive: the `migration` fixture produced the duplicate header and passed. Closes #148 Closes #149 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/cargo-anvil/Cargo.toml | 1 + crates/cargo-anvil/docs/design/updates.md | 52 +- crates/cargo-anvil/src/emit/managed_region.rs | 192 +++- crates/cargo-anvil/src/emit/mod.rs | 2 +- crates/cargo-anvil/src/region.rs | 854 ++++++++++++------ crates/cargo-anvil/src/run.rs | 45 +- crates/cargo-anvil/tests/fixtures.rs | 87 +- .../tests/fixtures/deny-conflict/Cargo.toml | 11 + .../tests/fixtures/deny-conflict/Justfile | 8 + .../deny-conflict/crates/alpha/Cargo.toml | 7 + .../deny-conflict/crates/alpha/src/lib.rs | 1 + .../tests/fixtures/deny-conflict/deny.toml | 7 + 12 files changed, 923 insertions(+), 344 deletions(-) create mode 100644 crates/cargo-anvil/tests/fixtures/deny-conflict/Cargo.toml create mode 100644 crates/cargo-anvil/tests/fixtures/deny-conflict/Justfile create mode 100644 crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/Cargo.toml create mode 100644 crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/src/lib.rs create mode 100644 crates/cargo-anvil/tests/fixtures/deny-conflict/deny.toml diff --git a/crates/cargo-anvil/Cargo.toml b/crates/cargo-anvil/Cargo.toml index 8518cb4e8..f13428ef2 100644 --- a/crates/cargo-anvil/Cargo.toml +++ b/crates/cargo-anvil/Cargo.toml @@ -43,6 +43,7 @@ insta = { workspace = true, features = ["filters"] } predicates = { workspace = true } serial_test = { workspace = true } tempfile = { workspace = true } +toml_edit = { workspace = true } walkdir = { workspace = true } # >>> anvil-managed: anvil-lints diff --git a/crates/cargo-anvil/docs/design/updates.md b/crates/cargo-anvil/docs/design/updates.md index 89a40f98e..8f64aac70 100644 --- a/crates/cargo-anvil/docs/design/updates.md +++ b/crates/cargo-anvil/docs/design/updates.md @@ -246,27 +246,37 @@ preserves their edit. TOML rejects a duplicate table header, so appending a region that declares `[lints]` to a host that already declares `[lints]` by hand does not merely duplicate text — it produces a manifest that will not parse. On **first introduction only** (once the region exists, -in-place replacement applies and there is nothing to adopt), the tool therefore removes -the hand-written table instead of duplicating it, including any comments and blank lines -within that table's range. - -Adoption is deliberately narrow. A hand-written table is removed only when **every** one -of its configuration lines already appears in the rendered region body; the body may -declare further lines of its own. Adoption is declined — the hand-written table stays -exactly where it is — when any of the following holds: - -- the table carries configuration the region body does not (silently discarding a user's - settings is a worse outcome than a visible parse failure); -- either the host or the body contains a multi-line string (`"""` or `'''`), which a - line-oriented scanner cannot classify safely; -- the header is an array of tables (`[[bin]]`), which TOML permits to repeat, so a second - one is not a duplicate; -- the table sits inside an existing managed region, which the tool already owns. - -A declined adoption is not a refusal to write: the region is still inserted, so a host -that declares a conflicting ordinary table still ends up with a duplicate-table parse -error. `deny.toml` with user-authored `[advisories]` entries is the case that hits this, -and resolving it is tracked separately. +in-place replacement applies and there is nothing to adopt), the tool therefore takes over +the hand-written table instead of duplicating it. + +The host is read with the TOML parser, with any existing managed regions blanked out, so +table headers are located by parsing rather than by scanning for a bracketed line. A +bracketed line inside a `"""` value is a value to the parser and can no longer be mistaken +for a header, and a host that already carries both a region copy and a hand-written copy — +the very duplicate-header file this repairs — still parses in that masked view. + +Each hand-written entry is then classified against the rendered region body: + +- **declared by both with the same value** — covered, and dropped, since the region + re-emits it; +- **declared only by hand** — kept as *residue*, and re-emitted directly after the + region's closing sentinel, where it continues the table the region opens. The entry's + original source slice is moved, so its comments and spacing survive byte-for-byte; +- **declared by both with different values** — a conflict. There is no output that keeps + both, because TOML forbids repeating the key inside one table, and no basis for choosing + between them, so the region is refused rather than written (see below). + +Adoption never touches an array of tables (`[[bin]]`), which TOML permits to repeat, so a +second one is not a duplicate; nor a table inside an existing managed region, which the +tool already owns; nor anything in a host the parser cannot read at all. + +Whatever adoption concludes, the spliced result is parsed before it is planned, with every +*other* managed region masked out. Two managed regions may legitimately declare the same +key while a migration is in flight — the old combined region is removed in the same pass +that writes the sections replacing it — so only collisions with text nothing is going to +remove count. If the result would not parse, the region is **refused**: the host is left +alone, a diagnostic naming the host and the reason is recorded, and every other artifact is +still planned. Refusing is scoped to the region, not the run. ### User-extension limits for TOML regions diff --git a/crates/cargo-anvil/src/emit/managed_region.rs b/crates/cargo-anvil/src/emit/managed_region.rs index 6bde5fa71..5194cd949 100644 --- a/crates/cargo-anvil/src/emit/managed_region.rs +++ b/crates/cargo-anvil/src/emit/managed_region.rs @@ -16,13 +16,17 @@ //! [`crate::run`]'s `HostTextCache` and //! [`updates.md`](../../../docs/design/updates.md). -use ohno::AppError; +use ohno::{AppError, app_err}; +use toml_edit::DocumentMut; use crate::checksum::checksum_str; use crate::decision::{Decision, DecisionInputs, UpdateDecision, decide}; use crate::manifest::{Manifest, RegionKey}; use crate::plan::{PlanItem, Target}; -use crate::region::{CommentSyntax, RegionPlacement, adopt_unmanaged_toml_tables, find_region, upsert_region_with_placement}; +use crate::region::{ + CommentSyntax, RegionPlacement, TomlAdoption, adopt_unmanaged_toml_tables, find_region, insert_after_region, + mask_other_managed_regions, upsert_region_with_placement, +}; /// Inputs that identify and render one managed region. #[derive(Clone, Copy)] @@ -119,6 +123,57 @@ pub fn plan_managed_region(manifest: &Manifest, host_text: Option<&str>, request Ok(item) } +/// Why introducing `request`'s region into its TOML host would produce a file +/// TOML cannot read, if it would. +/// +/// This is the backstop for the whole class of failure behind issue #148: +/// splicing a region that declares a whole table beside a hand-written copy of +/// that table yields two identical headers, which TOML rejects outright — and +/// the generator had already rewritten the file and recorded the region by the +/// time anything noticed. Adoption resolves the cases it can model; this +/// catches whatever is left by asking the parser, rather than by enumerating +/// shapes. +/// +/// Only an **introduction** is checked. Once the region exists, +/// `upsert_region_with_placement` replaces it where it stands and cannot +/// introduce a duplicate header — and a host the repository has since broken by +/// hand is not anvil's to refuse. +/// +/// Returns `None` for a host that is not TOML, for a region that is already +/// present, and for a splice whose result parses. +#[must_use] +pub fn toml_introduction_refusal(host_text: Option<&str>, request: ManagedRegionRequest<'_>) -> Option { + let ManagedRegionRequest { + host_relpath, + region_id, + rendered_body, + syntax, + placement, + } = request; + if !is_toml_host(host_relpath) { + return None; + } + let base = host_text.unwrap_or(""); + // A malformed region is a separate diagnosis, raised by the planner. + if !matches!(find_region(base, region_id, syntax), Ok(None)) { + return None; + } + + match splice(host_relpath, host_text, region_id, rendered_body, syntax, placement) { + Err(error) => Some(error.to_string()), + Ok(spliced) => mask_other_managed_regions(&spliced, syntax, region_id) + .parse::() + .err() + .map(|error| format!("splicing the region would leave {host_relpath} unparsable as TOML: {error}")), + } +} + +fn is_toml_host(host_relpath: &str) -> bool { + std::path::Path::new(host_relpath) + .extension() + .is_some_and(|ext| ext.eq_ignore_ascii_case("toml")) +} + fn splice( host_relpath: &str, host_text: Option<&str>, @@ -135,17 +190,38 @@ fn splice( // `upsert_region_with_placement` replaces it in place and there is nothing // to adopt. let adopted; - let is_toml_host = std::path::Path::new(host_relpath) - .extension() - .is_some_and(|ext| ext.eq_ignore_ascii_case("toml")); - let base = if is_toml_host && find_region(base, region_id, syntax)?.is_none() { - adopted = adopt_unmanaged_toml_tables(base, rendered_body, syntax); - adopted.as_str() + let mut residue = String::new(); + let base = if is_toml_host(host_relpath) && find_region(base, region_id, syntax)?.is_none() { + match adopt_unmanaged_toml_tables(base, rendered_body, syntax) { + TomlAdoption::Unchanged => base, + TomlAdoption::Adopted { text, residue: kept } => { + residue = kept; + adopted = text; + adopted.as_str() + } + // Unreachable in the normal path: `run` refuses the host before it + // ever plans a conflicting region (see `toml_adoption_refusal`). + // Reported rather than written, because every output available here + // either repeats a key TOML forbids or discards configuration. + TomlAdoption::Conflict { + table, + key, + managed, + hand_written, + } => { + return Err(app_err!( + "{host_relpath} declares `{key}` in `[{table}]` as {hand_written}, but the managed \ + region '{region_id}' declares it as {managed}. Adopting the table would discard \ + one of them and keeping both would repeat the key, which TOML rejects." + )); + } + } } else { base }; - upsert_region_with_placement(base, region_id, rendered_body, syntax, placement) + let spliced = upsert_region_with_placement(base, region_id, rendered_body, syntax, placement)?; + insert_after_region(&spliced, region_id, &residue, syntax) } #[cfg(test)] @@ -159,6 +235,96 @@ mod tests { ManagedRegionRequest::at_end(host_relpath, region_id, rendered_body, SYN) } + /// Issue #148, end to end. A `deny.toml` whose `[advisories]` carries the + /// repository's own accepted advisory used to receive a second + /// `[advisories]` header — a file `cargo deny` cannot read, written to disk + /// and recorded in the manifest before anything noticed, because the + /// fixtures only ever asserted on fragments of its text. + #[test] + fn splicing_beside_a_hand_written_table_produces_parsable_toml() { + let host = "[advisories]\n# waiting on upstream\nignore = [\"RUSTSEC-9999-0001\"]\n"; + let body = "[advisories]\nyanked = \"deny\"\nunmaintained = \"all\"\n"; + + let item = plan_managed_region( + &Manifest::default(), + Some(host), + request("deny.toml", "anvil-deny-advisories", body), + ) + .unwrap(); + let spliced = item.spliced_host.as_deref().unwrap(); + + let document = spliced + .parse::() + .unwrap_or_else(|error| panic!("spliced deny.toml must parse: {error}\n---\n{spliced}\n---")); + assert_eq!(spliced.matches("[advisories]").count(), 1, "no duplicate header:\n{spliced}"); + // The kept entry has to stay an `[advisories]` setting: relocated under + // the wrong header it is a different setting that cargo-deny ignores. + assert_eq!( + document["advisories"]["ignore"].as_array().unwrap().len(), + 1, + "the accepted advisory is still an [advisories] entry:\n{spliced}" + ); + assert_eq!( + document["advisories"]["yanked"].as_str(), + Some("deny"), + "the managed keys are present" + ); + assert!( + spliced.contains("# waiting on upstream"), + "the user's reasoning travels with it:\n{spliced}" + ); + } + + /// A key both sides declare with different values has no safe output: TOML + /// forbids repeating it, and choosing either value discards a decision + /// somebody made. The run refuses the region and leaves the host alone. + #[test] + fn a_conflicting_key_is_refused_rather_than_written() { + let host = "[advisories]\nyanked = \"warn\"\n"; + let body = "[advisories]\nyanked = \"deny\"\n"; + + let reason = toml_introduction_refusal(Some(host), request("deny.toml", "anvil-deny-advisories", body)) + .expect("a disagreement over `yanked` must be refused"); + + assert!(reason.contains("yanked"), "the refusal names the key: {reason}"); + } + + /// The refusal is a backstop, not a gate. An ordinary introduction — and an + /// adoption that keeps residue — has to pass it, or onboarding stops for + /// every repository that ever hand-wrote one of these tables. + #[test] + fn an_adoptable_host_is_not_refused() { + let host = "[advisories]\nignore = [\"RUSTSEC-9999-0001\"]\n"; + let body = "[advisories]\nyanked = \"deny\"\n"; + + assert_eq!( + toml_introduction_refusal(Some(host), request("deny.toml", "anvil-deny-advisories", body)), + None + ); + assert_eq!( + toml_introduction_refusal(None, request("deny.toml", "anvil-deny-advisories", body)), + None + ); + assert_eq!( + toml_introduction_refusal(Some("recipe:\n"), request("Justfile", "r", "body\n")), + None + ); + } + + /// Once the region exists it is replaced where it stands, so it cannot + /// introduce a duplicate header — and a host the repository has since + /// broken by hand is not anvil's to refuse. Checking an update too would + /// turn every such file into a refusal of a region that is already there. + #[test] + fn an_existing_region_is_not_re_checked() { + let host = "# >>> anvil-managed: r\n[advisories]\nyanked = \"deny\"\n# <<< anvil-managed: r\n\n[advisories]\nignore = []\n"; + + assert_eq!( + toml_introduction_refusal(Some(host), request("deny.toml", "r", "[advisories]\nyanked = \"deny\"\n")), + None + ); + } + #[test] fn missing_host_writes_new_file() { let item = plan_managed_region(&Manifest::default(), None, request("Justfile", "r", "body line\n")).unwrap(); @@ -264,11 +430,11 @@ mod tests { } /// The limit on adoption, and the more important half of it: a hand-written - /// table carrying a key the managed body does not have is configuration, - /// not a duplicate. Dropping it would silently delete a user's settings — - /// a worse outcome than the duplicate table adoption exists to prevent. + /// entry the managed body does not declare is configuration, not a + /// duplicate. It is never deleted — it is kept as residue and re-emitted + /// inside the table the region opens, which is where it was written. #[test] - fn a_table_with_extra_user_keys_is_never_dropped() { + fn a_hand_written_entry_is_never_dropped() { let host = "[advisories]\nignore = [\"RUSTSEC-9999-0001\"]\n"; let item = plan_managed_region( &Manifest::default(), diff --git a/crates/cargo-anvil/src/emit/mod.rs b/crates/cargo-anvil/src/emit/mod.rs index c01817e90..db71f66f6 100644 --- a/crates/cargo-anvil/src/emit/mod.rs +++ b/crates/cargo-anvil/src/emit/mod.rs @@ -14,5 +14,5 @@ pub mod managed_region; pub mod owned_file; -pub use managed_region::{ManagedRegionRequest, plan_managed_region}; +pub use managed_region::{ManagedRegionRequest, plan_managed_region, toml_introduction_refusal}; pub use owned_file::plan_owned_file; diff --git a/crates/cargo-anvil/src/region.rs b/crates/cargo-anvil/src/region.rs index 7a1689558..c4dcbfd43 100644 --- a/crates/cargo-anvil/src/region.rs +++ b/crates/cargo-anvil/src/region.rs @@ -27,7 +27,7 @@ use std::collections::BTreeMap; use ohno::{AppError, app_err, bail}; -use toml_edit::{DocumentMut, Table}; +use toml_edit::{Item, RawString, Table}; /// Comment syntax used by the host file. /// @@ -359,290 +359,486 @@ fn iterate_lines(text: &str) -> LineIter<'_> { LineIter { text, pos: 0 } } -/// Drop an outside-region copy of a TOML table whose configuration the region -/// body already covers, so introducing the region adopts a hand-written table -/// instead of appending a duplicate that TOML will not parse. +/// Insert `extra` directly after region `id`'s closing sentinel. +/// +/// Used to re-emit the hand-written configuration that adoption kept (see +/// [`TomlAdoption::Adopted`]). The position matters: TOML attributes a key to +/// whichever table header precedes it, so text placed here belongs to the table +/// the region just opened, which is exactly the table it was written under. +/// +/// # Errors +/// +/// Returns an error if the region is missing or malformed. +pub fn insert_after_region(text: &str, id: &str, extra: &str, syntax: CommentSyntax) -> Result { + if extra.is_empty() { + return Ok(text.to_owned()); + } + let Some(region) = find_region(text, id, syntax)? else { + return Err(app_err!("region '{id}' is missing from the host it was just spliced into")); + }; + let at = region.end_line.end; + + let mut out = String::with_capacity(text.len() + extra.len() + 1); + out.push_str(&text[..at]); + if !text[..at].ends_with('\n') { + out.push('\n'); + } + out.push_str(extra); + if !extra.ends_with('\n') { + out.push('\n'); + } + let rest = &text[at..]; + // The gap that followed the region is preserved, but a residue block that + // already ends in a newline must not be run straight into the next line of + // the file: that would attach the following header's comment to it. + if !rest.is_empty() && !rest.starts_with('\n') { + out.push('\n'); + } + out.push_str(rest); + Ok(out) +} + +/// Adopt an outside-region copy of a TOML table whose configuration the region +/// body already covers, so introducing the region takes over a hand-written +/// table instead of appending a duplicate that TOML will not parse. /// /// A managed region body such as `[lints]\nworkspace = true` is a whole table, /// and TOML rejects a duplicate table header outright — so appending it beside /// a hand-written `[lints]` does not produce redundant text, it produces a /// manifest that will not parse and takes the workspace with it. /// -/// Coverage is **one-way**: a table is adopted when every one of its -/// configuration lines also appears in the managed table. The managed table -/// may declare further lines of its own and adoption still applies; it is -/// unmanaged-only configuration that prevents it. A hand-written table -/// carrying anything extra is left -/// exactly where it is: dropping it would silently delete a user's -/// configuration, which is a worse failure than the duplicate this function -/// exists to prevent — a `deny.toml` whose `[advisories]` lists the repository's own -/// `ignore` entries is the case that matters, and it is covered by a fixture. -/// Comments and blank lines are ignored when comparing, since neither carries -/// configuration, and both sides are compared through the TOML parser rather -/// than as source text — so formatting that TOML itself ignores, such as the -/// spacing in `workspace=true` or the order two entries appear in, cannot -/// defeat the comparison and leave the duplicate this function exists to -/// remove. An array-of-tables (`[[bin]]`) is never adopted at all: -/// TOML lets those repeat, so a second one is not a duplicate and dropping it -/// would delete a genuine array element. +/// Each hand-written entry is classified against the managed table: +/// +/// * declared by both, with the same value — **covered**, and dropped, since +/// the region re-emits it verbatim. +/// * declared only by hand — **residue**, which is kept: it is returned +/// separately so the caller can re-emit it after the region's closing +/// sentinel, where it continues the very table the region opens. That is +/// what lets a `deny.toml` whose `[advisories]` carries the repository's own +/// `ignore` list be adopted at all, rather than declining and leaving a +/// duplicate header behind. +/// * declared by both with **different** values — a [`TomlAdoption::Conflict`]. +/// Keeping both would repeat one key inside one table, and dropping either +/// would lose configuration somebody chose, so this reports rather than +/// guesses. +/// +/// Both sides are compared through the TOML parser rather than as source text, +/// so formatting that TOML itself ignores — the spacing in `workspace=true`, +/// the order two entries appear in — cannot defeat the comparison. Residue is +/// carried across as its original source slice, so a user's comments and +/// spacing survive byte-for-byte. /// -/// Text inside an existing managed region is never examined, so a region that -/// legitimately owns the same table elsewhere in the file is untouched, and a -/// host containing a multi-line string is left alone entirely — its content is -/// beyond what a line-oriented scanner can judge. +/// An array-of-tables (`[[bin]]`) is never adopted: TOML lets those repeat, so +/// a second one is not a duplicate and dropping it would delete a genuine array +/// element. Text inside an existing managed region is never examined, so a +/// region that legitimately owns the same table elsewhere in the file is +/// untouched. +/// +/// A host that does not parse is returned untouched: a table this cannot read +/// is one it must not delete. #[must_use] -pub fn adopt_unmanaged_toml_tables(text: &str, body: &str, syntax: CommentSyntax) -> String { - // A multi-line string is content this line-oriented scanner cannot read. - // Its quote state does not survive the line break, so a `#` inside one - // looks like a comment and a bracketed line inside one looks like a table - // header — either of which would corrupt the comparison and could delete - // a table that genuinely differs. Rather than guess, decline adoption - // outright: leaving a visible duplicate-table failure is the documented - // preference over silently losing user configuration. - if contains_multi_line_string(text) || contains_multi_line_string(body) { - return text.to_owned(); - } - - let managed = parsed_tables(body, syntax); +pub fn adopt_unmanaged_toml_tables(text: &str, body: &str, syntax: CommentSyntax) -> TomlAdoption { + let Some(managed) = headed_tables(body) else { + return TomlAdoption::Unchanged; + }; if managed.is_empty() { - return text.to_owned(); - } - - let open = syntax.prefix().to_owned() + " >>> anvil-managed:"; - let close = syntax.prefix().to_owned() + " <<< anvil-managed:"; + return TomlAdoption::Unchanged; + } + + // Parse the host with its managed regions blanked out. Two things fall out + // of that. The region's own tables are invisible, so a region that + // legitimately owns the same table elsewhere in the file is never a + // candidate; and a host that already carries both a region copy and a + // hand-written copy still parses, even though as written it is the very + // duplicate-header file TOML rejects — which is exactly the file adoption + // exists to repair. Masking preserves length, so every span still indexes + // the original text. + let masked = mask_managed_regions(text, syntax); + let Some(candidates) = headed_tables(&masked) else { + return TomlAdoption::Unchanged; + }; - // Which unmanaged tables are safe to drop, decided up front so the rewrite - // below is a single pass with no lookahead. - let mut adoptable: Vec> = Vec::new(); - for (path, values) in parsed_tables(text, syntax) { - if let Some(managed_values) = managed.iter().find(|(name, _)| *name == path).map(|(_, values)| values) - && values.iter().all(|(key, value)| managed_values.get(key) == Some(value)) - { - adoptable.push(path); - } - } - if adoptable.is_empty() { - return text.to_owned(); - } + let protected = managed_region_ranges(text, syntax); + // Every header in the document, in order, bounds the table above it: a + // table's content runs until the next one starts. A managed region's + // opening sentinel bounds it too, so an adopted table can never swallow the + // sentinel of the region that follows it. + let mut boundaries: Vec = candidates.iter().map(|table| table.header.start).collect(); + boundaries.extend(protected.iter().map(|range| range.start)); + boundaries.sort_unstable(); - let mut out = String::with_capacity(text.len()); - let mut in_managed = false; - // Set while skipping an adopted table's body; cleared by the next table - // header or by a managed region's opener, so only that table is dropped - // and what follows survives. - let mut dropping = false; + let mut deletions: Vec = Vec::new(); + let mut residue = String::new(); - for line in iterate_lines(text) { - let raw = &text[line.start..line.end]; - let trimmed = raw.trim(); - if trimmed.starts_with(&open) { - in_managed = true; - // An adopted table's body ends here: whatever a managed region - // holds is the region's, and whatever follows its closer is the - // user's. Leaving the skip set would swallow both. - dropping = false; + for candidate in &candidates { + if candidate.array_of_tables { + continue; } - - if !in_managed { - if let Some(header) = toml_table_header(trimmed) { - // An array-of-tables header is never adoptable, but it does end - // the table above it, so the skip stops here either way. - dropping = !is_array_of_tables(header) && table_path(header).is_some_and(|path| adoptable.contains(&path)); - } - if dropping { - continue; + let Some(managed_values) = managed + .iter() + .find(|table| !table.array_of_tables && table.path == candidate.path) + .map(|table| &table.values) + else { + continue; + }; + let end = boundary_after(&boundaries, candidate.header.start, text.len()); + + let mut kept = String::new(); + for entry in &candidate.entries { + match managed_values.get(&entry.path) { + Some(managed_value) if *managed_value == entry.value => {} + Some(managed_value) => { + return TomlAdoption::Conflict { + table: candidate.path.join("."), + key: entry.path.join("."), + managed: managed_value.clone(), + hand_written: entry.value.clone(), + }; + } + None => kept.push_str(&text[entry.span.start..entry.span.end.min(end)]), } } - out.push_str(raw); + deletions.push(ByteRange { + start: candidate.header.start, + end, + }); + residue.push_str(&kept); + } + + if deletions.is_empty() { + return TomlAdoption::Unchanged; + } - if trimmed.starts_with(&close) { - in_managed = false; - } + // The output is the gaps between the deletions, copied in order. There is + // no streaming state to get wrong: the ranges are non-overlapping by + // construction, because each one ends where the next header or sentinel + // begins. + deletions.sort_unstable_by_key(|range| range.start); + let mut out = String::with_capacity(text.len()); + let mut cursor = 0; + for range in &deletions { + debug_assert!(range.start >= cursor, "adoption deletion ranges must not overlap"); + out.push_str(&text[cursor..range.start]); + cursor = range.end; } + out.push_str(&text[cursor..]); - out + TomlAdoption::Adopted { + text: out, + residue: tidy_residue(&residue), + } } -/// Collect each top-level TOML table outside any managed region, as its header -/// and the configuration lines beneath it. Whole-line comments are skipped and -/// a trailing comment is stripped from every header and configuration line, -/// since a comment carries no configuration and must not defeat a comparison. -/// Blank lines need no such handling: they are carried through as empty lines -/// and the TOML parser that performs the comparison ignores them. -fn toml_tables(text: &str, syntax: CommentSyntax) -> Vec<(&str, Vec<&str>)> { - let prefix = syntax.prefix(); - let open = prefix.to_owned() + " >>> anvil-managed:"; - let close = prefix.to_owned() + " <<< anvil-managed:"; +/// Trim the blank lines that bounded the residue inside the table it came +/// from, leaving exactly one trailing newline when anything is left. +/// +/// Only the edges are touched: a blank line the user put *between* two of their +/// own keys is theirs, and survives. +fn tidy_residue(residue: &str) -> String { + let trimmed = trim_leading_blank_lines(residue).trim_end(); + if trimmed.is_empty() { + String::new() + } else { + let mut out = String::with_capacity(trimmed.len() + 1); + out.push_str(trimmed); + out.push('\n'); + out + } +} - let mut tables: Vec<(&str, Vec<&str>)> = Vec::new(); - let mut in_managed = false; +/// What examining a TOML host for hand-written copies of a region's tables +/// found. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TomlAdoption { + /// The host declares none of the body's tables outside a managed region — + /// or could not be parsed, and so must not be edited. + Unchanged, + /// `text` is the host with the adopted tables removed. `residue` is the + /// hand-written configuration the managed body does not declare, to be + /// re-emitted directly after the region's closing sentinel so that it stays + /// inside the table the region opens. + Adopted { text: String, residue: String }, + /// A hand-written entry and a managed entry declare the same key with + /// different values. There is no output that keeps both — TOML forbids the + /// repeated key — and no way to choose between them, so the caller must + /// refuse rather than write. + Conflict { + /// Dotted path of the table both declare. + table: String, + /// Dotted path of the key they disagree on. + key: String, + /// The value the managed region body declares. + managed: String, + /// The value the host declares by hand. + hand_written: String, + }, +} - for line in iterate_lines(text) { - let trimmed = text[line.start..line.end].trim(); - if trimmed.starts_with(&open) { - in_managed = true; - continue; - } - if trimmed.starts_with(&close) { - in_managed = false; - continue; - } - if in_managed || trimmed.starts_with(prefix) { - continue; - } - if let Some(header) = toml_table_header(trimmed) { - tables.push((header, Vec::new())); - } else if let Some((_, lines)) = tables.last_mut() { - lines.push(strip_trailing_comment(trimmed)); - } - } +/// One explicitly headed TOML table, with the byte range of its header and the +/// configuration it declares. +struct HeadedTable { + path: Vec, + header: ByteRange, + array_of_tables: bool, + values: TableValues, + entries: Vec, +} - tables +/// One key/value entry of a headed table, with the source slice that carries +/// it — including any comment lines attached above it and its trailing comment. +struct TableEntry { + path: Vec, + value: String, + span: ByteRange, } /// The configuration a TOML table declares, as canonical path/value pairs. type TableValues = BTreeMap, String>; -/// A TOML table's canonical path and the configuration it declares. -type ParsedTable = (Vec, TableValues); - -/// Each adoption candidate in `text`, as the canonical path its header names -/// and the configuration it declares. +/// Every explicitly headed table in `text`, in document order. /// -/// An array-of-tables, and any table whose lines the TOML parser rejects, is -/// dropped from the result rather than compared: a table this cannot read is -/// one it must not delete. -fn parsed_tables(text: &str, syntax: CommentSyntax) -> Vec { - toml_tables(text, syntax) - .into_iter() - .filter(|(header, _)| !is_array_of_tables(header)) - .filter_map(|(header, lines)| Some((table_path(header)?, table_values(&lines)?))) - .collect() +/// Returns `None` when `text` is not valid TOML. Parsing the document rather +/// than scanning for lines that look like headers is what lets a host +/// containing a multi-line string be adopted: a bracketed line inside a `"""` +/// value is a value to the parser, and cannot be mistaken for a header. +/// +/// The document is parsed immutably, because [`toml_edit::DocumentMut`] +/// discards the source spans this needs. +fn headed_tables(text: &str) -> Option> { + let document = toml_edit::Document::parse(text).ok()?; + let mut tables = Vec::new(); + collect_headed_tables(document.as_table(), &mut Vec::new(), text, &mut tables); + tables.sort_by_key(|table| table.header.start); + Some(tables) } -/// The canonical path a table header names: `[ workspace . lints ]` and -/// `[workspace.lints]` both yield `["workspace", "lints"]`. -/// -/// The path is kept as segments rather than rejoined into a string so that a -/// quoted key containing a dot cannot be mistaken for a nested path — `["a.b"]` -/// and `[a.b]` name different tables and must not compare equal. -fn table_path(header: &str) -> Option> { - let document = header.parse::().ok()?; - let mut path = Vec::new(); - let mut table = document.as_table(); - loop { - let mut entries = table.iter(); - let (key, item) = entries.next()?; - // A header names exactly one table at each level, so a second entry - // means this line is not the header it appeared to be. - entries.next().is_none().then_some(())?; +/// Walk a table's children, recording every explicitly headed table and +/// recursing through the implicit ones a nested header creates. +fn collect_headed_tables(table: &Table, path: &mut Vec, text: &str, out: &mut Vec) { + for (key, item) in table { path.push(key.to_owned()); - match item.as_table() { - Some(inner) if !inner.is_empty() => table = inner, - _ => break, + match item { + Item::Table(child) => { + // An implicit table was never written as a header of its own — + // `[a.b]` creates one for `a` — so it is not a candidate, but + // its children still are. + if !child.is_implicit() + && let Some(header) = child.span() + { + out.push(HeadedTable { + path: path.clone(), + header: ByteRange { + start: header.start, + end: header.end, + }, + array_of_tables: false, + values: table_values(child), + entries: table_entries(child, text), + }); + } + collect_headed_tables(child, path, text, out); + } + Item::ArrayOfTables(array) => { + for child in array { + if let Some(header) = child.span() { + out.push(HeadedTable { + path: path.clone(), + header: ByteRange { + start: header.start, + end: header.end, + }, + array_of_tables: true, + values: TableValues::new(), + entries: Vec::new(), + }); + } + } + } + Item::Value(_) | Item::None => {} } + path.pop(); } - Some(path) } -/// The configuration a table's lines declare, as canonical path/value pairs. +/// The configuration a table declares, as canonical path/value pairs. /// -/// Comparing what the parser produces rather than the source lines themselves -/// is what makes adoption insensitive to formatting TOML ignores. Returns -/// `None` when the lines do not parse, which declines adoption for that table. -fn table_values(lines: &[&str]) -> Option { - let document = lines.join("\n").parse::().ok()?; - let mut values = BTreeMap::new(); - collect_values(document.as_table(), &mut Vec::new(), &mut values).then_some(values) +/// Descends through dotted keys so `rust.unsafe_op_in_unsafe_fn` is one entry +/// rather than a nested table, and stops at a nested *headed* table, which is +/// a candidate in its own right rather than part of this one. +fn table_values(table: &Table) -> TableValues { + let mut values = TableValues::new(); + collect_values(table, &mut Vec::new(), &mut values); + values } -/// Flatten a table's entries into path/value pairs, descending through dotted -/// keys so `rust.unsafe_op_in_unsafe_fn` is one entry rather than a nested -/// table. -/// -/// Returns `false` for anything that is neither a value nor a nested table. -/// Neither can arise from a table body with its headers already removed, and -/// refusing is the safe answer for a shape this does not model. -fn collect_values(table: &Table, path: &mut Vec, values: &mut TableValues) -> bool { - table.iter().all(|(key, item)| { +fn collect_values(table: &Table, path: &mut Vec, values: &mut TableValues) { + for (key, item) in table { path.push(key.to_owned()); - let understood = match item.as_value() { - Some(value) => { + match item { + Item::Value(value) => { values.insert(path.clone(), value.to_string().trim().to_owned()); - true } - None => item.as_table().is_some_and(|inner| collect_values(inner, path, values)), - }; + Item::Table(child) if child.is_dotted() => collect_values(child, path, values), + _ => {} + } path.pop(); - understood - }) + } } -/// Return a trimmed TOML table header, without its trailing comment. +/// The top-level entries of a table, each with the source slice that carries +/// it. /// -/// This is a boundary test: an array-of-tables header (`[[bin]]`) counts, so -/// that the keys beneath it are not attributed to the table above it. Whether -/// such a header may be *adopted* is a separate question, decided by -/// [`is_array_of_tables`]. -/// -/// The bracket shape alone is not enough. A whole-line element of a multi-line -/// array (`[1, 2]`) wears it too, and reading one as a header would split the -/// table it belongs to and leave both halves unparsable. The candidate is -/// therefore handed to the TOML parser, which is the only thing that can tell -/// the two apart. -fn toml_table_header(line: &str) -> Option<&str> { - let header = strip_trailing_comment(line); - (header.starts_with('[') && header.ends_with(']') && header.parse::().is_ok()).then_some(header) +/// An entry's slice runs from the start of its own leading trivia — the blank +/// lines and comments the parser attached to its key — to the start of the next +/// entry's, so relocating it carries its comments along and leaves nothing of +/// the next entry behind. The last entry runs to the end of the table, which +/// the caller clamps to the next header. +fn table_entries(table: &Table, text: &str) -> Vec { + let mut starts: Vec<(Vec, String, usize)> = Vec::new(); + for (key, item) in table { + let Some((key, _)) = table.get_key_value(key) else { + continue; + }; + let start = key + .leaf_decor() + .prefix() + .and_then(RawString::span) + .map_or_else(|| key.span().map_or(0, |span| span.start), |span| span.start); + match item { + Item::Value(value) => starts.push((vec![key.get().to_owned()], value.to_string().trim().to_owned(), start)), + Item::Table(child) if child.is_dotted() => { + let mut nested = TableValues::new(); + collect_values(child, &mut vec![key.get().to_owned()], &mut nested); + for (path, value) in nested { + starts.push((path, value, start)); + } + } + _ => {} + } + } + starts.sort_by_key(|(_, _, start)| *start); + + let mut entries = Vec::with_capacity(starts.len()); + for index in 0..starts.len() { + let (path, value, start) = &starts[index]; + let end = starts.get(index + 1).map_or(text.len(), |(_, _, next)| *next); + entries.push(TableEntry { + path: path.clone(), + value: value.clone(), + span: ByteRange { start: *start, end }, + }); + } + entries } -/// Whether a table header declares an array of tables (`[[bin]]`). -/// -/// TOML allows these to repeat, so a second one is not a duplicate and there -/// is no parse failure for adoption to fix. Adopting one would let a later -/// array element be deleted as though it were a duplicate of the first. -fn is_array_of_tables(header: &str) -> bool { - header.starts_with("[[") +/// The first boundary strictly after `start`, or `fallback` when none follows. +fn boundary_after(boundaries: &[usize], start: usize, fallback: usize) -> usize { + boundaries.iter().copied().find(|boundary| *boundary > start).unwrap_or(fallback) } -/// Whether `text` contains a TOML multi-line string delimiter. +/// Replace every managed region's bytes with spaces, keeping the newlines and +/// therefore every byte offset in the file. /// -/// Both the basic (`"""`) and literal (`'''`) forms count. This is a coarse -/// test on purpose: it decides only whether the line-oriented scanner can -/// classify the content safely, and being wrong in the cautious direction -/// merely declines an adoption that would otherwise have been safe. -fn contains_multi_line_string(text: &str) -> bool { - text.contains("\"\"\"") || text.contains("'''") +/// The masked copy is what the adoption parser reads. Blanking rather than +/// deleting is what keeps the spans it reports usable against the original +/// text. +fn mask_managed_regions(text: &str, syntax: CommentSyntax) -> String { + mask_regions(text, &managed_region_ranges(text, syntax)) } -/// Return `line` without a trailing TOML comment, if it has one. +/// Blank every managed region except `keep`, so what remains is the region +/// under consideration plus the repository's own hand-written content. /// -/// A `#` inside a quoted string is data rather than a comment, so quoting is -/// tracked: truncating there would corrupt the value and could make two -/// genuinely different keys compare equal. -fn strip_trailing_comment(line: &str) -> &str { - let mut quote = None; - let mut escaped = false; - let comment = line.char_indices().find_map(|(index, character)| match (quote, character) { - (None, '#') => Some(index), - (None, '\'' | '"') => { - quote = Some(character); - None - } - (Some('"'), '\\') if !escaped => { - escaped = true; - None +/// This is the view a TOML validity check has to take. Two managed regions can +/// legitimately declare the same key while a migration is in flight — the old +/// combined region is removed in the same pass that writes the sections +/// replacing it — so judging the intermediate text as a whole would refuse a +/// migration that is about to become valid. What matters is whether the region +/// being introduced collides with text nothing is going to remove. +#[must_use] +pub fn mask_other_managed_regions(text: &str, syntax: CommentSyntax, keep: &str) -> String { + let ranges: Vec = managed_region_ranges_with_ids(text, syntax) + .into_iter() + .filter_map(|(id, range)| (id != keep).then_some(range)) + .collect(); + mask_regions(text, &ranges) +} + +fn mask_regions(text: &str, ranges: &[ByteRange]) -> String { + if ranges.is_empty() { + return text.to_owned(); + } + let mut masked = text.as_bytes().to_vec(); + for range in ranges { + for byte in &mut masked[range.start..range.end] { + if *byte != b'\n' && *byte != b'\r' { + *byte = b' '; + } } - (Some(active), character) if character == active && !escaped => { - quote = None; - None + } + // Every replaced byte became an ASCII space and every retained byte is + // unchanged, so the result is still valid UTF-8. + String::from_utf8(masked).unwrap_or_else(|_| text.to_owned()) +} + +/// Byte ranges of the managed regions in `text`, from opening sentinel line to +/// closing sentinel line inclusive. +fn managed_region_ranges(text: &str, syntax: CommentSyntax) -> Vec { + managed_region_ranges_with_ids(text, syntax) + .into_iter() + .map(|(_, range)| range) + .collect() +} + +/// As [`managed_region_ranges`], paired with each region's id. +fn managed_region_ranges_with_ids(text: &str, syntax: CommentSyntax) -> Vec<(String, ByteRange)> { + let open = syntax.prefix().to_owned() + " >>> anvil-managed:"; + let close = syntax.prefix().to_owned() + " <<< anvil-managed:"; + + let mut ranges = Vec::new(); + let mut start = None; + for line in iterate_lines(text) { + let trimmed = text[line.start..line.end].trim(); + if let Some(id) = trimmed.strip_prefix(&open) { + start = Some((id.trim().to_owned(), line.start)); + } else if trimmed.starts_with(&close) + && let Some((id, open_at)) = start.take() + { + ranges.push(( + id, + ByteRange { + start: open_at, + end: line.end, + }, + )); } - _ => { - escaped = false; - None + } + // An unterminated region still shields everything below it: its body is the + // region's, not the user's, and `find_region` rejects the file separately. + if let Some((id, open_at)) = start { + ranges.push(( + id, + ByteRange { + start: open_at, + end: text.len(), + }, + )); + } + ranges +} + +/// Drop leading blank lines, so relocated residue does not carry the gap that +/// separated it from the header it used to sit under. +fn trim_leading_blank_lines(text: &str) -> &str { + let mut rest = text; + loop { + let trimmed = rest.trim_start_matches([' ', '\t']); + match trimmed.strip_prefix('\n').or_else(|| trimmed.strip_prefix("\r\n")) { + Some(next) => rest = next, + None => return rest, } - }); - line[..comment.unwrap_or(line.len())].trim_end() + } } struct LineIter<'a> { @@ -680,22 +876,49 @@ mod tests { const SYN: CommentSyntax = CommentSyntax::Hash; + /// The host text adoption produces, for the cases that expect no residue. + /// + /// Asserting the residue is empty here rather than discarding it keeps + /// these tests honest: a change that started keeping hand-written entries + /// would otherwise pass unnoticed. + fn adopted_text(text: &str, body: &str) -> String { + match adopt_unmanaged_toml_tables(text, body, SYN) { + TomlAdoption::Unchanged => text.to_owned(), + TomlAdoption::Adopted { text, residue } => { + assert_eq!(residue, "", "unexpected residue kept from the hand-written table"); + text + } + TomlAdoption::Conflict { table, key, .. } => panic!("unexpected conflict on `{key}` in `[{table}]`"), + } + } + + /// The host text and the residue adoption kept, for the cases that expect + /// hand-written entries to survive. + fn adopted_with_residue(text: &str, body: &str) -> (String, String) { + match adopt_unmanaged_toml_tables(text, body, SYN) { + TomlAdoption::Adopted { text, residue } => (text, residue), + other => panic!("expected the table to be adopted, got {other:?}"), + } + } + #[test] fn missing_region_returns_none() { assert_eq!(find_region("user content\n", "anvil-x", SYN).unwrap(), None); } - /// A multi-line string is content this line-oriented scanner cannot read: - /// its lines are values, not keys, and the quote state does not survive - /// the line break. Rather than guess at their meaning, adoption declines - /// outright — leaving a visible duplicate-table failure is the documented - /// preference over silently deleting user configuration. + /// The line-oriented scanner could not read a multi-line string — its quote + /// state does not survive the line break — so adoption used to decline for + /// the whole host whenever one appeared anywhere in it, disabling the + /// feature rather than handling the case. The parser has no such trouble. #[test] - fn a_multi_line_string_declines_adoption_entirely() { + fn a_multi_line_string_no_longer_defeats_adoption() { let text = "[lints]\nworkspace = true\n\n[package]\ndescription = \"\"\"\nnote # not a comment\n\"\"\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text(text, "[lints]\nworkspace = true\n"); - assert_eq!(adopted, text, "nothing is adopted while a multi-line string is present:\n{adopted}"); + assert_eq!( + adopted, "[package]\ndescription = \"\"\"\nnote # not a comment\n\"\"\"\n", + "the adoptable table is taken and the string is left alone:\n{adopted}" + ); } /// A bracketed line *inside* a multi-line string is a value, not a table @@ -704,7 +927,7 @@ mod tests { #[test] fn a_bracketed_line_inside_a_multi_line_string_is_not_a_table_header() { let text = "[package]\ndescription = \"\"\"\n[lints]\nworkspace = true\n\"\"\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text(text, "[lints]\nworkspace = true\n"); assert_eq!(adopted, text, "the string's content is left intact:\n{adopted}"); } @@ -715,59 +938,84 @@ mod tests { #[test] fn an_ordinary_quoted_value_still_permits_adoption() { let text = "[advisories]\nyanked = \"deny\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[advisories]\nyanked = \"deny\"\n", SYN); + let adopted = adopted_text(text, "[advisories]\nyanked = \"deny\"\n"); assert_eq!(adopted, "", "the table is still adopted:\n{adopted}"); } - /// The quote tracking in `strip_trailing_comment` decides whether a `#` is - /// a comment or data. Getting it wrong in either direction is harmful: a - /// `#` treated as a comment truncates a value, which can make two - /// genuinely different keys compare equal and adopt -- delete -- a table - /// that differs; a comment treated as data leaves it attached and defeats - /// adoption, leaving the duplicate table this module exists to remove. - /// Each case below is a distinct piece of that state machine. - #[test] - fn strip_trailing_comment_tracks_quoting() { - // A plain trailing comment goes, with its leading whitespace. - assert_eq!(strip_trailing_comment("a = 1 # note"), "a = 1"); - // No comment at all: the line is returned whole. - assert_eq!(strip_trailing_comment("a = 1"), "a = 1"); - // A `#` inside a quoted value is data, under either quote style. - assert_eq!(strip_trailing_comment("a = \"x#y\""), "a = \"x#y\""); - assert_eq!(strip_trailing_comment("a = 'x#y'"), "a = 'x#y'"); - // A quote closes, so a comment after a quoted value is still a comment. - assert_eq!(strip_trailing_comment("a = \"x\" # note"), "a = \"x\""); - // Only the matching quote character closes: an apostrophe inside a - // double-quoted value must not end it and expose the `#`. - assert_eq!(strip_trailing_comment("a = \"it's #1\""), "a = \"it's #1\""); - // An escaped quote does not close the value either. - assert_eq!(strip_trailing_comment("a = \"x\\\"#y\""), "a = \"x\\\"#y\""); - // ...but an escaped backslash is not itself an escape, so the quote - // that follows it does close, and the comment after it is a comment. - assert_eq!(strip_trailing_comment("a = \"x\\\\\" # note"), "a = \"x\\\\\""); - } - - /// The consequence of that tracking, at the level that matters: two values - /// differing only inside a quoted `#` must not be judged equal. Were the - /// `#` treated as a comment, both would truncate to the same prefix and - /// the hand-written table would be dropped -- deleting a real setting. + /// A value differing only inside a quoted `#` is a genuine disagreement, + /// not a comment to be stripped. Treating the `#` as a comment would + /// truncate both values to the same prefix and adopt — that is, delete — + /// a table that declares something else. #[test] fn a_quoted_hash_keeps_two_differing_values_distinct() { - let text = "[advisories]\nignore = [\"a#b\"]\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[advisories]\nignore = [\"a#c\"]\n", SYN); + let adoption = adopt_unmanaged_toml_tables("[advisories]\nignore = [\"a#b\"]\n", "[advisories]\nignore = [\"a#c\"]\n", SYN); + + assert_eq!( + adoption, + TomlAdoption::Conflict { + table: "advisories".to_owned(), + key: "ignore".to_owned(), + managed: "[\"a#c\"]".to_owned(), + hand_written: "[\"a#b\"]".to_owned(), + }, + "the disagreement is reported rather than resolved" + ); + } + + /// The defect behind issue #148: a hand-written table carrying + /// configuration the managed body does not declare cannot simply be + /// deleted, and appending the region beside it produces two identical + /// headers, which TOML rejects. The hand-written entry is kept and handed + /// back for the caller to re-emit after the region, inside the table the + /// region opens. + #[test] + fn a_hand_written_entry_the_body_does_not_declare_is_kept_as_residue() { + let text = "[advisories]\nignore = [\"RUSTSEC-9999-0001\"]\n"; + let (adopted, residue) = adopted_with_residue(text, "[advisories]\nyanked = \"deny\"\n"); + + assert_eq!(adopted, "", "the hand-written header is adopted:\n{adopted}"); + assert_eq!(residue, "ignore = [\"RUSTSEC-9999-0001\"]\n", "the user's entry survives"); + } + + /// Residue is carried across as its original source slice, so the comments + /// a user wrote to explain a setting travel with the setting. Rebuilding it + /// from parsed values would silently discard the reasoning and leave a bare + /// key behind. + #[test] + fn residue_keeps_the_comments_written_around_it() { + let text = "[advisories]\nyanked = \"deny\"\n# waiting on upstream\nignore = [\"RUSTSEC-9999-0001\"] # ours\n"; + let (_, residue) = adopted_with_residue(text, "[advisories]\nyanked = \"deny\"\n"); - assert_eq!(adopted, text, "the differing table is preserved:\n{adopted}"); + assert_eq!( + residue, "# waiting on upstream\nignore = [\"RUSTSEC-9999-0001\"] # ours\n", + "both the leading and the trailing comment travel with the entry" + ); + } + + /// Residue stops at the table it came from. An entry belonging to a later + /// table must not be dragged along, or it would silently change meaning: + /// relocated under the region's header it becomes a setting of a different + /// table entirely. + #[test] + fn residue_does_not_reach_past_the_adopted_table() { + let text = "[advisories]\nignore = [\"X\"]\n\n[bans]\nmultiple-versions = \"warn\"\n"; + let (adopted, residue) = adopted_with_residue(text, "[advisories]\nyanked = \"deny\"\n"); + + assert_eq!(residue, "ignore = [\"X\"]\n", "only the adopted table's entry is taken"); + assert_eq!( + adopted, "[bans]\nmultiple-versions = \"warn\"\n", + "the following table is left where it is:\n{adopted}" + ); } - /// The case that motivated moving the comparison onto the TOML parser: a /// hand-written `workspace=true` declares exactly what the rendered /// `workspace = true` does, and TOML does not care about the spacing. A /// source-text comparison judged them different, declined adoption, and /// appended the duplicate header this module exists to remove. #[test] fn spacing_around_the_assignment_does_not_defeat_adoption() { - let adopted = adopt_unmanaged_toml_tables("[lints]\nworkspace=true\n", "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text("[lints]\nworkspace=true\n", "[lints]\nworkspace = true\n"); assert_eq!(adopted, "", "the table is adopted despite the spacing:\n{adopted}"); } @@ -778,7 +1026,7 @@ mod tests { #[test] fn entry_order_does_not_defeat_adoption() { let text = "[advisories]\nyanked = \"deny\"\nunmaintained = \"warn\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[advisories]\nunmaintained = \"warn\"\nyanked = \"deny\"\n", SYN); + let adopted = adopted_text(text, "[advisories]\nunmaintained = \"warn\"\nyanked = \"deny\"\n"); assert_eq!(adopted, "", "the table is adopted despite the order:\n{adopted}"); } @@ -790,20 +1038,23 @@ mod tests { #[test] fn a_quoted_dotted_key_is_not_a_nested_path() { let text = "[\"a.b\"]\nx = 1\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[a.b]\nx = 1\n", SYN); + let adopted = adopted_text(text, "[a.b]\nx = 1\n"); assert_eq!(adopted, text, "the differently-named table is preserved:\n{adopted}"); } - /// Comparing canonical paths rather than raw header text makes `[bin]` and - /// `[[bin]]` share a path, so the array-of-tables exclusion has to hold in - /// the rewrite as well as in candidate selection. Were it dropped, the - /// array element would be deleted along with the adopted table. + /// `[bin]` beside `[[bin]]` is not a file TOML accepts at all — the second + /// header is a duplicate key — so the parser cannot read it and adoption + /// declines. That is the safe answer: the array element is never deleted, + /// which is what the exclusion exists to guarantee. The line scanner this + /// replaced did read such a file, and had to carry the array-of-tables + /// exclusion into the rewrite to avoid deleting the element. #[test] - fn an_array_of_tables_survives_adoption_of_a_table_sharing_its_name() { - let adopted = adopt_unmanaged_toml_tables("[bin]\nname = \"x\"\n\n[[bin]]\nname = \"x\"\n", "[bin]\nname = \"x\"\n", SYN); + fn an_array_of_tables_survives_a_table_sharing_its_name() { + let text = "[bin]\nname = \"x\"\n\n[[bin]]\nname = \"x\"\n"; + let adopted = adopted_text(text, "[bin]\nname = \"x\"\n"); - assert_eq!(adopted, "[[bin]]\nname = \"x\"\n", "the array element survives:\n{adopted}"); + assert_eq!(adopted, text, "the array element survives:\n{adopted}"); } /// A trailing comment on a key line carries no configuration, so it must @@ -814,7 +1065,7 @@ mod tests { #[test] fn a_key_line_with_a_trailing_comment_is_still_adoptable() { let text = "[lints]\nworkspace = true # our policy\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text(text, "[lints]\nworkspace = true\n"); assert_eq!(adopted, "", "the hand-written table is adopted whole:\n{adopted}"); } @@ -824,10 +1075,16 @@ mod tests { /// equal, adopting -- and therefore deleting -- a table that differs. #[test] fn a_hash_inside_a_quoted_value_is_not_treated_as_a_comment() { - let text = "[advisories]\nignore = [\"RUSTSEC-1#1\"]\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[advisories]\nignore = [\"RUSTSEC-2#2\"]\n", SYN); + let adoption = adopt_unmanaged_toml_tables( + "[advisories]\nignore = [\"RUSTSEC-1#1\"]\n", + "[advisories]\nignore = [\"RUSTSEC-2#2\"]\n", + SYN, + ); - assert_eq!(adopted, text, "differing quoted values are not adoptable:\n{adopted}"); + assert!( + matches!(adoption, TomlAdoption::Conflict { ref key, .. } if key == "ignore"), + "differing quoted values are a conflict, not a match: {adoption:?}" + ); } /// TOML allows an array-of-tables header to repeat, so a second `[[bin]]` @@ -836,7 +1093,7 @@ mod tests { #[test] fn an_array_of_tables_is_never_adopted() { let text = "[[bin]]\nname = \"a\"\n\n[[bin]]\nname = \"b\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[[bin]]\nname = \"a\"\n", SYN); + let adopted = adopted_text(text, "[[bin]]\nname = \"a\"\n"); assert_eq!(adopted, text, "every array element survives:\n{adopted}"); } @@ -848,7 +1105,7 @@ mod tests { #[test] fn an_array_of_tables_bounds_the_table_above_it() { let text = "[lints]\nworkspace = true\n\n[[bin]]\nname = \"a\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text(text, "[lints]\nworkspace = true\n"); assert_eq!( adopted, "[[bin]]\nname = \"a\"\n", @@ -867,7 +1124,7 @@ mod tests { other = true\n\ # <<< anvil-managed: other\n\ # a user comment\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text(text, "[lints]\nworkspace = true\n"); assert!( adopted.contains("# a user comment"), @@ -883,7 +1140,7 @@ mod tests { #[test] fn an_array_element_on_its_own_line_is_not_a_table_header() { let text = "[lints]\nworkspace = true\npairs = [\n [1, 2]\n]\n"; - let adopted = adopt_unmanaged_toml_tables(text, text, SYN); + let adopted = adopted_text(text, text); assert_eq!(adopted, "", "the table is adopted whole:\n{adopted}"); } @@ -895,7 +1152,7 @@ mod tests { [lints]\n\ workspace = true\n\ # <<< anvil-managed: existing\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let adopted = adopted_text(text, "[lints]\nworkspace = true\n"); assert!(adopted.starts_with("# >>> anvil-managed: existing")); assert!(adopted.contains("[lints]\nworkspace = true\n# <<< anvil-managed: existing")); @@ -913,9 +1170,16 @@ mod tests { [lints]\n\ workspace = true\n\ # <<< anvil-managed: existing\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[lints]\nworkspace = true\n", SYN); + let (adopted, residue) = adopted_with_residue(text, "[lints]\nworkspace = true\n"); - assert_eq!(adopted, text, "the unmanaged-only key is not deleted:\n{adopted}"); + assert_eq!( + residue, "rust.unsafe_code = \"forbid\"\n", + "the unmanaged-only key is kept, not deleted" + ); + assert!( + adopted.contains("# >>> anvil-managed: existing"), + "the existing region survives:\n{adopted}" + ); } /// Two tables under one parent are different tables. Comparing only the @@ -925,7 +1189,7 @@ mod tests { #[test] fn a_dotted_header_is_compared_past_its_first_segment() { let text = "[workspace.package]\nedition = \"2024\"\n"; - let adopted = adopt_unmanaged_toml_tables(text, "[workspace.lints]\nedition = \"2024\"\n", SYN); + let adopted = adopted_text(text, "[workspace.lints]\nedition = \"2024\"\n"); assert_eq!(adopted, text, "the differently-named table is preserved:\n{adopted}"); } diff --git a/crates/cargo-anvil/src/run.rs b/crates/cargo-anvil/src/run.rs index 7a086bb16..ca0e3213e 100644 --- a/crates/cargo-anvil/src/run.rs +++ b/crates/cargo-anvil/src/run.rs @@ -20,7 +20,7 @@ use crate::catalog::artifact::{Artifact, ComposedHost, HostSelector, RegionSpec} use crate::checksum::{checksum_str, normalize_line_endings}; use crate::cli::Cli; use crate::decision::{Decision, RemovalDecision, decide_removal}; -use crate::emit::{ManagedRegionRequest, plan_managed_region, plan_owned_file}; +use crate::emit::{ManagedRegionRequest, plan_managed_region, plan_owned_file, toml_introduction_refusal}; use crate::io::{read_file_if_present, resolve_existing_case_insensitive}; use crate::manifest::Manifest; use crate::plan::{Plan, PlanItem, Target}; @@ -461,17 +461,23 @@ fn push_region_at( return Ok(()); } }; - let item = plan_managed_region( - manifest, - current.as_deref(), - ManagedRegionRequest { - host_relpath: &host, - region_id: spec.id.as_str(), - rendered_body: body, - syntax: spec.syntax, - placement, - }, - )?; + let request = ManagedRegionRequest { + host_relpath: &host, + region_id: spec.id.as_str(), + rendered_body: body, + syntax: spec.syntax, + placement, + }; + // Introducing a region into a TOML host that already declares the same + // table by hand can produce a file TOML cannot read. Refuse the region + // rather than write it: `cargo deny` and `cargo` itself fail on the whole + // file, so a silent rewrite breaks the repository the generator was + // onboarding, and the manifest would record a region nothing can use. + if let Some(reason) = toml_introduction_refusal(current.as_deref(), request) { + refuse_region(plan, host, spec.id.as_str(), &reason); + return Ok(()); + } + let item = plan_managed_region(manifest, current.as_deref(), request)?; // Only a `Write` mutates the live host on disk; fold its spliced // output back into the accumulator so sibling regions compose. A // `Propose` writes a sibling, not the host, so it must not advance the @@ -485,6 +491,21 @@ fn push_region_at( Ok(()) } +/// Record that one region was refused: a diagnostic naming the host, and a +/// no-op so the plan still accounts for it. +/// +/// The refusal is scoped to the region, not the run — every other artifact is +/// still planned, which is what makes refusing an acceptable answer rather than +/// a wall in front of onboarding. +fn refuse_region(plan: &mut Plan, host: String, id: &str, reason: &str) { + plan.refusal(format!( + "Refused to manage {host} [{id}]: {reason}. Nothing was written to it, and other \ + artifacts were still planned. Reconcile the hand-written table with the managed \ + one -- or empty the region to opt out of it." + )); + plan.push(PlanItem::noop(Target::Region { host, id: id.to_owned() }, Decision::LeaveAlone)); +} + /// Where a region belongs inside a composed host whose order is semantic. /// /// An existing region is updated where it is found, so this only decides where diff --git a/crates/cargo-anvil/tests/fixtures.rs b/crates/cargo-anvil/tests/fixtures.rs index 088c56aed..19fd0a03e 100644 --- a/crates/cargo-anvil/tests/fixtures.rs +++ b/crates/cargo-anvil/tests/fixtures.rs @@ -85,6 +85,21 @@ fn region_decision(outcome: &RunOutcome, host: &str, id: &str) -> Decision { .decision } +/// Read a TOML host anvil wrote and assert it **parses**. +/// +/// Substring assertions are what let a broken host survive: a `deny.toml` +/// carrying two `[advisories]` headers contains every string these fixtures +/// look for and still fails the first `cargo deny` that reads it. Anything +/// anvil writes to a `.toml` host has to be a file TOML accepts. +fn read_parsing_toml(tmp: &TempDir, relpath: &str) -> String { + let path = tmp.path().join(relpath); + let text = std::fs::read_to_string(&path).unwrap(); + if let Err(error) = text.parse::() { + panic!("{relpath} is not valid TOML: {error}\n---\n{text}\n---"); + } + text +} + /// `single-crate`: a manifest with a bare `[package]` and no /// `[workspace]` should still get the per-crate lints region (not the /// workspace one), the Justfile imports region, and the full @@ -175,6 +190,54 @@ fn user_edit_inside_region_is_left_alone() { ); } +/// `deny-conflict`: a `deny.toml` whose hand-written `[advisories]` sets +/// `yanked` to something other than the managed body's value. No output keeps +/// both — TOML forbids the repeated key — so the region is refused, the host is +/// left byte-for-byte alone, and the rest of the onboarding still happens. +#[test] +fn a_conflicting_toml_host_is_refused_not_corrupted() { + let tmp = stage_fixture("deny-conflict"); + + let outcome = run(&tmp); + + let after = read_parsing_toml(&tmp, "deny.toml"); + assert!( + after.contains("yanked = \"warn\""), + "the repository's own value is never overwritten;\ngot:\n{after}" + ); + assert!( + !after.contains("anvil-deny-advisories"), + "the conflicting region is not spliced in;\ngot:\n{after}" + ); + assert_eq!( + after.matches("[advisories]").count(), + 1, + "and no duplicate header is produced;\ngot:\n{after}" + ); + assert_eq!( + region_decision(&outcome, "deny.toml", "anvil-deny-advisories"), + Decision::LeaveAlone, + "the conflicting region is planned as a no-op" + ); + assert!( + outcome.plan.refusals().iter().any(|reason| reason.contains("yanked")), + "the refusal names the key that disagrees; got: {:#?}", + outcome.plan.refusals() + ); + + // The refusal is scoped to the region it applies to. The rest of the host, + // and the rest of the onboarding, still happens -- which is what makes + // refusing tolerable rather than a wall. + assert!( + after.contains("anvil-deny-licenses"), + "the non-conflicting sections are still written;\ngot:\n{after}" + ); + assert!( + tmp.path().join("justfiles/anvil/mod.just").is_file(), + "other artifacts are still written" + ); +} + /// `migration`: a workspace that already has a hand-written /// `Justfile`, a `[workspace.lints]` block, and a `deny.toml` should /// get anvil's regions spliced in without losing any user content. @@ -193,7 +256,7 @@ fn migration_preserves_user_content() { "anvil imports region must be spliced into the existing Justfile" ); - let cargo = std::fs::read_to_string(tmp.path().join("Cargo.toml")).unwrap(); + let cargo = read_parsing_toml(&tmp, "Cargo.toml"); assert!( cargo.contains("lto = \"thin\""), "user-authored [profile.release] must survive migration; got:\n{cargo}" @@ -203,12 +266,32 @@ fn migration_preserves_user_content() { "anvil workspace lints region must be spliced into Cargo.toml" ); - let deny = std::fs::read_to_string(tmp.path().join("deny.toml")).unwrap(); + // The defect this fixture used to hide: the hand-written `[advisories]` + // declares an `ignore` list the managed body does not, so adoption cannot + // simply delete the table. Appending the region regardless produced a + // second `[advisories]` header, which TOML rejects outright -- and every + // assertion below still passed, because they only ever looked for a + // substring of its text. + let deny = read_parsing_toml(&tmp, "deny.toml"); assert!( deny.contains("RUSTSEC-9999-0001"), "user-authored deny.toml content must survive migration; got:\n{deny}" ); assert!(deny.contains("anvil-deny"), "anvil deny region must be spliced into deny.toml"); + assert_eq!( + deny.matches("[advisories]").count(), + 1, + "the hand-written table must be adopted, not duplicated; got:\n{deny}" + ); + // Kept configuration has to land *inside* the table the region opens, or + // it silently changes meaning -- a relocated `ignore` that ends up under + // `[bans]` is a different setting that cargo-deny will not honor. + let advisories = deny.parse::().unwrap(); + assert_eq!( + advisories["advisories"]["ignore"].as_array().unwrap().len(), + 1, + "the user's accepted advisory must still be an [advisories] entry; got:\n{deny}" + ); // Idempotence: re-run leaves everything alone. let outcome2 = run(&tmp); diff --git a/crates/cargo-anvil/tests/fixtures/deny-conflict/Cargo.toml b/crates/cargo-anvil/tests/fixtures/deny-conflict/Cargo.toml new file mode 100644 index 000000000..ad126206b --- /dev/null +++ b/crates/cargo-anvil/tests/fixtures/deny-conflict/Cargo.toml @@ -0,0 +1,11 @@ +[workspace] +resolver = "2" +members = ["crates/*"] + +[workspace.package] +edition = "2024" + +# Pre-existing user customization that anvil must not touch. +[profile.release] +lto = "thin" +codegen-units = 1 diff --git a/crates/cargo-anvil/tests/fixtures/deny-conflict/Justfile b/crates/cargo-anvil/tests/fixtures/deny-conflict/Justfile new file mode 100644 index 000000000..b2b1e92eb --- /dev/null +++ b/crates/cargo-anvil/tests/fixtures/deny-conflict/Justfile @@ -0,0 +1,8 @@ +# Pre-existing user Justfile. Ox-check should splice its imports +# region without touching these recipes. + +default: + @echo "user default recipe" + +my-custom-recipe: + @echo "user content preserved" diff --git a/crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/Cargo.toml b/crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/Cargo.toml new file mode 100644 index 000000000..42c9fd533 --- /dev/null +++ b/crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "alpha" +version = "0.1.0" +edition = "2024" + +[lints] +workspace = true diff --git a/crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/src/lib.rs b/crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/src/lib.rs new file mode 100644 index 000000000..22bde8d06 --- /dev/null +++ b/crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/src/lib.rs @@ -0,0 +1 @@ +// stub diff --git a/crates/cargo-anvil/tests/fixtures/deny-conflict/deny.toml b/crates/cargo-anvil/tests/fixtures/deny-conflict/deny.toml new file mode 100644 index 000000000..b5834e024 --- /dev/null +++ b/crates/cargo-anvil/tests/fixtures/deny-conflict/deny.toml @@ -0,0 +1,7 @@ +# Pre-existing user deny.toml that disagrees with the managed body: it sets +# `yanked` to something other than what anvil declares. There is no output that +# keeps both -- TOML forbids repeating the key inside one table -- so anvil must +# refuse the region rather than pick a winner. + +[advisories] +yanked = "warn" From 1d0948d3b6c06b3681e19098cd6fc02f2f746170 Mon Sep 17 00:00:00 2001 From: Vaiz <4908982+Vaiz@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:50:55 +0100 Subject: [PATCH 2/3] fix(cargo-anvil): make region masking infallible and cover the TOML residue paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Masking a managed region built a byte vector and converted it back with a fallible `String::from_utf8`, whose error arm silently returned the text unmasked. That arm can only be reached if the invariant it guards has already broken, and returning unmasked text there would hand the adoption parser the region's own tables as though a human had written them. The copy is now assembled as a `String`, so it is valid UTF-8 by construction and there is no error arm to get wrong; every masked byte is still a single ASCII space and the line breaks are still kept, so every byte offset and span is unchanged. Recovering a `Key` from a key that table iteration just yielded is likewise infallible, and the `continue` that guarded it would have silently dropped one of the user's entries — the exact failure this module exists to prevent. It now says so with an `expect`. Adds tests for the residue and masking paths that had no coverage: a residue insertion whose region is missing, the newlines supplied when the host or the residue lacks one, the blank line kept between relocated residue and what followed the region, masking of an unterminated region, and the stripping of leading blank lines from a relocated entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/cargo-anvil/src/emit/managed_region.rs | 2 +- crates/cargo-anvil/src/region.rs | 100 ++++++++++++++++-- 2 files changed, 90 insertions(+), 12 deletions(-) diff --git a/crates/cargo-anvil/src/emit/managed_region.rs b/crates/cargo-anvil/src/emit/managed_region.rs index 5194cd949..5e7808857 100644 --- a/crates/cargo-anvil/src/emit/managed_region.rs +++ b/crates/cargo-anvil/src/emit/managed_region.rs @@ -200,7 +200,7 @@ fn splice( adopted.as_str() } // Unreachable in the normal path: `run` refuses the host before it - // ever plans a conflicting region (see `toml_adoption_refusal`). + // ever plans a conflicting region (see `toml_introduction_refusal`). // Reported rather than written, because every output available here // either repeats a key TOML forbids or discards configuration. TomlAdoption::Conflict { diff --git a/crates/cargo-anvil/src/region.rs b/crates/cargo-anvil/src/region.rs index c4dcbfd43..bfc23e923 100644 --- a/crates/cargo-anvil/src/region.rs +++ b/crates/cargo-anvil/src/region.rs @@ -697,9 +697,12 @@ fn collect_values(table: &Table, path: &mut Vec, values: &mut TableValue fn table_entries(table: &Table, text: &str) -> Vec { let mut starts: Vec<(Vec, String, usize)> = Vec::new(); for (key, item) in table { - let Some((key, _)) = table.get_key_value(key) else { - continue; - }; + // Iteration hands back the key as a `&str`, dropping the `Key` that + // carries the decor and span this needs. Looking it straight back up is + // infallible — the key came from this very table — and skipping an + // entry that failed the lookup would silently drop the user's + // configuration, which is the whole failure this module exists to stop. + let (key, _) = table.get_key_value(key).expect("a key yielded by a table is present in it"); let start = key .leaf_decor() .prefix() @@ -769,17 +772,22 @@ fn mask_regions(text: &str, ranges: &[ByteRange]) -> String { if ranges.is_empty() { return text.to_owned(); } - let mut masked = text.as_bytes().to_vec(); + // Copied through as text rather than mutated as bytes, so the result is + // valid UTF-8 by construction and no fallible conversion is needed. Every + // masked byte becomes a one-byte space and the line breaks are kept, so the + // copy has the same length as the original and every offset still lands on + // the same character. + let mut masked = String::with_capacity(text.len()); + let mut cursor = 0; for range in ranges { - for byte in &mut masked[range.start..range.end] { - if *byte != b'\n' && *byte != b'\r' { - *byte = b' '; - } + masked.push_str(&text[cursor..range.start]); + for byte in text[range.start..range.end].bytes() { + masked.push(if byte == b'\n' || byte == b'\r' { char::from(byte) } else { ' ' }); } + cursor = range.end; } - // Every replaced byte became an ASCII space and every retained byte is - // unchanged, so the result is still valid UTF-8. - String::from_utf8(masked).unwrap_or_else(|_| text.to_owned()) + masked.push_str(&text[cursor..]); + masked } /// Byte ranges of the managed regions in `text`, from opening sentinel line to @@ -1528,4 +1536,74 @@ mod tests { let region = find_region(text, "x", SYN).unwrap().unwrap(); assert_eq!(region.body_str(), "body\n"); } + + /// Residue is inserted after the region the same pass spliced it in, so a + /// region that is not there means the splice did not do what it reported. + /// Reporting that is what keeps the user's configuration from being + /// dropped in silence. + #[test] + fn residue_insertion_reports_a_region_the_splice_did_not_leave_behind() { + let err = insert_after_region("user content\n", "x", "ignore = []\n", SYN).unwrap_err(); + + assert!(err.to_string().contains("region 'x' is missing"), "the region is named:\n{err}"); + } + + /// A host whose last line is the closing sentinel, and a residue block + /// written without one, both lack the newline the next line needs. Without + /// them the residue would be appended to the sentinel and to whatever + /// follows it, turning two lines into one. + #[test] + fn residue_insertion_supplies_the_newlines_the_host_and_the_residue_lack() { + let text = "# >>> anvil-managed: x\nyanked = \"deny\"\n# <<< anvil-managed: x"; + let out = insert_after_region(text, "x", "ignore = []", SYN).unwrap(); + + assert_eq!( + out, + "# >>> anvil-managed: x\nyanked = \"deny\"\n# <<< anvil-managed: x\nignore = []\n" + ); + } + + /// Residue that already ends in a newline still needs a blank line before + /// the content that followed the region. Run straight together, the next + /// line's leading comment would read as part of the relocated entry. + #[test] + fn residue_insertion_separates_the_residue_from_what_followed_the_region() { + let text = "# >>> anvil-managed: x\nyanked = \"deny\"\n# <<< anvil-managed: x\n[bans]\nmultiple-versions = \"warn\"\n"; + let out = insert_after_region(text, "x", "ignore = []\n", SYN).unwrap(); + + assert_eq!( + out, + "# >>> anvil-managed: x\nyanked = \"deny\"\n# <<< anvil-managed: x\nignore = []\n\n[bans]\nmultiple-versions = \"warn\"\n" + ); + } + + /// A region left unterminated still owns everything below it — that text is + /// the region's, not the user's. Masking only as far as a closing sentinel + /// that never arrives would expose the region's own tables to adoption as + /// though a human had written them. + #[test] + fn an_unterminated_region_is_masked_to_the_end_of_the_file() { + let text = "[advisories]\n# >>> anvil-managed: x\nyanked = \"deny\"\n"; + let masked = mask_managed_regions(text, SYN); + + assert_eq!(masked.len(), text.len(), "masking leaves every byte offset where it was"); + assert!( + masked.starts_with("[advisories]\n"), + "text above the region is untouched:\n{masked}" + ); + assert_eq!( + masked["[advisories]\n".len()..].trim(), + "", + "everything from the opening sentinel down is blanked:\n{masked}" + ); + } + + /// An entry's source slice starts at its leading trivia, so it carries the + /// blank lines that separated it from the header it used to sit under. + /// Kept, that gap would push the relocated entry away from the region whose + /// table now owns it. + #[test] + fn relocated_residue_loses_the_blank_lines_above_it() { + assert_eq!(trim_leading_blank_lines("\n \n\tignore = []\n"), "\tignore = []\n"); + } } From b7dedf2c886fc92d4c79f4228871a67f75f33576 Mon Sep 17 00:00:00 2001 From: Vaiz <4908982+Vaiz@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:25:03 +0100 Subject: [PATCH 3/3] test(cargo-anvil): pin the dotted-key and region-masking behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mutation gate reported five surviving mutants in `region.rs`, each of them a real hole in the tests rather than an artefact of the tool: * `collect_values` and `table_entries` descend into a child table only when it is dotted. Flipping that guard either way went unnoticed, so nothing pinned the distinction between `a.b = 1` — configuration belonging to the table being read — and `[a.b]`, which is a table of its own. Getting it wrong deletes a hand-written key or relocates a nested table out from under its own header. * `mask_other_managed_regions` could return an empty string, and the newline test inside `mask_regions` could be inverted to blank the line breaks as well, with no test noticing. Both destroy the property the masking exists for: a copy of the host whose every byte offset still lands where it did in the original. Four tests close those holes. No production code changes. --- crates/cargo-anvil/src/region.rs | 79 ++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/crates/cargo-anvil/src/region.rs b/crates/cargo-anvil/src/region.rs index bfc23e923..d2bed794c 100644 --- a/crates/cargo-anvil/src/region.rs +++ b/crates/cargo-anvil/src/region.rs @@ -1606,4 +1606,83 @@ mod tests { fn relocated_residue_loses_the_blank_lines_above_it() { assert_eq!(trim_leading_blank_lines("\n \n\tignore = []\n"), "\tignore = []\n"); } + + /// A dotted key is configuration like any other. Not descending into it + /// would hide a genuine disagreement, and the hand-written value would be + /// deleted in favour of the managed one instead of being reported. + #[test] + fn a_differing_dotted_key_is_reported_as_a_conflict() { + let adoption = adopt_unmanaged_toml_tables( + "[lints]\nrust.unsafe_code = \"forbid\"\n", + "[lints]\nrust.unsafe_code = \"allow\"\n", + SYN, + ); + + assert_eq!( + adoption, + TomlAdoption::Conflict { + table: "lints".to_owned(), + key: "rust.unsafe_code".to_owned(), + managed: "\"allow\"".to_owned(), + hand_written: "\"forbid\"".to_owned(), + }, + "the dotted key is compared rather than skipped" + ); + } + + /// `[workspace.package]` declares table `package`, not a key of + /// `[workspace]`. Folding its values into the parent's would make the + /// managed table look as though it already declared `package.edition`, and + /// the hand-written copy of that key would be deleted instead of kept. + #[test] + fn a_nested_headed_table_is_not_part_of_the_table_that_declares_it() { + let text = "[workspace]\nmembers = []\npackage.edition = \"2024\"\n"; + let body = "[workspace]\nmembers = []\n\n[workspace.package]\nedition = \"2024\"\n"; + let (adopted, residue) = adopted_with_residue(text, body); + + assert_eq!(residue, "package.edition = \"2024\"\n", "the hand-written dotted key is kept"); + assert_eq!(adopted, "", "the hand-written table is adopted:\n{adopted}"); + } + + /// The same distinction seen from the host: a nested headed table is not an + /// entry of the table above it. Counted as one, it would be relocated out + /// from under its own header and become a setting of a different table. + #[test] + fn a_nested_headed_table_is_not_an_entry_of_the_table_above_it() { + let text = "[workspace]\nmembers = []\n\n[workspace.package]\nedition = \"2024\"\n"; + let adopted = adopted_text(text, "[workspace]\nmembers = []\n"); + + assert_eq!( + adopted, "[workspace.package]\nedition = \"2024\"\n", + "the nested table stays where it was written:\n{adopted}" + ); + } + + /// The refusal check masks every managed region except the one being + /// introduced, which has to stay readable for the check to judge it. The + /// blanking keeps every line break, so the parser reports the same spans + /// against the copy as against the original. + #[test] + fn masking_keeps_the_named_region_and_every_line_break() { + let text = "[advisories]\n\ + # >>> anvil-managed: a\n\ + yanked = \"deny\"\n\ + # <<< anvil-managed: a\n\ + # >>> anvil-managed: b\n\ + unmaintained = \"warn\"\n\ + # <<< anvil-managed: b\n"; + let masked = mask_other_managed_regions(text, SYN, "b"); + + assert_eq!(masked.len(), text.len(), "every byte offset is where it was"); + assert_eq!( + masked.matches('\n').count(), + text.matches('\n').count(), + "the line breaks survive the blanking:\n{masked}" + ); + assert!( + masked.contains("unmaintained = \"warn\""), + "the region under consideration is left readable:\n{masked}" + ); + assert!(!masked.contains("yanked = \"deny\""), "the other region is blanked:\n{masked}"); + } }