diff --git a/crates/dependable-core/src/result.rs b/crates/dependable-core/src/result.rs index 11ade39..693f534 100644 --- a/crates/dependable-core/src/result.rs +++ b/crates/dependable-core/src/result.rs @@ -228,6 +228,24 @@ impl DependencyStatus { } } + /// Whether a newer release exists that this dependency could move to. + /// + /// The one predicate behind every "there is something to do here" decision: + /// which rows `fix` plans a rewrite for, and which of the rows it cannot + /// rewrite are worth saying so about. Those two answers must be the same set + /// or the two commands contradict each other, which is exactly the defect + /// this replaced three hand-written copies of the same `matches!` to prevent. + /// + /// [`Vulnerable`](Self::Vulnerable) counts: a vulnerable-but-current + /// dependency is the one most worth upgrading. + #[must_use] + pub fn has_update(&self) -> bool { + matches!( + self, + Self::PatchAvailable | Self::UpdateAvailable | Self::Outdated | Self::Vulnerable + ) + } + /// A stable uppercase token for machine-readable output. #[must_use] pub fn token(&self) -> &'static str { diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index af05511..e18eaac 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -134,15 +134,7 @@ pub struct ManifestCheck { impl ManifestCheck { /// Results that represent an available upgrade (patch/update/outdated/vulnerable). pub fn outdated(&self) -> impl Iterator { - self.results.iter().filter(|r| { - matches!( - r.status, - DependencyStatus::PatchAvailable - | DependencyStatus::UpdateAvailable - | DependencyStatus::Outdated - | DependencyStatus::Vulnerable - ) - }) + self.results.iter().filter(|r| r.status.has_update()) } /// Results with known advisories on the current version. diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index 32f9c22..744563e 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -19,7 +19,7 @@ use std::path::Path; use anyhow::Context; use dependable_fetch::core::{BareVersion, Ecosystem, ManifestKind}; -use dependable_fetch::{CheckResult, DependencyKind, DependencyStatus}; +use dependable_fetch::{CheckResult, DependencyKind}; /// A single applied (or would-be-applied) version change. #[derive(Debug, Clone)] @@ -29,6 +29,159 @@ pub struct FixRecord { pub to: String, } +/// Why `fix` left an update alone that `check` reported. +/// +/// Most of these are [`rewrite_constraint`] refusing to substitute a new version +/// into a constraint. The last three are [`plan_fixes`] itself, which drops a row +/// before the constraint is ever consulted — a pin held back for want of `--all`, +/// a row with no version resolved to write, and a row whose constraint already +/// names the target. Those three were silent, and silence is issue #93's exact +/// symptom: `check` reports an update, `fix` says everything is up to date. An +/// override is the one skip still not on this list, and [`plan_fixes`] says why. +/// +/// Carried out of the planner rather than recomputed, because the answer is only +/// live at the point the guard fires: reconstructing it later would mean a second +/// copy of every guard below, kept in step with this one by hope. It reaches the +/// user as the second half of a `note:` line — see [`DeclineReason::explain`] — +/// so a dependency `check` reports an update for and `fix` leaves alone says so +/// instead of vanishing into "everything is already up to date". +/// +/// `#[non_exhaustive]`: match with a wildcard arm so a new guard is additive. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[non_exhaustive] +pub enum DeclineReason { + /// A comma-separated range: Cargo's `>=1.0, <2.0`. + CommaRange, + /// A space- or `|`-separated range: `>=1.0.0 <2.0.0`, `^1 || ^2`. + MultiClause, + /// An `@` qualifier: a Composer stability flag (`@dev`, `^1.0@beta`) or an + /// npm alias (`npm:pkg@1.0.0`). + Qualifier, + /// A dist-tag or channel name: `latest`, `next`. + DistTag, + /// A wildcard behind an operator (`^1.x`, `=1.*`), whose rewrite would not be + /// a bare version at all. + WildcardOperator, + /// A wildcard in an ecosystem that reads a bare version as one release: + /// npm's `"lodash": "1.x"`. + WildcardPins, + /// A wildcard in an ecosystem that reads a bare version as a minimum: + /// NuGet's `1.*`, Gradle's `1.+`. + WildcardUnbounds, + /// A wildcard whose shape no bare version reproduces even where the bare + /// reading is a caret: `*`, `1.2.*`, `1.+`. + WildcardShape, + /// A constraint whose every written component is zero, where a bare version + /// is read as a caret: Cargo's `0.*`, `0`, `0.0`. The shape is one a caret + /// otherwise reproduces; what does not survive is the *bound*, because a + /// caret is minor-scoped below 1.0.0. + CaretBoundNarrows, + /// A partial version, which is an X-range wherever a bare version is exact: + /// npm's `"react": "16"`. + PartialVersion, + /// A partial version behind a tilde operator, which reads its upper bound off + /// the number of components it was given: `~1`, `~> 1.0`, `~=1.4`. + TildeArity, + /// An exact pin (`=1.0.100`) with no `--all` to authorise moving it. Not a + /// refusal by the constraint's *shape* — the rewrite would succeed — but the + /// author holds the version deliberately, and the action that changes it is a + /// flag rather than a rewrite of the constraint. + Pinned, + /// No version resolved to write: the row has an update and neither + /// `latest_compatible` (the default target) nor `latest_available` (under + /// `--all`) named one. The usual cause is a constraint no published release + /// satisfies, whose newer releases are all beyond it. + NoTarget, + /// The constraint already names the version that would have been written. + /// Reached most sharply by a `Vulnerable` row whose only fixed release is the + /// one already in force: there is an update to report and nothing to write. + AlreadyAtTarget, +} + +impl DeclineReason { + /// The clause that completes a `note:` line, reading on from + /// "… is available, but " — or, for a decline with no version to name, from + /// "an update was reported, but ". [`crate::runner`] picks the opening; every + /// clause here has to read after either. + /// + /// Every reason says what the constraint *is*, not that a rule fired — the + /// point of the note is to let the author decide whether to widen the + /// constraint by hand, and a rule name would not help them do that. + #[must_use] + pub fn explain(self) -> &'static str { + match self { + Self::CommaRange => { + "a comma-separated range has two bounds and one version cannot carry both" + } + Self::MultiClause => { + "a space- or `||`-separated range has more than one clause and one version \ + cannot carry them all" + } + Self::Qualifier => { + "an `@` qualifier — a stability flag or an alias — describes the range, not the \ + version" + } + Self::DistTag => "a dist-tag names a release channel, not a version", + Self::WildcardOperator => { + "an operator in front of a wildcard is a range the new version would not reproduce" + } + Self::WildcardPins => { + "a wildcard already tracks new releases, and a bare version here would pin it to \ + one" + } + Self::WildcardUnbounds => { + "a wildcard already tracks new releases, and a bare version here would drop its \ + upper bound" + } + Self::WildcardShape => { + "a wildcard already tracks new releases, and no bare version covers the same range" + } + Self::CaretBoundNarrows => { + "every component it writes is zero, and below 1.0.0 a caret is minor-scoped, so \ + a concrete release here would sit under the upper bound the constraint already \ + reaches" + } + Self::PartialVersion => { + "a partial version is an X-range that already tracks new releases" + } + Self::TildeArity => { + "a tilde reads its upper bound off the number of components it was given, and a \ + full version here would narrow that bound" + } + Self::Pinned => "the constraint pins one release, and only `--all` moves a pin", + Self::NoTarget => { + "no release the constraint admits was resolved, so there is nothing to write" + } + Self::AlreadyAtTarget => { + "the constraint already names it, and nothing newer satisfies the constraint" + } + } + } +} + +/// An update `check` reports that `fix` will not write. +/// +/// The whole point of recording it: without one, a declined constraint and a +/// dependency with nothing to do are the same empty result, and `fix` answers +/// "everything is already up to date" to a manifest `check` just said had an +/// update waiting. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Declined { + /// The dependency's name. + pub name: String, + /// The constraint left in place, verbatim. + pub constraint: String, + /// The version that would have been written had nothing refused it. + /// + /// [`None`] where there was no such version to name — + /// [`DeclineReason::NoTarget`] is exactly that case. The note then opens "an + /// update was reported, but …" instead of naming a release, because printing + /// an invented or empty version would be worse than printing none. + pub target: Option, + /// Why it was not. + pub reason: DeclineReason, +} + /// A byte-range replacement within one line of the manifest. struct Edit { line: usize, @@ -53,6 +206,9 @@ pub struct PlannedFix { updated: String, /// What changed, for reporting. pub records: Vec, + /// The updates this rewrite declined to make, and why — reported so `check` + /// and `fix` do not appear to contradict each other. + pub declined: Vec, } /// Compute the rewrite for `manifest` without touching it. @@ -76,12 +232,13 @@ pub fn plan(manifest: &Path, results: &[CheckResult], all: bool) -> anyhow::Resu // `detect` does not recognize — is not an error here: the rewrite still runs, // under the reading that declines the most (see [`rewrite_constraint`]). let ecosystem = ManifestKind::detect(manifest).map(ManifestKind::ecosystem); - let (updated, records) = plan_fixes(&content, results, all, ecosystem) + let (updated, records, declined) = plan_fixes(&content, results, all, ecosystem) .with_context(|| format!("rewriting {}", manifest.display()))?; Ok(PlannedFix { path: manifest.to_path_buf(), updated, records, + declined, }) } @@ -136,14 +293,29 @@ pub fn commit(planned: &PlannedFix) -> anyhow::Result<()> { /// [`rewrite_constraint`] explains what turns on it — and `None` means the /// manifest kind was not recognized, which is treated as the most restrictive /// answer rather than as permission. +/// +/// Returns the rewritten content, the changes made, and the changes *not* made: +/// every dependency with an update available that this loop left alone, whether +/// [`rewrite_constraint`] declined it or the loop itself dropped it — a pin +/// without `--all`, a row with no version resolved to write, or a constraint +/// already naming the target. The third list exists because it cannot be +/// recovered afterwards — the caller would have to redo the rewritability, +/// pinning, and target selection above *and* every guard inside +/// [`rewrite_constraint`] to learn what this loop already knew and threw away. +/// +/// The plan set and the declined set together account for every `has_update()` +/// row that is rewritable and not an override. That is the invariant the summary +/// line in [`crate::runner`] rests on: "everything is already up to date" is only +/// printable when both are empty. fn plan_fixes( content: &str, results: &[CheckResult], all: bool, ecosystem: Option, -) -> anyhow::Result<(String, Vec)> { +) -> anyhow::Result<(String, Vec, Vec)> { let mut edits: Vec = Vec::new(); let mut records = Vec::new(); + let mut declined = Vec::new(); for result in results { let item = &result.item; // `is_rewritable` and not `is_checkable`: a workspace member inheriting a @@ -158,17 +330,33 @@ fn plan_fixes( // vulnerable release. Reporting that a newer version exists is useful; rewriting // the pin to it defeats the reason the entry was written, so `fix` declines the // whole kind rather than trying to guess which overrides are safe to move. + // + // Skipped outright rather than recorded in `declined`: that list is for an + // update the author could act on. An override is not rewritable by this + // tool at all, whatever it says and whatever flags are passed, so a note + // offering to explain the refusal would be describing a decision the + // author cannot change and did not make. Contrast the pin below, which is + // also not a shape refusal and *is* recorded, because `--all` acts on it. if item.kind == DependencyKind::Override { continue; } - let updatable = matches!( - result.status, - DependencyStatus::PatchAvailable - | DependencyStatus::UpdateAvailable - | DependencyStatus::Outdated - | DependencyStatus::Vulnerable - ); - if !updatable || (item.is_pinned() && !all) { + if !result.status.has_update() { + continue; + } + // A pin is a constraint, and it refused. Recorded rather than dropped: + // this is the single most common way a dependency is deliberately held + // back, so leaving it silent reproduces issue #93's symptom — `check` + // reports the update, `fix` prints "everything is already up to date" — + // by default configuration, on the path a user is likeliest to hit. The + // version named is the one `--all` would write, because `--all` is the + // action the note points at. + if item.is_pinned() && !all { + declined.push(Declined { + name: item.name.clone(), + constraint: item.version_constraint.clone(), + target: result.latest_available.clone(), + reason: DeclineReason::Pinned, + }); continue; } @@ -177,12 +365,44 @@ fn plan_fixes( } else { result.latest_compatible.as_ref() }; - let Some(target) = target else { continue }; - let Some(new_constraint) = rewrite_constraint(&item.version_constraint, target, ecosystem) - else { + // An update with nothing to write it from: the row is `has_update()` and + // the target this run would have used is absent. Usually a constraint no + // published release satisfies, whose newer releases all sit beyond it. + // Also a decline — `check` said something, so `fix` has to. + let Some(target) = target else { + declined.push(Declined { + name: item.name.clone(), + constraint: item.version_constraint.clone(), + target: None, + reason: DeclineReason::NoTarget, + }); continue; }; + let new_constraint = match rewrite_constraint(&item.version_constraint, target, ecosystem) { + Ok(new_constraint) => new_constraint, + Err(reason) => { + declined.push(Declined { + name: item.name.clone(), + constraint: item.version_constraint.clone(), + target: Some(target.clone()), + reason, + }); + continue; + } + }; + // Already at the target: nothing to write, and — until this was recorded + // — nothing said either. The constraint would have accepted the rewrite, + // which is why this is not a shape refusal; what refused is the range, + // and a `Vulnerable` row whose only fixed release is the one already in + // force reaches it with an advisory attached. `check` reports that row, + // so `fix` cannot answer it with "everything is already up to date". if new_constraint == item.version_constraint { + declined.push(Declined { + name: item.name.clone(), + constraint: item.version_constraint.clone(), + target: Some(target.clone()), + reason: DeclineReason::AlreadyAtTarget, + }); continue; } @@ -205,11 +425,15 @@ fn plan_fixes( } else { apply_edits(content, &edits)? }; - Ok((updated, records)) + // One note per distinct decline: the same crate under `[dependencies]` and + // `[dev-dependencies]` is one fact about one constraint, not two. + declined.sort(); + declined.dedup(); + Ok((updated, records, declined)) } /// Build a new constraint from `original`, preserving its leading operator/`v` -/// prefix and substituting `new_version`. Returns `None` for the forms that +/// prefix and substituting `new_version`. Returns [`Err`] for the forms that /// can't be rewritten without changing their meaning: a comma-separated range /// (Cargo `>=1.0, <2.0`), a space-separated range (npm/pubspec `>=1.0.0 <2.0.0`), /// a `||` alternation (`^1 || ^2`), a dist-tag (`latest`), anything carrying an @@ -218,6 +442,11 @@ fn plan_fixes( /// `~> 1.0`, `~=1.4`), and — depending on `ecosystem` — a wildcard (`*`, `1.x`, /// `1.*`) or a partial version (npm `"16"`, Cargo `"0"`). /// +/// The error is a [`DeclineReason`] and not a bare `None`, because *which* guard +/// fired is the only thing that makes the resulting note actionable, and this is +/// the sole place that knows it. An `Option` return threw that away at the one +/// boundary where it was still free. +/// /// `ecosystem` is what the wildcard and partial-version guards turn on, because /// both ask the same question: the rewrite writes the new version back bare, so /// is a bare version in this ecosystem still the range the author had? `None` @@ -225,14 +454,17 @@ fn plan_fixes( /// [`BareVersion::Exact`], which declines strictly more than either other /// reading, so an unknown manifest is never rewritten into something a known one /// would have refused. +/// +/// # Errors +/// Returns the [`DeclineReason`] for the first guard that refuses the rewrite. fn rewrite_constraint( original: &str, new_version: &str, ecosystem: Option, -) -> Option { +) -> Result { let trimmed = original.trim(); if trimmed.contains(',') { - return None; + return Err(DeclineReason::CommaRange); } const OP_CHARS: &[char] = &['^', '~', '>', '<', '=', '!', 'v', 'V', ' ', '\t']; let prefix: String = trimmed @@ -243,7 +475,7 @@ fn rewrite_constraint( // clause (range upper bound or alternative) we'd silently drop — leave it be. let rest = &trimmed[prefix.len()..]; if rest.contains([' ', '\t', '|']) { - return None; + return Err(DeclineReason::MultiClause); } // An `@` never belongs to a version: it introduces a Composer stability flag // (`@dev`, `2.8.*@dev`, `^1.0@beta`) or an npm alias target (`npm:pkg@1.0.0`). @@ -253,13 +485,13 @@ fn rewrite_constraint( // pin. That is the harm of #87, so take the same call already taken for a // dist-tag and decline. if rest.contains('@') { - return None; + return Err(DeclineReason::Qualifier); } // A dist-tag / channel name (`latest`, `next`, `beta`, …) starts with a letter // once any operator prefix is removed — it names a channel, not a version // range, so it must never be pinned to a concrete version (npm D8). if rest.starts_with(|c: char| c.is_ascii_alphabetic()) { - return None; + return Err(DeclineReason::DistTag); } let bare = ecosystem.map_or(BareVersion::Exact, Ecosystem::bare_version); @@ -303,13 +535,47 @@ fn rewrite_constraint( // - An operator in front (`^1.x`, `=1.*`, Python's `==1.*`) means the // result is not a bare version at all, so the caret reading that // justifies the rewrite does not apply to it. - if bare != BareVersion::Caret || !prefix.is_empty() || !is_minor_wildcard(rest) { - return None; + // + // The three conditions are checked in order of what the note should say, + // not in the order they were written: an operator answers for the whole + // constraint whatever the ecosystem reads a bare version as, and the + // ecosystem's reading answers before the wildcard's shape because it is + // the more specific harm. + if !prefix.is_empty() { + return Err(DeclineReason::WildcardOperator); + } + match bare { + BareVersion::Exact => return Err(DeclineReason::WildcardPins), + BareVersion::Minimum => return Err(DeclineReason::WildcardUnbounds), + BareVersion::Caret => {} + // A reading added since this was written. Decline, as every non-caret + // reading already does, under the reason that names no particular + // harm — inventing one for a reading this code has never seen would + // be worse than saying only that the shapes do not correspond. + _ => return Err(DeclineReason::WildcardShape), + } + if !is_minor_wildcard(rest) { + return Err(DeclineReason::WildcardShape); + } + // Shape is settled and the bound is not. `0.*` is the one minor wildcard + // a caret reproduces the *shape* of and not the *range*: it means + // `>=0.0.0, <1.0.0`, and the `0.y.z` written back is `^0.y.z`, which + // reaches only to `<0.(y+1).0`. A manifest that admitted `0.11.0` stops + // admitting it. + // + // Asked after the shape guard, and reported under its own reason rather + // than folded into `is_minor_wildcard`: a constraint that refuses for its + // shape should say so, and `0.*` does not refuse for its shape. A note + // reading "no bare version covers the same range" would send the author + // looking for the wrong thing. + if !caret_bound_survives_substitution(rest.split('.').next().unwrap_or_default()) { + return Err(DeclineReason::CaretBoundNarrows); } } else if is_partial_version(rest) { if prefix.is_empty() { // The same harm one wildcard character away, reached under two of the - // three readings. + // three readings — and under two different reasons, because the two + // readings lose two different things. // // Where a bare version is exact, npm treats a partial version as an // X-range — `"react": "16"` is `16.x`, `"1.0"` is `1.0.x` — so @@ -327,7 +593,9 @@ fn rewrite_constraint( // and `<2.0.0` holds — and stays rewritable, except where every // component the author wrote is zero. `0` is `^0` (`<1.0.0`) and // `0.0` is `^0.0` (`<0.1.0`), bounds no concrete release reproduces: - // see [`caret_bound_survives_substitution`]. + // see [`caret_bound_survives_substitution`]. That is not the X-range + // harm above and does not borrow its note; it is the bound, not the + // shape. // // [`BareVersion::Minimum`] keeps the rewrite unconditionally. NuGet's // `1.0` is `>=1.0` and its `0` is `>=0.0.0`; both are floors with no @@ -336,10 +604,11 @@ fn rewrite_constraint( // The exactness question is asked through the predicate the enum // exports for it rather than by comparing the enum here, so an // unrecognized manifest (`None`) answers it the declining way. - if ecosystem.is_none_or(Ecosystem::bare_version_is_exact) - || (bare == BareVersion::Caret && !caret_bound_survives_substitution(rest)) - { - return None; + if ecosystem.is_none_or(Ecosystem::bare_version_is_exact) { + return Err(DeclineReason::PartialVersion); + } + if bare == BareVersion::Caret && !caret_bound_survives_substitution(rest) { + return Err(DeclineReason::CaretBoundNarrows); } } else if prefix.contains('~') { // A tilde operator reads its upper bound off the *number of @@ -360,24 +629,31 @@ fn rewrite_constraint( // // A full-arity `~1.0.0` is untouched — `is_partial_version` is false // for it — which is what keeps ordinary tilde constraints rewritable. - return None; + // + // Its own reason, not [`DeclineReason::PartialVersion`]: the arity is + // only a harm because the operator reads it, and a note about an + // X-range would describe a constraint the author did not write. + return Err(DeclineReason::TildeArity); } } - Some(format!("{prefix}{new_version}")) + Ok(format!("{prefix}{new_version}")) } /// Whether `rest` — a wildcard constraint with its operator prefix already -/// stripped — is the one wildcard shape a caret reading reproduces: a *non-zero* -/// numeric major followed by a wildcard in the minor position, `1.*` or `1.x`. +/// stripped — is the one wildcard *shape* a caret reading reproduces: a numeric +/// major followed by a wildcard in the minor position, `1.*` or `1.x`. /// /// Deliberately narrow. `*`, `1.2.*`, and NuGet's `1.0.0.*` are all wildcards /// too, and a caret over a concrete release matches none of their ranges. The /// wildcard character must be `*`, `x`, or `X`: Gradle's `+` is a prefix range /// with its own resolution rules and no ecosystem that reads a bare version as a -/// caret accepts it. And the major must not be zero, because a caret is -/// *minor*-scoped below 1.0.0 — `0.*` is `<1.0.0` and no concrete `0.y.z` -/// reaches that far, so the upper bound this whole rewrite is justified by would -/// not survive. See [`caret_bound_survives_substitution`]. +/// caret accepts it. +/// +/// Shape only. `0.*` has the right shape and still loses the author's upper +/// bound, because a caret is *minor*-scoped below 1.0.0; that is a separate +/// question, asked separately by the caller through +/// [`caret_bound_survives_substitution`] so the two refusals can report the two +/// different reasons. fn is_minor_wildcard(rest: &str) -> bool { let mut segments = rest.split('.'); let (Some(major), Some(minor), None) = (segments.next(), segments.next(), segments.next()) @@ -386,7 +662,6 @@ fn is_minor_wildcard(rest: &str) -> bool { }; !major.is_empty() && major.bytes().all(|b| b.is_ascii_digit()) - && caret_bound_survives_substitution(major) && matches!(minor, "*" | "x" | "X") } @@ -521,7 +796,7 @@ mod tests { "the fixture must produce an override item" ); - let (updated, records) = plan_fixes( + let (updated, records, declined) = plan_fixes( content, &results, true, @@ -529,7 +804,7 @@ mod tests { ) .expect("the plan applies"); - // The override is declined; its non-override neighbour is still fixed, so this + // The override is skipped; its non-override neighbour is still fixed, so this // asserts the guard rather than a `--all` path that happens to do nothing. assert_eq!( records @@ -543,6 +818,16 @@ mod tests { updated.contains(r#""minimist": "1.2.6""#), "the override was rewritten: {updated}" ); + // And it is skipped *silently*. A `Declined` says a constraint refused a + // rewrite the author could permit by widening it; an override refuses for a + // reason that has nothing to do with its constraint and that no edit to the + // constraint would change, so reporting one here would tell the author to go + // fix a string that is not the problem. + assert_eq!( + declined, + [], + "the override was reported as a declined update" + ); } /// Every ecosystem, so a guard that is ecosystem-independent is asserted @@ -570,7 +855,7 @@ mod tests { let it = Some(ecosystem); assert_eq!( rewrite_constraint("^1.0", "1.5.0", it).as_deref(), - Some("^1.5.0"), + Ok("^1.5.0"), "{ecosystem:?}" ); // Full arity, because a tilde reads its upper bound off the number of @@ -579,44 +864,46 @@ mod tests { // claim, not this test's. assert_eq!( rewrite_constraint("~1.0.0", "1.5.0", it).as_deref(), - Some("~1.5.0"), + Ok("~1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint(">=1.0", "1.5.0", it).as_deref(), - Some(">=1.5.0"), + Ok(">=1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint("v1.2.3", "1.5.0", it).as_deref(), - Some("v1.5.0"), + Ok("v1.5.0"), "{ecosystem:?}" ); // A full bare version names one release under every reading, and // moving it forward is what `fix` is for. assert_eq!( rewrite_constraint("1.0.0", "1.5.0", it).as_deref(), - Some("1.5.0"), + Ok("1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint("=1.2.0", "1.5.0", it).as_deref(), - Some("=1.5.0"), + Ok("=1.5.0"), "{ecosystem:?}" ); // The bare wildcard `*` is a range, not a version — see // `rewrite_never_narrows_a_wildcard_to_a_pin`. Declined everywhere, // Cargo included: `*` admits every major and a caret admits one. - assert_eq!(rewrite_constraint("*", "1.5.0", it), None, "{ecosystem:?}"); + assert!( + rewrite_constraint("*", "1.5.0", it).is_err(), + "{ecosystem:?}" + ); } } #[test] fn rewrite_skips_multi_constraint() { for ecosystem in EVERY_ECOSYSTEM { - assert_eq!( - rewrite_constraint(">=1.0,<2.0", "1.5.0", Some(ecosystem)), - None, + assert!( + rewrite_constraint(">=1.0,<2.0", "1.5.0", Some(ecosystem)).is_err(), "{ecosystem:?}" ); } @@ -630,24 +917,24 @@ mod tests { // channel name means the same thing wherever one is written. for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); - assert_eq!( - rewrite_constraint("latest", "2.3.0", it), - None, + assert!( + rewrite_constraint("latest", "2.3.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("next", "2.3.0", it), - None, + assert!( + rewrite_constraint("next", "2.3.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("beta", "2.3.0", it), - None, + assert!( + rewrite_constraint("beta", "2.3.0", it).is_err(), "{ecosystem:?}" ); // The bare wildcard `*` is declined for the same reason: it is a range // the author chose, and pinning it would narrow their manifest (#87). - assert_eq!(rewrite_constraint("*", "2.3.0", it), None, "{ecosystem:?}"); + assert!( + rewrite_constraint("*", "2.3.0", it).is_err(), + "{ecosystem:?}" + ); } } @@ -670,40 +957,37 @@ mod tests { continue; } let it = Some(ecosystem); - assert_eq!( - rewrite_constraint("1.x", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.x", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.*", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.*", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.X", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.X", "2.0.0", it).is_err(), "{ecosystem:?}" ); // Gradle's dynamic version has the same shape (issue #87), and NuGet's // floating `1.*` resolves differently from a bare `2.0.0`. - assert_eq!( - rewrite_constraint("1.+", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.+", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^1.x", "2.0.0", it), - None, + assert!( + rewrite_constraint("^1.x", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.2.x", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.2.x", "2.0.0", it).is_err(), "{ecosystem:?}" ); // The bare wildcard is the same kind of thing. - assert_eq!(rewrite_constraint("*", "2.0.0", it), None, "{ecosystem:?}"); + assert!( + rewrite_constraint("*", "2.0.0", it).is_err(), + "{ecosystem:?}" + ); } // Cargo reads a bare version as a caret, which reproduces exactly one @@ -711,16 +995,16 @@ mod tests { let cargo = Some(Ecosystem::Rust); // Gradle's `+` is a prefix range with its own resolution rules, and no // caret-reading ecosystem accepts it as a wildcard at all. - assert_eq!(rewrite_constraint("1.+", "2.0.0", cargo), None); + assert!(rewrite_constraint("1.+", "2.0.0", cargo).is_err()); // An operator means what gets written back is not a bare version, so the // caret reading that would justify the rewrite does not apply to it. - assert_eq!(rewrite_constraint("^1.x", "2.0.0", cargo), None); - assert_eq!(rewrite_constraint("=1.*", "2.0.0", cargo), None); + assert!(rewrite_constraint("^1.x", "2.0.0", cargo).is_err()); + assert!(rewrite_constraint("=1.*", "2.0.0", cargo).is_err()); // `1.2.*` is `>=1.2.0, <1.3.0`; a caret over any 1.2.z release reaches to // `<2.0.0`, so substituting *widens* what the author admitted. - assert_eq!(rewrite_constraint("1.2.x", "2.0.0", cargo), None); + assert!(rewrite_constraint("1.2.x", "2.0.0", cargo).is_err()); // `*` is every version; any concrete release confines it to one major. - assert_eq!(rewrite_constraint("*", "2.0.0", cargo), None); + assert!(rewrite_constraint("*", "2.0.0", cargo).is_err()); } /// The other side of issue #92: declining every wildcard was conservatism, not @@ -734,15 +1018,15 @@ mod tests { let cargo = Some(Ecosystem::Rust); assert_eq!( rewrite_constraint("1.*", "1.0.219", cargo).as_deref(), - Some("1.0.219") + Ok("1.0.219") ); assert_eq!( rewrite_constraint("1.x", "1.0.219", cargo).as_deref(), - Some("1.0.219") + Ok("1.0.219") ); assert_eq!( rewrite_constraint("1.X", "1.0.219", cargo).as_deref(), - Some("1.0.219") + Ok("1.0.219") ); // The same input is declined for every ecosystem that reads a bare version // any other way — the whole point of asking which one this is. @@ -750,15 +1034,14 @@ mod tests { if ecosystem == Ecosystem::Rust { continue; } - assert_eq!( - rewrite_constraint("1.*", "1.0.219", Some(ecosystem)), - None, + assert!( + rewrite_constraint("1.*", "1.0.219", Some(ecosystem)).is_err(), "{ecosystem:?}" ); } // And a manifest whose kind was not recognized gets the reading that // declines the most, never the one that permits the most. - assert_eq!(rewrite_constraint("1.*", "1.0.219", None), None); + assert!(rewrite_constraint("1.*", "1.0.219", None).is_err()); } /// The zero-major hole in that permit. Cargo's caret is *minor*-scoped below @@ -775,52 +1058,72 @@ mod tests { #[test] fn rewrite_declines_a_zero_major_wildcard_and_partial() { let cargo = Some(Ecosystem::Rust); - // The wildcard forms, in all three spellings the permit accepts. - assert_eq!(rewrite_constraint("0.*", "0.10.0", cargo), None); - assert_eq!(rewrite_constraint("0.x", "0.10.0", cargo), None); - assert_eq!(rewrite_constraint("0.X", "0.10.0", cargo), None); + // The wildcard forms, in all three spellings the permit accepts. The + // reason is its own and not `WildcardShape`: the shape is fine, and a + // note saying "no bare version covers the same range" would point the + // author at the wrong half of the constraint. + for original in ["0.*", "0.x", "0.X"] { + assert_eq!( + rewrite_constraint(original, "0.10.0", cargo), + Err(DeclineReason::CaretBoundNarrows), + "{original}" + ); + } // The partial forms, which carry no wildcard character for `is_wildcard` // to see at all. - assert_eq!(rewrite_constraint("0", "0.10.0", cargo), None); - assert_eq!(rewrite_constraint("0.0", "0.0.9", cargo), None); + assert_eq!( + rewrite_constraint("0", "0.10.0", cargo), + Err(DeclineReason::CaretBoundNarrows) + ); + assert_eq!( + rewrite_constraint("0.0", "0.0.9", cargo), + Err(DeclineReason::CaretBoundNarrows) + ); // A non-zero component anywhere restores the guarantee, so the common // pre-1.0 Cargo constraint stays rewritable: `^0.9` and `^0.9.5` both stop // below `0.10.0`. Declining these too would cost real fixes for nothing. assert_eq!( rewrite_constraint("0.9", "0.9.5", cargo).as_deref(), - Some("0.9.5") + Ok("0.9.5") ); assert_eq!( rewrite_constraint("0.9.1", "0.9.5", cargo).as_deref(), - Some("0.9.5") + Ok("0.9.5") ); // The wildcard forms are declined in every other ecosystem too, each for // the reason that already applied: none of them reads a bare version as a - // caret, so no wildcard survives substitution there. + // caret, so no wildcard survives substitution there — and the note they + // get names *that*, which is why the reason is asked for rather than the + // bare refusal. for ecosystem in EVERY_ECOSYSTEM { if ecosystem == Ecosystem::Rust { continue; } + let expected = if ecosystem.bare_version_is_exact() { + DeclineReason::WildcardPins + } else { + DeclineReason::WildcardUnbounds + }; for original in ["0.*", "0.x", "0.X"] { assert_eq!( rewrite_constraint(original, "0.10.0", Some(ecosystem)), - None, + Err(expected), "{ecosystem:?} {original}" ); } } // A bare `0` is a different question under each reading, and only the // caret one is harmed. Where a bare version is exact the partial-version - // guard already declined it; where it is a minimum, `0` is `>=0.0.0` and - // `0.10.0` is `>=0.10.0` — a raised floor with no upper bound on either - // side, which is what `fix` is for. + // guard already declined it — under its own reason, not this one; where it + // is a minimum, `0` is `>=0.0.0` and `0.10.0` is `>=0.10.0` — a raised + // floor with no upper bound on either side, which is what `fix` is for. assert_eq!( rewrite_constraint("0", "0.10.0", Some(Ecosystem::Npm)), - None + Err(DeclineReason::PartialVersion) ); assert_eq!( rewrite_constraint("0", "0.10.0", Some(Ecosystem::CSharp)).as_deref(), - Some("0.10.0") + Ok("0.10.0") ); } @@ -843,46 +1146,39 @@ mod tests { for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); // Hex: `~> 1.0` is `>=1.0.0, <2.0.0`; `~> 1.7.10` is `>=1.7.10, <1.8.0`. - assert_eq!( - rewrite_constraint("~> 1.0", "1.7.10", it), - None, - "{ecosystem:?}" - ); - assert_eq!( - rewrite_constraint("~> 1", "1.7.10", it), - None, - "{ecosystem:?}" - ); + // The reason is the arity one in every ecosystem, because the operator + // is what makes the arity matter — a note about an X-range would + // describe a constraint the author did not write. + for original in ["~> 1.0", "~> 1", "~1", "~1.0"] { + assert_eq!( + rewrite_constraint(original, "1.7.10", it), + Err(DeclineReason::TildeArity), + "{ecosystem:?} {original}" + ); + } // PEP 440: `~=1.4` is `>=1.4.0, <2.0.0`; `~=1.5.0` is `>=1.5.0, <1.6.0`. assert_eq!( rewrite_constraint("~=1.4", "1.5.0", it), - None, - "{ecosystem:?}" - ); - // npm and Cargo: `~1` is major-wide, `~1.5.0` is minor-wide. - assert_eq!(rewrite_constraint("~1", "1.5.0", it), None, "{ecosystem:?}"); - assert_eq!( - rewrite_constraint("~1.0", "1.5.0", it), - None, + Err(DeclineReason::TildeArity), "{ecosystem:?}" ); // Full arity supplies every component the rewrite writes back, so it // stays rewritable — the guard must not swallow ordinary constraints. assert_eq!( rewrite_constraint("~1.0.0", "1.5.0", it).as_deref(), - Some("~1.5.0"), + Ok("~1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint("~> 1.0.0", "1.5.0", it).as_deref(), - Some("~> 1.5.0"), + Ok("~> 1.5.0"), "{ecosystem:?}" ); // Bounded to the tilde family. `>=1.0` means `>=1.0.0` at any arity, // so raising its floor is safe and this guard leaves it alone. assert_eq!( rewrite_constraint(">=1.0", "1.5.0", it).as_deref(), - Some(">=1.5.0"), + Ok(">=1.5.0"), "{ecosystem:?}" ); } @@ -897,33 +1193,33 @@ mod tests { #[test] fn rewrite_declines_a_partial_version_where_a_bare_version_is_exact() { let npm = Some(Ecosystem::Npm); - assert_eq!(rewrite_constraint("16", "16.14.0", npm), None); - assert_eq!(rewrite_constraint("1.0", "1.5.0", npm), None); + assert!(rewrite_constraint("16", "16.14.0", npm).is_err()); + assert!(rewrite_constraint("1.0", "1.5.0", npm).is_err()); // Cargo's `1.0` is `^1.0` and `1.5.0` is `^1.5.0`: the floor rises and the // upper bound holds, which is what every other `fix` rewrite does. assert_eq!( rewrite_constraint("1.0", "1.5.0", Some(Ecosystem::Rust)).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // NuGet's `1.0` is `>= 1.0` and `1.5.0` is `>= 1.5.0` — a raised floor too. assert_eq!( rewrite_constraint("1.0", "1.5.0", Some(Ecosystem::CSharp)).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // Only the *partial* form is a range. A full bare version is a pin, and // moving a pin forward is exactly what `fix` is asked to do. assert_eq!( rewrite_constraint("16.0.0", "16.14.0", npm).as_deref(), - Some("16.14.0") + Ok("16.14.0") ); // An operator makes it a range in its own right, npm included: `^16` is a // caret range and `^16.14.0` is that range with a raised floor. assert_eq!( rewrite_constraint("^16", "16.14.0", npm).as_deref(), - Some("^16.14.0") + Ok("^16.14.0") ); // An unrecognized manifest declines, like every exact reading. - assert_eq!(rewrite_constraint("16", "16.14.0", None), None); + assert!(rewrite_constraint("16", "16.14.0", None).is_err()); } /// A wildcard segment is not always the whole dot-segment. Composer allows a @@ -940,29 +1236,24 @@ mod tests { fn rewrite_declines_a_wildcard_wearing_a_stability_flag() { for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); - assert_eq!( - rewrite_constraint("2.8.*@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("2.8.*@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("2.8.x@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("2.8.x@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.*@stable", "7.0.0", it), - None, + assert!( + rewrite_constraint("1.*@stable", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("*@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("*@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^2.8.*@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("^2.8.*@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); } @@ -981,33 +1272,28 @@ mod tests { for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); // The bare flag: a range over every version, collapsed to a pin. - assert_eq!( - rewrite_constraint("@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); // Flag on an operator-led constraint, and on a bare version. - assert_eq!( - rewrite_constraint(">=2.8@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint(">=2.8@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("2.8@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("2.8@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^1.0@beta", "7.0.0", it), - None, + assert!( + rewrite_constraint("^1.0@beta", "7.0.0", it).is_err(), "{ecosystem:?}" ); // npm's alias form carries an `@` too. The dist-tag guard caught it only // incidentally, because `npm:` happens to start with a letter; now it is // declined for the reason that actually applies. - assert_eq!( - rewrite_constraint("npm:pkg@1.0.0", "7.0.0", it), - None, + assert!( + rewrite_constraint("npm:pkg@1.0.0", "7.0.0", it).is_err(), "{ecosystem:?}" ); } @@ -1035,37 +1321,37 @@ mod tests { // Go: a pseudo-version and the `+incompatible` marker. assert_eq!( rewrite_constraint("v0.0.0-20191109021931-daa7c04131f5", "1.5.0", go).as_deref(), - Some("v1.5.0") + Ok("v1.5.0") ); assert_eq!( rewrite_constraint("v2.0.0+incompatible", "1.5.0", go).as_deref(), - Some("v1.5.0") + Ok("v1.5.0") ); // Semver build metadata and prereleases — note the dotted identifiers, // which a leading-character test for `x` would have to survive. assert_eq!( rewrite_constraint("1.2.3+build.5", "1.5.0", rust).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); assert_eq!( rewrite_constraint("1.0.0-alpha+exp.sha.5114f85", "1.5.0", rust).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // From the semver spec itself: a prerelease whose identifiers include `x`. assert_eq!( rewrite_constraint("1.0.0-x.7.z.92", "1.5.0", rust).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // NuGet's four-part version — four numeric segments, which the // partial-version guard must not mistake for a truncated one. assert_eq!( rewrite_constraint("1.0.0.4", "1.5.0", nuget).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // Python epochs and compatible-release operators. assert_eq!( rewrite_constraint("1!2.0", "1.5.0", python).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // At full arity: `~=1.4` and `~> 1.0` are *partial*, and a tilde reads its // upper bound off the number of components it was given, so those two are @@ -1073,20 +1359,20 @@ mod tests { // for. See `rewrite_declines_a_partial_version_behind_a_tilde`. assert_eq!( rewrite_constraint("~=1.4.2", "1.5.0", python).as_deref(), - Some("~=1.5.0") + Ok("~=1.5.0") ); // Hex's `~>`, whose space belongs to the operator prefix. assert_eq!( rewrite_constraint("~> 1.0.0", "1.5.0", hex).as_deref(), - Some("~> 1.5.0") + Ok("~> 1.5.0") ); // Declined already, and for a different reason: NuGet's bracketed range // holds a comma. The wildcard guard must not change that verdict. - assert_eq!(rewrite_constraint("[1.0,2.0)", "1.5.0", nuget), None); + assert!(rewrite_constraint("[1.0,2.0)", "1.5.0", nuget).is_err()); // Python's `==1.*` is a wildcard, and stays declined — twice over: Python // reads a bare version exactly, and the `==` means the rewrite would not // have produced a bare version anyway. - assert_eq!(rewrite_constraint("==1.*", "1.5.0", python), None); + assert!(rewrite_constraint("==1.*", "1.5.0", python).is_err()); } #[test] @@ -1096,25 +1382,261 @@ mod tests { // Dropping a clause is a loss in every ecosystem, so assert it in all nine. for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); - assert_eq!( - rewrite_constraint(">=1.0.0 <2.0.0", "1.5.0", it), - None, + assert!( + rewrite_constraint(">=1.0.0 <2.0.0", "1.5.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^1.0.0 || ^2.0.0", "1.5.0", it), - None, + assert!( + rewrite_constraint("^1.0.0 || ^2.0.0", "1.5.0", it).is_err(), "{ecosystem:?}" ); // A single constraint that merely spaces its operator is still rewritten. assert_eq!( rewrite_constraint(">= 1.0.0", "1.5.0", it).as_deref(), - Some(">= 1.5.0"), + Ok(">= 1.5.0"), "{ecosystem:?}" ); } } + /// A decline is only worth carrying out of the planner if it says which + /// guard fired, because that is the whole content of the note the user sees. + /// One assertion per variant, so a guard that starts answering under another + /// reason changes a test rather than quietly changing what `fix` tells people. + #[test] + fn a_decline_names_the_guard_that_refused_it() { + let cargo = Some(Ecosystem::Rust); + let npm = Some(Ecosystem::Npm); + let nuget = Some(Ecosystem::CSharp); + + let reason = |original, ecosystem| rewrite_constraint(original, "2.0.0", ecosystem).err(); + + assert_eq!(reason(">=1.0,<2.0", cargo), Some(DeclineReason::CommaRange)); + assert_eq!( + reason(">=1.0.0 <2.0.0", npm), + Some(DeclineReason::MultiClause) + ); + assert_eq!( + reason("^1.0.0 || ^2.0.0", npm), + Some(DeclineReason::MultiClause) + ); + assert_eq!(reason("2.8.*@dev", cargo), Some(DeclineReason::Qualifier)); + assert_eq!(reason("latest", npm), Some(DeclineReason::DistTag)); + + // The wildcard family, whose reason is the point of #92: the same three + // characters are declined for three different harms. + // + // An operator answers first, and for every ecosystem — with one in front, + // what gets written back is not a bare version at all, so what a bare + // version *means* here cannot be the reason. + assert_eq!(reason("^1.x", cargo), Some(DeclineReason::WildcardOperator)); + assert_eq!(reason("^1.x", npm), Some(DeclineReason::WildcardOperator)); + // Then the ecosystem's reading, which is the more specific harm than the + // shape: npm pins, NuGet loses the upper bound. + assert_eq!(reason("1.x", npm), Some(DeclineReason::WildcardPins)); + assert_eq!(reason("1.*", nuget), Some(DeclineReason::WildcardUnbounds)); + // And only where a bare version is already a caret does the shape get to + // be the reason — there the reading is fine and the wildcard is not. + assert_eq!(reason("1.2.*", cargo), Some(DeclineReason::WildcardShape)); + assert_eq!(reason("*", cargo), Some(DeclineReason::WildcardShape)); + + assert_eq!(reason("16", npm), Some(DeclineReason::PartialVersion)); + + // An unrecognized manifest reads a bare version exactly, so it declines + // under that reading rather than under a reason of its own. + assert_eq!(reason("1.x", None), Some(DeclineReason::WildcardPins)); + } + + /// The issue #93 defect at the planner's own boundary: `plan_fixes` used to + /// return an empty record list for a wildcard it declined, which is the same + /// answer it returns for a manifest with nothing to do. The declined list is + /// what tells those two apart. + #[test] + fn a_declined_constraint_leaves_a_record_of_what_was_not_done() { + let content = r#"{ + "name": "demo", + "dependencies": { + "lodash": "1.x", + "react": "^18.0.0" + } +} +"#; + let results = results_for( + ManifestKind::PackageJson, + content, + &[("lodash", "1.9.0"), ("react", "18.2.0")], + ); + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + + // The wildcard is untouched and the ordinary caret is rewritten: the + // decline is a record, not a refusal to plan the rest of the manifest. + assert!(updated.contains(r#""lodash": "1.x""#)); + assert!(updated.contains(r#""react": "^18.2.0""#)); + assert_eq!(records.len(), 1); + assert_eq!( + declined, + vec![Declined { + name: "lodash".to_string(), + constraint: "1.x".to_string(), + target: Some("1.9.0".to_string()), + reason: DeclineReason::WildcardPins, + }] + ); + } + + /// A pin is a constraint, and `fix` refusing to move it without `--all` is a + /// refusal the author can act on. Silent, it was issue #93's symptom reached + /// by default configuration on the single most common way a dependency is + /// deliberately held back. + #[test] + fn a_pin_held_back_for_want_of_all_is_recorded() { + let content = "[dependencies]\nserde = \"=1.0.100\"\n"; + let results = results_for(ManifestKind::CargoToml, content, &[("serde", "1.0.219")]); + + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::CargoToml.ecosystem()), + ) + .expect("the plan applies"); + assert_eq!(updated, content, "the pin was rewritten without `--all`"); + assert!(records.is_empty()); + assert_eq!( + declined, + vec![Declined { + name: "serde".to_string(), + constraint: "=1.0.100".to_string(), + // What `--all` would write, because `--all` is what the note points at. + target: Some("1.0.219".to_string()), + reason: DeclineReason::Pinned, + }] + ); + assert!( + DeclineReason::Pinned.explain().contains("--all"), + "the note must name the action that moves a pin" + ); + + // With `--all` the pin is rewritten, and there is nothing left to say. + let (updated, records, declined) = plan_fixes( + content, + &results, + true, + Some(ManifestKind::CargoToml.ecosystem()), + ) + .expect("the plan applies"); + assert!(updated.contains("serde = \"=1.0.219\"")); + assert_eq!(records.len(), 1); + assert!(declined.is_empty(), "{declined:?}"); + } + + /// `check` reported an update and the run resolved no version to write it + /// from — a constraint no published release satisfies, whose newer releases + /// all sit beyond it. The note has no version to name, so it opens on the fact + /// it does have. + #[test] + fn an_update_with_no_resolved_target_is_recorded() { + let content = "[dependencies]\nserde = \"^1.0\"\n"; + let mut results: Vec = parse(ManifestKind::CargoToml, content) + .unwrap() + .items + .into_iter() + .map(|item| CheckResult::new(item, DependencyStatus::UpdateAvailable)) + .collect(); + // Neither field: the default path reads `latest_compatible` and `--all` + // reads `latest_available`, and this row answers neither. + for result in &mut results { + result.latest_compatible = None; + result.latest_available = None; + } + + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::CargoToml.ecosystem()), + ) + .expect("the plan applies"); + assert_eq!(updated, content); + assert!(records.is_empty()); + assert_eq!( + declined, + vec![Declined { + name: "serde".to_string(), + constraint: "^1.0".to_string(), + target: None, + reason: DeclineReason::NoTarget, + }] + ); + } + + /// The constraint already names the version that would have been written. + /// Reached most sharply by a `Vulnerable` row whose only fixed release is the + /// one already in force: `check` reports it, so `fix` cannot answer with + /// "everything is already up to date". + #[test] + fn a_constraint_already_at_its_target_is_recorded() { + let content = r#"{ + "name": "demo", + "dependencies": { + "lodash": "1.0.0" + } +} +"#; + let mut results = results_for(ManifestKind::PackageJson, content, &[("lodash", "1.0.0")]); + results[0].status = DependencyStatus::Vulnerable; + + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert_eq!(updated, content); + assert!(records.is_empty()); + assert_eq!( + declined, + vec![Declined { + name: "lodash".to_string(), + constraint: "1.0.0".to_string(), + target: Some("1.0.0".to_string()), + reason: DeclineReason::AlreadyAtTarget, + }] + ); + } + + /// A dependency with nothing available must not be reported as left alone — + /// the note claims `check` had something to say, so anything up to date has + /// to be filtered out before the constraint is ever consulted. + #[test] + fn an_up_to_date_dependency_is_not_a_decline() { + let content = r#"{ + "name": "demo", + "dependencies": { + "lodash": "1.x" + } +} +"#; + // `results_for` marks its targets `UpdateAvailable`; naming none leaves + // the manifest's only dependency with no result at all. + let (_, records, declined) = plan_fixes( + content, + &results_for(ManifestKind::PackageJson, content, &[]), + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert!(records.is_empty()); + assert!(declined.is_empty()); + } + #[test] fn apply_edits_replaces_recorded_span() { // `serde = "^1.0"` — replace the `^1.0` span (bytes 9..13) on line 1. @@ -1154,6 +1676,7 @@ mod tests { assert_eq!(out, "a=1.9 b=2.9\n"); } + use dependable_fetch::DependencyStatus; use dependable_fetch::core::{DependencyKind, parse, resolve_workspace_inheritance}; /// Parse `content`, then build an `UpdateAvailable` result with the given @@ -1202,7 +1725,7 @@ mod tests { content, &[("react", "18.2.0"), ("typescript", "5.4.5")], ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1246,7 +1769,7 @@ mod tests { content, &[("monolog/monolog", "2.9.1")], ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1278,7 +1801,7 @@ mod tests { content, &[("http", "1.2.0"), ("provider", "6.1.0")], ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1328,7 +1851,7 @@ mod tests { "the old guards would both have passed" ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( member, &results, false, @@ -1363,7 +1886,7 @@ mod tests { ); assert_eq!(declaration.version_line, 1, "and the span points at it"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( root, &results, false, @@ -1445,7 +1968,7 @@ mod tests { "the fixture must produce a checkable item" ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1487,7 +2010,7 @@ mod tests { "the `--all` branch reads `latest_available`, so the fixture must set it" ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, true, @@ -1538,7 +2061,7 @@ mod tests { ); assert_eq!(results.len(), 2, "the fixture must produce two items"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1570,7 +2093,7 @@ mod tests { let results = results_for(ManifestKind::PackageJson, content, &[("lodash", "1.9.0")]); assert_eq!(results.len(), 1, "the fixture must produce one item"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1598,7 +2121,7 @@ mod tests { ); assert_eq!(results.len(), 2, "the fixture must produce two items"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1627,19 +2150,19 @@ mod tests { /// scoped to the forms whose meaning depends on the ecosystem. #[test] fn an_unrecognized_manifest_kind_declines_every_ecosystem_dependent_form() { - assert_eq!(rewrite_constraint("1.*", "1.5.0", None), None); - assert_eq!(rewrite_constraint("1.x", "1.5.0", None), None); - assert_eq!(rewrite_constraint("1.0", "1.5.0", None), None); - assert_eq!(rewrite_constraint("16", "16.14.0", None), None); + assert!(rewrite_constraint("1.*", "1.5.0", None).is_err()); + assert!(rewrite_constraint("1.x", "1.5.0", None).is_err()); + assert!(rewrite_constraint("1.0", "1.5.0", None).is_err()); + assert!(rewrite_constraint("16", "16.14.0", None).is_err()); // Not ecosystem-dependent: an operator-led range and a full bare version // mean the same thing everywhere, so they are still rewritten. assert_eq!( rewrite_constraint("^1.0", "1.5.0", None).as_deref(), - Some("^1.5.0") + Ok("^1.5.0") ); assert_eq!( rewrite_constraint("1.0.0", "1.5.0", None).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); } } diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index cb0b082..e4c55ee 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -958,12 +958,15 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { // *after* each write, the failing iteration also destroyed the record of what had // already changed. let mut planned = Vec::new(); + let mut inherited = 0usize; for manifest in &manifests { let Some(report) = engine.check_manifest(manifest).await? else { continue; }; - report_inherited_skips(manifest, &report); - planned.push(fix::plan(manifest, &report.results, args.all)?); + inherited += report_inherited_skips(manifest, &report); + let plan = fix::plan(manifest, &report.results, args.all)?; + report_declined_fixes(manifest, &plan.declined); + planned.push(plan); } let mut total = 0; @@ -984,8 +987,32 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { total += 1; } } + let declined: usize = planned.iter().map(|plan| plan.declined.len()).sum(); + // Every category this run emitted a note for, not just the constraint + // declines. An inherited dependency is left behind in exactly the sense the + // line below means, and counting only one of the two categories printed the + // clean line on stdout directly over an inherited-skip note on stderr. + let left_alone = declined + inherited; if total == 0 { - println!("Everything is already up to date."); + // "Everything is already up to date" is only true when nothing was left + // behind. Saying it over an update this run declined to write is the + // contradiction with `check` that this whole path exists to remove, so the + // count of what was left alone takes over the line and points at the notes + // that explain it. + // + // The notes are on stderr and this line is on stdout, so it says *where*: + // `dependable fix > fix.log` puts the summary in the log and the notes on + // the terminal, and "see the notes above" would name nothing the reader of + // either stream can find. + if left_alone == 0 { + println!("Everything is already up to date."); + } else { + println!( + "Nothing to rewrite. {left_alone} available update{} left alone; \ + see the notes on stderr.", + if left_alone == 1 { "" } else { "s" } + ); + } } else if !args.dry_run { println!( "\nUpdated {total} dependenc{}.", @@ -1038,29 +1065,29 @@ fn resolve_report_settings(args: &crate::cli::ReportArgs, cfg: &Config) -> Setti /// silently left alone by `fix`, because the version string is in the workspace root and /// there is no line here to rewrite. Without this the two commands appear to contradict /// each other, and nothing points at the file that can actually be changed. -fn report_inherited_skips(manifest: &Path, report: &ManifestReport) { +/// +/// Returns how many it named, because the summary line has to know. These never +/// reach [`fix::plan`]'s declined list — the item is not rewritable, so the +/// planner drops it before any constraint is consulted — and a summary counting +/// only that list printed "Everything is already up to date." on stdout over the +/// note this function had just written to stderr. +#[must_use] +fn report_inherited_skips(manifest: &Path, report: &ManifestReport) -> usize { let Some(root) = &report.workspace_root else { - return; + return 0; }; let mut names: Vec<&str> = report .results .iter() .filter(|result| { - result.item.source == PackageSource::Inherited - && matches!( - result.status, - DependencyStatus::PatchAvailable - | DependencyStatus::UpdateAvailable - | DependencyStatus::Outdated - | DependencyStatus::Vulnerable - ) + result.item.source == PackageSource::Inherited && result.status.has_update() }) .map(|result| result.item.name.as_str()) .collect(); names.sort_unstable(); names.dedup(); if names.is_empty() { - return; + return 0; } eprintln!( "note: {} inherits {} from the workspace; upgrade {} in {}", @@ -1069,6 +1096,37 @@ fn report_inherited_skips(manifest: &Path, report: &ManifestReport) { if names.len() == 1 { "it" } else { "them" }, root.display() ); + names.len() +} + +/// Say which available updates this manifest's own constraints refused, and why. +/// +/// The sibling of [`report_inherited_skips`], for the other way `fix` can decline +/// an upgrade `check` just reported: there the version string lives in another +/// file, here it lives in a constraint that a concrete version would not +/// reproduce — a wildcard, a dist-tag, a two-bound range. Both are silent skips, +/// and silence is what makes the two commands look like they disagree. +/// +/// stderr, like its sibling: a note is not part of the record of what `fix` +/// changed, and piping stdout must not swallow it or mix it into that record. +fn report_declined_fixes(manifest: &Path, declined: &[fix::Declined]) { + for item in declined { + // A decline usually knows the release it would have written, and naming it + // is half the note's value. [`fix::DeclineReason::NoTarget`] is the case + // that does not, and an empty or invented version there would be worse + // than an opening that claims less. + let available = match &item.target { + Some(target) => format!("{target} is available"), + None => "an update was reported".to_string(), + }; + eprintln!( + "note: left {} = {} alone in {}: {available}, but {}", + item.name, + item.constraint, + manifest.display(), + item.reason.explain() + ); + } } /// Read whole-template overrides from `/dependable-templates/`. diff --git a/crates/dependable/tests/cli_fix.rs b/crates/dependable/tests/cli_fix.rs index 5cdc7d8..7e7d20f 100644 --- a/crates/dependable/tests/cli_fix.rs +++ b/crates/dependable/tests/cli_fix.rs @@ -147,3 +147,599 @@ fn a_read_only_manifest_is_not_destroyed() { "a read-only manifest was truncated" ); } + +// --------------------------------------------------------------------------- +// Declined updates (issue #93) +// +// `check` reports an update, `fix` cannot rewrite the constraint that carries it, +// and until now `fix` said "Everything is already up to date." — the contradiction +// this section falsifies. Proving it needs a registry that actually offers a newer +// release, so these run against a throwaway HTTP server on loopback: hermetic, no +// dependency added, and the real fetch path rather than a stub of it. +// --------------------------------------------------------------------------- + +/// A single-shot registry: a path-to-JSON-body table served on loopback. +/// +/// Deliberately minimal rather than a mock-server crate. `dependable` has no +/// dev-dependencies at all, and the two registries these tests need — an npm +/// packument and a PyPI release map — are each one GET returning one document. +/// Every response closes the connection, so no keep-alive state has to be modelled. +fn registry(routes: Vec<(String, String)>) -> String { + use std::io::{BufRead as _, BufReader, Write as _}; + use std::net::TcpListener; + + let listener = TcpListener::bind("127.0.0.1:0").expect("bind a loopback port"); + let addr = listener.local_addr().expect("read the bound port"); + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + let routes = routes.clone(); + std::thread::spawn(move || { + let mut reader = BufReader::new(stream.try_clone().expect("clone the socket")); + let mut request = String::new(); + if reader.read_line(&mut request).is_err() { + return; + } + // Drain the headers so the client is never left writing into a + // socket nobody is reading, which some stacks report as a reset + // rather than as the response we are about to send. + let mut line = String::new(); + while reader.read_line(&mut line).is_ok_and(|n| n > 2) { + line.clear(); + } + let path = request.split_whitespace().nth(1).unwrap_or("").to_string(); + let body = routes + .iter() + .find(|(route, _)| *route == path) + .map(|(_, body)| body.clone()); + let response = match body { + Some(body) => format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: \ + {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ), + None => "HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: \ + close\r\n\r\n" + .to_string(), + }; + let _ = stream.write_all(response.as_bytes()); + let _ = stream.flush(); + }); + } + }); + format!("http://{addr}") +} + +/// An npm abbreviated packument: the version keys and the `latest` dist-tag are +/// all the version checker reads. +fn packument(versions: &[&str], latest: &str) -> String { + let entries: Vec = versions + .iter() + .map(|v| format!("\"{v}\":{{\"name\":\"lodash\",\"version\":\"{v}\"}}")) + .collect(); + format!( + "{{\"name\":\"lodash\",\"dist-tags\":{{\"latest\":\"{latest}\"}},\"versions\":{{{}}}}}", + entries.join(",") + ) +} + +/// A crates.io sparse-index body: one JSON document per line, and `vers` is the +/// only field the version list is built from. +fn index(versions: &[&str]) -> String { + versions + .iter() + .map(|v| format!("{{\"vers\":\"{v}\"}}")) + .collect::>() + .join("\n") +} + +/// Point the ecosystem fetchers at `base` and switch OSV off, so a run touches +/// nothing but the loopback registry. +/// +/// All three registries every time, including the Cargo one: a fixture with no +/// manifest of a given kind never asks, and a shared config is one fewer thing +/// for a new test to get subtly wrong. +fn write_config(dir: &Path, base: &str) -> PathBuf { + let config = dir.join(".dependable.toml"); + fs::write( + &config, + format!( + "[npm]\nregistry = \"{base}\"\n\n[python]\nregistry = \"{base}/pypi\"\n\n\ + [rust]\nregistry = \"{base}\"\n\n[php]\nregistry = \"{base}\"\n\n\ + [vulnerability]\nenabled = false\n" + ), + ) + .unwrap(); + config +} + +fn run_with_config(dir: &Path, config: &Path, args: &[&str]) -> Output { + run_in(dir, dir, config, args) +} + +/// Run `fix` over `scan` from inside `home`, which is also the process's working +/// directory. Separate arguments because the workspace-member case turns on the +/// two differing: the scan root is the member, and the workspace root that owns +/// the constraint sits above it. +fn run_in(home: &Path, scan: &Path, config: &Path, args: &[&str]) -> Output { + let mut command = Command::new(env!("CARGO_BIN_EXE_dependable")); + command + .arg("fix") + .arg(scan) + .arg("--config") + .arg(config) + .arg("--no-cache") + .arg("--no-vuln") + .args(args); + command.env_remove("DEPENDABLE_FAIL_ON"); + // A user `.npmrc` would override the configured registry and send the run at + // the real npm. + command.env("HOME", home); + command.current_dir(home); + command.output().expect("run dependable fix") +} + +/// Issue #93, exactly as reported: `"lodash": "1.x"` with `1.9.0` in range and +/// `2.0.0` published. `check` reports the update; `fix` cannot write it, because a +/// bare version in npm is one release and the author asked for a line of them. +/// Before this, the run printed "Everything is already up to date." over the top +/// of it and `--dry-run` printed nothing at all. +#[test] +fn a_declined_wildcard_is_reported_instead_of_claimed_up_to_date() { + let dir = workdir("fix_declined_wildcard"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + let original = + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.x\"\n }\n}\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stderr.contains(&format!( + "note: left lodash = 1.x alone in {}: 1.9.0 is available, but a wildcard already \ + tracks new releases, and a bare version here would pin it to one", + manifest.display() + )), + "no note for the declined wildcard.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "fix claimed everything was up to date over an update it declined:\n{stdout}" + ); + assert!( + stdout.contains("Nothing to rewrite. 1 available update left alone"), + "stdout: {stdout}" + ); + // Declining is still declining: the constraint is untouched. + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// `--dry-run` printed nothing whatsoever for the same manifest — the worst form +/// of the defect, because it is the mode people use to find out whether there is +/// anything to do. +#[test] +fn a_dry_run_reports_a_declined_wildcard_too() { + let dir = workdir("fix_declined_wildcard_dry"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.x\"\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &["--dry-run"]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stderr.contains("note: left lodash = 1.x alone in ") + && stderr.contains("package.json: 1.9.0 is available, but a wildcard"), + "stderr: {stderr}" + ); + assert!( + stdout.contains("Nothing to rewrite. 1 available update left alone"), + "stdout: {stdout}" + ); +} + +/// A dist-tag has been silent since long before the wildcard was. `"latest"` +/// resolves to the newest release, so the update only shows once a lockfile holds +/// an older one — and then `fix` must say why it will not pin the channel. +#[test] +fn a_declined_dist_tag_is_reported() { + let dir = workdir("fix_declined_dist_tag"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"latest\"\n }\n}\n", + ) + .unwrap(); + fs::write( + dir.join("package-lock.json"), + "{\n \"lockfileVersion\": 3,\n \"packages\": {\n \"node_modules/lodash\": {\n \ + \"version\": \"1.0.0\"\n }\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stderr.contains("note: left lodash = latest alone in ") + && stderr.contains( + "package.json: 2.0.0 is available, but a dist-tag names a release channel, not \ + a version" + ), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); +} + +/// A compound range: two bounds one version cannot carry. Python, whose +/// comma-separated spelling is a different guard from the space- and +/// `||`-separated one `a_declined_multi_clause_range_is_reported` covers. +#[test] +fn a_declined_comma_range_is_reported() { + let dir = workdir("fix_declined_comma_range"); + let base = registry(vec![( + "/pypi/requests/json".to_string(), + "{\"releases\":{\"1.0.0\":[],\"1.9.0\":[],\"2.0.0\":[]}}".to_string(), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("requirements.txt"); + let original = "requests>=1.0,<2.0\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stderr.contains("note: left requests = >=1.0,<2.0 alone in ") + && stderr.contains( + "requirements.txt: 1.9.0 is available, but a comma-separated range has two \ + bounds and one version cannot carry both" + ), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// The line `fix` prints when there is genuinely nothing to do must survive: a +/// note-driven summary that fired on an ordinary up-to-date run would trade one +/// wrong message for another. +#[test] +fn a_run_with_no_declines_still_says_everything_is_up_to_date() { + let dir = workdir("fix_no_declines"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0"], "1.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.0.0\"\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stdout.contains("Everything is already up to date."), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!(!stderr.contains("note: left"), "stderr: {stderr}"); +} + +// --------------------------------------------------------------------------- +// The other ways an update is left behind +// +// A constraint refusing the rewrite is one of them, and the section above covers +// it. These are the rest: a dependency whose version lives in another file, a pin +// held back for want of `--all`, and a constraint already naming the only version +// in range. Each was silent, and each reproduced issue #93's symptom — `check` +// reports an update, `fix` prints "Everything is already up to date." — without a +// wildcard anywhere in sight. +// --------------------------------------------------------------------------- + +/// A workspace member whose constraint lives in the root, scanned from the member. +/// `collect_manifests` finds only the member; `check_manifest` still resolves the +/// root, so the inherited-skip note fires — and the summary used to print +/// "Everything is already up to date." on stdout directly over it. +#[test] +fn an_inherited_skip_is_not_contradicted_by_the_summary() { + let dir = workdir("fix_inherited_skip"); + let member = dir.join("crates/app"); + fs::create_dir_all(&member).unwrap(); + // A repository boundary: without it the lockfile and workspace walks climb + // out of the temp directory and into the repository this test runs inside. + fs::create_dir_all(dir.join(".git")).unwrap(); + // `2.0.0` is what makes it an update: `1.0.100` is a caret constraint, so + // `1.0.219` satisfies it and `check` calls the member up to date. A release + // outside the range is what `check` reports and `fix` cannot write here. + let base = registry(vec![( + "/se/rd/serde".to_string(), + index(&["1.0.100", "1.0.219", "2.0.0"]), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("Cargo.toml"), + "[workspace]\nresolver = \"2\"\nmembers = [\"crates/app\"]\n\n\ + [workspace.dependencies]\nserde = \"1.0.100\"\n", + ) + .unwrap(); + let manifest = member.join("Cargo.toml"); + let original = "[package]\nname = \"app\"\nversion = \"0.1.0\"\n\n[dependencies]\nserde.workspace = true\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_in(&dir, &member, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stderr.contains("inherits serde from the workspace"), + "no inherited-skip note.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "the summary contradicted the note printed two lines earlier:\n{stdout}" + ); + assert!( + stdout.contains("Nothing to rewrite. 1 available update left alone"), + "stdout: {stdout}\nstderr: {stderr}" + ); + // And the count came from the inherited category alone. An inherited item is + // not rewritable, so the planner drops it before any constraint is consulted + // and it never reaches the declined list — which is precisely why a summary + // counting only that list contradicted the note above. + assert!( + !stderr.contains("note: left"), + "this fixture must produce no constraint decline: {stderr}" + ); + // The member's own file has no version string to rewrite, and did not gain one. + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// A pin is a constraint, and `--all` is the action that moves it. Left silent, +/// this reproduced issue #93 by default configuration on the commonest way a +/// dependency is deliberately held back. +#[test] +fn a_pin_held_back_for_want_of_all_is_reported() { + let dir = workdir("fix_declined_pin"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0"], "1.9.0"), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + let original = + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"=1.0.0\"\n }\n}\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stderr.contains("note: left lodash = =1.0.0 alone in ") + && stderr.contains( + "1.9.0 is available, but the constraint pins one release, and only `--all` \ + moves a pin" + ), + "no note naming `--all` for the pin.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); + + // And `--all` does the thing the note named, with nothing left to say. + let output = run_with_config(&dir, &config, &["--all"]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stdout.contains("lodash =1.0.0 → =1.9.0"), + "stdout: {stdout}" + ); + assert!(!stderr.contains("note: left lodash"), "stderr: {stderr}"); + assert!( + fs::read_to_string(&manifest).unwrap().contains("=1.9.0"), + "`--all` did not move the pin" + ); +} + +/// The constraint already names the only version in range, and a newer release +/// exists outside it. Nothing to write, and — until now — nothing said: the +/// clean line went out over an update `check` had just reported. The same +/// `continue` carries a `Vulnerable` row whose only fixed release is the one +/// already in force. +#[test] +fn a_constraint_already_at_its_target_is_reported() { + let dir = workdir("fix_already_at_target"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + let original = + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.0.0\"\n }\n}\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stderr.contains("note: left lodash = 1.0.0 alone in ") + && stderr.contains( + "1.0.0 is available, but the constraint already names it, and nothing newer \ + satisfies the constraint" + ), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// The summary is on stdout and the notes are on stderr, so it has to say which +/// stream to look at. "see the notes above" named nothing findable for anyone +/// redirecting one of the two — `dependable fix > fix.log` puts the summary in +/// the log and the notes on the terminal. +#[test] +fn the_summary_names_the_stream_the_notes_went_to() { + let dir = workdir("fix_summary_names_stderr"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.x\"\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stdout.contains("see the notes on stderr"), + "stdout: {stdout}" + ); + // The two streams stay apart: the note is not duplicated onto stdout, so the + // summary is pointing somewhere else rather than at itself. + assert!(!stdout.contains("note: left"), "stdout: {stdout}"); + assert!(stderr.contains("note: left lodash"), "stderr: {stderr}"); +} + +/// A Packagist metadata-v2 document: the `version` of each entry is all the +/// version checker reads. +fn packagist(name: &str, versions: &[&str]) -> String { + let entries: Vec = versions + .iter() + .map(|v| format!("{{\"version\":\"{v}\"}}")) + .collect(); + format!("{{\"packages\":{{\"{name}\":[{}]}}}}", entries.join(",")) +} + +/// A space-separated range and a `||` alternation, both of which the constraint +/// front end now translates rather than rejecting — so `DeclineReason::MultiClause` +/// is reached end to end, where it used to be unreachable because such a +/// constraint became `DependencyStatus::Error` long before `fix` saw it. +#[test] +fn a_declined_multi_clause_range_is_reported() { + let dir = workdir("fix_declined_multi_clause"); + let base = registry(vec![ + ( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0", "3.0.0"], "3.0.0"), + ), + ( + "/react".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0", "3.0.0"], "3.0.0"), + ), + ]); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + let original = "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \">=1.0.0 \ + <2.0.0\",\n \"react\": \"^1.0.0 || ^2.0.0\"\n }\n}\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + for (name, constraint, target) in [ + ("lodash", ">=1.0.0 <2.0.0", "1.9.0"), + ("react", "^1.0.0 || ^2.0.0", "2.0.0"), + ] { + assert!( + stderr.contains(&format!("note: left {name} = {constraint} alone in ")) + && stderr.contains(&format!( + "{target} is available, but a space- or `||`-separated range has more than \ + one clause and one version cannot carry them all" + )), + "no note for {name}.\nstdout: {stdout}\nstderr: {stderr}" + ); + } + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// A Composer stability flag. The front end now translates `2.8.*@dev` instead of +/// rejecting it, so the row carries an update and `DeclineReason::Qualifier` is +/// reached end to end — the flag qualifies the *range*, and substituting a +/// concrete release would drop it. Issue #87's harm, refused. +#[test] +fn a_declined_composer_stability_flag_is_reported() { + let dir = workdir("fix_declined_stability_flag"); + let base = registry(vec![( + "/p2/acme/lib.json".to_string(), + packagist("acme/lib", &["1.0.0", "2.8.5", "3.0.0"]), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("composer.json"); + let original = + "{\n \"name\": \"app/app\",\n \"require\": {\n \"acme/lib\": \"2.8.*@dev\"\n }\n}\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stderr.contains("note: left acme/lib = 2.8.*@dev alone in ") + && stderr.contains( + "2.8.5 is available, but an `@` qualifier — a stability flag or an alias — \ + describes the range, not the version" + ), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +}