Apply feedback on merge into cargo

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:
 - `<name>`, like `cargo add serde` (latest version will be used)
 - `<name>@<version-req>`, like `cargo add serde@1` or `cargo add serde@=1.0.38`
 - `<path>`, 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,15 +148,13 @@ 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")
                 .long("tag")
                 .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,16 +180,15 @@ 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 <PKGID>`"),
+                anyhow::format_err!("no packages selected.  Please specify one with `-p <PKGID>`"),
                 101,
             ));
         }
         1 => packages[0],
         len => {
             return Err(CliError::new(
                 anyhow::format_err!(
-                    "{} packages selected.  Please specify one with `-p <PKGID>`",
-                    len
+                    "{len} packages selected.  Please specify one with `-p <PKGID>`",
                 ),
                 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<DepOp> = Vec::new();
@@ -315,15 +303,30 @@ fn resolve_bool_arg(yes: bool, no: bool) -> Option<bool> {
     }
 }
 
-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<Item = &str> {
+    // 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())
 }

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> {
-        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('\\')
 }

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<Self> {
+    pub fn from_toml(crate_root: &Path, key: &str, item: &toml_edit::Item) -> CargoResult<Self> {
         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::<Option<IndexSet<String>>>()?,
+                        .map(|v| {
+                            v.as_str().map(|s| s.to_owned()).ok_or_else(|| {
+                                invalid_type(key, "features", v.type_name(), "string")
+                            })
+                        })
+                        .collect::<CargoResult<IndexSet<String>>>()?,
                 )
             } 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() {