diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 7d089e0a32..caf5854720 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -1,9 +1,7 @@ -#![allow(clippy::bool_assert_comparison)] - +use cargo::core::dependency::DepKind; use cargo::util::command_prelude::*; use cargo::CargoResult; use cargo_add::ops::add; -use cargo_add::ops::cargo_add::parse_feature; use cargo_add::ops::AddOptions; use cargo_add::ops::DepOp; use cargo_add::ops::DepTable; @@ -37,7 +35,7 @@ EXAMPLES: .long_help( "Reference to a package to add as a dependency -You can reference a packages by: +You can reference a package by: - ``, like `cargo add serde` (latest version will be used) - `@`, like `cargo add serde@1` or `cargo add serde@=1.0.38` - ``, like `cargo add ./crates/parser/` @@ -46,12 +44,10 @@ Additionally, you can specify features for a dependency by following it with a ` ), clap::Arg::new("no-default-features") .long("no-default-features") - .help("Disable the default features") - .long_help(None), + .help("Disable the default features"), clap::Arg::new("default-features") .long("default-features") .help("Re-enable the default features") - .long_help(None) .overrides_with("no-default-features"), clap::Arg::new("features") .short('F') @@ -94,7 +90,6 @@ Example uses: .takes_value(true) .value_name("NAME") .help("Package registry for this dependency") - .long_help(None) .conflicts_with("git"), ]) .arg_manifest_path() @@ -104,12 +99,10 @@ Example uses: .long("package") .takes_value(true) .value_name("SPEC") - .help("Package to modify") - .long_help(None), + .help("Package to modify"), clap::Arg::new("offline") .long("offline") .help("Run without accessing the network") - .long_help(None), ]) .arg_quiet() .arg_dry_run("Don't actually write the manifest") @@ -139,8 +132,6 @@ Build-dependencies are the only dependencies available for use by build scripts .value_name("TARGET") .forbid_empty_values(true) .help("Add as dependency to the given target platform") - .long_help(None) - .group("section"), ]) .next_help_heading("UNSTABLE") .args([ @@ -157,7 +148,6 @@ Without any other information, cargo will use latest commit on the main branch." .takes_value(true) .value_name("BRANCH") .help("Git branch to download the crate from") - .long_help(None) .requires("git") .group("git-ref"), clap::Arg::new("tag") @@ -165,7 +155,6 @@ Without any other information, cargo will use latest commit on the main branch." .takes_value(true) .value_name("TAG") .help("Git tag to download the crate from") - .long_help(None) .requires("git") .group("git-ref"), clap::Arg::new("rev") @@ -191,7 +180,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { let spec = match packages.len() { 0 => { return Err(CliError::new( - anyhow::format_err!("No packages selected. Please specify one with `-p `"), + anyhow::format_err!("no packages selected. Please specify one with `-p `"), 101, )); } @@ -199,8 +188,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { len => { return Err(CliError::new( anyhow::format_err!( - "{} packages selected. Please specify one with `-p `", - len + "{len} packages selected. Please specify one with `-p `", ), 101, )); @@ -243,18 +231,18 @@ fn parse_dependencies<'m>(config: &Config, matches: &'m ArgMatches) -> CargoResu let optional = optional(matches); if crates.len() > 1 && git.is_some() { - anyhow::bail!("Cannot specify multiple crates with path or git or vers"); + anyhow::bail!("cannot specify multiple crates with path or git or vers"); } if git.is_some() && !config.cli_unstable().unstable_options { anyhow::bail!("`--git` is unstable and requires `-Z unstable-options`"); } if crates.len() > 1 && rename.is_some() { - anyhow::bail!("Cannot specify multiple crates with rename"); + anyhow::bail!("cannot specify multiple crates with rename"); } if crates.len() > 1 && features.is_some() { - anyhow::bail!("Cannot specify multiple crates with features"); + anyhow::bail!("cannot specify multiple crates with features"); } let mut deps: Vec = Vec::new(); @@ -315,15 +303,30 @@ fn resolve_bool_arg(yes: bool, no: bool) -> Option { } } -fn parse_section(matches: &ArgMatches) -> DepTable<'_> { - if matches.is_present("dev") { - DepTable::Development +fn parse_section(matches: &ArgMatches) -> DepTable { + let kind = if matches.is_present("dev") { + DepKind::Development } else if matches.is_present("build") { - DepTable::Build - } else if let Some(target) = matches.value_of("target") { - assert!(!target.is_empty(), "Target specification may not be empty"); - DepTable::Target(target) + DepKind::Build } else { - DepTable::Normal + DepKind::Normal + }; + + let mut table = DepTable::new().set_kind(kind); + + if let Some(target) = matches.value_of("target") { + assert!(!target.is_empty(), "Target specification may not be empty"); + table = table.set_target(target); } + + table +} + +/// Split feature flag list +fn parse_feature(feature: &str) -> impl Iterator { + // Not re-using `CliFeatures` because it uses a BTreeSet and loses user's ordering + feature + .split_whitespace() + .flat_map(|s| s.split(',')) + .filter(|s| !s.is_empty()) } diff --git a/crates/cargo-add/src/cargo/ops/cargo_add/crate_spec.rs b/crates/cargo-add/src/cargo/ops/cargo_add/crate_spec.rs index 301a4b99c5..69a38d09f9 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/crate_spec.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/crate_spec.rs @@ -1,6 +1,6 @@ //! Crate name parsing. -use anyhow::Context; +use anyhow::Context as _; use cargo::CargoResult; use super::get_manifest_from_path; @@ -40,25 +40,11 @@ impl CrateSpec { .map(|(n, v)| (n, Some(v))) .unwrap_or((pkg_id, None)); - let invalid: Vec<_> = name - .chars() - .filter(|c| !is_name_char(*c)) - .map(|c| c.to_string()) - .collect(); - if !invalid.is_empty() { - anyhow::bail!( - "Name `{}` contains invalid characters: {}", - name, - invalid.join(", ") - ); - } - if name.is_empty() { - anyhow::bail!("pkg id has empty name: `{}`", pkg_id); - } + cargo::util::validate_package_name(name, "dependency name", "")?; if let Some(version) = version { semver::VersionReq::parse(version) - .with_context(|| format!("Invalid version requirement `{}`", version))?; + .with_context(|| format!("invalid version requirement `{version}`"))?; } Self::PkgId { @@ -96,18 +82,6 @@ impl CrateSpec { } } -impl std::str::FromStr for CrateSpec { - type Err = anyhow::Error; - - fn from_str(s: &str) -> CargoResult { - Self::resolve(s) - } -} - -fn is_name_char(c: char) -> bool { - c.is_alphanumeric() || ['-', '_'].contains(&c) -} - fn is_path_like(s: &str) -> bool { s.contains('/') || s.contains('\\') } diff --git a/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs b/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs index 58adcc0916..95931e51eb 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs @@ -216,57 +216,93 @@ impl Dependency { impl Dependency { /// Create a dependency from a TOML table entry - pub fn from_toml(crate_root: &Path, key: &str, item: &toml_edit::Item) -> Option { + pub fn from_toml(crate_root: &Path, key: &str, item: &toml_edit::Item) -> CargoResult { if let Some(version) = item.as_str() { let dep = Self::new(key).set_source(RegistrySource::new(version)); - Some(dep) + Ok(dep) } else if let Some(table) = item.as_table_like() { let (name, rename) = if let Some(value) = table.get("package") { - (value.as_str()?.to_owned(), Some(key.to_owned())) + ( + value + .as_str() + .ok_or_else(|| invalid_type(key, "package", value.type_name(), "string"))? + .to_owned(), + Some(key.to_owned()), + ) } else { (key.to_owned(), None) }; - let source: Source = if let Some(git) = table.get("git") { - let mut src = GitSource::new(git.as_str()?); - if let Some(value) = table.get("branch") { - src = src.set_branch(value.as_str()?); - } - if let Some(value) = table.get("tag") { - src = src.set_tag(value.as_str()?); - } - if let Some(value) = table.get("rev") { - src = src.set_rev(value.as_str()?); - } - src.into() - } else if let Some(path) = table.get("path") { - let path = crate_root.join(path.as_str()?); - let mut src = PathSource::new(path); - if let Some(value) = table.get("version") { - src = src.set_version(value.as_str()?); - } - src.into() - } else if let Some(version) = table.get("version") { - let src = RegistrySource::new(version.as_str()?); - src.into() - } else { - return None; - }; + let source: Source = + if let Some(git) = table.get("git") { + let mut src = GitSource::new( + git.as_str() + .ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?, + ); + if let Some(value) = table.get("branch") { + src = src.set_branch(value.as_str().ok_or_else(|| { + invalid_type(key, "branch", value.type_name(), "string") + })?); + } + if let Some(value) = table.get("tag") { + src = src.set_tag(value.as_str().ok_or_else(|| { + invalid_type(key, "tag", value.type_name(), "string") + })?); + } + if let Some(value) = table.get("rev") { + src = src.set_rev(value.as_str().ok_or_else(|| { + invalid_type(key, "rev", value.type_name(), "string") + })?); + } + src.into() + } else if let Some(path) = table.get("path") { + let path = crate_root + .join(path.as_str().ok_or_else(|| { + invalid_type(key, "path", path.type_name(), "string") + })?); + let mut src = PathSource::new(path); + if let Some(value) = table.get("version") { + src = src.set_version(value.as_str().ok_or_else(|| { + invalid_type(key, "version", value.type_name(), "string") + })?); + } + src.into() + } else if let Some(version) = table.get("version") { + let src = RegistrySource::new(version.as_str().ok_or_else(|| { + invalid_type(key, "version", version.type_name(), "string") + })?); + src.into() + } else { + anyhow::bail!("Unrecognized dependency source for `{key}`"); + }; let registry = if let Some(value) = table.get("registry") { - Some(value.as_str()?.to_owned()) + Some( + value + .as_str() + .ok_or_else(|| invalid_type(key, "registry", value.type_name(), "string"))? + .to_owned(), + ) } else { None }; let default_features = table.get("default-features").and_then(|v| v.as_bool()); + if table.contains_key("default_features") { + anyhow::bail!("Use of `default_features` in `{key}` is unsupported"); + } let features = if let Some(value) = table.get("features") { Some( value - .as_array()? + .as_array() + .ok_or_else(|| invalid_type(key, "features", value.type_name(), "array"))? .iter() - .map(|v| v.as_str().map(|s| s.to_owned())) - .collect::>>()?, + .map(|v| { + v.as_str().map(|s| s.to_owned()).ok_or_else(|| { + invalid_type(key, "features", v.type_name(), "string") + }) + }) + .collect::>>()?, ) } else { None @@ -286,9 +322,9 @@ impl Dependency { available_features, optional, }; - Some(dep) + Ok(dep) } else { - None + anyhow::bail!("Unrecognized` dependency entry format for `{key}"); } } @@ -390,7 +426,6 @@ impl Dependency { /// Modify existing entry to match this dependency pub fn update_toml(&self, crate_root: &Path, item: &mut toml_edit::Item) { - #[allow(clippy::if_same_then_else)] if str_or_1_len_table(item) { // Nothing to preserve *item = self.to_toml(crate_root); @@ -494,6 +529,10 @@ impl Dependency { } } +fn invalid_type(dep: &str, key: &str, actual: &str, expected: &str) -> anyhow::Error { + anyhow::format_err!("Found {actual} for {key} when {expected} was expected for {dep}") +} + impl std::fmt::Display for Dependency { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { if let Some(source) = self.source() { diff --git a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs index 8c8994ef8b..411b5a333d 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -3,13 +3,86 @@ use std::ops::{Deref, DerefMut}; use std::path::{Path, PathBuf}; use std::str; -use anyhow::Context; +use anyhow::Context as _; +use cargo::core::dependency::DepKind; use cargo::util::interning::InternedString; use cargo::CargoResult; use super::dependency::Dependency; -const DEP_TABLES: &[&str] = &["dependencies", "dev-dependencies", "build-dependencies"]; +/// Dependency table to add dep to +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DepTable { + kind: DepKind, + target: Option, +} + +impl DepTable { + const KINDS: &'static [Self] = &[ + Self::new().set_kind(DepKind::Normal), + Self::new().set_kind(DepKind::Development), + Self::new().set_kind(DepKind::Build), + ]; + + /// Reference to a Dependency Table + pub const fn new() -> Self { + Self { + kind: DepKind::Normal, + target: None, + } + } + + /// Choose the type of dependency + pub const fn set_kind(mut self, kind: DepKind) -> Self { + self.kind = kind; + self + } + + /// Choose the platform for the dependency + pub fn set_target(mut self, target: impl Into) -> Self { + self.target = Some(target.into()); + self + } + + /// Type of dependency + pub fn kind(&self) -> DepKind { + self.kind + } + + /// Platform for the dependency + pub fn target(&self) -> Option<&str> { + self.target.as_deref() + } + + /// Keys to the table + pub fn to_table(&self) -> Vec<&str> { + if let Some(target) = &self.target { + vec!["target", target, self.kind_table()] + } else { + vec![self.kind_table()] + } + } + + fn kind_table(&self) -> &str { + match self.kind { + DepKind::Normal => "dependencies", + DepKind::Development => "dev-dependencies", + DepKind::Build => "build-dependencies", + } + } +} + +impl Default for DepTable { + fn default() -> Self { + Self::new() + } +} + +impl From for DepTable { + fn from(other: DepKind) -> Self { + Self::new().set_kind(other) + } +} /// A Cargo manifest #[derive(Debug, Clone)] @@ -90,10 +163,11 @@ impl Manifest { /// Get all sections in the manifest that exist and might contain dependencies. /// The returned items are always `Table` or `InlineTable`. - pub fn get_sections(&self) -> Vec<(Vec, toml_edit::Item)> { + pub fn get_sections(&self) -> Vec<(DepTable, toml_edit::Item)> { let mut sections = Vec::new(); - for dependency_type in DEP_TABLES { + for table in DepTable::KINDS { + let dependency_type = table.kind_table(); // Dependencies can be in the three standard sections... if self .data @@ -101,10 +175,7 @@ impl Manifest { .map(|t| t.is_table_like()) .unwrap_or(false) { - sections.push(( - vec![String::from(*dependency_type)], - self.data[dependency_type].clone(), - )) + sections.push((table.clone(), self.data[dependency_type].clone())) } // ... and in `target..(build-/dev-)dependencies`. @@ -119,11 +190,7 @@ impl Manifest { let dependency_table = target_table.get(dependency_type)?; dependency_table.as_table_like().map(|_| { ( - vec![ - "target".to_string(), - target_name.to_string(), - String::from(*dependency_type), - ], + table.clone().set_target(target_name), dependency_table.clone(), ) }) @@ -135,6 +202,34 @@ impl Manifest { sections } + pub fn get_legacy_sections(&self) -> Vec { + let mut result = Vec::new(); + + for dependency_type in ["dev_dependencies", "build_dependencies"] { + if self.data.contains_key(dependency_type) { + result.push(dependency_type.to_owned()); + } + + // ... and in `target..(build-/dev-)dependencies`. + result.extend( + self.data + .as_table() + .get("target") + .and_then(toml_edit::Item::as_table_like) + .into_iter() + .flat_map(toml_edit::TableLike::iter) + .filter_map(|(target_name, target_table)| { + if target_table.as_table_like()?.contains_key(dependency_type) { + Some(format!("target.{target_name}.{dependency_type}")) + } else { + None + } + }), + ); + } + result + } + /// returns features exposed by this manifest pub fn features(&self) -> CargoResult>> { let mut features: BTreeMap> = match self.data.as_table().get("features") @@ -241,13 +336,13 @@ impl LocalManifest { { if self.manifest.data.contains_key("workspace") { anyhow::bail!( - "Found virtual manifest at {}, but this command requires running against an \ + "found virtual manifest at {}, but this command requires running against an \ actual package in this workspace.", self.path.display() ); } else { anyhow::bail!( - "Missing expected `package` or `project` fields in {}", + "missing expected `package` or `project` fields in {}", self.path.display() ); } @@ -264,17 +359,7 @@ impl LocalManifest { pub fn get_dependency_versions<'s>( &'s self, dep_key: &'s str, - ) -> impl Iterator, CargoResult)> + 's { - self.filter_dependencies(move |key| key == dep_key) - } - - fn filter_dependencies<'s, P>( - &'s self, - mut predicate: P, - ) -> impl Iterator, CargoResult)> + 's - where - P: FnMut(&str) -> bool + 's, - { + ) -> impl Iterator)> + 's { let crate_root = self.path.parent().expect("manifest path is absolute"); self.get_sections() .into_iter() @@ -284,7 +369,7 @@ impl LocalManifest { table .into_iter() .filter_map(|(key, item)| { - if predicate(&key) { + if key.as_str() == dep_key { Some((table_path.clone(), key, item)) } else { None @@ -296,17 +381,7 @@ impl LocalManifest { .flatten() .map(move |(table_path, dep_key, dep_item)| { let dep = Dependency::from_toml(crate_root, &dep_key, &dep_item); - match dep { - Some(dep) => (table_path, Ok(dep)), - None => { - let message = anyhow::format_err!( - "Invalid dependency {}.{}", - table_path.join("."), - dep_key - ); - (table_path, Err(message)) - } - } + (table_path, dep) }) } @@ -461,13 +536,13 @@ pub fn str_or_1_len_table(item: &toml_edit::Item) -> bool { } fn parse_manifest_err() -> anyhow::Error { - anyhow::format_err!("Unable to parse external Cargo.toml") + anyhow::format_err!("unable to parse external Cargo.toml") } fn non_existent_table_err(table: impl std::fmt::Display) -> anyhow::Error { - anyhow::format_err!("The table `{}` could not be found.", table) + anyhow::format_err!("the table `{table}` could not be found.") } fn invalid_cargo_config() -> anyhow::Error { - anyhow::format_err!("Invalid cargo config") + anyhow::format_err!("invalid cargo config") } diff --git a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs index f46c2f3c95..70059cc190 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -8,7 +8,8 @@ use std::collections::BTreeSet; use std::collections::VecDeque; use std::path::Path; -use anyhow::Context; +use anyhow::Context as _; +use cargo::core::dependency::DepKind; use cargo::core::Registry; use cargo::CargoResult; use cargo::Config; @@ -23,6 +24,8 @@ use dependency::RegistrySource; use dependency::Source; use manifest::LocalManifest; +pub use manifest::DepTable; + /// Information on what dependencies should be added #[derive(Clone, Debug)] pub struct AddOptions<'a> { @@ -33,7 +36,7 @@ pub struct AddOptions<'a> { /// Dependencies to add or modify pub dependencies: Vec, /// Which dependency section to add these to - pub section: DepTable<'a>, + pub section: DepTable, /// Act as if dependencies will be added pub dry_run: bool, } @@ -47,9 +50,15 @@ pub fn add(workspace: &cargo::core::Workspace, options: &AddOptions<'_>) -> Carg .map(String::from) .collect::>(); - let manifest_path = options.spec.manifest_path(); - let manifest_path = manifest_path.to_path_buf(); + let manifest_path = options.spec.manifest_path().to_path_buf(); let mut manifest = LocalManifest::try_new(&manifest_path)?; + let legacy = manifest.get_legacy_sections(); + if !legacy.is_empty() { + anyhow::bail!( + "Deprecated dependency sections are unsupported: {}", + legacy.join(", ") + ); + } let mut registry = cargo::core::registry::PackageRegistry::new(options.config)?; @@ -64,7 +73,7 @@ pub fn add(workspace: &cargo::core::Workspace, options: &AddOptions<'_>) -> Carg &manifest, raw, workspace, - options.section, + &options.section, options.config, &mut registry, ) @@ -96,7 +105,7 @@ pub fn add(workspace: &cargo::core::Workspace, options: &AddOptions<'_>) -> Carg options .config .shell() - .warn(format!("Unrecognized features: {:?}", unknown_features))?; + .warn(format!("unrecognized features: {unknown_features:?}"))?; }; } @@ -104,7 +113,7 @@ pub fn add(workspace: &cargo::core::Workspace, options: &AddOptions<'_>) -> Carg if let Some(Source::Path(src)) = dep.source() { if src.path == manifest.path.parent().unwrap_or_else(|| Path::new("")) { anyhow::bail!( - "Cannot add `{}` as a dependency to itself", + "cannot add `{}` as a dependency to itself", manifest.package_name()? ) } @@ -161,35 +170,11 @@ pub struct DepOp { pub tag: Option, } -/// Dependency table to add dep to -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum DepTable<'m> { - /// Used for building final artifact - Normal, - /// Used for testing - Development, - /// Used for build.rs - Build, - /// Used for building final artifact only on specific target platforms - Target(&'m str), -} - -impl<'m> DepTable<'m> { - fn to_table(self) -> Vec<&'m str> { - match self { - Self::Normal => vec!["dependencies"], - Self::Development => vec!["dev-dependencies"], - Self::Build => vec!["build-dependencies"], - Self::Target(target) => vec!["target", target, "dependencies"], - } - } -} - fn resolve_dependency( manifest: &LocalManifest, arg: &DepOp, ws: &cargo::core::Workspace, - section: DepTable<'_>, + section: &DepTable, config: &Config, registry: &mut cargo::core::registry::PackageRegistry, ) -> CargoResult { @@ -198,7 +183,7 @@ fn resolve_dependency( let mut spec_dep = crate_spec.to_dependency()?; spec_dep = populate_dependency(spec_dep, arg); - let old_dep = get_existing_dependency(manifest, spec_dep.toml_key(), section); + let old_dep = get_existing_dependency(manifest, spec_dep.toml_key(), section)?; let mut dependency = if let Some(mut old_dep) = old_dep.clone() { if spec_dep.source().is_some() { @@ -215,8 +200,7 @@ fn resolve_dependency( match &crate_spec { CrateSpec::Path(path) => { anyhow::bail!( - "Cannot specify a git URL (`{}`) with a path (`{}`).", - url, + "cannot specify a git URL (`{url}`) with a path (`{}`).", path.display() ) } @@ -225,11 +209,7 @@ fn resolve_dependency( version_req: Some(v), } => { // crate specifier includes a version (e.g. `docopt@0.8`) - anyhow::bail!( - "Cannot specify a git URL (`{}`) with a version (`{}`).", - url, - v - ) + anyhow::bail!("cannot specify a git URL (`{url}`) with a version (`{v}`).",) } CrateSpec::PkgId { name: _, @@ -257,9 +237,9 @@ fn resolve_dependency( // information, otherwise, trust the user. let mut src = PathSource::new(package.root()); // dev-dependencies do not need the version populated - if section != DepTable::Development { + if section.kind() != DepKind::Development { let op = ""; - let v = format!("{op}{version}", op = op, version = package.version()); + let v = format!("{op}{version}", version = package.version()); src = src.set_version(v); } dependency = dependency.set_source(src); @@ -268,7 +248,7 @@ fn resolve_dependency( if dependency.name != latest.name { config.shell().warn(format!( - "Translating `{}` to `{}`", + "translating `{}` to `{}`", dependency.name, latest.name, ))?; dependency.name = latest.name; // Normalize the name @@ -280,7 +260,7 @@ fn resolve_dependency( } let version_required = dependency.source().and_then(|s| s.as_registry()).is_some(); - let version_optional_in_section = section == DepTable::Development; + let version_optional_in_section = section.kind() == DepKind::Development; let preserve_existing_version = old_dep .as_ref() .map(|d| d.version().is_some()) @@ -302,40 +282,44 @@ fn resolve_dependency( fn get_existing_dependency( manifest: &LocalManifest, dep_key: &str, - section: DepTable<'_>, -) -> Option { + section: &DepTable, +) -> CargoResult> { #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)] enum Key { + Error, Dev, Build, - Target, - Runtime, + Normal, Existing, } - let target_section = section.to_table(); let mut possible: Vec<_> = manifest .get_dependency_versions(dep_key) - .filter_map(|(path, dep)| dep.ok().map(|dep| (path, dep))) .map(|(path, dep)| { - let key = if path == target_section { - Key::Existing + let key = if path == *section { + (Key::Existing, true) + } else if dep.is_err() { + (Key::Error, path.target().is_some()) } else { - match path[0].as_str() { - "dependencies" => Key::Runtime, - "target" => Key::Target, - "build-dependencies" => Key::Build, - "dev-dependencies" => Key::Dev, - other => unreachable!("Unknown dependency section: {}", other), - } + let key = match path.kind() { + DepKind::Normal => Key::Normal, + DepKind::Build => Key::Build, + DepKind::Development => Key::Dev, + }; + (key, path.target().is_some()) }; (key, dep) }) .collect(); possible.sort_by_key(|(key, _)| *key); - let (key, mut dep) = possible.pop()?; + let (key, dep) = if let Some(item) = possible.pop() { + item + } else { + return Ok(None); + }; + let mut dep = dep?; - if key != Key::Existing { + if key.0 != Key::Existing { // When the dep comes from a different section, we only care about the source and not any // of the other fields, like `features` let unrelated = dep; @@ -346,13 +330,13 @@ fn get_existing_dependency( // dev-dependencies do not need the version populated when path is set though we // should preserve it if the user chose to populate it. let version_required = unrelated.source().and_then(|s| s.as_registry()).is_some(); - let version_optional_in_section = section == DepTable::Development; + let version_optional_in_section = section.kind() == DepKind::Development; if !version_required && version_optional_in_section { dep = dep.clear_version(); } } - Some(dep) + Ok(Some(dep)) } fn get_latest_dependency( @@ -373,14 +357,13 @@ fn get_latest_dependency( let latest = possibilities .iter() .max_by_key(|s| { + // Fallback to a pre-release if no official release is available by sorting them as + // less. let stable = s.version().pre.is_empty(); (stable, s.version()) }) .ok_or_else(|| { - anyhow::format_err!( - "The crate `{}` could not be found in registry index.", - dependency - ) + anyhow::format_err!("the crate `{dependency}` could not be found in registry index.") })?; let mut dep = Dependency::from(latest); if let Some(reg_name) = dependency.registry.as_deref() { @@ -390,13 +373,6 @@ fn get_latest_dependency( } fn populate_dependency(mut dependency: Dependency, arg: &DepOp) -> Dependency { - let requested_features: Option> = arg.features.as_ref().map(|v| { - v.iter() - .flat_map(|s| parse_feature(s)) - .map(|f| f.to_owned()) - .collect() - }); - if let Some(registry) = &arg.registry { if registry.is_empty() { dependency.registry = None; @@ -418,8 +394,8 @@ fn populate_dependency(mut dependency: Dependency, arg: &DepOp) -> Dependency { dependency.default_features = Some(false); } } - if let Some(value) = requested_features { - dependency = dependency.extend_features(value); + if let Some(value) = arg.features.as_ref() { + dependency = dependency.extend_features(value.iter().cloned()); } if let Some(rename) = &arg.rename { @@ -429,15 +405,6 @@ fn populate_dependency(mut dependency: Dependency, arg: &DepOp) -> Dependency { dependency } -/// Split feature flag list -pub fn parse_feature(feature: &str) -> impl Iterator { - // Not re-using `CliFeatures` because it uses a BTreeSet and loses user's ordering - feature - .split_whitespace() - .flat_map(|s| s.split(',')) - .filter(|s| !s.is_empty()) -} - /// Lookup available features fn populate_available_features( mut dependency: Dependency, @@ -457,17 +424,18 @@ fn populate_available_features( std::task::Poll::Pending => registry.block_until_ready()?, } }; + // Ensure widest feature flag compatibility by picking the earliest version that could show up + // in the lock file for a given version requirement. let lowest_common_denominator = possibilities .iter() .min_by_key(|s| { + // Fallback to a pre-release if no official release is available by sorting them as + // more. let is_pre = !s.version().pre.is_empty(); (is_pre, s.version()) }) .ok_or_else(|| { - anyhow::format_err!( - "The crate `{}` could not be found in registry index.", - dependency - ) + anyhow::format_err!("the crate `{dependency}` could not be found in registry index.") })?; dependency = dependency.set_available_features_from_cargo(lowest_common_denominator.features()); @@ -508,7 +476,7 @@ fn print_msg( } else { format!("{} for target `{}`", §ion[2], §ion[1]) }; - write!(message, " {}", section)?; + write!(message, " {section}")?; write!(message, ".")?; let mut activated: IndexSet<_> = dep.features.iter().flatten().map(|s| s.as_str()).collect(); diff --git a/crates/cargo-add/tests/snapshots/add/add_normalized_name_external.stderr b/crates/cargo-add/tests/snapshots/add/add_normalized_name_external.stderr index df4ef820d0..c7d4511432 100644 --- a/crates/cargo-add/tests/snapshots/add/add_normalized_name_external.stderr +++ b/crates/cargo-add/tests/snapshots/add/add_normalized_name_external.stderr @@ -1,6 +1,6 @@ Updating `dummy-registry` index -warning: Translating `linked_hash_map` to `linked-hash-map` -warning: Translating `Inflector` to `inflector` +warning: translating `linked_hash_map` to `linked-hash-map` +warning: translating `Inflector` to `inflector` Adding linked-hash-map v0.5.4 to dependencies. Features: - clippy diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/Cargo.toml b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/Cargo.toml new file mode 100644 index 0000000000..c0fc374eae --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/Cargo.toml @@ -0,0 +1,8 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dependencies] +my-package = { version = "99999.0.0", default_features = false } diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/src/lib.rs b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/src/lib.rs new file mode 100644 index 0000000000..e69de29bb2 diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_default_features.out/Cargo.toml b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.out/Cargo.toml new file mode 100644 index 0000000000..c0fc374eae --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.out/Cargo.toml @@ -0,0 +1,8 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dependencies] +my-package = { version = "99999.0.0", default_features = false } diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_default_features.stderr b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.stderr new file mode 100644 index 0000000000..16a9d1040f --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.stderr @@ -0,0 +1 @@ +error: Use of `default_features` in `my-package` is unsupported diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_default_features.stdout b/crates/cargo-add/tests/snapshots/add/deprecated_default_features.stdout new file mode 100644 index 0000000000..e69de29bb2 diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_section.in/Cargo.toml b/crates/cargo-add/tests/snapshots/add/deprecated_section.in/Cargo.toml new file mode 100644 index 0000000000..a83d2c621f --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/deprecated_section.in/Cargo.toml @@ -0,0 +1,11 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dev_dependencies] +my-package = "99999.0.0" + +[build_dependencies] +my-package = "99999.0.0" diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_section.in/src/lib.rs b/crates/cargo-add/tests/snapshots/add/deprecated_section.in/src/lib.rs new file mode 100644 index 0000000000..e69de29bb2 diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_section.out/Cargo.toml b/crates/cargo-add/tests/snapshots/add/deprecated_section.out/Cargo.toml new file mode 100644 index 0000000000..a83d2c621f --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/deprecated_section.out/Cargo.toml @@ -0,0 +1,11 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dev_dependencies] +my-package = "99999.0.0" + +[build_dependencies] +my-package = "99999.0.0" diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_section.stderr b/crates/cargo-add/tests/snapshots/add/deprecated_section.stderr new file mode 100644 index 0000000000..b3b9c10f9b --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/deprecated_section.stderr @@ -0,0 +1 @@ +error: Deprecated dependency sections are unsupported: dev_dependencies, build_dependencies diff --git a/crates/cargo-add/tests/snapshots/add/deprecated_section.stdout b/crates/cargo-add/tests/snapshots/add/deprecated_section.stdout new file mode 100644 index 0000000000..e69de29bb2 diff --git a/crates/cargo-add/tests/snapshots/add/features_unknown.stderr b/crates/cargo-add/tests/snapshots/add/features_unknown.stderr index dac014141c..5bbc100a00 100644 --- a/crates/cargo-add/tests/snapshots/add/features_unknown.stderr +++ b/crates/cargo-add/tests/snapshots/add/features_unknown.stderr @@ -1,5 +1,5 @@ Updating `dummy-registry` index -warning: Unrecognized features: ["noze"] +warning: unrecognized features: ["noze"] Adding your-face v99999.0.0 to dependencies. Features: + noze diff --git a/crates/cargo-add/tests/snapshots/add/git_conflicts_namever.stderr b/crates/cargo-add/tests/snapshots/add/git_conflicts_namever.stderr index 765a7b6200..207e0ded3a 100644 --- a/crates/cargo-add/tests/snapshots/add/git_conflicts_namever.stderr +++ b/crates/cargo-add/tests/snapshots/add/git_conflicts_namever.stderr @@ -1 +1 @@ -error: Cannot specify a git URL (`https://github.com/dcjanus/invalid`) with a version (`0.4.3`). +error: cannot specify a git URL (`https://github.com/dcjanus/invalid`) with a version (`0.4.3`). diff --git a/crates/cargo-add/tests/snapshots/add/infer_prerelease.in b/crates/cargo-add/tests/snapshots/add/infer_prerelease.in new file mode 120000 index 0000000000..87eae14531 --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/infer_prerelease.in @@ -0,0 +1 @@ +add-basic.in/ \ No newline at end of file diff --git a/crates/cargo-add/tests/snapshots/add/infer_prerelease.out/Cargo.toml b/crates/cargo-add/tests/snapshots/add/infer_prerelease.out/Cargo.toml new file mode 100644 index 0000000000..4a86322add --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/infer_prerelease.out/Cargo.toml @@ -0,0 +1,8 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dependencies] +prerelease_only = "0.2.0-alpha.1" diff --git a/crates/cargo-add/tests/snapshots/add/infer_prerelease.stderr b/crates/cargo-add/tests/snapshots/add/infer_prerelease.stderr new file mode 100644 index 0000000000..0696d8f7b2 --- /dev/null +++ b/crates/cargo-add/tests/snapshots/add/infer_prerelease.stderr @@ -0,0 +1,2 @@ + Updating `dummy-registry` index + Adding prerelease_only v0.2.0-alpha.1 to dependencies. diff --git a/crates/cargo-add/tests/snapshots/add/infer_prerelease.stdout b/crates/cargo-add/tests/snapshots/add/infer_prerelease.stdout new file mode 100644 index 0000000000..e69de29bb2 diff --git a/crates/cargo-add/tests/snapshots/add/invalid_inline_path_self.stderr b/crates/cargo-add/tests/snapshots/add/invalid_inline_path_self.stderr index 4b66e27fd5..62a25dbb42 100644 --- a/crates/cargo-add/tests/snapshots/add/invalid_inline_path_self.stderr +++ b/crates/cargo-add/tests/snapshots/add/invalid_inline_path_self.stderr @@ -1,2 +1,2 @@ Adding cargo-list-test-fixture (local) to dependencies. -error: Cannot add `cargo-list-test-fixture` as a dependency to itself +error: cannot add `cargo-list-test-fixture` as a dependency to itself diff --git a/crates/cargo-add/tests/snapshots/add/invalid_name_external.stderr b/crates/cargo-add/tests/snapshots/add/invalid_name_external.stderr index 4f32e41046..5e574cedad 100644 --- a/crates/cargo-add/tests/snapshots/add/invalid_name_external.stderr +++ b/crates/cargo-add/tests/snapshots/add/invalid_name_external.stderr @@ -1,2 +1,2 @@ Updating `dummy-registry` index -error: The crate `lets_hope_nobody_ever_publishes_this_crate` could not be found in registry index. +error: the crate `lets_hope_nobody_ever_publishes_this_crate` could not be found in registry index. diff --git a/crates/cargo-add/tests/snapshots/add/invalid_vers.stderr b/crates/cargo-add/tests/snapshots/add/invalid_vers.stderr index e28186f03a..13a110305e 100644 --- a/crates/cargo-add/tests/snapshots/add/invalid_vers.stderr +++ b/crates/cargo-add/tests/snapshots/add/invalid_vers.stderr @@ -1,4 +1,4 @@ -error: Invalid version requirement `invalid version string` +error: invalid version requirement `invalid version string` Caused by: unexpected character 'i' while parsing major version number diff --git a/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_features.stderr b/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_features.stderr index 86c861845a..55f4e940e0 100644 --- a/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_features.stderr +++ b/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_features.stderr @@ -1 +1 @@ -error: Cannot specify multiple crates with features +error: cannot specify multiple crates with features diff --git a/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_git.stderr b/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_git.stderr index 0c09e548d3..b733f99b04 100644 --- a/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_git.stderr +++ b/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_git.stderr @@ -1 +1 @@ -error: Cannot specify multiple crates with path or git or vers +error: cannot specify multiple crates with path or git or vers diff --git a/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_rename.stderr b/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_rename.stderr index 862214ee81..cbc5cdf7af 100644 --- a/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_rename.stderr +++ b/crates/cargo-add/tests/snapshots/add/multiple_conflicts_with_rename.stderr @@ -1 +1 @@ -error: Cannot specify multiple crates with rename +error: cannot specify multiple crates with rename diff --git a/crates/cargo-add/tests/snapshots/add/overwrite_name_dev_noop.stderr b/crates/cargo-add/tests/snapshots/add/overwrite_name_dev_noop.stderr index 5f877228d5..255589813c 100644 --- a/crates/cargo-add/tests/snapshots/add/overwrite_name_dev_noop.stderr +++ b/crates/cargo-add/tests/snapshots/add/overwrite_name_dev_noop.stderr @@ -1,4 +1,4 @@ -warning: Unrecognized features: ["one", "two"] +warning: unrecognized features: ["one", "two"] Adding cargo-list-test-fixture-dependency (local) to dev-dependencies. Features: + one diff --git a/crates/cargo-add/tests/snapshots/add/overwrite_name_noop.stderr b/crates/cargo-add/tests/snapshots/add/overwrite_name_noop.stderr index 14bee7827f..84e4f5d3a2 100644 --- a/crates/cargo-add/tests/snapshots/add/overwrite_name_noop.stderr +++ b/crates/cargo-add/tests/snapshots/add/overwrite_name_noop.stderr @@ -1,4 +1,4 @@ -warning: Unrecognized features: ["one", "two"] +warning: unrecognized features: ["one", "two"] Adding cargo-list-test-fixture-dependency (local) to optional dependencies. Features: + one diff --git a/crates/cargo-add/tests/snapshots/add/overwrite_path_noop.stderr b/crates/cargo-add/tests/snapshots/add/overwrite_path_noop.stderr index 14bee7827f..84e4f5d3a2 100644 --- a/crates/cargo-add/tests/snapshots/add/overwrite_path_noop.stderr +++ b/crates/cargo-add/tests/snapshots/add/overwrite_path_noop.stderr @@ -1,4 +1,4 @@ -warning: Unrecognized features: ["one", "two"] +warning: unrecognized features: ["one", "two"] Adding cargo-list-test-fixture-dependency (local) to optional dependencies. Features: + one diff --git a/crates/cargo-add/tests/testsuite/cargo_add.rs b/crates/cargo-add/tests/testsuite/cargo_add.rs index 11daae25ce..d7707c5ea0 100644 --- a/crates/cargo-add/tests/testsuite/cargo_add.rs +++ b/crates/cargo-add/tests/testsuite/cargo_add.rs @@ -133,6 +133,9 @@ fn add_registry_packages(alt: bool) { .publish(); } + cargo_test_support::registry::Package::new("prerelease_only", "0.2.0-alpha.1") + .alternative(alt) + .publish(); cargo_test_support::registry::Package::new("test_breaking", "0.2.0") .alternative(alt) .publish(); @@ -226,6 +229,24 @@ fn add_normalized_name_external() { ); } +#[cargo_test] +fn infer_prerelease() { + init_registry(); + let project_root = project_from_template("tests/snapshots/add/infer_prerelease.in"); + let cwd = &project_root; + + cargo_command() + .arg("add") + .args(["prerelease_only"]) + .current_dir(cwd) + .assert() + .success() + .stdout_matches_path("tests/snapshots/add/infer_prerelease.stdout") + .stderr_matches_path("tests/snapshots/add/infer_prerelease.stderr"); + + assert().subset_matches("tests/snapshots/add/infer_prerelease.out", &project_root); +} + #[cargo_test] fn build() { init_registry(); @@ -1826,3 +1847,42 @@ fn workspace_name() { assert().subset_matches("tests/snapshots/add/workspace_name.out", &project_root); } + +#[cargo_test] +fn deprecated_default_features() { + init_registry(); + let project_root = project_from_template("tests/snapshots/add/deprecated_default_features.in"); + let cwd = &project_root; + + cargo_command() + .arg("add") + .args(["my-package"]) + .current_dir(&cwd) + .assert() + .failure() + .stdout_matches_path("tests/snapshots/add/deprecated_default_features.stdout") + .stderr_matches_path("tests/snapshots/add/deprecated_default_features.stderr"); + + assert().subset_matches( + "tests/snapshots/add/deprecated_default_features.out", + &project_root, + ); +} + +#[cargo_test] +fn deprecated_section() { + init_registry(); + let project_root = project_from_template("tests/snapshots/add/deprecated_section.in"); + let cwd = &project_root; + + cargo_command() + .arg("add") + .args(["my-package"]) + .current_dir(&cwd) + .assert() + .failure() + .stdout_matches_path("tests/snapshots/add/deprecated_section.stdout") + .stderr_matches_path("tests/snapshots/add/deprecated_section.stderr"); + + assert().subset_matches("tests/snapshots/add/deprecated_section.out", &project_root); +}