diff --git a/crates/cargo-anvil/docs/design/updates.md b/crates/cargo-anvil/docs/design/updates.md index 89a40f98e..7c27f0f9a 100644 --- a/crates/cargo-anvil/docs/design/updates.md +++ b/crates/cargo-anvil/docs/design/updates.md @@ -202,8 +202,7 @@ The sentinel pair serves two purposes: surrounding content. 2. **Body delimitation.** Everything between the sentinels (exclusive) is "the region's body." Lines outside the sentinels are user-owned and preserved verbatim, with the - single exception described in *Adopting a hand-written table on first introduction* - below. + single exception described in *Adopting a hand-written table* below. For TOML hosts the sentinels are TOML line comments around the affected content. To work with TOML's no-duplicate-table rule, anvil writes a single parent-table header @@ -225,7 +224,13 @@ clippy.pedantic = "warn" rust.missing_docs = "warn" ``` -Whitespace and comments inside the region are preserved verbatim by the rewrite. +Inserted or rewritten regions use the host's first line ending (CRLF or LF) for +their body, sentinels, and new separator lines. A host without any line breaks, +including a new or empty file, defaults to LF. For mixed-ending hosts, the first +line ending decides the generated style; existing user content is not normalized. +The style is captured before TOML adoption, even if adoption removes the whole host. +Other template whitespace and comments are preserved. Already-in-sync regions are +not rewritten solely to change their line endings. Checksums (in `.anvil.lock`) are computed over the body bytes with line endings normalized to LF so a Git checkout setting `core.autocrlf=true` doesn't trigger spurious "user edited" detection. Trailing whitespace on individual lines and a @@ -241,32 +246,90 @@ while still failing cloud workflows on anything the catalog covers. Users who wa behavior set per-lint `"deny"` values inside the region — the dirty-file flow then preserves their edit. -### Adopting a hand-written table on first introduction +### Adopting a hand-written table 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. +a manifest that will not parse. The tool therefore takes over the hand-written table +instead of duplicating it, both when the region is first introduced and when an existing +region's body gains a table (see below). + +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 **last** table the region body opens. + The entry's original source slice is moved, so its comments and spacing survive + byte-for-byte. A dotted assignment carries its own prefix with it, because + `rust.a = 1` and `rust.b = 2` are one dotted sub-table to the parser and each leaf must + be located by its own source position rather than the prefix key's — reading them all as + starting where the prefix does gives every leaf but the last an empty slice, which + deletes the setting instead of relocating it; +- **declared only by hand, in a table that is not the body's last** — not relocatable. + Residue lands after the closing sentinel, so TOML would read it as a setting of the last + table the body opens: a hand-written `[Hunspell]` key would come back as + `Hunspell.quirks.`, which still parses and is never read. The region is refused + rather than written, and the diagnostic asks the user to remove the settings the region + does not declare; +- **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, judged as +the pass will leave the file: only the regions this pass **removes** are 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 — +and blanking exactly those is what keeps the migration valid while still judging every +region that is staying. A region is masked only if the catalog no longer declares it *and* +the removal decision is to remove it; a customized orphan is kept, so it stays in the file +and keeps the tables it declares. If the result would not parse, the region is **refused**: +that region is left unchanged, and a diagnostic names the host and the reason. The +diagnostic also explains that other regions in the same file and other artifacts may still +be updated. Refusing is scoped to the region, not the host or the run. + +Masking every *other* region instead — the narrower question "does this region collide with +hand-written text" — is what let two regions *of the catalog* compose into a duplicate +header with neither able to see the other. It is still asked, but only after a refusal, to +tell the two faults apart: if the file parses with every other region blanked, nothing +hand-written is involved and the collision is between two regions anvil owns. That refusal +reads differently on purpose — both regions are anvil's own, so no edit to the host resolves +it, and the diagnostic asks for a bug report instead of sending the reader to reconcile a +table they never wrote. It names the table and the sibling holding it where both declare it +with a header, and otherwise quotes the parser, since a table declared by a dotted +assignment has no header to name. + +Adoption runs when a region is **updated**, not only when it is introduced. Replacing a +region where it stands cannot add a header, but its *body* can: a template that gains a +table the host already declares by hand collides on the next run, from a host that was +valid before it. Reconciling it is deliberate rather than refusing, because the edit that +clears it by hand — drop the header, move the remaining settings below the closing sentinel +so TOML still reads them as that table's — is not something a diagnostic can usefully +describe, and its likeliest reading ("remove the table") costs the user the setting they +wrote. An ordinary update is unaffected: adoption masks every managed region before +parsing, so tables the region already owns are invisible and it reports no change. + +The diagnostic asks the user to reconcile the hand-written table with the managed one +before retrying. No managed region was introduced on refusal, so there is no region to +empty at that point. When adoption succeeds, residue insertion preserves any existing +LF or CRLF blank-line gap before the content that followed the region, and the relocated +residue keeps the line ending it was written with rather than being terminated with a +lone LF. + +`tests/toml_adoption.rs` snapshots each of these outcomes end to end — the host as the +user wrote it, the decision per region, the refusal if any, and the host anvil left +behind. The unit tests pin the rules; those snapshots are what makes the resulting file +reviewable, which is what the substring assertions this behaviour replaced could not do. ### User-extension limits for TOML regions diff --git a/crates/cargo-anvil/src/anvil/artifacts/region.rs b/crates/cargo-anvil/src/anvil/artifacts/region.rs index 0deba72c8..db6928b86 100644 --- a/crates/cargo-anvil/src/anvil/artifacts/region.rs +++ b/crates/cargo-anvil/src/anvil/artifacts/region.rs @@ -241,7 +241,7 @@ pub fn gitattributes() -> Artifact { #[cfg_attr(coverage_nightly, coverage(off))] mod tests { use super::*; - use crate::region::upsert_region; + use crate::region::{TomlAdoption, adopt_unmanaged_toml_tables, upsert_region}; #[test] fn embedded_catalog_uses_dotted_keys() { @@ -396,6 +396,43 @@ mod tests { assert!(SPELLCHECK_BODY.contains("allow_concatenation = true")); } + /// The shipped spellcheck body opens two tables, so residue re-emitted + /// after its closing sentinel is read as a `[Hunspell.quirks]` setting. A + /// hand-written `[Hunspell]` key that the body does not declare therefore + /// cannot be relocated, and adoption must refuse rather than quietly move + /// it into a table cargo-spellcheck never reads it from. + #[test] + fn a_hand_written_hunspell_setting_is_never_moved_into_quirks() { + let host = "[Hunspell]\nlang = \"en_US\"\ntransform_regex = [\"^'\"]\n"; + let adoption = adopt_unmanaged_toml_tables(host, SPELLCHECK_BODY, CommentSyntax::Hash); + + assert_eq!( + adoption, + TomlAdoption::Unrelocatable { + table: "Hunspell".to_owned(), + tail_table: "Hunspell.quirks".to_owned(), + }, + "the real spellcheck body refuses rather than re-attributing the setting" + ); + } + + /// The refusal tells the user to remove the settings the region does not + /// declare, so doing exactly that must let the same host onboard. + #[test] + fn removing_the_unrelocatable_setting_lets_the_spellcheck_region_onboard() { + let host = "[Hunspell]\nlang = \"en_US\"\n"; + let adoption = adopt_unmanaged_toml_tables(host, SPELLCHECK_BODY, CommentSyntax::Hash); + + assert_eq!( + adoption, + TomlAdoption::Adopted { + text: String::new(), + residue: String::new(), + }, + "the remedy the diagnostic names is the one that works" + ); + } + #[test] fn clippy_body_carries_companion_tunings_for_catalog_lints() { assert!(CLIPPY_BODY.contains("allow-panic-in-tests = true")); diff --git a/crates/cargo-anvil/src/emit/managed_region.rs b/crates/cargo-anvil/src/emit/managed_region.rs index 6bde5fa71..2053b6f6e 100644 --- a/crates/cargo-anvil/src/emit/managed_region.rs +++ b/crates/cargo-anvil/src/emit/managed_region.rs @@ -16,13 +16,19 @@ //! [`crate::run`]'s `HostTextCache` and //! [`updates.md`](../../../docs/design/updates.md). -use ohno::AppError; +use std::collections::BTreeSet; + +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, declared_tables, find_region, insert_after_region, + managed_region_ids, mask_other_managed_regions, mask_retiring_managed_regions, text_newline, upsert_region_with_newline, +}; /// Inputs that identify and render one managed region. #[derive(Clone, Copy)] @@ -31,7 +37,7 @@ pub struct ManagedRegionRequest<'a> { pub host_relpath: &'a str, /// Stable identifier written into the region sentinels. pub region_id: &'a str, - /// Byte-exact content rendered between the sentinels. + /// Template content rendered between the sentinels using the host's line endings. pub rendered_body: &'a str, /// Comment flavor used by the host file. pub syntax: CommentSyntax, @@ -119,6 +125,138 @@ pub fn plan_managed_region(manifest: &Manifest, host_text: Option<&str>, request Ok(item) } +/// Why writing `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. +/// +/// Both introducing a region and updating one are checked. An update used to +/// be assumed safe, on the grounds that replacing a region where it stands +/// cannot add a header — but the *body* can: a template that gains a table the +/// host already declares by hand collides on the next run, from a host that +/// was perfectly valid before it. +/// +/// `retiring` names the managed regions this pass removes from the host. They +/// are blanked and everything else is judged as written, so a migration that is +/// about to become valid is not refused while a sibling that is *staying* is +/// still seen. The alternative — masking every region but this one — is what +/// let two regions of the catalog compose into a duplicate header with neither +/// able to see it. +/// +/// Returns `None` for a host that is not TOML and for a splice whose result +/// parses. +#[must_use] +pub fn toml_introduction_refusal( + host_text: Option<&str>, + request: ManagedRegionRequest<'_>, + retiring: &BTreeSet, +) -> 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 find_region(base, region_id, syntax).is_err() { + return None; + } + + let spliced = match splice(host_relpath, host_text, region_id, rendered_body, syntax, placement) { + Err(error) => return Some(TomlRefusal::Host(error.to_string())), + Ok(spliced) => spliced, + }; + let error = mask_retiring_managed_regions(&spliced, syntax, retiring) + .parse::() + .err()?; + // Which fault is this? Blanking every other managed region leaves this + // region beside the repository's own text alone. If *that* parses, nothing + // hand-written is involved and the collision is between two regions anvil + // owns -- a defect to report rather than a table the reader can reconcile, + // since no edit to the host resolves it. + if mask_other_managed_regions(&spliced, syntax, region_id) + .parse::() + .is_ok() + { + return Some(TomlRefusal::Sibling( + colliding_sibling(&spliced, rendered_body, syntax, region_id, retiring).map_or_else( + || format!("splicing the region would leave {host_relpath} unparsable as TOML: {error}"), + |(table, owner)| { + format!("this region declares `[{table}]`, which the managed region '{owner}' already declares in the same file") + }, + ), + )); + } + Some(TomlRefusal::Host(format!( + "splicing the region would leave {host_relpath} unparsable as TOML: {error}" + ))) +} + +/// The table and region id of the sibling this body collides with, when one can +/// be named. +/// +/// Purely for the diagnostic: the parser has already decided. A sibling that +/// declares the table through a dotted assignment rather than a header cannot +/// be named this way, and the caller falls back to the parser's own message. +fn colliding_sibling( + spliced: &str, + rendered_body: &str, + syntax: CommentSyntax, + region_id: &str, + retiring: &BTreeSet, +) -> Option<(String, String)> { + let mine = declared_tables(rendered_body); + managed_region_ids(spliced, syntax) + .into_iter() + .filter(|id| id != region_id && !retiring.contains(id)) + .find_map(|id| { + let body = find_region(spliced, &id, syntax).ok()??; + let shared = declared_tables(body.body_str()).intersection(&mine).next()?.clone(); + Some((shared, id)) + }) +} + +/// Which of the two faults a refused TOML splice is. +/// +/// They read differently to the user on purpose: one names an edit that +/// resolves it, and the other cannot, so it asks for a bug report instead. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TomlRefusal { + /// The region collides with the repository's own hand-written text, which + /// the user can reconcile. + Host(String), + /// Two of anvil's own regions compose into a file TOML cannot read. No edit + /// to the host fixes it. + Sibling(String), +} + +impl TomlRefusal { + /// The parser's account of what went wrong, without the verdict. + #[cfg(test)] + fn reason(&self) -> &str { + match self { + Self::Host(reason) | Self::Sibling(reason) => reason, + } + } +} + +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>, @@ -128,24 +266,66 @@ fn splice( placement: RegionPlacement, ) -> Result { let base = host_text.unwrap_or(""); + let newline = text_newline(base); - // Introducing a region into a TOML host: adopt any hand-written copy of the + // Writing a region into a TOML host: adopt any hand-written copy of the // tables the body declares, rather than appending a duplicate that TOML - // will refuse to parse. Only on introduction — once the region exists, - // `upsert_region_with_placement` replaces it in place and there is nothing - // to adopt. + // will refuse to parse. + // + // This runs on updates as well as introductions. It was once scoped to + // introductions, on the reasoning that replacing an existing region in + // place cannot add a header — true of the splice, but not of the body: a + // template that gains a table the host declares by hand collides on the + // next run. Reconciling it here is what spares the user an edit they would + // have to reverse-engineer, since the correct one (drop the header, move + // the extras below the closing sentinel) is not something a diagnostic can + // usefully describe. On an ordinary update there is nothing to adopt: + // adoption masks every managed region before parsing, so tables the region + // already owns are invisible and it returns `Unchanged`. 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) { + 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_introduction_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." + )); + } + // Also unreachable in the normal path, and refused for the same + // reason: the only place the entries could go is where TOML reads + // them as another table's. + TomlAdoption::Unrelocatable { table, tail_table } => { + return Err(app_err!( + "{host_relpath} declares settings in `[{table}]` that the managed region \ + '{region_id}' does not, and the region's body ends in `[{tail_table}]`, so \ + re-emitting them after the region would make them settings of `[{tail_table}]` \ + instead. Remove them from `[{table}]` and re-run." + )); + } + } } else { base }; - upsert_region_with_placement(base, region_id, rendered_body, syntax, placement) + let spliced = upsert_region_with_newline(base, region_id, rendered_body, syntax, placement, newline)?; + insert_after_region(&spliced, region_id, &residue, syntax) } #[cfg(test)] @@ -159,6 +339,256 @@ mod tests { ManagedRegionRequest::at_end(host_relpath, region_id, rendered_body, SYN) } + /// The backstop with nothing retiring, which is every case but a migration. + fn refusal(host_text: Option<&str>, request: ManagedRegionRequest<'_>) -> Option { + toml_introduction_refusal(host_text, request, &BTreeSet::new()) + } + + /// A sibling region that is *staying* is judged as written, so the region + /// being spliced sees the table it declares. Both are anvil's own, so the + /// verdict is `Sibling` — the wording that asks for a bug report rather + /// than sending the reader to reconcile a table nobody hand-wrote. + #[test] + fn a_sibling_region_declaring_the_same_table_is_anvils_own_fault() { + let host = "# >>> anvil-managed: other\n[licenses]\nallow = [\"MIT\"]\n# <<< anvil-managed: other\n"; + let body = "[licenses]\nconfidence-threshold = 0.9\n"; + + let verdict = refusal(Some(host), request("deny.toml", "r", body)).expect("two regions cannot both declare [licenses]"); + + assert!(matches!(verdict, TomlRefusal::Sibling(_)), "anvil's own fault: {verdict:?}"); + } + + /// The same collision against a sibling this pass is *removing* is a + /// migration, not a fault: the old region is blanked, so the splice is + /// judged against the file as it will be left. + #[test] + fn a_retiring_sibling_does_not_refuse_the_region_replacing_it() { + let host = "# >>> anvil-managed: old\n[licenses]\nallow = [\"MIT\"]\n# <<< anvil-managed: old\n"; + let body = "[licenses]\nconfidence-threshold = 0.9\n"; + let retiring = BTreeSet::from(["old".to_owned()]); + + assert_eq!( + toml_introduction_refusal(Some(host), request("deny.toml", "r", body), &retiring), + None, + "the region being removed must not block the one replacing it" + ); + } + + /// A dotted assignment declares its table just as a header does, so a + /// sibling writing `[a]` beside a region writing `a.b = 1` is the same + /// collision. Nothing enumerates table *headers* any more — the parser is + /// asked, and it rejects declaring `a` twice. + #[test] + fn a_dotted_key_collides_with_a_siblings_header() { + let host = "# >>> anvil-managed: other\nlints.rust.unsafe_code = \"deny\"\n# <<< anvil-managed: other\n"; + let body = "[lints]\nworkspace = true\n"; + + let verdict = refusal(Some(host), request("Cargo.toml", "r", body)).expect("a dotted key declares the table too"); + + assert!(matches!(verdict, TomlRefusal::Sibling(_)), "anvil's own fault: {verdict:?}"); + } + + /// 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 = + refusal(Some(host), request("deny.toml", "anvil-deny-advisories", body)).expect("a disagreement over `yanked` must be refused"); + let reason = reason.reason(); + + assert!(reason.contains("yanked"), "the refusal names the key: {reason}"); + } + + /// A hand-written setting the body does not declare, in a table the body + /// does not open last, has nowhere to go: re-emitted after the region it + /// becomes a setting of the body's trailing table. The run refuses the + /// region and names both tables, so the user can see what would have moved + /// where. + #[test] + fn a_setting_that_would_change_table_is_refused_rather_than_written() { + let host = "[Hunspell]\nlang = \"en_US\"\ntransform_regex = [\"^'\"]\n"; + let body = "[Hunspell]\nlang = \"en_US\"\n\n[Hunspell.quirks]\nallow_concatenation = true\n"; + + let reason = refusal(Some(host), request("spellcheck.toml", "anvil-spellcheck", body)) + .expect("a setting that cannot keep its table must be refused"); + let reason = reason.reason(); + + assert!( + reason.contains("[Hunspell]") && reason.contains("[Hunspell.quirks]"), + "the refusal names the table and where residue would land: {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!(refusal(Some(host), request("deny.toml", "anvil-deny-advisories", body)), None); + assert_eq!(refusal(None, request("deny.toml", "anvil-deny-advisories", body)), None); + assert_eq!(refusal(Some("recipe:\n"), request("Justfile", "r", "body\n")), None); + } + + /// A region already on disk beside a hand-written copy of its own table is + /// the duplicate-header file adoption exists to repair — so an update + /// repairs it rather than walking past it. The host below does not parse as + /// written: two `[advisories]` headers. Adoption masks the region, sees the + /// hand-written copy, and takes it over. + #[test] + fn an_existing_region_beside_a_hand_written_copy_is_repaired() { + let host = "# >>> anvil-managed: r\n[advisories]\nyanked = \"warn\"\n# <<< anvil-managed: r\n\n[advisories]\nignore = []\n"; + assert!(host.parse::().is_err(), "the host starts out broken"); + + let request = request("deny.toml", "r", "[advisories]\nyanked = \"deny\"\n"); + assert_eq!(refusal(Some(host), request), None, "repairable, not refused"); + + let mut manifest = Manifest::default(); + manifest.set_region("deny.toml", "r", checksum_str("[advisories]\nyanked = \"warn\"\n")); + let item = plan_managed_region(&manifest, Some(host), request).unwrap(); + let spliced = item.spliced_host.as_deref().expect("the region is written"); + + let document = spliced + .parse::() + .unwrap_or_else(|error| panic!("the repaired host must parse: {error}\n---\n{spliced}\n---")); + assert_eq!(spliced.matches("[advisories]").count(), 1, "one header survives:\n{spliced}"); + assert!( + document["advisories"]["ignore"].as_array().is_some(), + "and the hand-written entry is kept:\n{spliced}" + ); + } + + /// The update path used to be assumed safe: replacing a region where it + /// stands cannot add a header. The *body* can. A template that gains a + /// table the host already declares by hand collided on the next run, from a + /// host that was valid before it — `decision=Write`, no refusal, two + /// `[licenses]` headers on disk. + /// + /// Reconciling it here rather than refusing is deliberate. The edit that + /// clears it by hand — drop the header, move the extras below the closing + /// sentinel so TOML still reads them as that table's — is not something a + /// diagnostic can usefully describe, and the likeliest reading of one + /// ("remove the table") costs the user the setting they wrote. + #[test] + fn an_update_whose_body_gains_a_table_adopts_the_hand_written_copy() { + let host = "\ +[licenses] +unused-allowed-license = \"allow\" + +# >>> anvil-managed: r +[advisories] +yanked = \"deny\" +# <<< anvil-managed: r +"; + assert!(host.parse::().is_ok(), "the host is valid before the bump"); + + let old_body = "[advisories]\nyanked = \"deny\"\n"; + let new_body = "[advisories]\nyanked = \"deny\"\n\n[licenses]\nallow = [\"MIT\"]\n"; + let mut manifest = Manifest::default(); + manifest.set_region("deny.toml", "r", checksum_str(old_body)); + + let request = request("deny.toml", "r", new_body); + assert_eq!(refusal(Some(host), request), None, "adoption resolves it"); + + let item = plan_managed_region(&manifest, Some(host), request).unwrap(); + assert_eq!(item.decision, Decision::Write, "the template moved, so the region is rewritten"); + let spliced = item.spliced_host.as_deref().expect("the region is written"); + + let document = spliced + .parse::() + .unwrap_or_else(|error| panic!("the updated host must parse: {error}\n---\n{spliced}\n---")); + assert_eq!(spliced.matches("[licenses]").count(), 1, "no duplicate header:\n{spliced}"); + assert_eq!( + document["licenses"]["unused-allowed-license"].as_str(), + Some("allow"), + "the user's own setting is never dropped, and still configures `[licenses]`:\n{spliced}" + ); + assert!(document["licenses"]["allow"].as_array().is_some(), "alongside the managed keys"); + } + + /// Adoption on the update path does not reach for anything it did not + /// reach for before: with nothing hand-written outside a region, every + /// table the body declares is already the region's own and invisible behind + /// the mask, so an ordinary template bump is byte-identical to what it was. + #[test] + fn an_ordinary_update_is_untouched_by_adoption() { + let host = "# >>> anvil-managed: r\n[advisories]\nyanked = \"warn\"\n# <<< anvil-managed: r\n"; + let new_body = "[advisories]\nyanked = \"deny\"\n"; + let mut manifest = Manifest::default(); + manifest.set_region("deny.toml", "r", checksum_str("[advisories]\nyanked = \"warn\"\n")); + + let item = plan_managed_region(&manifest, Some(host), request("deny.toml", "r", new_body)).unwrap(); + + assert_eq!(item.decision, Decision::Write); + assert_eq!( + item.spliced_host.as_deref(), + Some("# >>> anvil-managed: r\n[advisories]\nyanked = \"deny\"\n# <<< anvil-managed: r\n"), + "the region is replaced in place, with nothing relocated" + ); + } + + /// An update whose body disagrees with a hand-written value still has no + /// safe output, so it refuses exactly as an introduction does — and the + /// remedy the diagnostic names is one the user can actually carry out. + #[test] + fn an_update_that_conflicts_with_a_hand_written_value_is_refused() { + let host = + "[licenses]\nconfidence-threshold = 0.8\n\n# >>> anvil-managed: r\n[advisories]\nyanked = \"deny\"\n# <<< anvil-managed: r\n"; + let new_body = "[advisories]\nyanked = \"deny\"\n\n[licenses]\nconfidence-threshold = 0.93\n"; + + let reason = + refusal(Some(host), request("deny.toml", "r", new_body)).expect("a disagreement over `confidence-threshold` must be refused"); + let reason = reason.reason(); + + assert!(reason.contains("confidence-threshold"), "the refusal names the key: {reason}"); + } + #[test] fn missing_host_writes_new_file() { let item = plan_managed_region(&Manifest::default(), None, request("Justfile", "r", "body line\n")).unwrap(); @@ -177,6 +607,68 @@ mod tests { assert!(spliced.contains("# >>> anvil-managed: r")); } + #[test] + fn adoption_keeps_the_original_line_ending_when_it_removes_the_whole_host() { + for newline in ["\n", "\r\n"] { + for residue in ["", "ignore = []"] { + let host = format!("[advisories]{newline}yanked = \"deny\"{newline}{residue}{newline}"); + let body = "[advisories]\nyanked = \"deny\"\n"; + let item = plan_managed_region(&Manifest::default(), Some(&host), request("deny.toml", "r", body)).unwrap(); + let spliced = item.spliced_host.as_deref().unwrap(); + let expected = format!( + "# >>> anvil-managed: r{newline}[advisories]{newline}yanked = \"deny\"{newline}# <<< anvil-managed: r{newline}" + ); + let expected = if residue.is_empty() { + expected + } else { + format!("{expected}{residue}{newline}") + }; + assert_eq!(spliced, expected); + let document = spliced.parse::().unwrap(); + assert_eq!(document["advisories"]["yanked"].as_str(), Some("deny")); + if !residue.is_empty() { + assert!(document["advisories"]["ignore"].is_array()); + } + + let mut manifest = Manifest::default(); + manifest.set_region("deny.toml", "r", checksum_str(body)); + let repeated = plan_managed_region(&manifest, Some(spliced), request("deny.toml", "r", body)).unwrap(); + assert_eq!(repeated.decision, Decision::InSync); + } + } + } + + #[test] + fn crlf_region_updates_and_proposals_keep_normalized_checksum_decisions() { + let old_body = "old\n"; + let host = "# user\r\n\r\n# >>> anvil-managed: r\r\nold\r\n# <<< anvil-managed: r\r\n"; + for (last_body, decision) in [(old_body, Decision::Write), ("original\n", Decision::Propose)] { + let mut manifest = Manifest::default(); + manifest.set_region("Justfile", "r", checksum_str(last_body)); + let item = plan_managed_region(&manifest, Some(host), request("Justfile", "r", "new\n")).unwrap(); + assert_eq!(item.decision, decision); + assert_eq!( + item.spliced_host.as_deref().unwrap(), + "# user\r\n\r\n# >>> anvil-managed: r\r\nnew\r\n# <<< anvil-managed: r\r\n" + ); + } + } + + #[test] + fn adopting_an_unterminated_crlf_host_keeps_the_relocated_entry_crlf() { + let host = "[advisories]\r\nignore = []"; + let item = plan_managed_region( + &Manifest::default(), + Some(host), + request("deny.toml", "r", "[advisories]\nyanked = \"deny\"\n"), + ) + .unwrap(); + assert_eq!( + item.spliced_host.as_deref().unwrap(), + "# >>> anvil-managed: r\r\n[advisories]\r\nyanked = \"deny\"\r\n# <<< anvil-managed: r\r\nignore = []\r\n" + ); + } + /// A member manifest that already declares `[lints] workspace = true` by /// hand must not gain a second `[lints]` table when the managed region is /// first introduced. TOML rejects a duplicate table outright, so appending @@ -264,11 +756,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(), @@ -374,12 +866,15 @@ mod tests { assert_eq!(item.decision, Decision::Propose); } - /// Adoption applies only when the region is first introduced. Once the - /// region exists, the host outside the sentinels is untouched and the - /// body is replaced in place, so a hand-written table stays exactly where - /// it is whatever its relationship to the rendered body. + /// The counterpart to the introduction case, and once the reverse of it: + /// this asserted that an existing region left a hand-written `[lints]` + /// exactly where it was, which — with the body declaring `[lints]` too — + /// is a `Cargo.toml` carrying two `[lints]` headers, i.e. one TOML rejects + /// and cargo cannot read. Adoption now runs here too, so the hand-written + /// copy is taken over instead. Its lone entry is one the body declares + /// identically, so it is covered and simply dropped. #[test] - fn an_existing_region_does_not_re_run_table_adoption() { + fn an_existing_region_adopts_a_hand_written_table_its_body_declares() { let host = "[lints]\nworkspace = true\n\n# >>> anvil-managed: r\nold = true\n# <<< anvil-managed: r\n"; let mut manifest = Manifest::default(); manifest.set_region("Cargo.toml", "r", checksum_str("old = true\n")); @@ -388,10 +883,11 @@ mod tests { assert_eq!(item.decision, Decision::Write); let spliced = item.spliced_host.as_deref().unwrap(); - assert!( - spliced.starts_with("[lints]\nworkspace = true\n"), - "the hand-written table is left alone:\n{spliced}" - ); + spliced + .parse::() + .unwrap_or_else(|error| panic!("the updated manifest must parse: {error}\n---\n{spliced}\n---")); + assert_eq!(spliced.matches("[lints]").count(), 1, "no duplicate header:\n{spliced}"); + assert!(spliced.contains("workspace = true"), "the setting survives:\n{spliced}"); } #[test] diff --git a/crates/cargo-anvil/src/emit/mod.rs b/crates/cargo-anvil/src/emit/mod.rs index c01817e90..ccb5e065c 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, TomlRefusal, 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..4ea41e216 100644 --- a/crates/cargo-anvil/src/region.rs +++ b/crates/cargo-anvil/src/region.rs @@ -24,10 +24,10 @@ //! Empty body (just the sentinels with no content between them) is the //! opt-out signal — see [`updates.md`](../../docs/design/updates.md). -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use ohno::{AppError, app_err, bail}; -use toml_edit::{DocumentMut, Table}; +use toml_edit::{Item, Key, RawString, Table}; /// Comment syntax used by the host file. /// @@ -178,9 +178,9 @@ pub fn find_region<'a>(text: &'a str, id: &str, syntax: CommentSyntax) -> Result /// Replace the body of region `id` in `text`, or append a new region if /// none exists. /// -/// `new_body` is inserted between the sentinel lines verbatim, with a -/// single newline between each sentinel and the body. If `new_body` does -/// not end with `\n`, one is added before the closing sentinel. +/// Generated content and separators use the host's first line ending, or LF +/// when it has none. Other body bytes are preserved. An unterminated body +/// receives a newline before the closing sentinel. /// /// # Errors /// @@ -205,12 +205,24 @@ pub fn upsert_region_with_placement( syntax: CommentSyntax, placement: RegionPlacement, ) -> Result { - let rendered = render_region(id, new_body, syntax); + upsert_region_with_newline(text, id, new_body, syntax, placement, text_newline(text)) +} + +/// Use the original host's line ending even when adoption removed all its text. +pub(crate) fn upsert_region_with_newline( + text: &str, + id: &str, + new_body: &str, + syntax: CommentSyntax, + placement: RegionPlacement, + newline: &str, +) -> Result { + let rendered = render_region(id, new_body, syntax, newline); if let Some(region) = find_region(text, id, syntax)? { if placement == RegionPlacement::Start { let without_region = remove_region(text, id, syntax)?; - return Ok(prepend_region(&without_region, &rendered)); + return Ok(prepend_region(&without_region, &rendered, newline)); } let mut out = String::with_capacity(text.len() + rendered.len()); out.push_str(&text[..region.start_line.start]); @@ -220,7 +232,7 @@ pub fn upsert_region_with_placement( } if placement == RegionPlacement::Start { - return Ok(prepend_region(text, &rendered)); + return Ok(prepend_region(text, &rendered, newline)); } if let RegionPlacement::At(offset) = placement { @@ -253,12 +265,10 @@ pub fn upsert_region_with_placement( let (before, after) = text.split_at(offset); let mut out = String::with_capacity(text.len() + rendered.len() + 2); out.push_str(before); - if !before.is_empty() && !before.ends_with("\n\n") { - out.push('\n'); - } + separate_region(&mut out, newline); out.push_str(&rendered); - if !after.is_empty() && !after.starts_with('\n') { - out.push('\n'); + if !after.is_empty() && leading_newline_len(after) == 0 { + out.push_str(newline); } out.push_str(after); return Ok(out); @@ -268,46 +278,66 @@ pub fn upsert_region_with_placement( // if the file is non-empty and doesn't end in two newlines. let mut out = String::with_capacity(text.len() + rendered.len() + 1); out.push_str(text); - if !text.is_empty() { - if !text.ends_with('\n') { - out.push('\n'); - } - if !text.ends_with("\n\n") && !text.is_empty() { - out.push('\n'); - } - } + separate_region(&mut out, newline); out.push_str(&rendered); Ok(out) } -fn prepend_region(text: &str, rendered: &str) -> String { +fn prepend_region(text: &str, rendered: &str, newline: &str) -> String { let mut out = String::with_capacity(text.len() + rendered.len() + 1); out.push_str(rendered); - if !text.is_empty() && !text.starts_with('\n') { - out.push('\n'); + if !text.is_empty() && leading_newline_len(text) == 0 { + out.push_str(newline); } out.push_str(text); out } -/// Render an isolated region — sentinels plus body — without splicing it -/// into a host. -#[must_use] -pub fn render_region(id: &str, body: &str, syntax: CommentSyntax) -> String { +fn separate_region(out: &mut String, newline: &str) { + if !out.is_empty() { + if !out.ends_with('\n') { + out.push_str(newline); + } + if trailing_blank_line_len(out) == 0 { + out.push_str(newline); + } + } +} + +fn leading_newline_len(text: &str) -> usize { + if text.starts_with("\r\n") { + 2 + } else { + usize::from(text.starts_with('\n')) + } +} + +fn trailing_blank_line_len(text: &str) -> usize { + if text.ends_with("\n\r\n") { + 2 + } else { + usize::from(text.ends_with("\n\n")) + } +} + +fn render_region(id: &str, body: &str, syntax: CommentSyntax, newline: &str) -> String { let prefix = syntax.prefix(); let mut out = String::with_capacity(body.len() + 80); out.push_str(prefix); out.push_str(" >>> anvil-managed: "); out.push_str(id); - out.push('\n'); - out.push_str(body); - if !body.is_empty() && !body.ends_with('\n') { - out.push('\n'); + out.push_str(newline); + for line in body.split_inclusive('\n') { + let content = line + .strip_suffix('\n') + .map_or(line, |content| content.strip_suffix('\r').unwrap_or(content)); + out.push_str(content); + out.push_str(newline); } out.push_str(prefix); out.push_str(" <<< anvil-managed: "); out.push_str(id); - out.push('\n'); + out.push_str(newline); out } @@ -336,17 +366,15 @@ pub fn remove_region(text: &str, id: &str, syntax: CommentSyntax) -> Result 0 { + cut_end += trailing_blank; } else { // Region sits at end-of-file: there's no trailing blank to // eat. Pull back the leading blank instead so the file doesn't // end with an orphan blank line where the region used to be. let prefix = &text[..cut_start]; - if prefix.ends_with("\n\n") { - cut_start -= 1; - } + cut_start -= trailing_blank_line_len(prefix); } let mut out = String::with_capacity(text.len() - (cut_end - cut_start)); @@ -359,290 +387,606 @@ 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 newline = text_newline(text); + + let mut out = String::with_capacity(text.len() + extra.len() + 1); + out.push_str(&text[..at]); + if !text[..at].ends_with('\n') { + out.push_str(newline); + } + out.push_str(extra); + if !extra.ends_with('\n') { + out.push_str(newline); + } + 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() && leading_newline_len(rest) == 0 { + out.push_str(newline); + } + 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 **last** table the region body 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 only by hand, in a table the body opens but does not open **last** +/// — a [`TomlAdoption::Unrelocatable`]. There is nowhere after the region that +/// TOML still reads as that table, so relocating the entry would silently make +/// it a setting of another table; this reports instead. +/// * 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. +/// +/// 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. /// -/// 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. +/// 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(); - } + 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; + }; - let open = syntax.prefix().to_owned() + " >>> anvil-managed:"; - let close = syntax.prefix().to_owned() + " <<< anvil-managed:"; + let protected = managed_region_ranges(text, syntax); + // Residue is re-emitted directly after the region's closing sentinel, so + // TOML attributes it to the LAST table the body opens. Only that table's + // hand-written extras can be relocated without changing what they + // configure. + let tail = managed.last().map(|table| table.path.clone()).unwrap_or_default(); + // 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 deletions: Vec = Vec::new(); + let mut residue = String::new(); + + for candidate in &candidates { + if candidate.array_of_tables { + 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)]), + } + } - // 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); + deletions.push(ByteRange { + start: candidate.header.start, + end, + }); + if !kept.trim().is_empty() && candidate.path != tail { + return TomlAdoption::Unrelocatable { + table: candidate.path.join("."), + tail_table: tail.join("."), + }; } + residue.push_str(&kept); } - if adoptable.is_empty() { - return text.to_owned(); + + if deletions.is_empty() { + return TomlAdoption::Unchanged; } + // 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 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; - - 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; - } - - 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; - } - } - - out.push_str(raw); + 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..]); - if trimmed.starts_with(&close) { - in_managed = false; - } + TomlAdoption::Adopted { + text: out, + residue: tidy_residue(&residue, text_newline(text)), } +} - out +/// 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. The terminator restored is the one the +/// residue itself was written with, so relocating a block out of a CRLF host +/// does not leave it ending in a lone `\n`. A residue with no line break uses +/// the original host's line ending. +/// +/// The trailing edge is trimmed of all whitespace, not just line breaks, so a +/// last line that ended in spaces or tabs loses them rather than being +/// re-emitted with trailing whitespace before the terminator this restores. +fn tidy_residue(residue: &str, host_newline: &str) -> String { + let newline = if residue.contains('\n') { + text_newline(residue) + } else { + host_newline + }; + let trimmed = trim_leading_blank_lines(residue).trim_end(); + if trimmed.is_empty() { + String::new() + } else { + let mut out = String::with_capacity(trimmed.len() + newline.len()); + out.push_str(trimmed); + out.push_str(newline); + out + } } -/// 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:"; +/// The first line ending in `text`, or LF when there is no line break. +pub(crate) fn text_newline(text: &str) -> &'static str { + match text.find('\n') { + Some(at) if text[..at].ends_with('\r') => "\r\n", + _ => "\n", + } +} - 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 last 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, + }, + /// A hand-written table carries configuration the managed body does not + /// declare, but it is not the last table the body opens — and residue is + /// re-emitted after the closing sentinel, where TOML would attribute it to + /// that last table instead. Relocating it would silently move the setting + /// into a different table, so the caller must refuse rather than write. + Unrelocatable { + /// Dotted path of the hand-written table whose extra entries have + /// nowhere to go. + table: String, + /// Dotted path of the last table the managed body opens, which is where + /// re-emitted residue lands. + tail_table: 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. +/// The tables a TOML body declares with an explicit header, as dotted paths. /// -/// 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) +/// Diagnostic only: the parser decides whether a splice is valid, and this +/// names the table two of anvil's own regions both declare so the refusal can +/// say which one. Arrays of tables are excluded, since TOML permits `[[bin]]` +/// to repeat. Returns an empty set for a body that is not valid TOML on its +/// own, in which case the caller falls back to the parser's own words. +#[must_use] +pub fn declared_tables(body: &str) -> BTreeSet { + headed_tables(body) + .unwrap_or_default() .into_iter() - .filter(|(header, _)| !is_array_of_tables(header)) - .filter_map(|(header, lines)| Some((table_path(header)?, table_values(&lines)?))) + .filter(|table| !table.array_of_tables) + .map(|table| table.path.join(".")) .collect() } -/// The canonical path a table header names: `[ workspace . lints ]` and -/// `[workspace.lints]` both yield `["workspace", "lints"]`. +/// Every explicitly headed table in `text`, in document order. /// -/// 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(())?; +/// 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) +} + +/// 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 - }) + } +} + +/// The top-level entries of a table, each with the source slice that carries +/// it. +/// +/// 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(); + collect_entry_starts(table, text, &mut Vec::new(), &mut starts); + 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 } -/// Return a trimmed TOML table header, without its trailing comment. +/// Record every assignment the table declares with the position of the +/// assignment 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`]. +/// Dotted assignments sharing a prefix are one dotted sub-table to the parser, +/// so descending into it is what gives each leaf its own position. Reading the +/// prefix key's position instead made every leaf beneath it start at the same +/// byte, which left all but the last of them with an empty slice — and an entry +/// whose slice is empty is deleted with its table and re-emitted as nothing. +fn collect_entry_starts(table: &Table, text: &str, path: &mut Vec, out: &mut Vec<(Vec, String, usize)>) { + for (key, item) in table { + // 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"); + path.push(key.get().to_owned()); + match item { + Item::Value(value) => out.push((path.clone(), value.to_string().trim().to_owned(), entry_start(key, text))), + Item::Table(child) if child.is_dotted() => collect_entry_starts(child, text, path, out), + _ => {} + } + path.pop(); + } +} + +/// Where the assignment that carries `key` begins in `text`. /// -/// 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) +/// The parser attaches an entry's leading trivia — its blank lines and comments +/// — to the key it precedes, so that prefix is the start whenever there is one. +/// Without one the entry starts at the beginning of its own line, which for a +/// dotted assignment is several segments left of the leaf: `rust.a = 1` hands +/// back only `a`, and a slice starting there would relocate the setting without +/// the `rust.` prefix that decides which table it lands in. +fn entry_start(key: &Key, text: &str) -> usize { + if let Some(prefix) = key.leaf_decor().prefix().and_then(RawString::span) { + return prefix.start; + } + let at = key.span().map_or(0, |span| span.start); + text[..at].rfind('\n').map_or(0, |newline| newline + 1) } -/// Whether a table header declares an array of tables (`[[bin]]`). +/// 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) +} + +/// Replace every managed region's bytes with spaces, keeping the newlines and +/// therefore every byte offset in the file. /// -/// 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 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)) } -/// Whether `text` contains a TOML multi-line string delimiter. +/// Blank every managed region except `keep`, so what remains is the region +/// under consideration plus the repository's own hand-written content. /// -/// 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("'''") +/// This hides a sibling region that is *staying*, so it answers only "does this +/// region collide with hand-written text". Use +/// [`mask_retiring_managed_regions`] for the question a validity check +/// actually has to ask; this one distinguishes the two faults once one has been +/// found. +#[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) } -/// Return `line` without a trailing TOML comment, if it has one. +/// Blank the managed regions named in `retiring`, so what remains is the file +/// as this pass will leave it. /// -/// 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 the regions this pass removes are blanked and everything +/// else is judged as written. Masking *every* other region instead would hide a +/// sibling that is staying, and two regions of the catalog declaring one table +/// would compose into a duplicate header that neither could see. +#[must_use] +pub fn mask_retiring_managed_regions(text: &str, syntax: CommentSyntax, retiring: &BTreeSet) -> String { + if retiring.is_empty() { + return text.to_owned(); + } + let ranges: Vec = managed_region_ranges_with_ids(text, syntax) + .into_iter() + .filter_map(|(id, range)| retiring.contains(&id).then_some(range)) + .collect(); + mask_regions(text, &ranges) +} + +/// The ids of the managed regions in `text`, in document order. +#[must_use] +pub fn managed_region_ids(text: &str, syntax: CommentSyntax) -> Vec { + managed_region_ranges_with_ids(text, syntax).into_iter().map(|(id, _)| id).collect() +} + +fn mask_regions(text: &str, ranges: &[ByteRange]) -> String { + if ranges.is_empty() { + return text.to_owned(); + } + // 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 { + 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 { ' ' }); } - (Some(active), character) if character == active && !escaped => { - quote = None; - None + cursor = range.end; + } + masked.push_str(&text[cursor..]); + masked +} + +/// 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 +1024,52 @@ 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}]`"), + TomlAdoption::Unrelocatable { table, tail_table } => { + panic!("unexpected refusal to relocate `[{table}]` past `[{tail_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 +1078,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 +1089,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, text, "the differing table is preserved:\n{adopted}"); + 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!( + 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 +1177,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 +1189,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 +1216,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 +1226,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 +1244,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 +1256,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 +1275,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 +1291,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 +1303,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 +1321,103 @@ 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 dotted assignments sharing a prefix are one dotted sub-table to the + /// parser, which hands the whole sub-table back under a single key. Reading + /// that key's position as the position of every leaf beneath it gave the + /// first leaf an empty source slice, so a hand-written setting was deleted + /// with the table it sat in and no residue was kept for it. + #[test] + fn each_dotted_assignment_keeps_its_own_source_slice() { + let text = "[lints]\nrust.a_custom = \"warn\"\nrust.unsafe_op_in_unsafe_fn = \"warn\"\n"; + let (adopted, residue) = adopted_with_residue(text, "[lints]\nrust.unsafe_op_in_unsafe_fn = \"warn\"\n"); + + assert_eq!(adopted, "", "the hand-written header is adopted:\n{adopted}"); + assert_eq!( + residue, "rust.a_custom = \"warn\"\n", + "the unmanaged dotted assignment survives with its own prefix" + ); + } + + /// The managed leaf may be written first, so the survivor is the *last* of + /// the group. Its slice must still stop at the end of its own line rather + /// than running to the next header, or the residue would swallow whatever + /// the user wrote after it. + #[test] + fn a_dotted_assignment_after_a_managed_one_keeps_its_own_slice() { + let text = "[lints]\nrust.unsafe_op_in_unsafe_fn = \"warn\"\nrust.a_custom = \"warn\"\n\n[bans]\nx = 1\n"; + let (adopted, residue) = adopted_with_residue(text, "[lints]\nrust.unsafe_op_in_unsafe_fn = \"warn\"\n"); + + assert_eq!(residue, "rust.a_custom = \"warn\"\n", "only the unmanaged assignment is kept"); + assert_eq!(adopted, "[bans]\nx = 1\n", "the following table is left alone:\n{adopted}"); + } + + /// Residue is re-emitted after the region's closing sentinel, so TOML reads + /// it as part of the LAST table the body opens. A body that opens more than + /// one table therefore cannot relocate an earlier table's extras — doing so + /// silently turns a `[Hunspell]` setting into a `[Hunspell.quirks]` one, + /// which still parses and is never read. Refusing is the only answer that + /// keeps the setting meaning what it says. + #[test] + fn residue_that_would_land_in_another_table_is_refused() { + let text = "[Hunspell]\nlang = \"en_US\"\ntransform_regex = [\"^'\"]\n"; + let adoption = adopt_unmanaged_toml_tables( + text, + "[Hunspell]\nlang = \"en_US\"\n\n[Hunspell.quirks]\nallow_concatenation = true\n", + SYN, + ); + + assert_eq!( + adoption, + TomlAdoption::Unrelocatable { + table: "Hunspell".to_owned(), + tail_table: "Hunspell.quirks".to_owned(), + }, + "the setting is not quietly moved into the trailing table" + ); + } + + /// The refusal is about where residue *lands*, not about how many tables the + /// body opens: extras belonging to the last table are still relocatable, and + /// a multi-table body that produces no residue at all still adopts. + #[test] + fn residue_from_the_body_s_last_table_is_still_relocated() { + let body = "[Hunspell]\nlang = \"en_US\"\n\n[Hunspell.quirks]\nallow_concatenation = true\n"; + let text = "[Hunspell]\nlang = \"en_US\"\n\n[Hunspell.quirks]\nallow_concatenation = true\ntransform_regex = [\"^'\"]\n"; + let (adopted, residue) = adopted_with_residue(text, body); + + assert_eq!(adopted, "", "both hand-written headers are adopted:\n{adopted}"); + assert_eq!( + residue, "transform_regex = [\"^'\"]\n", + "the extra setting of the trailing table travels with it" + ); + } + + /// A dotted assignment with no comment above it is located by the start of + /// its own line, which is one byte past the newline that ends the line + /// before. Starting *at* that newline instead drags a blank line into the + /// residue — invisible at the edges, where the residue is trimmed, but not + /// between two kept entries. + #[test] + fn a_relocated_assignment_starts_after_the_preceding_newline() { + let text = "[lints]\nrust.a = 1\n# managed below\nrust.b = 2\nrust.c = 3\n"; + let (_, residue) = adopted_with_residue(text, "[lints]\nrust.b = 2\n"); + + assert_eq!( + residue, "rust.a = 1\nrust.c = 3\n", + "no blank line is introduced between the kept assignments" + ); } /// Two tables under one parent are different tables. Comparing only the @@ -925,7 +1427,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}"); } @@ -1035,6 +1537,111 @@ mod tests { assert_eq!(new, "# >>> anvil-managed: x\nbody\n# <<< anvil-managed: x\n"); } + #[test] + fn generated_regions_and_separators_match_host_line_endings() { + for newline in ["\n", "\r\n"] { + let region = format!("# >>> anvil-managed: x{newline}body{newline}next{newline}# <<< anvil-managed: x{newline}"); + for body in ["body\nnext\n", "body\r\nnext\r\n", "body\r\nnext\n", "body\nnext"] { + for gap in ["", newline] { + let before = format!("before{newline}{gap}"); + let after = format!("{gap}after{newline}"); + let appended = upsert_region(&before, "x", body, SYN).unwrap(); + assert_eq!(appended, format!("before{newline}{newline}{region}")); + let prepended = upsert_region_with_placement(&after, "x", body, SYN, RegionPlacement::Start).unwrap(); + assert_eq!(prepended, format!("{region}{newline}after{newline}")); + let host = format!("{before}{after}"); + let inserted = upsert_region_with_placement(&host, "x", body, SYN, RegionPlacement::At(before.len())).unwrap(); + assert_eq!(inserted, format!("before{newline}{newline}{region}{newline}after{newline}")); + } + } + } + } + + #[test] + fn generated_regions_default_to_lf_without_a_host_line_ending() { + for host in ["", "unterminated"] { + for placement in [RegionPlacement::Start, RegionPlacement::End, RegionPlacement::At(host.len())] { + let out = upsert_region_with_placement(host, "x", "body\r\n", SYN, placement).unwrap(); + assert!(!out.contains('\r')); + assert!(out.contains("# >>> anvil-managed: x\nbody\n# <<< anvil-managed: x\n")); + if !host.is_empty() { + assert!(out.contains("unterminated\n\n") || out.ends_with("\n\nunterminated")); + } + } + } + } + + #[test] + fn mixed_host_uses_first_line_ending_without_normalizing_user_content() { + for (host, newline) in [("first\r\nsecond\n", "\r\n"), ("first\nsecond\r\n", "\n")] { + let out = upsert_region(host, "x", "body\n", SYN).unwrap(); + assert_eq!( + out, + format!("{host}{newline}# >>> anvil-managed: x{newline}body{newline}# <<< anvil-managed: x{newline}") + ); + } + } + + #[test] + fn crlf_updates_and_start_repositioning_preserve_line_endings() { + let before = "before\r\n\r\n"; + let old = "# >>> anvil-managed: x\r\nold\r\n# <<< anvil-managed: x\r\n"; + let host = format!("{before}{old}"); + for body in ["new\n", ""] { + let rendered = if body.is_empty() { + "# >>> anvil-managed: x\r\n# <<< anvil-managed: x\r\n" + } else { + "# >>> anvil-managed: x\r\nnew\r\n# <<< anvil-managed: x\r\n" + }; + let updated = upsert_region(&host, "x", body, SYN).unwrap(); + assert_eq!(updated, format!("{before}{rendered}")); + assert_eq!(upsert_region(&updated, "x", body, SYN).unwrap(), updated); + let moved = upsert_region_with_placement(&host, "x", body, SYN, RegionPlacement::Start).unwrap(); + assert_eq!(moved, format!("{rendered}\r\nbefore\r\n")); + assert_eq!( + upsert_region_with_placement(&moved, "x", body, SYN, RegionPlacement::Start).unwrap(), + moved + ); + } + } + + #[test] + fn crlf_insertion_after_an_unterminated_line_adds_a_complete_separator() { + let host = "first\r\nlast"; + for placement in [RegionPlacement::End, RegionPlacement::At(host.len())] { + let out = upsert_region_with_placement(host, "x", "body\n", SYN, placement).unwrap(); + assert_eq!( + out, + "first\r\nlast\r\n\r\n# >>> anvil-managed: x\r\nbody\r\n# <<< anvil-managed: x\r\n" + ); + } + } + + #[test] + fn crlf_region_removal_consumes_a_complete_adjacent_blank_line() { + let region = "# >>> anvil-managed: x\r\nbody\r\n# <<< anvil-managed: x\r\n"; + for (host, expected) in [ + (format!("before\r\n\r\n{region}"), "before\r\n"), + (format!("before\r\n\r\n{region}\r\nafter\r\n"), "before\r\n\r\nafter\r\n"), + ] { + assert_eq!(remove_region(&host, "x", SYN).unwrap(), expected); + } + } + + #[test] + fn crlf_residue_insertion_supplies_matching_terminators_and_separator() { + let region = "# >>> anvil-managed: x\r\n[advisories]\r\n# <<< anvil-managed: x"; + assert_eq!( + insert_after_region(region, "x", "ignore = []", SYN).unwrap(), + format!("{region}\r\nignore = []\r\n") + ); + let host = format!("{region}\r\n[licenses]\r\n"); + assert_eq!( + insert_after_region(&host, "x", "ignore = []", SYN).unwrap(), + format!("{region}\r\nignore = []\r\n\r\n[licenses]\r\n") + ); + } + /// `At` exists for hosts whose region order is semantic: a region added in a /// later release has to land at its declared position, not at end-of-file. /// The offset the caller computes points at the end of the preceding @@ -1156,13 +1763,13 @@ mod tests { #[test] fn render_region_with_empty_body() { - let s = render_region("x", "", SYN); + let s = render_region("x", "", SYN, "\n"); assert_eq!(s, "# >>> anvil-managed: x\n# <<< anvil-managed: x\n"); } #[test] fn render_region_adds_trailing_newline() { - let s = render_region("x", "body", SYN); + let s = render_region("x", "body", SYN, "\n"); assert_eq!(s, "# >>> anvil-managed: x\nbody\n# <<< anvil-managed: x\n"); } @@ -1264,4 +1871,214 @@ 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" + ); + } + + #[test] + fn residue_insertion_preserves_existing_lf_and_crlf_gaps() { + for newline in ["\n", "\r\n"] { + for blank_lines in [1, 2] { + let prefix = format!("# >>> anvil-managed: x{newline}[advisories]{newline}# <<< anvil-managed: x{newline}"); + let residue = format!("ignore = []{newline}"); + let rest = format!("{}# User bans{newline}[bans]{newline}", newline.repeat(blank_lines)); + let text = format!("{prefix}{rest}"); + + let out = insert_after_region(&text, "x", &residue, SYN).unwrap(); + + assert_eq!(out, format!("{prefix}{residue}{rest}")); + } + } + } + + /// 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"); + } + + /// The terminator `tidy_residue` restores is the one the residue was + /// written with. A lone `\n` appended to a CRLF block would leave the + /// relocated configuration with a line ending the rest of the file does not + /// use, in a host `insert_after_region` already goes out of its way to keep + /// consistent. + #[test] + fn relocated_residue_keeps_the_line_ending_it_was_written_with() { + assert_eq!( + tidy_residue("\r\n\r\nignore = []\r\nyanked = \"warn\"\r\n", "\n"), + "ignore = []\r\nyanked = \"warn\"\r\n", + "a CRLF residue stays CRLF to its last line" + ); + assert_eq!( + tidy_residue("\n\nignore = []\nyanked = \"warn\"\n", "\r\n"), + "ignore = []\nyanked = \"warn\"\n", + "an LF residue is unaffected" + ); + } + + /// A single-line CRLF residue carries no interior `\r\n` once its own + /// terminator is trimmed, so the newline style has to be read from the + /// residue as it arrived rather than from what survives trimming. + #[test] + fn a_single_line_crlf_residue_keeps_its_terminator() { + assert_eq!(tidy_residue("\r\nignore = []\r\n", "\n"), "ignore = []\r\n"); + } + + /// A residue with no line ending needs a terminator matching the host. + #[test] + fn an_unterminated_residue_uses_the_host_line_ending() { + for newline in ["\n", "\r\n"] { + assert_eq!(tidy_residue("ignore = []", newline), format!("ignore = []{newline}")); + } + } + + /// 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. + /// + /// Kept, here, means refused: the key belongs to `[workspace]`, and the + /// body's last header is `[workspace.package]`, so there is nowhere after + /// the region that still reads it as a `[workspace]` setting. + #[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 adoption = adopt_unmanaged_toml_tables(text, body, SYN); + + assert_eq!( + adoption, + TomlAdoption::Unrelocatable { + table: "workspace".to_owned(), + tail_table: "workspace.package".to_owned(), + }, + "the hand-written dotted key is neither folded into the managed table nor relocated" + ); + } + + /// 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}"); + } } diff --git a/crates/cargo-anvil/src/run.rs b/crates/cargo-anvil/src/run.rs index 7a086bb16..328d3d857 100644 --- a/crates/cargo-anvil/src/run.rs +++ b/crates/cargo-anvil/src/run.rs @@ -20,13 +20,13 @@ 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, TomlRefusal, 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::manifest::{Manifest, RegionKey}; use crate::plan::{Plan, PlanItem, Target}; #[cfg(test)] use crate::region::upsert_region; -use crate::region::{CommentSyntax, RegionPlacement, find_region, remove_region, upsert_region_with_placement}; +use crate::region::{CommentSyntax, RegionPlacement, find_region, managed_region_ids, remove_region, upsert_region_with_placement}; use crate::workspace::{self, Workspace}; /// Outcome of an `update` invocation. @@ -156,6 +156,28 @@ fn enforce_single_tool_guard(catalog: &Catalog, args: &Cli, manifest: &Manifest) /// against the discovered workspace (see [`push_region`]). Every path is /// resolved to its on-disk casing so anvil follows whatever a repo already /// uses (e.g. `justfile` vs `Justfile`). +/// Every `(host, region id)` this pass declares, resolved to the casing on +/// disk. +/// +/// Computed before anything is planned, because the validity check each region +/// runs has to know which of the regions already in its host are on their way +/// out — and removals are not planned until every region has been visited. +fn live_region_keys(repo_root: &Path, workspace: &Workspace, catalog: &Catalog) -> BTreeSet<(String, String)> { + catalog + .artifacts() + .iter() + .filter_map(|artifact| match artifact { + Artifact::Region(spec) => Some(spec), + Artifact::OwnedFile(_) => None, + }) + .flat_map(|spec| { + region_host_paths(workspace, spec) + .into_iter() + .map(|host| (resolve_existing_case_insensitive(repo_root, host), spec.id.as_str().to_owned())) + }) + .collect() +} + fn build_plan( repo_root: &Path, workspace: &Workspace, @@ -167,7 +189,10 @@ fn build_plan( let mut hosts = HostTextCache::default(); // Hosts already reported as unsafe to compose. Every region targeting one // hits the same fault, and four copies of one message is noise. - let mut composed = ComposedHosts::default(); + let mut composed = ComposedHosts { + live: live_region_keys(repo_root, workspace, catalog), + ..ComposedHosts::default() + }; for artifact in catalog.artifacts() { match artifact { @@ -297,8 +322,7 @@ impl HostTextCache { } } -/// Dispatch one managed-region artifact into the plan, expanding its host -/// selector against the discovered workspace. +/// The host paths one region spec targets in this workspace. /// /// - [`HostSelector::Path`] — a single literal host. /// - [`HostSelector::EachMemberManifest`] — one host per workspace member (no @@ -306,6 +330,34 @@ impl HostTextCache { /// - [`HostSelector::WorkspaceCargoToml`] / [`HostSelector::SingleCrateCargoToml`] /// — the root `Cargo.toml`, gated on whether it declares a `[workspace]` /// table. +/// +/// Shared with the live-key set [`build_plan`] computes up front, so the two +/// cannot drift: a region skipped here because the workspace has the other +/// shape must not be counted as live, or the pass would treat the region it is +/// about to retire as one that is staying. +fn region_host_paths<'a>(workspace: &'a Workspace, spec: &'a RegionSpec) -> Vec<&'a str> { + match &spec.host { + HostSelector::Path(path) => vec![path.as_str()], + HostSelector::WorkspaceCargoToml => { + if workspace.has_workspace_table { + vec!["Cargo.toml"] + } else { + Vec::new() + } + } + HostSelector::SingleCrateCargoToml => { + if workspace.has_workspace_table { + Vec::new() + } else { + vec!["Cargo.toml"] + } + } + HostSelector::EachMemberManifest => workspace.members.iter().map(|member| member.manifest_relpath.as_str()).collect(), + } +} + +/// Dispatch one managed-region artifact into the plan, expanding its host +/// selector against the discovered workspace. fn push_region( repo_root: &Path, workspace: &Workspace, @@ -315,25 +367,8 @@ fn push_region( composed: &mut ComposedHosts, spec: &RegionSpec, ) -> Result<(), AppError> { - match &spec.host { - HostSelector::Path(path) => { - push_region_at(repo_root, manifest, plan, hosts, composed, path, spec)?; - } - HostSelector::WorkspaceCargoToml => { - if workspace.has_workspace_table { - push_region_at(repo_root, manifest, plan, hosts, composed, "Cargo.toml", spec)?; - } - } - HostSelector::SingleCrateCargoToml => { - if !workspace.has_workspace_table { - push_region_at(repo_root, manifest, plan, hosts, composed, "Cargo.toml", spec)?; - } - } - HostSelector::EachMemberManifest => { - for member in &workspace.members { - push_region_at(repo_root, manifest, plan, hosts, composed, &member.manifest_relpath, spec)?; - } - } + for host in region_host_paths(workspace, spec) { + push_region_at(repo_root, manifest, plan, hosts, composed, host, spec)?; } Ok(()) } @@ -361,39 +396,7 @@ fn push_region_at( if let Some(declared) = composed_host && !composed.states.contains_key(&host) { - let state = match hosts.get_or_read(repo_root, &host)? { - Some(text) => composed_host_state(declared.order, &host, &text, manifest), - // Nothing on disk. The scaffold becomes the base the first region - // splices into, carrying the parts of the file that cannot live - // inside a region -- the `# syntax=` parser directive above all. It - // is written once and never reconciled; everything outside the - // sentinels is the repository's from then on. - None => ComposedHostState::SeedFromScaffold, - }; - // Resolved through a case variant. Case-insensitive resolution is right - // for an ordinary host, whose consumers open it by whatever name it - // has, but a composed host is read by something that requires the - // declared spelling: the container driver refuses any other, because - // `BuildKit` derives the ignore file's name from the Dockerfile's and - // there is no flag to point it elsewhere. Keeping the file up to date - // would leave the two halves disagreeing about one on-disk state, with - // the generator reporting the tree in sync while every recipe that uses - // it exits 1. The generator is the component that just wrote the file, - // so it is the one positioned to say so. - // - // A content state that already refuses keeps its own diagnosis: it - // describes the deeper problem, and its recovery -- move the file - // aside, restore the regions -- resolves the spelling along the way, - // whereas renaming first would only surface the same refusal again. - let state = if host == declared.path || matches!(state, ComposedHostState::Unsafe(_)) { - state - } else { - ComposedHostState::Unsafe(format!( - "it must be named exactly `{}`, and the recipes that consume it refuse any other \ - spelling, so anvil would be maintaining a file nothing can use. Rename it", - declared.path - )) - }; + let state = classify_composed_host(repo_root, manifest, hosts, declared, &host)?; if matches!(state, ComposedHostState::SeedFromScaffold) { hosts.set(&host, declared.scaffold.to_owned()); } @@ -461,17 +464,40 @@ 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, + }; + // Writing 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. + // + // The check judges the host as this pass will leave it, with only the + // regions being removed blanked out. That is what lets it see a sibling + // region of the catalog declaring the same table -- masking every other + // region instead hid each sibling from the other, and the two composed into + // a duplicate header that nothing refused. + let retiring = current + .as_deref() + .map(|text| composed.retiring_regions(manifest, &host, text, spec.syntax)) + .unwrap_or_default(); + match toml_introduction_refusal(current.as_deref(), request, &retiring) { + Some(TomlRefusal::Host(reason)) => { + refuse_region(plan, host, spec.id.as_str(), &reason); + return Ok(()); + } + Some(TomlRefusal::Sibling(reason)) => { + refuse_sibling_region(plan, host, spec.id.as_str(), &reason); + return Ok(()); + } + None => {} + } + 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 +511,91 @@ fn push_region_at( Ok(()) } +/// Classify a composed host once per pass, before any region touches it. +/// +/// Split out of `push_region_at` because it answers a different question: +/// whether the file on disk is the shape a composed host must be, independent +/// of which region is being planned. Later regions targeting the same host see +/// text this pass has already spliced, which is partially composed by +/// construction, so the answer is computed once and cached. +fn classify_composed_host( + repo_root: &Path, + manifest: &Manifest, + hosts: &mut HostTextCache, + declared: ComposedHost, + host: &str, +) -> Result { + let state = match hosts.get_or_read(repo_root, host)? { + Some(text) => composed_host_state(declared.order, host, &text, manifest), + // Nothing on disk. The scaffold becomes the base the first region + // splices into, carrying the parts of the file that cannot live + // inside a region -- the `# syntax=` parser directive above all. It + // is written once and never reconciled; everything outside the + // sentinels is the repository's from then on. + None => ComposedHostState::SeedFromScaffold, + }; + // Resolved through a case variant. Case-insensitive resolution is right + // for an ordinary host, whose consumers open it by whatever name it + // has, but a composed host is read by something that requires the + // declared spelling: the container driver refuses any other, because + // `BuildKit` derives the ignore file's name from the Dockerfile's and + // there is no flag to point it elsewhere. Keeping the file up to date + // would leave the two halves disagreeing about one on-disk state, with + // the generator reporting the tree in sync while every recipe that uses + // it exits 1. The generator is the component that just wrote the file, + // so it is the one positioned to say so. + // + // A content state that already refuses keeps its own diagnosis: it + // describes the deeper problem, and its recovery -- move the file + // aside, restore the regions -- resolves the spelling along the way, + // whereas renaming first would only surface the same refusal again. + if host == declared.path || matches!(state, ComposedHostState::Unsafe(_)) { + Ok(state) + } else { + Ok(ComposedHostState::Unsafe(format!( + "it must be named exactly `{}`, and the recipes that consume it refuse any other \ + spelling, so anvil would be maintaining a file nothing can use. Rename it", + declared.path + ))) + } +} + +/// 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) { + // Some reasons are whole sentences and some are a parser's error text, so + // the sentence break is supplied only when the reason has not already + // written one. + let stop = if reason.trim_end().ends_with('.') { "" } else { "." }; + plan.refusal(format!( + "Refused to manage {host} [{id}]: {reason}{stop} This region was left unchanged; other regions in the same \ + file and other artifacts may still be updated. Reconcile the hand-written table with the managed \ + one before retrying." + )); + plan.push(PlanItem::noop(Target::Region { host, id: id.to_owned() }, Decision::LeaveAlone)); +} + +/// Record that two of anvil's own regions compose into a file TOML cannot read. +/// +/// Deliberately not [`refuse_region`]: every other refusal ends by asking the +/// user to reconcile a hand-written table, and here there isn't one. Both +/// regions are anvil's own, so nothing the user can do to the host resolves it +/// — sending them to reconcile a table they never wrote would be a worse +/// outcome than saying plainly that this is a defect to report. +fn refuse_sibling_region(plan: &mut Plan, host: String, id: &str, reason: &str) { + let stop = if reason.trim_end().ends_with('.') { "" } else { "." }; + plan.refusal(format!( + "Refused to manage {host} [{id}]: {reason}{stop} Both are anvil's own regions, so {host} cannot be edited \ + to fix this — please report it. This region was left unchanged; other regions in the same file and other \ + artifacts may still be updated." + )); + 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 @@ -587,6 +698,44 @@ struct ComposedHosts { /// Hosts whose refusal has already been reported, so one fault produces one /// diagnostic rather than one per region. reported: BTreeSet, + /// Every `(host, region id)` this pass declares. A managed region found in + /// a host that is absent from this set is an orphan: the pass may be about + /// to remove it, in which case the tables it declares must not be held + /// against the region being written. + live: BTreeSet<(String, String)>, +} + +impl ComposedHosts { + /// The managed regions of `host_text` this pass will actually remove. + /// + /// A region is retiring only if the catalog no longer declares it *and* the + /// removal decision is to remove it: a customized orphan is kept, stays in + /// the file, and so still owns the tables it declares. Getting that wrong + /// in either direction is a real fault — treating a kept orphan as gone + /// writes a duplicate header, and treating a removed one as staying refuses + /// a migration that is about to become valid. + fn retiring_regions(&self, manifest: &Manifest, host_relpath: &str, host_text: &str, syntax: CommentSyntax) -> BTreeSet { + managed_region_ids(host_text, syntax) + .into_iter() + .filter(|id| !self.live.contains(&(host_relpath.to_owned(), id.clone()))) + .filter(|id| { + let key = RegionKey { + host: host_relpath.to_owned(), + id: id.clone(), + }; + let Some(last) = manifest.regions.get(&key) else { + // Never recorded, so anvil does not own it and will not + // remove it, whatever the sentinels say. + return false; + }; + let body = find_region(host_text, id, syntax) + .ok() + .flatten() + .map(|region| checksum_str(region.body_str())); + matches!(decide_removal(last, body.as_deref()), RemovalDecision::Remove) + }) + .collect() + } } /// Classify a composed host before anything is written to it. @@ -896,6 +1045,75 @@ mod tests { fs::write(path, contents).unwrap(); } + /// The refusal diagnostic joins a reason to a fixed remedy, and the two + /// classes of reason punctuate themselves differently: adoption writes + /// whole sentences, while the parser backstop appends `toml_edit`'s error + /// text, which does not end in a full stop. Supplying the break + /// unconditionally produced `TOML rejects.. This region`, and omitting it + /// unconditionally would run the parser's message straight into the remedy. + mod refuse_region { + use super::*; + + fn refusal_for(reason: &str) -> String { + let mut plan = Plan::default(); + super::super::refuse_region(&mut plan, "deny.toml".to_owned(), "anvil-deny-advisories", reason); + plan.refusals().first().expect("a refusal is recorded").clone() + } + + #[test] + fn a_reason_that_ends_a_sentence_is_not_given_a_second_full_stop() { + let refusal = refusal_for("keeping both would repeat the key, which TOML rejects."); + + assert!( + refusal.contains("which TOML rejects. This region was left unchanged"), + "one full stop, one space: {refusal}" + ); + assert!(!refusal.contains(".."), "no doubled full stop: {refusal}"); + } + + #[test] + fn a_reason_without_a_full_stop_is_given_one() { + let refusal = refusal_for("splicing the region would leave deny.toml unparsable as TOML: expected `]`"); + + assert!( + refusal.contains("expected `]`. This region was left unchanged"), + "the reason is closed before the remedy begins: {refusal}" + ); + } + + /// Trailing whitespace on the reason must not defeat the check: the + /// break is decided by the last non-space character, and the reason is + /// still emitted exactly as it arrived. + #[test] + fn a_trailing_space_does_not_hide_the_full_stop() { + let refusal = refusal_for("which TOML rejects. "); + + assert!(!refusal.contains(".."), "no doubled full stop: {refusal}"); + assert!( + refusal.contains("which TOML rejects. This region was left unchanged"), + "the reason's own trailing space is preserved: {refusal}" + ); + } + + /// The plan still accounts for the region it refused, as a no-op — + /// otherwise the summary would simply not mention it. + #[test] + fn the_refused_region_is_still_planned_as_a_no_op() { + let mut plan = Plan::default(); + super::super::refuse_region(&mut plan, "deny.toml".to_owned(), "anvil-deny-advisories", "because."); + + let item = plan.items().first().expect("the region is planned"); + assert_eq!(item.decision, Decision::LeaveAlone); + assert_eq!( + item.target, + Target::Region { + host: "deny.toml".to_owned(), + id: "anvil-deny-advisories".to_owned(), + } + ); + } + } + /// `composed_placement` decides where an *absent* region lands in a host /// whose order is semantic. Every branch matters: getting it wrong puts a /// newly added region after ones it must precede, which for a Dockerfile @@ -2027,6 +2245,24 @@ mod tests { /// A catalog with two managed regions targeting the same host file — /// the shape that `deny.toml`'s per-section split uses. Built on the /// `anvil` identity so the single-tool guard stays satisfied. + /// One region on one host, for staging the state a later catalog grows out + /// of. + fn one_region_catalog(host: &str, id: &str, body: &str) -> Catalog { + use crate::catalog::CliMeta; + use crate::catalog::artifact::RegionId; + + let id: &'static str = Box::leak(id.to_owned().into_boxed_str()); + Catalog::builder(CliMeta::new("anvil")) + .with_artifact(Artifact::region(RegionSpec { + host: HostSelector::Path(host.to_owned()), + id: RegionId::new(id), + body: body.to_owned(), + syntax: CommentSyntax::Hash, + })) + .build() + .unwrap() + } + fn two_region_catalog(host: &str, id_a: &str, body_a: &str, id_b: &str, body_b: &str) -> Catalog { use crate::catalog::CliMeta; use crate::catalog::artifact::RegionId; @@ -2080,6 +2316,141 @@ mod tests { assert!(!second.plan.has_changes(), "second run should be idempotent"); } + /// Two catalog regions on one host that declare the same table compose into + /// a file with two `[licenses]` headers, which TOML rejects. Neither region + /// could see the problem while the backstop masked every *other* managed + /// region before it checked: each sibling was invisible to the other, and + /// both planned a `Write` that left `shared.toml` unreadable. Only the + /// regions this pass *removes* are masked now, so the second region sees + /// the first. + /// + /// Both regions are anvil's own, so there is no edit to `shared.toml` that + /// resolves it — the diagnostic says so and asks for a report instead of + /// sending the reader to reconcile a table they did not write. The first + /// region still writes: refusing is per region, and one of the two is + /// legitimate. + #[cfg_attr(miri, ignore = "uses filesystem; miri isolation forbids it")] + #[test] + fn two_regions_claiming_one_table_refuse_the_second() { + let tmp = empty_workspace(); + let catalog = two_region_catalog( + "shared.toml", + "anvil-sec-a", + "[licenses]\nallow = [\"MIT\"]\n", + "anvil-sec-b", + "[licenses]\nconfidence-threshold = 0.9\n", + ); + + let outcome = run_update(&catalog, &local_only(), tmp.path()).unwrap(); + + let shared = fs::read_to_string(tmp.path().join("shared.toml")).unwrap(); + shared + .parse::() + .unwrap_or_else(|error| panic!("the host must stay readable: {error}\n---\n{shared}\n---")); + assert_eq!(shared.matches("[licenses]").count(), 1, "no duplicate header:\n{shared}"); + assert!(shared.contains("anvil-sec-a"), "the first claim is honored:\n{shared}"); + assert!(!shared.contains("anvil-sec-b"), "the colliding sibling is not written:\n{shared}"); + + let refusal = outcome + .plan + .refusals() + .iter() + .find(|reason| reason.contains("anvil-sec-b")) + .unwrap_or_else(|| panic!("the collision is reported; got {:#?}", outcome.plan.refusals())); + assert!(refusal.contains("[licenses]"), "it names the table: {refusal}"); + assert!(refusal.contains("anvil-sec-a"), "and the sibling holding it: {refusal}"); + assert!( + refusal.contains("please report it"), + "and asks for a report, since no edit to the host fixes it: {refusal}" + ); + assert!( + !refusal.contains("Reconcile the hand-written table"), + "it must not send the reader to reconcile a table they did not write: {refusal}" + ); + assert_eq!( + outcome.plan.dry_run_exit_code(), + 1, + "and a catalog defect fails the drift gate rather than passing quietly" + ); + } + + /// A sibling that is *already on disk* is the harder half of the same + /// fault, and the claim registry got it exactly backwards. `anvil-sec-b` + /// exists with `[licenses]`; a later catalog adds `anvil-sec-a` ahead of it + /// declaring the same table. Whichever region claimed first won, so A wrote + /// and B was refused — but refusing B leaves B's existing region **in the + /// file**, and the host ends up carrying two `[licenses]` headers, which is + /// the very outcome the backstop exists to prevent. + /// + /// Judging the host as this pass will leave it inverts that: B is staying, + /// so A is the one refused, and the file on disk stays readable. + #[cfg_attr(miri, ignore = "uses filesystem; miri isolation forbids it")] + #[test] + fn a_region_added_ahead_of_an_existing_sibling_does_not_break_the_host() { + let tmp = empty_workspace(); + let threshold = "[licenses]\nconfidence-threshold = 0.9\n"; + let existing = one_region_catalog("shared.toml", "anvil-sec-b", threshold); + run_update(&existing, &local_only(), tmp.path()).unwrap(); + + let grown = two_region_catalog( + "shared.toml", + "anvil-sec-a", + "[licenses]\nallow = [\"MIT\"]\n", + "anvil-sec-b", + threshold, + ); + let outcome = run_update(&grown, &local_only(), tmp.path()).unwrap(); + + let shared = fs::read_to_string(tmp.path().join("shared.toml")).unwrap(); + shared + .parse::() + .unwrap_or_else(|error| panic!("the host must stay readable: {error}\n---\n{shared}\n---")); + assert_eq!(shared.matches("[licenses]").count(), 1, "no duplicate header:\n{shared}"); + assert!(shared.contains("anvil-sec-b"), "the region already on disk is kept:\n{shared}"); + assert!( + !shared.contains("anvil-sec-a"), + "the region that would collide is not written:\n{shared}" + ); + + let refusal = outcome + .plan + .refusals() + .iter() + .find(|reason| reason.contains("anvil-sec-a")) + .unwrap_or_else(|| panic!("the collision is reported; got {:#?}", outcome.plan.refusals())); + assert!(refusal.contains("please report it"), "it asks for a report: {refusal}"); + } + + /// A dotted assignment declares its table exactly as a header does, so a + /// region writing `lints.rust.* = ...` and a sibling writing `[lints]` + /// compose into a file TOML rejects. The claim registry enumerated headers + /// only, so the dotted side claimed nothing and both regions passed. The + /// parser has no such blind spot. + #[cfg_attr(miri, ignore = "uses filesystem; miri isolation forbids it")] + #[test] + fn a_dotted_region_and_a_headed_sibling_do_not_compose_into_a_broken_host() { + let tmp = empty_workspace(); + let catalog = two_region_catalog( + "shared.toml", + "anvil-sec-a", + "lints.rust.unsafe_code = \"deny\"\n", + "anvil-sec-b", + "[lints]\nworkspace = true\n", + ); + + let outcome = run_update(&catalog, &local_only(), tmp.path()).unwrap(); + + let shared = fs::read_to_string(tmp.path().join("shared.toml")).unwrap(); + shared + .parse::() + .unwrap_or_else(|error| panic!("the host must stay readable: {error}\n---\n{shared}\n---")); + assert!( + outcome.plan.refusals().iter().any(|reason| reason.contains("anvil-sec-b")), + "the collision is reported; got {:#?}", + outcome.plan.refusals() + ); + } + /// Splitting one region into several on the same host (the `deny.toml` /// migration): the old combined region is removed while the new /// per-section regions are written, all in one host. The removal must diff --git a/crates/cargo-anvil/tests/fixtures.rs b/crates/cargo-anvil/tests/fixtures.rs index 088c56aed..f832db206 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,76 @@ 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 +/// hand-written value is preserved, and other regions in the same host can +/// still be written. +#[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() + ); + assert!( + outcome.plan.refusals().iter().any(|reason| { + reason.contains("deny.toml [anvil-deny-advisories]") + && reason.contains("This region was left unchanged; other regions in the same file") + && reason.contains("and other artifacts may still be updated.") + && reason.contains("Reconcile the hand-written table with the managed one before retrying.") + && !reason.contains("empty the region") + }), + "the refusal must not claim that the whole host was left unchanged; 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" + ); + + let reconciled = after.replace("yanked = \"warn\"", "yanked = \"deny\""); + std::fs::write(tmp.path().join("deny.toml"), reconciled).unwrap(); + let retried = run(&tmp); + assert!(retried.plan.refusals().is_empty(), "reconciling the conflict clears the refusal"); + assert_eq!(region_decision(&retried, "deny.toml", "anvil-deny-advisories"), Decision::Write); + let adopted = read_parsing_toml(&tmp, "deny.toml"); + assert!(adopted.contains("# >>> anvil-managed: anvil-deny-advisories")); + assert_eq!(adopted.matches("[advisories]").count(), 1); + assert!(adopted.contains("yanked = \"deny\"")); +} + /// `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 +278,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 +288,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" diff --git a/crates/cargo-anvil/tests/snapshots/toml_adoption__adopts_a_hand_written_table.snap b/crates/cargo-anvil/tests/snapshots/toml_adoption__adopts_a_hand_written_table.snap new file mode 100644 index 000000000..2db008c97 --- /dev/null +++ b/crates/cargo-anvil/tests/snapshots/toml_adoption__adopts_a_hand_written_table.snap @@ -0,0 +1,59 @@ +--- +source: crates/cargo-anvil/tests/toml_adoption.rs +expression: "report(before, &tmp, \"deny.toml\", &outcome)" +--- +--- deny.toml, as the user wrote it --- +# We accept this one until upstream ships a fix. +[advisories] +ignore = ["RUSTSEC-9999-0001"] + +--- decisions --- +anvil-deny-advisories: Write +anvil-deny-bans: Write +anvil-deny-licenses: Write +anvil-deny-sources: Write + +--- refusals --- +(none) + +--- deny.toml, as anvil left it --- +# We accept this one until upstream ships a fix. + +# >>> anvil-managed: anvil-deny-advisories +[advisories] +yanked = "deny" +# Scope of unmaintained-crate checks: "all" surfaces transitive +# dependencies too; tighten to "workspace" if the noise is high. +unmaintained = "all" +# <<< anvil-managed: anvil-deny-advisories +ignore = ["RUSTSEC-9999-0001"] + +# >>> anvil-managed: anvil-deny-licenses +[licenses] +allow = [ + "MIT", + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "BSD-2-Clause", + "BSD-3-Clause", + "ISC", + "MPL-2.0", + "Unicode-DFS-2016", + "Unicode-3.0", + "Zlib", +] +confidence-threshold = 0.93 +# <<< anvil-managed: anvil-deny-licenses + +# >>> anvil-managed: anvil-deny-bans +[bans] +multiple-versions = "warn" +wildcards = "deny" +allow-wildcard-paths = true +# <<< anvil-managed: anvil-deny-bans + +# >>> anvil-managed: anvil-deny-sources +[sources] +unknown-registry = "deny" +unknown-git = "deny" +# <<< anvil-managed: anvil-deny-sources diff --git a/crates/cargo-anvil/tests/snapshots/toml_adoption__keeps_an_unmanaged_dotted_lint.snap b/crates/cargo-anvil/tests/snapshots/toml_adoption__keeps_an_unmanaged_dotted_lint.snap new file mode 100644 index 000000000..4df862bdf --- /dev/null +++ b/crates/cargo-anvil/tests/snapshots/toml_adoption__keeps_an_unmanaged_dotted_lint.snap @@ -0,0 +1,115 @@ +--- +source: crates/cargo-anvil/tests/toml_adoption.rs +expression: "report(before, &tmp, \"Cargo.toml\", &outcome)" +--- +--- Cargo.toml, as the user wrote it --- +[workspace] +resolver = "2" +members = ["crates/*"] + +[workspace.lints] +# House rule, not in anvil's catalog. +rust.a_custom_lint = "warn" +rust.unsafe_op_in_unsafe_fn = "warn" + +--- decisions --- +anvil-workspace-lints: Write + +--- refusals --- +(none) + +--- Cargo.toml, as anvil left it --- +[workspace] +resolver = "2" +members = ["crates/*"] + +# >>> anvil-managed: anvil-workspace-lints +[workspace.lints] +# Catalog of opinionated lints, in dotted-key form so users can extend the +# same scope (`[workspace.lints]` or `[lints]`) outside the sentinels. +# The host-specific table header (`[workspace.lints]` or `[lints]`) is +# prepended by cargo-anvil based on whether the manifest is a workspace +# root or a single-crate Cargo.toml. + +# --- rust ------------------------------------------------------------------ +rust.ambiguous_negative_literals = "warn" +rust.missing_debug_implementations = "warn" +rust.redundant_imports = "warn" +rust.redundant_lifetimes = "warn" +rust.trivial_numeric_casts = "warn" +rust.unsafe_op_in_unsafe_fn = "warn" +rust.unused_lifetimes = "warn" +# `unexpected_cfgs` is on-by-default at warn since Rust 1.80; combined +# with the catalog's `-D warnings` cloud-workflow policy, any custom cfg name +# becomes a hard build failure. Pre-declare the cfgs that +# `cargo llvm-cov` sets so the recommended coverage-exclusion pattern +# `#[cfg_attr(coverage_nightly, coverage(off))]` works out of the box. +# Adopters who need additional cfg names take ownership of this one +# line (edit the check-cfg array); anvil's drift detector will +# emit a `.anvil-proposed` sibling on future catalog bumps so the +# customization is preserved. +rust.unexpected_cfgs = { level = "warn", check-cfg = [ + 'cfg(coverage,coverage_nightly)', + 'cfg(loom)', + 'cfg(miri_race_coverage)', + 'cfg(miri_strict_provenance)', + 'cfg(miri_tree_borrows)', +] } + +# --- rustdoc --------------------------------------------------------------- +rustdoc.broken_intra_doc_links = "warn" +rustdoc.missing_crate_level_docs = "warn" +rustdoc.unescaped_backticks = "warn" + +# --- clippy: category gates (priority -1 so per-lint allows can override) -- +clippy.cargo = { level = "warn", priority = -1 } +clippy.complexity = { level = "warn", priority = -1 } +clippy.correctness = { level = "warn", priority = -1 } +clippy.nursery = { level = "warn", priority = -1 } +clippy.pedantic = { level = "warn", priority = -1 } +clippy.perf = { level = "warn", priority = -1 } +clippy.style = { level = "warn", priority = -1 } +clippy.suspicious = { level = "warn", priority = -1 } + +# --- clippy: opinionated additions ----------------------------------------- +# Two-repo consensus (oxidizer + oxidizer-github). Restriction-group +# lints that catch real code-smell cases. Adding a workspace-wide lint +# means adopters can only opt out per-crate or by taking ownership of +# this region; only enable when the consensus is strong enough to +# justify that cost. +clippy.allow_attributes = "warn" +clippy.allow_attributes_without_reason = "warn" +clippy.as_pointer_underscore = "warn" +clippy.assertions_on_result_states = "warn" +clippy.clone_on_ref_ptr = "warn" +clippy.deref_by_slicing = "warn" +clippy.disallowed_script_idents = "warn" +clippy.empty_drop = "warn" +clippy.empty_enum_variants_with_brackets = "warn" +clippy.fn_to_numeric_cast_any = "warn" +clippy.if_then_some_else_none = "warn" +clippy.map_err_ignore = "warn" +clippy.multiple_unsafe_ops_per_block = "warn" +clippy.redundant_type_annotations = "warn" +clippy.renamed_function_params = "warn" +clippy.semicolon_outside_block = "warn" +clippy.undocumented_unsafe_blocks = "warn" +clippy.unnecessary_safety_comment = "warn" +clippy.unnecessary_safety_doc = "warn" +clippy.unneeded_field_pattern = "warn" +clippy.unused_result_ok = "warn" +clippy.unwrap_used = "warn" + +# --- clippy: opinionated suppressions of category-enabled lints ------------ +clippy.missing_const_for_fn = "allow" +clippy.multiple_crate_versions = "allow" +clippy.option_if_let_else = "allow" +clippy.redundant_pub_crate = "allow" +clippy.should_panic_without_expect = "allow" +clippy.significant_drop_tightening = "allow" +# Blocked by Clippy bug: https://github.com/rust-lang/rust-clippy/issues/15036 +clippy.wildcard_imports = "allow" + +# <<< anvil-managed: anvil-workspace-lints +# House rule, not in anvil's catalog. +rust.a_custom_lint = "warn" diff --git a/crates/cargo-anvil/tests/snapshots/toml_adoption__preserves_a_crlf_host.snap b/crates/cargo-anvil/tests/snapshots/toml_adoption__preserves_a_crlf_host.snap new file mode 100644 index 000000000..1b56dcef4 --- /dev/null +++ b/crates/cargo-anvil/tests/snapshots/toml_adoption__preserves_a_crlf_host.snap @@ -0,0 +1,59 @@ +--- +source: crates/cargo-anvil/tests/toml_adoption.rs +expression: show_line_endings(&rendered) +--- +--- deny.toml, as the user wrote it --- +# We accept this one until upstream ships a fix. +[advisories] +ignore = ["RUSTSEC-9999-0001"] + +--- decisions --- +anvil-deny-advisories: Write +anvil-deny-bans: Write +anvil-deny-licenses: Write +anvil-deny-sources: Write + +--- refusals --- +(none) + +--- deny.toml, as anvil left it --- +# We accept this one until upstream ships a fix. + +# >>> anvil-managed: anvil-deny-advisories +[advisories] +yanked = "deny" +# Scope of unmaintained-crate checks: "all" surfaces transitive +# dependencies too; tighten to "workspace" if the noise is high. +unmaintained = "all" +# <<< anvil-managed: anvil-deny-advisories +ignore = ["RUSTSEC-9999-0001"] + +# >>> anvil-managed: anvil-deny-licenses +[licenses] +allow = [ + "MIT", + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "BSD-2-Clause", + "BSD-3-Clause", + "ISC", + "MPL-2.0", + "Unicode-DFS-2016", + "Unicode-3.0", + "Zlib", +] +confidence-threshold = 0.93 +# <<< anvil-managed: anvil-deny-licenses + +# >>> anvil-managed: anvil-deny-bans +[bans] +multiple-versions = "warn" +wildcards = "deny" +allow-wildcard-paths = true +# <<< anvil-managed: anvil-deny-bans + +# >>> anvil-managed: anvil-deny-sources +[sources] +unknown-registry = "deny" +unknown-git = "deny" +# <<< anvil-managed: anvil-deny-sources diff --git a/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_a_key_both_sides_declare.snap b/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_a_key_both_sides_declare.snap new file mode 100644 index 000000000..a72752bc3 --- /dev/null +++ b/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_a_key_both_sides_declare.snap @@ -0,0 +1,50 @@ +--- +source: crates/cargo-anvil/tests/toml_adoption.rs +expression: "report(before, &tmp, \"deny.toml\", &outcome)" +--- +--- deny.toml, as the user wrote it --- +[advisories] +yanked = "warn" + +--- decisions --- +anvil-deny-advisories: LeaveAlone +anvil-deny-bans: Write +anvil-deny-licenses: Write +anvil-deny-sources: Write + +--- refusals --- +Refused to manage deny.toml [anvil-deny-advisories]: deny.toml declares `yanked` in `[advisories]` as "warn", but the managed region 'anvil-deny-advisories' declares it as "deny". Adopting the table would discard one of them and keeping both would repeat the key, which TOML rejects. This region was left unchanged; other regions in the same file and other artifacts may still be updated. Reconcile the hand-written table with the managed one before retrying. + +--- deny.toml, as anvil left it --- +[advisories] +yanked = "warn" + +# >>> anvil-managed: anvil-deny-licenses +[licenses] +allow = [ + "MIT", + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "BSD-2-Clause", + "BSD-3-Clause", + "ISC", + "MPL-2.0", + "Unicode-DFS-2016", + "Unicode-3.0", + "Zlib", +] +confidence-threshold = 0.93 +# <<< anvil-managed: anvil-deny-licenses + +# >>> anvil-managed: anvil-deny-bans +[bans] +multiple-versions = "warn" +wildcards = "deny" +allow-wildcard-paths = true +# <<< anvil-managed: anvil-deny-bans + +# >>> anvil-managed: anvil-deny-sources +[sources] +unknown-registry = "deny" +unknown-git = "deny" +# <<< anvil-managed: anvil-deny-sources diff --git a/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_residue_that_cannot_stay_in_its_table.snap b/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_residue_that_cannot_stay_in_its_table.snap new file mode 100644 index 000000000..5fd9ba742 --- /dev/null +++ b/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_residue_that_cannot_stay_in_its_table.snap @@ -0,0 +1,17 @@ +--- +source: crates/cargo-anvil/tests/toml_adoption.rs +expression: "report(before, &tmp, \"spellcheck.toml\", &outcome)" +--- +--- spellcheck.toml, as the user wrote it --- +[Hunspell] +transform_regex = ["^[0-9]+$"] + +--- decisions --- +anvil-spellcheck: LeaveAlone + +--- refusals --- +Refused to manage spellcheck.toml [anvil-spellcheck]: spellcheck.toml declares settings in `[Hunspell]` that the managed region 'anvil-spellcheck' does not, and the region's body ends in `[Hunspell.quirks]`, so re-emitting them after the region would make them settings of `[Hunspell.quirks]` instead. Remove them from `[Hunspell]` and re-run. This region was left unchanged; other regions in the same file and other artifacts may still be updated. Reconcile the hand-written table with the managed one before retrying. + +--- spellcheck.toml, as anvil left it --- +[Hunspell] +transform_regex = ["^[0-9]+$"] diff --git a/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_two_regions_claiming_one_table.snap b/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_two_regions_claiming_one_table.snap new file mode 100644 index 000000000..dafbab352 --- /dev/null +++ b/crates/cargo-anvil/tests/snapshots/toml_adoption__refuses_two_regions_claiming_one_table.snap @@ -0,0 +1,19 @@ +--- +source: crates/cargo-anvil/tests/toml_adoption.rs +expression: "report(\"(anvil creates this file)\\n\", &tmp, \"deny.toml\", &outcome)" +--- +--- deny.toml, as the user wrote it --- +(anvil creates this file) + +--- decisions --- +anvil-licenses-basics: Write +anvil-licenses-threshold: LeaveAlone + +--- refusals --- +Refused to manage deny.toml [anvil-licenses-threshold]: this region declares `[licenses]`, which the managed region 'anvil-licenses-basics' already declares in the same file. Both are anvil's own regions, so deny.toml cannot be edited to fix this — please report it. This region was left unchanged; other regions in the same file and other artifacts may still be updated. + +--- deny.toml, as anvil left it --- +# >>> anvil-managed: anvil-licenses-basics +[licenses] +allow = ["MIT"] +# <<< anvil-managed: anvil-licenses-basics diff --git a/crates/cargo-anvil/tests/toml_adoption.rs b/crates/cargo-anvil/tests/toml_adoption.rs new file mode 100644 index 000000000..b48a31a97 --- /dev/null +++ b/crates/cargo-anvil/tests/toml_adoption.rs @@ -0,0 +1,327 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#![cfg(not(miri))] // miri can't sandbox FS ops these tests do (TempDir, assert_cmd, etc.) +#![allow( + clippy::expect_used, + clippy::unwrap_used, + reason = "panic-on-failure idioms are appropriate in tests" +)] + +//! Snapshot tests for onboarding a TOML host that already declares the +//! region's table by hand. +//! +//! Each test stages one hand-written host, runs `cargo anvil` over it, and +//! snapshots a single report: the host as the user wrote it, the decision +//! anvil reached for every region of that host, any refusal it raised, and the +//! host as anvil left it. The assertions elsewhere pin individual properties — +//! that a file parses, that a key survives, that a refusal names a table. These +//! snapshots pin the thing a reader actually has to judge: what the user's file +//! looks like afterwards. +//! +//! The scenarios are the ones this behaviour was built and reviewed against: +//! adopting a hand-written table (issue #148), refusing a key both sides +//! declare, refusing residue that cannot stay in the table it came from, +//! keeping an unmanaged dotted assignment, and leaving a CRLF host's line +//! endings alone. +//! +//! Reviewed with `cargo insta review`; snapshots live under `tests/snapshots/`. + +use std::fmt::Write as _; +use std::path::Path; + +use cargo_anvil::test_support::{Cli, RunOutcome, Target, run_update}; +use cargo_anvil::{Artifact, Catalog, CliMeta, CommentSyntax, HostSelector, RegionId, RegionSpec}; +use tempfile::TempDir; + +/// A workspace with nothing in it but the manifest anvil needs to find, plus +/// the one hand-written host under test. Everything else in the tree is +/// anvil's own output and is left out of the report. +fn workspace_with(host_relpath: &str, host: &str) -> TempDir { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + write( + &root.join("Cargo.toml"), + "[workspace]\nresolver = \"2\"\nmembers = [\"crates/*\"]\n", + ); + write( + &root.join("crates/alpha/Cargo.toml"), + "[package]\nname = \"alpha\"\nversion = \"0.1.0\"\nedition = \"2024\"\n", + ); + write(&root.join("crates/alpha/src/lib.rs"), ""); + write(&root.join(host_relpath), host); + tmp +} + +fn write(path: &Path, contents: &str) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, contents).unwrap(); +} + +fn run(tmp: &TempDir) -> RunOutcome { + run_update( + &cargo_anvil::Catalog::anvil(), + &Cli { + backends: vec![], + no_backends: true, + dry_run: false, + force: false, + }, + tmp.path(), + ) + .unwrap() +} + +/// Render the one thing worth reviewing: what the user wrote, what anvil +/// decided about it, and what the user is left with. +/// +/// The written host is re-read from disk rather than taken from the plan, so +/// the snapshot shows the bytes that actually landed. It is parsed on the way +/// past: a host anvil cannot read back is the defect this whole path exists to +/// prevent, and a snapshot of a broken file would otherwise just be accepted +/// on review. +fn report(before: &str, tmp: &TempDir, host_relpath: &str, outcome: &RunOutcome) -> String { + let after = std::fs::read_to_string(tmp.path().join(host_relpath)).unwrap(); + if let Err(error) = after.parse::() { + panic!("{host_relpath} is not valid TOML after the run: {error}\n---\n{after}\n---"); + } + + let mut out = String::new(); + writeln!(out, "--- {host_relpath}, as the user wrote it ---").unwrap(); + out.push_str(before); + ensure_trailing_newline(&mut out); + + writeln!(out, "\n--- decisions ---").unwrap(); + let mut regions: Vec<(&String, String)> = outcome + .plan + .items() + .iter() + .filter_map(|item| match &item.target { + Target::Region { host, id } if host == host_relpath => Some((id, format!("{:?}", item.decision))), + _ => None, + }) + .collect(); + regions.sort(); + for (id, decision) in regions { + writeln!(out, "{id}: {decision}").unwrap(); + } + + writeln!(out, "\n--- refusals ---").unwrap(); + let refusals: Vec<&String> = outcome + .plan + .refusals() + .iter() + .filter(|reason| reason.contains(host_relpath)) + .collect(); + if refusals.is_empty() { + out.push_str("(none)\n"); + } else { + for reason in refusals { + writeln!(out, "{reason}").unwrap(); + } + } + + writeln!(out, "\n--- {host_relpath}, as anvil left it ---").unwrap(); + out.push_str(&after); + ensure_trailing_newline(&mut out); + out +} + +fn ensure_trailing_newline(out: &mut String) { + if !out.ends_with('\n') { + out.push('\n'); + } +} + +/// Issue #148, end to end. A repository that already keeps its own +/// `[advisories]` table gets the managed region spliced into that table rather +/// than beside it: one header, the managed keys, and the accepted advisory the +/// managed body says nothing about carried out to just after the region, where +/// TOML still reads it as an `[advisories]` setting. +/// +/// The bug this replaced appended a second `[advisories]` header, which +/// `cargo deny` rejects outright — and wrote it to disk and recorded it in the +/// manifest before anything noticed. +#[test] +fn adopts_a_hand_written_table() { + let before = "\ +# We accept this one until upstream ships a fix. +[advisories] +ignore = [\"RUSTSEC-9999-0001\"] +"; + let tmp = workspace_with("deny.toml", before); + let outcome = run(&tmp); + + insta::assert_snapshot!("adopts_a_hand_written_table", report(before, &tmp, "deny.toml", &outcome)); +} + +/// A key both sides declare, with different values, has no output that keeps +/// both: TOML forbids repeating it inside one table, and picking either value +/// silently discards a decision somebody made. Anvil refuses that one region +/// and leaves the user's value exactly as it was. +/// +/// The refusal is scoped to the region, not to the host or the run — the +/// snapshot shows `anvil-deny-licenses`, `-bans` and `-sources` onboarding into +/// the same file in the same pass. That is what makes refusing an acceptable +/// answer rather than a wall in front of adoption: the user reconciles one +/// table and re-runs. +#[test] +fn refuses_a_key_both_sides_declare() { + let before = "\ +[advisories] +yanked = \"warn\" +"; + let tmp = workspace_with("deny.toml", before); + let outcome = run(&tmp); + + insta::assert_snapshot!("refuses_a_key_both_sides_declare", report(before, &tmp, "deny.toml", &outcome)); +} + +/// Residue is re-emitted after the region's closing sentinel, so TOML reads it +/// as a setting of whichever table the body opens **last**. The shipped +/// spellcheck body opens `[Hunspell]` and then `[Hunspell.quirks]`, so a +/// hand-written `[Hunspell]` key that the body does not declare would come back +/// as `Hunspell.quirks.` — valid TOML that parses cleanly and that +/// cargo-spellcheck never reads. +/// +/// Anvil refuses instead, naming both tables and the edit that clears it. A +/// file that quietly means something else is exactly the failure mode this PR +/// rejects elsewhere, when it disproves issue #148's dotted-key option. +#[test] +fn refuses_residue_that_cannot_stay_in_its_table() { + let before = "\ +[Hunspell] +transform_regex = [\"^[0-9]+$\"] +"; + let tmp = workspace_with("spellcheck.toml", before); + let outcome = run(&tmp); + + insta::assert_snapshot!( + "refuses_residue_that_cannot_stay_in_its_table", + report(before, &tmp, "spellcheck.toml", &outcome) + ); +} + +/// Dotted assignments that share a prefix are one dotted sub-table to the +/// parser, not several independent keys. Locating each leaf by the prefix key +/// gave every leaf but the last an empty slice, so `rust.a_custom_lint` was +/// deleted with its table and re-emitted as nothing — silent data loss in the +/// one direction the design forbids outright. +/// +/// The snapshot shows the house rule surviving as residue while +/// `rust.unsafe_op_in_unsafe_fn`, which the managed body declares identically, +/// is dropped as covered. +#[test] +fn keeps_an_unmanaged_dotted_lint() { + let before = "\ +[workspace] +resolver = \"2\" +members = [\"crates/*\"] + +[workspace.lints] +# House rule, not in anvil's catalog. +rust.a_custom_lint = \"warn\" +rust.unsafe_op_in_unsafe_fn = \"warn\" +"; + let tmp = workspace_with("Cargo.toml", before); + let outcome = run(&tmp); + + insta::assert_snapshot!("keeps_an_unmanaged_dotted_lint", report(before, &tmp, "Cargo.toml", &outcome)); +} + +/// Two regions *of the catalog* on one host that declare the same table. Each +/// is invisible to the other's parser check — the backstop masks every other +/// managed region before it parses — so both used to plan a write and compose a +/// `deny.toml` with two `[licenses]` headers. +/// +/// This refusal reads differently from the others on purpose. Both regions are +/// anvil's own, so there is no edit to the host that resolves it; telling the +/// user to reconcile a table they never wrote would send them chasing nothing. +/// It asks for a bug report instead, and the first region still onboards. +#[test] +fn refuses_two_regions_claiming_one_table() { + let tmp = workspace_with("deny.toml", ""); + std::fs::remove_file(tmp.path().join("deny.toml")).unwrap(); + + let catalog = Catalog::builder(CliMeta::new("anvil")) + .with_artifact(Artifact::region(RegionSpec { + host: HostSelector::Path("deny.toml".to_owned()), + id: RegionId::new("anvil-licenses-basics"), + body: "[licenses]\nallow = [\"MIT\"]\n".to_owned(), + syntax: CommentSyntax::Hash, + })) + .with_artifact(Artifact::region(RegionSpec { + host: HostSelector::Path("deny.toml".to_owned()), + id: RegionId::new("anvil-licenses-threshold"), + body: "[licenses]\nconfidence-threshold = 0.93\n".to_owned(), + syntax: CommentSyntax::Hash, + })) + .build() + .unwrap(); + + let outcome = run_update( + &catalog, + &Cli { + backends: vec![], + no_backends: true, + dry_run: false, + force: false, + }, + tmp.path(), + ) + .unwrap(); + + insta::assert_snapshot!( + "refuses_two_regions_claiming_one_table", + report("(anvil creates this file)\n", &tmp, "deny.toml", &outcome) + ); +} + +/// A CRLF host stays CRLF throughout: carried-over user content, generated +/// bodies, sentinels, and separator lines. The lock's normalized checksums +/// still recognize the generated regions as in sync on the next run. +/// +/// Line endings are shown as markers because that is the whole subject here — +/// a snapshot of the raw bytes would show two files that look identical. +#[test] +fn preserves_a_crlf_host() { + let before = "# We accept this one until upstream ships a fix.\r\n[advisories]\r\nignore = [\"RUSTSEC-9999-0001\"]\r\n"; + let tmp = workspace_with("deny.toml", before); + let outcome = run(&tmp); + + let rendered = report(before, &tmp, "deny.toml", &outcome); + insta::assert_snapshot!("preserves_a_crlf_host", show_line_endings(&rendered)); + let after = std::fs::read_to_string(tmp.path().join("deny.toml")).unwrap(); + assert!(!after.replace("\r\n", "").contains('\n'), "the whole host must stay CRLF"); + + let repeated = run(&tmp); + assert!(repeated.plan.refusals().is_empty()); + for item in repeated.plan.items() { + if matches!(&item.target, Target::Region { host, .. } if host == "deny.toml") { + assert_eq!(item.decision, cargo_anvil::test_support::Decision::InSync); + } + } + assert_eq!(std::fs::read_to_string(tmp.path().join("deny.toml")).unwrap(), after); +} + +/// Make line endings visible so a CRLF/LF difference is reviewable rather than +/// invisible. +fn show_line_endings(text: &str) -> String { + let mut out = String::with_capacity(text.len() * 2); + let mut rest = text; + while let Some(at) = rest.find('\n') { + let (line, tail) = rest.split_at(at); + if let Some(line) = line.strip_suffix('\r') { + out.push_str(line); + out.push_str("\n"); + } else { + out.push_str(line); + out.push_str("\n"); + } + rest = &tail[1..]; + } + out.push_str(rest); + out +}