diff --git a/crates/dependable-core/src/ecosystem.rs b/crates/dependable-core/src/ecosystem.rs index ebbeb82..8272d01 100644 --- a/crates/dependable-core/src/ecosystem.rs +++ b/crates/dependable-core/src/ecosystem.rs @@ -2,6 +2,31 @@ use serde::{Deserialize, Serialize}; +/// How an ecosystem's resolver reads a version written with **no operator**. +/// +/// The distinction is what makes a rewrite safe or unsafe. `dependable fix` +/// replaces a constraint's version span and keeps its operator prefix, so a +/// constraint that carried no operator is written back as a bare version — and +/// what a bare version *means* decides whether the rewrite preserved the +/// author's constraint or quietly replaced it with a different one. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[non_exhaustive] +pub enum BareVersion { + /// One release and no other: `1.9.0` matches `1.9.0` alone. + Exact, + /// A caret range: at least this release, below the next major — + /// `1.9.0` is `>=1.9.0, <2.0.0`. + Caret, + /// An inclusive minimum with no upper bound: `1.9.0` is `>=1.9.0`. + /// + /// A floor, not a preference: the resolver may pick any release at or above + /// it and never one below. An ecosystem whose bare version merely *suggests* + /// a release — one a resolver is free to satisfy with something older, as + /// Maven's nearest-wins mediation is — is not this reading, and no reading + /// here yet describes it. + Minimum, +} + /// A package ecosystem. /// /// Every variant is wired end-to-end: a parser, a registry fetcher, and an OSV @@ -72,6 +97,50 @@ impl Ecosystem { } } + /// How this ecosystem's resolver reads a version written with no operator. + /// + /// A wrong answer rewrites someone's manifest into a constraint they did not + /// write, so every variant is settled from the resolver's own documentation + /// rather than by family resemblance — the three ecosystems that look alike + /// here (Go, NuGet, Gradle) all read a bare version as a minimum, while three + /// that look like Cargo (npm, Composer, pub) do not. + /// + /// | Ecosystem | Reading | Why | + /// | --- | --- | --- | + /// | Rust | [`Caret`](BareVersion::Caret) | The Cargo book: "Specifying only the version number is equivalent to a caret requirement" — `serde = "1.0"` *is* `^1.0`. | + /// | Go | [`Minimum`](BareVersion::Minimum) | A `require` line states the lowest version the module needs; minimal version selection then builds with the highest such requirement in the graph. | + /// | Npm | [`Exact`](BareVersion::Exact) | node-semver: a fully specified version is a comparator with an implicit `=`. A *partial* bare version is an X-range instead (`"16"` is `16.x`), which is why a caller must not treat `Exact` as "every bare string names one release". | + /// | Python | [`Exact`](BareVersion::Exact) | PEP 508 has no bare form at all — an operator is mandatory — so the only reading that occurs is Poetry's, whose "exact requirements" are written bare and install "this version and this version only". | + /// | Php | [`Exact`](BareVersion::Exact) | Composer's exact version constraint is the bare form: "install this version and this version only". A range needs the wildcard spelled out (`1.0.*`). | + /// | Dart | [`Exact`](BareVersion::Exact) | pub's traditional-syntax table reads `1.2.3` as "only the given version", and the docs steer authors to `^1.2.3` precisely because the bare form is that restrictive. | + /// | CSharp | [`Minimum`](BareVersion::Minimum) | NuGet's range table: `1.0` is `x ≥ 1.0`, "minimum version, inclusive". `[1.0]` is how an exact match is written. | + /// | Elixir | [`Exact`](BareVersion::Exact) | A Hex requirement with no operator is an equality requirement: `Version.match?("2.0.1", "2.0.0")` is false. Floating needs `~>`. | + /// | Jvm | [`Minimum`](BareVersion::Minimum) | Answered for the Gradle version catalogs this variant currently reaches, and only those: a plain Gradle version string is a *required* version — the minimum, "optimistically upgraded" by conflict resolution — not a pin; `strictly` is the pinning form. Maven's plain `` is **not** settled by this row. It is a soft requirement — a preference nearest-wins mediation may satisfy with an *older* release — which is not a floor and so not [`Minimum`](BareVersion::Minimum); the reading it needs is unresolved, and has to be settled before a `pom.xml` parser reaches this variant. | + #[must_use] + pub fn bare_version(self) -> BareVersion { + match self { + Ecosystem::Rust => BareVersion::Caret, + Ecosystem::Go | Ecosystem::CSharp | Ecosystem::Jvm => BareVersion::Minimum, + Ecosystem::Npm + | Ecosystem::Python + | Ecosystem::Php + | Ecosystem::Dart + | Ecosystem::Elixir => BareVersion::Exact, + } + } + + /// Whether a version written with no operator pins exactly one release. + /// + /// The question a rewriter asks most often, and the one with the sharpest + /// consequence: where a bare version is an exact pin, replacing a floating + /// constraint with a concrete release destroys the range the author asked + /// for. Shorthand for [`Self::bare_version`], which carries the full reading + /// and the reasoning behind it. + #[must_use] + pub fn bare_version_is_exact(self) -> bool { + matches!(self.bare_version(), BareVersion::Exact) + } + /// The page a person would open to read about `name`. /// /// Distinct from [`Self::default_registry`], which is the API this tool @@ -251,6 +320,63 @@ mod tests { } } + /// Every variant states how its resolver reads a bare version, so adding an + /// ecosystem forces the decision rather than inheriting a default. The + /// expected value is spelled out per variant on purpose: a loop asserting + /// only "it returns something" would pass with every answer wrong, and a + /// wrong answer here silently rewrites a manifest into a different + /// constraint. + #[test] + fn every_ecosystem_states_how_it_reads_a_bare_version() { + let expected = [ + // `serde = "1.0"` is `^1.0` — the Cargo book says so outright. + (Ecosystem::Rust, BareVersion::Caret), + // A `require` line is the lowest version the module needs; MVS takes + // the highest such requirement across the graph. + (Ecosystem::Go, BareVersion::Minimum), + // node-semver: a full version is a comparator with an implicit `=`. + (Ecosystem::Npm, BareVersion::Exact), + // Poetry's "exact requirements" are the bare form; PEP 508 has none. + (Ecosystem::Python, BareVersion::Exact), + // Composer: "this version and this version only". + (Ecosystem::Php, BareVersion::Exact), + // pub's traditional syntax: `1.2.3` is "only the given version". + (Ecosystem::Dart, BareVersion::Exact), + // NuGet's range table: `1.0` is `x >= 1.0`, minimum inclusive. + (Ecosystem::CSharp, BareVersion::Minimum), + // A Hex requirement with no operator is an equality requirement. + (Ecosystem::Elixir, BareVersion::Exact), + // A plain Gradle version is `require`: a minimum, upgradable by + // conflict resolution. + (Ecosystem::Jvm, BareVersion::Minimum), + ]; + assert_eq!(expected.len(), ALL.len(), "every variant must be listed"); + for (ecosystem, reading) in expected { + assert_eq!(ecosystem.bare_version(), reading, "{ecosystem:?}"); + } + } + + /// The shorthand and the full reading cannot drift apart: one is defined in + /// terms of the other, and this pins that they stay that way. + #[test] + fn the_exactness_shorthand_agrees_with_the_full_reading() { + for ecosystem in ALL { + assert_eq!( + ecosystem.bare_version_is_exact(), + ecosystem.bare_version() == BareVersion::Exact, + "{ecosystem:?}" + ); + } + // The two ecosystems the distinction was drawn for: Cargo reads a bare + // version as a range, npm as one release. + assert!(!Ecosystem::Rust.bare_version_is_exact()); + assert!(Ecosystem::Npm.bare_version_is_exact()); + // And the one that is neither: a bare NuGet version floats upward, but + // without an upper bound, so it is not a pin either. + assert!(!Ecosystem::CSharp.bare_version_is_exact()); + assert_eq!(Ecosystem::CSharp.bare_version(), BareVersion::Minimum); + } + #[test] fn rust_maps_to_crates_io_for_osv() { assert_eq!(Ecosystem::Rust.osv_name(), "crates.io"); diff --git a/crates/dependable-core/src/lib.rs b/crates/dependable-core/src/lib.rs index 3f30f33..c3741b8 100644 --- a/crates/dependable-core/src/lib.rs +++ b/crates/dependable-core/src/lib.rs @@ -15,7 +15,7 @@ pub mod parsers; pub mod result; pub mod semver; -pub use ecosystem::Ecosystem; +pub use ecosystem::{BareVersion, Ecosystem}; pub use error::ParseError; pub use graph::{ DependencyGraph, Node, NodeKind, PathPredicate, Placement, Tree, TreeNode, TreeOptions, Visit, diff --git a/crates/dependable-fetch/src/lib.rs b/crates/dependable-fetch/src/lib.rs index 098d04d..0ba6bc5 100644 --- a/crates/dependable-fetch/src/lib.rs +++ b/crates/dependable-fetch/src/lib.rs @@ -78,10 +78,11 @@ pub use registries::{ // parsers, `check_version`, ...). pub use dependable_core as core; pub use dependable_core::{ - CheckResult, DependencyGraph, DependencyKind, DependencyStatus, Ecosystem, ErrorOrigin, - Evaluation, Item, LockfileKind, ManifestKind, Node, NodeKind, PackageSource, ParseError, - ParsedManifest, PathPredicate, Placement, Tree, TreeNode, TreeOptions, UnstableFilter, Visit, - Visitor, WalkOptions, WalkStats, WorkspaceDecl, resolve_workspace_inheritance, + BareVersion, CheckResult, DependencyGraph, DependencyKind, DependencyStatus, Ecosystem, + ErrorOrigin, Evaluation, Item, LockfileKind, ManifestKind, Node, NodeKind, PackageSource, + ParseError, ParsedManifest, PathPredicate, Placement, Tree, TreeNode, TreeOptions, + UnstableFilter, Visit, Visitor, WalkOptions, WalkStats, WorkspaceDecl, + resolve_workspace_inheritance, }; /// One-import convenience for consumers: `use dependable_fetch::prelude::*;`. diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index b998a21..32f9c22 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -5,12 +5,20 @@ //! surrounding formatting and comments untouched. The leading operator/`v` prefix //! is preserved (`^1.0` → `^1.5.0`, `v1.2.3` → `v1.5.0`) so a constraint's meaning //! is not silently changed (e.g. an npm caret range is not turned into a pin). +//! +//! Preserving the operator is not enough on its own, because a constraint that +//! carried no operator is written back as a bare version — and what a bare +//! version means is an ecosystem question, not a string one. Cargo's `1.*` and +//! npm's `1.x` are the same shape and opposite calls: `1.0.219` is `^1.0.219` in +//! one and one release in the other. The manifest path answers it, via +//! [`Ecosystem::bare_version`]; see [`rewrite_constraint`]. use std::collections::HashMap; use std::io::Write as _; use std::path::Path; use anyhow::Context; +use dependable_fetch::core::{BareVersion, Ecosystem, ManifestKind}; use dependable_fetch::{CheckResult, DependencyKind, DependencyStatus}; /// A single applied (or would-be-applied) version change. @@ -63,7 +71,12 @@ pub struct PlannedFix { pub fn plan(manifest: &Path, results: &[CheckResult], all: bool) -> anyhow::Result { let content = std::fs::read_to_string(manifest) .with_context(|| format!("reading {}", manifest.display()))?; - let (updated, records) = plan_fixes(&content, results, all) + // What a rewritten constraint *means* is an ecosystem question, and the file + // name is where the answer is. `None` — a manifest discovery surfaced but + // `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) .with_context(|| format!("rewriting {}", manifest.display()))?; Ok(PlannedFix { path: manifest.to_path_buf(), @@ -115,13 +128,19 @@ pub fn commit(planned: &PlannedFix) -> anyhow::Result<()> { } /// Compute the rewritten manifest and the applied records from `content` and the -/// check `results`, with no filesystem IO (the file boundary lives in -/// [`apply_fixes`]). Format-agnostic: it edits each recorded version span in place, -/// so JSON, YAML, and TOML manifests are rewritten without reformatting. +/// check `results`, with no filesystem IO (the file boundary lives in [`plan`]). +/// Format-agnostic: it edits each recorded version span in place, so JSON, YAML, +/// and TOML manifests are rewritten without reformatting. +/// +/// `ecosystem` decides which constraints are safe to rewrite at all — +/// [`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. fn plan_fixes( content: &str, results: &[CheckResult], all: bool, + ecosystem: Option, ) -> anyhow::Result<(String, Vec)> { let mut edits: Vec = Vec::new(); let mut records = Vec::new(); @@ -159,7 +178,8 @@ fn plan_fixes( result.latest_compatible.as_ref() }; let Some(target) = target else { continue }; - let Some(new_constraint) = rewrite_constraint(&item.version_constraint, target) else { + let Some(new_constraint) = rewrite_constraint(&item.version_constraint, target, ecosystem) + else { continue; }; if new_constraint == item.version_constraint { @@ -190,13 +210,26 @@ fn plan_fixes( /// Build a new constraint from `original`, preserving its leading operator/`v` /// prefix and substituting `new_version`. Returns `None` for the forms that -/// can't be rewritten to a single version 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`), a wildcard (`*`, `1.x`, `1.*`), or anything carrying an `@` -/// (a Composer stability flag such as `@dev` or `^1.0@beta`, an npm alias such -/// as `npm:pkg@1.0.0`). -fn rewrite_constraint(original: &str, new_version: &str) -> Option { +/// 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 +/// `@` (a Composer stability flag such as `@dev` or `^1.0@beta`, an npm alias +/// such as `npm:pkg@1.0.0`), a partial version behind a tilde operator (`~1`, +/// `~> 1.0`, `~=1.4`), and — depending on `ecosystem` — a wildcard (`*`, `1.x`, +/// `1.*`) or a partial version (npm `"16"`, Cargo `"0"`). +/// +/// `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` +/// means the manifest kind was not recognized; it is read as +/// [`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. +fn rewrite_constraint( + original: &str, + new_version: &str, + ecosystem: Option, +) -> Option { let trimmed = original.trim(); if trimmed.contains(',') { return None; @@ -228,20 +261,178 @@ fn rewrite_constraint(original: &str, new_version: &str) -> Option { if rest.starts_with(|c: char| c.is_ascii_alphabetic()) { return None; } - // A wildcard (`*`, `1.x`, `1.*`, Gradle's `1.+`) is a range the author chose, - // not a version — substituting a concrete release narrows it to a pin (#87). - // The decline is deliberately blanket rather than per-ecosystem: substituting - // is safe where a bare version is a caret or an inclusive minimum (Cargo, Go, - // Dart) and unsafe where it is an exact pin (npm, Composer, Hex), and this - // signature carries no `Ecosystem` to tell them apart. `apply_fixes` does hold - // the manifest path and could supply one — #92 tracks narrowing the decline to - // the ecosystems that need it. Until then, decline as a dist-tag is declined. + + let bare = ecosystem.map_or(BareVersion::Exact, Ecosystem::bare_version); + if is_wildcard(rest) { - return None; + // A wildcard (`*`, `1.x`, `1.*`, Gradle's `1.+`) is a range the author + // chose, not a version. Substituting a concrete release preserves it in + // exactly one situation: the constraint carries no operator, the wildcard + // sits in the minor position over a major a caret can key its bound off, + // and the ecosystem reads the bare version we write back as a caret range. + // Then `1.*` (`>=1.0.0, <2.0.0`) becomes `1.0.219` (`>=1.0.219, <2.0.0`) + // — the floor is raised, which is what `fix` does to every other + // constraint, and the author's upper bound survives. + // + // That last guarantee belongs to the default path. Under `--all` the + // target is `latest_available` rather than `latest_compatible`, so + // `serde = "1.*"` becomes `serde = "3.4.0"` and the author's upper bound + // does *not* survive. Deliberate: `--all` is documented as updating + // "beyond the declared constraint" and already does exactly this to + // `^1.0`, so carving wildcards out of it would make one flag mean two + // things. + // + // Every other combination changes what the constraint admits (#87, #92): + // + // - [`BareVersion::Exact`] collapses the range to one release: npm's + // `"lodash": "1.x"` would become `"1.9.0"`, and Composer and Hex read a + // bare version the same way. + // - [`BareVersion::Minimum`] loses the upper bound instead: NuGet's `1.*` + // is any 1.x resolved to the newest, while a bare `1.9.0` is `>= 1.9.0` + // resolved to the *oldest* — a different range and a different pick. + // Gradle's `1.+` against its `require` semantics is the same trade. + // - A bare `*` is every version, and any concrete release confines it to + // one major — a narrowing even where the bare form is a caret. + // - `1.2.*` is `>=1.2.0, <1.3.0`, and a caret over any 1.2.z release + // reaches to `<2.0.0` — a widening even where the bare form is a caret. + // - `0.*` is `>=0.0.0, <1.0.0`, but a caret is *minor*-scoped below + // 1.0.0: 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 — + // a narrowing, and the one shape `is_minor_wildcard` would otherwise + // wave through. + // - 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; + } + } else if is_partial_version(rest) { + if prefix.is_empty() { + // The same harm one wildcard character away, reached under two of the + // three readings. + // + // 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 + // rewriting it to `"16.14.0"` pins a dependency that was tracking a + // line of releases, with no `*` anywhere for `is_wildcard` to see. + // Guarded for every ecosystem that reads a bare version exactly, not + // just npm: Composer normalizes a partial to a full version and Hex + // and pub reject one outright, so there the rewrite would have been + // harmless — but "harmless" is the whole claim being made, and + // declining costs a fix on a constraint that already pins while a + // wrong `true` costs the author their range. + // + // Where a bare version is a *caret*, a partial is usually safe — + // Cargo's `1.0` is `^1.0` and `1.5.0` is `^1.5.0`, so the floor rises + // 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`]. + // + // [`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 + // upper bound to lose, and raising a floor is exactly what `fix` is. + // + // 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; + } + } else if prefix.contains('~') { + // A tilde operator reads its upper bound off the *number of + // components* it was given, so substituting the three-component + // version `fix` writes back collapses the range the author asked for. + // Hex's `~> 1.0` is `>=1.0.0, <2.0.0` while `~> 1.7.10` is + // `>=1.7.10, <1.8.0`; PEP 440's `~=1.4` is `>=1.4.0, <2.0.0` while + // `~=1.5.0` is `>=1.5.0, <1.6.0`; npm's and Cargo's `~1` is + // major-wide while `~1.5.0` is minor-wide. That is #87's harm behind + // an operator instead of a wildcard. + // + // Declined for `~`, `~>` and `~=` alike, in every ecosystem — `~` is + // the only operator character any of the three carries. The arity + // each is sensitive at differs (npm's `~1.2` is already minor-wide), + // and declining a form that happened to be safe costs a fix while + // permitting one that is not costs the author their range: the same + // call already taken for `1.2.*`. + // + // 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; + } } Some(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`. +/// +/// 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`]. +fn is_minor_wildcard(rest: &str) -> bool { + let mut segments = rest.split('.'); + let (Some(major), Some(minor), None) = (segments.next(), segments.next(), segments.next()) + else { + return false; + }; + !major.is_empty() + && major.bytes().all(|b| b.is_ascii_digit()) + && caret_bound_survives_substitution(major) + && matches!(minor, "*" | "x" | "X") +} + +/// Whether a caret reading of the concrete release written back reproduces the +/// upper bound of a constraint that supplied only `components` — the dotted +/// numeric prefix of a minor wildcard (`1`, of `1.*`) or the whole of a partial +/// version (`1.0`, or `0`). +/// +/// A caret keys its bound off the **leftmost non-zero component**: `^1`, `^1.2` +/// and `^1.2.3` all stop below `2.0.0`, while `^0.9` stops below `0.10.0` and +/// `^0.0.5` below `0.0.6`. So a constraint that wrote a non-zero component keeps +/// its bound under substitution — every release the constraint admits agrees +/// with it up to and including that component, so the release written back keys +/// its own caret off the same one. +/// +/// A constraint whose components are *all zero* has no such component, and takes +/// its bound from a position it never wrote — always wider than any concrete +/// release can reproduce. `0.*` and a bare `0` are both `<1.0.0` while every +/// `0.y.z` reaches at most `<0.(y+1).0`; `0.0` is `<0.1.0` while every `0.0.z` +/// reaches only `<0.0.(z+1)`. Substituting narrows all three — issue #87's harm +/// arriving through the door #92 opened. +/// +/// Callers pass components already known to be non-empty and all-digit. An empty +/// segment would answer `false` here, which declines, so the failure direction +/// is the safe one either way. +fn caret_bound_survives_substitution(components: &str) -> bool { + components + .split('.') + .any(|segment| segment.bytes().any(|byte| byte != b'0')) +} + +/// Whether `rest` — a constraint with its operator prefix already stripped, and +/// already known to hold no wildcard — names fewer components than a full +/// version: `16` or `1.0` rather than `1.0.0`. +/// +/// Every component must be pure digits, which is what keeps the concrete forms +/// out: Python's `1!2.0` carries an epoch, semver build metadata and prereleases +/// put non-digits in the last component, and NuGet's `1.0.0.4` has four. +fn is_partial_version(rest: &str) -> bool { + let segments: Vec<&str> = rest.split('.').collect(); + segments.len() < 3 + && segments + .iter() + .all(|segment| !segment.is_empty() && segment.bytes().all(|b| b.is_ascii_digit())) +} + /// Whether `rest` — a constraint with its leading operator prefix already /// stripped, and already known to carry no `@` — floats over a range of versions /// rather than naming one. @@ -330,7 +521,13 @@ mod tests { "the fixture must produce an override item" ); - let (updated, records) = plan_fixes(content, &results, true).expect("the plan applies"); + let (updated, records) = plan_fixes( + content, + &results, + true, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); // The override is declined; its non-override neighbour is still fixed, so this // asserts the guard rather than a `--all` path that happens to do nothing. @@ -348,72 +545,385 @@ mod tests { ); } + /// Every ecosystem, so a guard that is ecosystem-independent is asserted + /// against all of them rather than against a convenient one — and so a new + /// variant cannot quietly opt out of a claim made here. + const EVERY_ECOSYSTEM: [Ecosystem; 9] = [ + Ecosystem::Rust, + Ecosystem::Go, + Ecosystem::Npm, + Ecosystem::Python, + Ecosystem::Php, + Ecosystem::Dart, + Ecosystem::CSharp, + Ecosystem::Elixir, + Ecosystem::Jvm, + ]; + + /// Preserving the operator prefix has nothing to do with the ecosystem: every + /// form here either carries an operator or is a full concrete version, so what + /// a *bare* version means cannot bear on it. Asserted against all nine rather + /// than one, which is what makes that a claim instead of an assumption. #[test] fn rewrite_preserves_operator_prefix() { + for ecosystem in EVERY_ECOSYSTEM { + let it = Some(ecosystem); + assert_eq!( + rewrite_constraint("^1.0", "1.5.0", it).as_deref(), + Some("^1.5.0"), + "{ecosystem:?}" + ); + // Full arity, because a tilde reads its upper bound off the number of + // components it was given: a partial `~1.0` is declined by the arity + // guard, which is `rewrite_declines_a_partial_version_behind_a_tilde`'s + // claim, not this test's. + assert_eq!( + rewrite_constraint("~1.0.0", "1.5.0", it).as_deref(), + Some("~1.5.0"), + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint(">=1.0", "1.5.0", it).as_deref(), + Some(">=1.5.0"), + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("v1.2.3", "1.5.0", it).as_deref(), + Some("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"), + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("=1.2.0", "1.5.0", it).as_deref(), + Some("=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:?}"); + } + } + + #[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, + "{ecosystem:?}" + ); + } + } + + #[test] + fn rewrite_skips_dist_tags() { + // npm dist-tags / channels are not version ranges — never pin them, so a + // `"latest"` dependency keeps tracking the channel after `--fix`. The + // guard is a leading-letter test, so it fires for every ecosystem, and a + // 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, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("next", "2.3.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("beta", "2.3.0", it), + None, + "{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:?}"); + } + } + + /// Issue #87: a wildcard is a range, not a version. Rewriting `1.x` to a + /// concrete release narrows what the author wrote into a pin — in npm a bare + /// version is an exact match, so the floating constraint is destroyed. None of + /// the other guards sees a wildcard: there is no comma, no space or `|` after + /// the (empty) operator prefix, and `1.x` starts with a digit so the dist-tag + /// guard passes it through. + /// + /// Issue #92 narrowed the decline to the ecosystems that need it, so the rule + /// is now asserted in two halves. Wherever a bare version is *not* a caret + /// range, every wildcard shape is still declined; and where it is — Cargo + /// alone — every shape a caret does not reproduce is still declined too. The + /// one shape that survives has its own test. + #[test] + fn rewrite_never_narrows_a_wildcard_to_a_pin() { + for ecosystem in EVERY_ECOSYSTEM { + if ecosystem.bare_version() == BareVersion::Caret { + continue; + } + let it = Some(ecosystem); + assert_eq!( + rewrite_constraint("1.x", "2.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("1.*", "2.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("1.X", "2.0.0", it), + None, + "{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, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("^1.x", "2.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("1.2.x", "2.0.0", it), + None, + "{ecosystem:?}" + ); + // The bare wildcard is the same kind of thing. + assert_eq!(rewrite_constraint("*", "2.0.0", it), None, "{ecosystem:?}"); + } + + // Cargo reads a bare version as a caret, which reproduces exactly one + // wildcard shape. The rest are declined there too, each for its own reason. + 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); + // 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); + // `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); + // `*` is every version; any concrete release confines it to one major. + assert_eq!(rewrite_constraint("*", "2.0.0", cargo), None); + } + + /// The other side of issue #92: declining every wildcard was conservatism, not + /// necessity. Where the ecosystem reads a bare version as a caret range, `1.*` + /// and `1.0.219` are the same *kind* of constraint — `>=1.0.0, <2.0.0` and + /// `>=1.0.219, <2.0.0` — so substituting raises the floor exactly as it does + /// for `^1.0`, and the manifest keeps a range in the form the Cargo book calls + /// equivalent. Before this, `serde = "1.*"` was simply left behind by `fix`. + #[test] + fn rewrite_updates_a_minor_wildcard_where_a_bare_version_is_a_caret() { + let cargo = Some(Ecosystem::Rust); assert_eq!( - rewrite_constraint("^1.0", "1.5.0").as_deref(), - Some("^1.5.0") + rewrite_constraint("1.*", "1.0.219", cargo).as_deref(), + Some("1.0.219") ); assert_eq!( - rewrite_constraint("~1.0", "1.5.0").as_deref(), - Some("~1.5.0") + rewrite_constraint("1.x", "1.0.219", cargo).as_deref(), + Some("1.0.219") ); assert_eq!( - rewrite_constraint(">=1.0", "1.5.0").as_deref(), - Some(">=1.5.0") + rewrite_constraint("1.X", "1.0.219", cargo).as_deref(), + Some("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. + for ecosystem in EVERY_ECOSYSTEM { + if ecosystem == Ecosystem::Rust { + continue; + } + assert_eq!( + rewrite_constraint("1.*", "1.0.219", Some(ecosystem)), + None, + "{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); + } + + /// The zero-major hole in that permit. Cargo's caret is *minor*-scoped below + /// 1.0.0, so the guarantee the wildcard rewrite is justified by — the author's + /// upper bound survives — does not hold there. `0.*` is `>=0.0.0, <1.0.0`, and + /// the release written back is some `0.y.z`, which as a bare version is + /// `^0.y.z` and reaches only to `<0.(y+1).0`. A manifest that admitted + /// `0.11.0` stops admitting it: issue #87's harm, reached through the door + /// issue #92 opened, and declined for the same reason `1.2.*` and `*` are. + /// + /// The bare partial forms reach it without a wildcard character and are + /// declined by the same predicate: `0` is `^0` (`<1.0.0`) and `0.0` is `^0.0` + /// (`<0.1.0`), and no concrete release reproduces either bound. + #[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 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); + // 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("v1.2.3", "1.5.0").as_deref(), - Some("v1.5.0") + rewrite_constraint("0.9", "0.9.5", cargo).as_deref(), + Some("0.9.5") ); assert_eq!( - rewrite_constraint("1.0.0", "1.5.0").as_deref(), - Some("1.5.0") + rewrite_constraint("0.9.1", "0.9.5", cargo).as_deref(), + Some("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. + for ecosystem in EVERY_ECOSYSTEM { + if ecosystem == Ecosystem::Rust { + continue; + } + for original in ["0.*", "0.x", "0.X"] { + assert_eq!( + rewrite_constraint(original, "0.10.0", Some(ecosystem)), + None, + "{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. assert_eq!( - rewrite_constraint("=1.2.0", "1.5.0").as_deref(), - Some("=1.5.0") + rewrite_constraint("0", "0.10.0", Some(Ecosystem::Npm)), + None + ); + assert_eq!( + rewrite_constraint("0", "0.10.0", Some(Ecosystem::CSharp)).as_deref(), + Some("0.10.0") ); - // The bare wildcard `*` is a range, not a version — see - // `rewrite_never_narrows_a_wildcard_to_a_pin`. - assert_eq!(rewrite_constraint("*", "1.5.0"), None); - } - - #[test] - fn rewrite_skips_multi_constraint() { - assert_eq!(rewrite_constraint(">=1.0,<2.0", "1.5.0"), None); } + /// A tilde operator reads its upper bound off the *number of components* it + /// was given, so a partial version behind one describes a wider range than the + /// three-component version `fix` writes back. This repository's own + /// translators pin the arities rather than leaving them to memory: + /// `semver::elixir` expands `~> a.b` to `>=a.b.0, <(a+1).0.0` against + /// `~> a.b.c`'s `=1.4.0, <2.0.0` against `~=1.4.2`'s `>=1.4.2, <1.5.0`. + /// + /// So `{:phoenix, "~> 1.0"}` rewritten to `~> 1.7.10` turns the author's 1.x + /// range into a 1.7.x one — issue #87's harm behind an operator instead of a + /// wildcard. Declined for `~`, `~>` and `~=` alike and in every ecosystem: the + /// arity each is sensitive at differs, and declining a form that happened to + /// be safe costs a fix while permitting one that is not costs the author their + /// range — the call already taken for `1.2.*`. #[test] - fn rewrite_skips_dist_tags() { - // npm dist-tags / channels are not version ranges — never pin them, so a - // `"latest"` dependency keeps tracking the channel after `--fix`. - assert_eq!(rewrite_constraint("latest", "2.3.0"), None); - assert_eq!(rewrite_constraint("next", "2.3.0"), None); - assert_eq!(rewrite_constraint("beta", "2.3.0"), None); - // The wildcard `*` is declined for the same reason: it is a range the - // author chose, and pinning it would narrow their manifest (issue #87). - assert_eq!(rewrite_constraint("*", "2.3.0"), None); + fn rewrite_declines_a_partial_version_behind_a_tilde() { + 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:?}" + ); + // 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, + "{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"), + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("~> 1.0.0", "1.5.0", it).as_deref(), + Some("~> 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"), + "{ecosystem:?}" + ); + } } - /// Issue #87: a wildcard is a range, not a version. Rewriting `1.x` to a - /// concrete release narrows what the author wrote into a pin — in npm a bare - /// version is an exact match, so the floating constraint is destroyed. None of - /// the three existing guards sees a wildcard: there is no comma, no space or - /// `|` after the (empty) operator prefix, and `1.x` starts with a digit so the - /// dist-tag guard passes it through. + /// Issue #92's second gap, and the one with no `*` in it. npm reads a partial + /// version as an X-range — `"react": "16"` is `16.x`, `"1.0"` is `1.0.x` — so + /// rewriting one to `"16.14.0"` pins a dependency that was tracking a line of + /// releases. `is_wildcard` sees nothing to object to, and the only thing + /// separating it from Cargo's `"1.0"`, where that rewrite is correct, is which + /// ecosystem is being written. #[test] - fn rewrite_never_narrows_a_wildcard_to_a_pin() { - assert_eq!(rewrite_constraint("1.x", "2.0.0"), None); - assert_eq!(rewrite_constraint("1.*", "2.0.0"), None); - assert_eq!(rewrite_constraint("1.X", "2.0.0"), None); - // 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"), None); - assert_eq!(rewrite_constraint("^1.x", "2.0.0"), None); - assert_eq!(rewrite_constraint("1.2.x", "2.0.0"), None); - // The bare wildcard is the same kind of thing. - assert_eq!(rewrite_constraint("*", "2.0.0"), None); + 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); + // 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") + ); + // 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") + ); + // 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") + ); + // 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") + ); + // An unrecognized manifest declines, like every exact reading. + assert_eq!(rewrite_constraint("16", "16.14.0", None), None); } /// A wildcard segment is not always the whole dot-segment. Composer allows a @@ -422,13 +932,40 @@ mod tests { /// whole-segment guard hands back `7.0.0`, an exact pin in Composer that /// destroys both the wildcard and the stability flag. That is issue #87 one /// flag away from the guard. + /// + /// The `@` guard that catches these runs before the ecosystem is consulted, so + /// the verdict is the same for all nine — including Cargo, where the wildcard + /// alone would now be rewritten. #[test] fn rewrite_declines_a_wildcard_wearing_a_stability_flag() { - assert_eq!(rewrite_constraint("2.8.*@dev", "7.0.0"), None); - assert_eq!(rewrite_constraint("2.8.x@dev", "7.0.0"), None); - assert_eq!(rewrite_constraint("1.*@stable", "7.0.0"), None); - assert_eq!(rewrite_constraint("*@dev", "7.0.0"), None); - assert_eq!(rewrite_constraint("^2.8.*@dev", "7.0.0"), None); + for ecosystem in EVERY_ECOSYSTEM { + let it = Some(ecosystem); + assert_eq!( + rewrite_constraint("2.8.*@dev", "7.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("2.8.x@dev", "7.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("1.*@stable", "7.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("*@dev", "7.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("^2.8.*@dev", "7.0.0", it), + None, + "{ecosystem:?}" + ); + } } /// A Composer stability flag qualifies the range, and nothing in a rewritten @@ -437,19 +974,43 @@ mod tests { /// version used to pass every guard and be rewritten flag-free — `">=2.8@dev"` /// became `">=7.0.0"`, and the unbounded `"@dev"` ("any version, dev /// stability") became the exact pin `"7.0.0"`: issue #87's harm again, reached - /// without a wildcard. An `@` never belongs to a version, so decline the lot. + /// without a wildcard. An `@` never belongs to a version, so decline the lot, + /// in every ecosystem — the guard runs before the ecosystem is consulted. #[test] fn rewrite_declines_a_stability_flag_on_a_plain_version() { - // The bare flag: a range over every version, collapsed to a pin. - assert_eq!(rewrite_constraint("@dev", "7.0.0"), None); - // Flag on an operator-led constraint, and on a bare version. - assert_eq!(rewrite_constraint(">=2.8@dev", "7.0.0"), None); - assert_eq!(rewrite_constraint("2.8@dev", "7.0.0"), None); - assert_eq!(rewrite_constraint("^1.0@beta", "7.0.0"), None); - // 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"), None); + 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, + "{ecosystem:?}" + ); + // Flag on an operator-led constraint, and on a bare version. + assert_eq!( + rewrite_constraint(">=2.8@dev", "7.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("2.8@dev", "7.0.0", it), + None, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("^1.0@beta", "7.0.0", it), + None, + "{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, + "{ecosystem:?}" + ); + } } /// The other side of the guard: every concrete form the shipped parsers @@ -458,69 +1019,100 @@ mod tests { /// silently stop `fix` from working on ordinary dependencies, so each shape /// is asserted by name. None of them contains an `@`, which is what makes /// declining every `@` form safe. + /// + /// Each shape is now asserted against the ecosystem that actually writes it, + /// which is a stronger claim than the single-ecosystem version it replaces: a + /// partial-version or wildcard guard that fired on the wrong reading would + /// take one of these with it. #[test] fn rewrite_leaves_every_concrete_version_form_rewritable() { + let go = Some(Ecosystem::Go); + let rust = Some(Ecosystem::Rust); + let nuget = Some(Ecosystem::CSharp); + let python = Some(Ecosystem::Python); + let hex = Some(Ecosystem::Elixir); + // Go: a pseudo-version and the `+incompatible` marker. assert_eq!( - rewrite_constraint("v0.0.0-20191109021931-daa7c04131f5", "1.5.0").as_deref(), + rewrite_constraint("v0.0.0-20191109021931-daa7c04131f5", "1.5.0", go).as_deref(), Some("v1.5.0") ); assert_eq!( - rewrite_constraint("v2.0.0+incompatible", "1.5.0").as_deref(), + rewrite_constraint("v2.0.0+incompatible", "1.5.0", go).as_deref(), Some("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").as_deref(), + rewrite_constraint("1.2.3+build.5", "1.5.0", rust).as_deref(), Some("1.5.0") ); assert_eq!( - rewrite_constraint("1.0.0-alpha+exp.sha.5114f85", "1.5.0").as_deref(), + rewrite_constraint("1.0.0-alpha+exp.sha.5114f85", "1.5.0", rust).as_deref(), Some("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").as_deref(), + rewrite_constraint("1.0.0-x.7.z.92", "1.5.0", rust).as_deref(), Some("1.5.0") ); - // NuGet's four-part version. + // 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").as_deref(), + rewrite_constraint("1.0.0.4", "1.5.0", nuget).as_deref(), Some("1.5.0") ); // Python epochs and compatible-release operators. assert_eq!( - rewrite_constraint("1!2.0", "1.5.0").as_deref(), + rewrite_constraint("1!2.0", "1.5.0", python).as_deref(), Some("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 + // declined by the arity guard rather than concrete forms this test speaks + // for. See `rewrite_declines_a_partial_version_behind_a_tilde`. assert_eq!( - rewrite_constraint("~=1.4", "1.5.0").as_deref(), + rewrite_constraint("~=1.4.2", "1.5.0", python).as_deref(), Some("~=1.5.0") ); // Hex's `~>`, whose space belongs to the operator prefix. assert_eq!( - rewrite_constraint("~> 1.0", "1.5.0").as_deref(), + rewrite_constraint("~> 1.0.0", "1.5.0", hex).as_deref(), Some("~> 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"), None); - // Python's `==1.*` is a wildcard, and stays declined. - assert_eq!(rewrite_constraint("==1.*", "1.5.0"), None); + assert_eq!(rewrite_constraint("[1.0,2.0)", "1.5.0", nuget), None); + // 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); } #[test] fn rewrite_skips_space_and_pipe_compound_constraints() { // npm / pubspec space-separated ranges and `||` alternations can't collapse // to a single version without dropping a clause, so they are left untouched. - assert_eq!(rewrite_constraint(">=1.0.0 <2.0.0", "1.5.0"), None); - assert_eq!(rewrite_constraint("^1.0.0 || ^2.0.0", "1.5.0"), None); - // A single constraint that merely spaces its operator is still rewritten. - assert_eq!( - rewrite_constraint(">= 1.0.0", "1.5.0").as_deref(), - Some(">= 1.5.0") - ); + // 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, + "{ecosystem:?}" + ); + assert_eq!( + rewrite_constraint("^1.0.0 || ^2.0.0", "1.5.0", it), + None, + "{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"), + "{ecosystem:?}" + ); + } } #[test] @@ -562,9 +1154,7 @@ mod tests { assert_eq!(out, "a=1.9 b=2.9\n"); } - use dependable_fetch::core::{ - DependencyKind, ManifestKind, parse, resolve_workspace_inheritance, - }; + use dependable_fetch::core::{DependencyKind, parse, resolve_workspace_inheritance}; /// Parse `content`, then build an `UpdateAvailable` result with the given /// target for each named dependency — enough to drive `plan_fixes`. The target @@ -612,7 +1202,13 @@ mod tests { content, &[("react", "18.2.0"), ("typescript", "5.4.5")], ); - let (updated, records) = plan_fixes(content, &results, false).expect("the plan applies"); + let (updated, records) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); assert_eq!( updated, @@ -650,7 +1246,13 @@ mod tests { content, &[("monolog/monolog", "2.9.1")], ); - let (updated, records) = plan_fixes(content, &results, false).expect("the plan applies"); + let (updated, records) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::ComposerJson.ecosystem()), + ) + .expect("the plan applies"); assert_eq!( updated, @@ -676,7 +1278,13 @@ mod tests { content, &[("http", "1.2.0"), ("provider", "6.1.0")], ); - let (updated, records) = plan_fixes(content, &results, false).expect("the plan applies"); + let (updated, records) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PubspecYaml.ecosystem()), + ) + .expect("the plan applies"); // Versions bumped, indentation and the trailing comment untouched. assert_eq!( @@ -720,7 +1328,13 @@ mod tests { "the old guards would both have passed" ); - let (updated, records) = plan_fixes(member, &results, false).expect("the plan applies"); + let (updated, records) = plan_fixes( + member, + &results, + false, + Some(ManifestKind::CargoToml.ecosystem()), + ) + .expect("the plan applies"); assert!(records.is_empty(), "{records:?}"); assert_eq!( @@ -749,7 +1363,13 @@ mod tests { ); assert_eq!(declaration.version_line, 1, "and the span points at it"); - let (updated, records) = plan_fixes(root, &results, false).expect("the plan applies"); + let (updated, records) = plan_fixes( + root, + &results, + false, + Some(ManifestKind::CargoToml.ecosystem()), + ) + .expect("the plan applies"); assert_eq!(records.len(), 1, "{records:?}"); assert_eq!(updated, "[workspace.dependencies]\nserde = \"1.0.219\"\n"); @@ -825,7 +1445,13 @@ mod tests { "the fixture must produce a checkable item" ); - let (updated, records) = plan_fixes(content, &results, false).expect("the plan applies"); + let (updated, records) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); assert!(records.is_empty(), "{records:?}"); assert_eq!(updated, content, "the manifest must be byte-identical"); @@ -861,7 +1487,13 @@ mod tests { "the `--all` branch reads `latest_available`, so the fixture must set it" ); - let (updated, records) = plan_fixes(content, &results, true).expect("the plan applies"); + let (updated, records) = plan_fixes( + content, + &results, + true, + Some(ManifestKind::ComposerJson.ecosystem()), + ) + .expect("the plan applies"); // The wildcard is declined; its non-wildcard neighbour still gets fixed, so // this asserts the guard and not a `--all` path that simply does nothing. @@ -884,4 +1516,130 @@ mod tests { "# ); } + + /// Issue #92 end to end, and the case the issue was opened for. + /// + /// `serde = "1.*"` locked at `1.0.100` with `1.0.219` published: `check` + /// reports an upgrade, and before #92 `fix` declined it because the guard from + /// #87 could not tell Cargo from npm. Cargo reads a bare version as a caret, so + /// `1.0.219` here *is* `^1.0.219` — still a range, still bounded below 2.0, and + /// the form the Cargo book calls equivalent to what was written. + /// + /// The `1.2.*` neighbour is the boundary: its range stops at `1.3.0` and a + /// caret does not, so it stays untouched in the same file, under the same + /// ecosystem, on the same run. + #[test] + fn a_cargo_minor_wildcard_is_updated_by_fix() { + let content = "[dependencies]\nserde = \"1.*\"\nclap = \"1.2.*\"\n"; + let results = results_for( + ManifestKind::CargoToml, + content, + &[("serde", "1.0.219"), ("clap", "1.2.9")], + ); + assert_eq!(results.len(), 2, "the fixture must produce two items"); + + let (updated, records) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::CargoToml.ecosystem()), + ) + .expect("the plan applies"); + + assert_eq!( + records + .iter() + .map(|record| (record.name.as_str(), record.to.as_str())) + .collect::>(), + [("serde", "1.0.219")], + "{records:?}" + ); + assert_eq!( + updated, + "[dependencies]\nserde = \"1.0.219\"\nclap = \"1.2.*\"\n" + ); + } + + /// The same manifest shape in the ecosystem that must still decline it: npm + /// reads a bare `1.0.219` as that release and nothing else, so the identical + /// rewrite would destroy the range. One ecosystem apart, opposite verdicts — + /// which is the whole of #92. + #[test] + fn the_same_wildcard_is_still_declined_for_npm() { + let content = "{\n \"dependencies\": {\n \"lodash\": \"1.*\"\n }\n}\n"; + 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( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + + assert!(records.is_empty(), "{records:?}"); + assert_eq!(updated, content, "the manifest must be byte-identical"); + } + + /// Issue #92's second gap, end to end: npm reads `"react": "16"` as `16.x`, + /// and `fix` used to write `"16.14.0"` into it — a pin, with no wildcard + /// character anywhere for the #87 guard to catch. Its `^18.0.0` neighbour is + /// rewritten on the same run, so this asserts the new guard rather than a path + /// that quietly does nothing. + #[test] + fn an_npm_partial_version_is_left_untouched_by_fix() { + let content = + "{\n \"dependencies\": {\n \"react\": \"16\",\n \"vue\": \"^3.0.0\"\n }\n}\n"; + let results = results_for( + ManifestKind::PackageJson, + content, + &[("react", "16.14.0"), ("vue", "3.4.21")], + ); + assert_eq!(results.len(), 2, "the fixture must produce two items"); + + let (updated, records) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + + assert_eq!( + records + .iter() + .map(|record| record.name.as_str()) + .collect::>(), + ["vue"], + "{records:?}" + ); + assert_eq!( + updated, + "{\n \"dependencies\": {\n \"react\": \"16\",\n \"vue\": \"^3.4.21\"\n }\n}\n" + ); + } + + /// A manifest whose kind `ManifestKind::detect` does not recognize reaches the + /// rewriter with no ecosystem, and the answer is to decline, not to guess: + /// every reading that could apply is one where at least one of these rewrites + /// destroys the constraint. The `^1.0` neighbour still moves, so the decline is + /// 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); + // 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") + ); + assert_eq!( + rewrite_constraint("1.0.0", "1.5.0", None).as_deref(), + Some("1.5.0") + ); + } }