Update for latest review feedback

crates/cargo-add/Cargo.toml

@@ -46,6 +46,7 @@ git2 = "0.14"
 clap = { version = "3.1", features = ["wrap_help"], optional = true }
 #cargo = "0.59.0"
 cargo.git = "https://github.com/rust-lang/cargo"
+cargo-util.git = "https://github.com/rust-lang/cargo"
 toml_edit = { version = "0.13.3", features = ["easy"] }
 indexmap = "1"
 url = "2.2.2"

crates/cargo-add/src/bin/cargo/commands/add.rs

@@ -230,21 +230,6 @@ 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");
-    }
-    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");
-    }
-
-    if crates.len() > 1 && features.is_some() {
-        anyhow::bail!("cannot specify multiple crates with features");
-    }
-
     let mut deps: Vec<DepOp> = Vec::new();
     for crate_spec in crates {
         if let Some(features) = crate_spec.strip_prefix('+') {
@@ -277,6 +262,22 @@ fn parse_dependencies<'m>(config: &Config, matches: &'m ArgMatches) -> CargoResu
             deps.push(dep);
         }
     }
+
+    if deps.len() > 1 && git.is_some() {
+        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 deps.len() > 1 && rename.is_some() {
+        anyhow::bail!("cannot specify multiple crates with `--rename`");
+    }
+
+    if deps.len() > 1 && features.is_some() {
+        anyhow::bail!("cannot specify multiple crates with `--features`");
+    }
+
     Ok(deps)
 }
 

crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs

@@ -288,7 +288,7 @@ impl Dependency {
 
             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");
+                anyhow::bail!("Use of `default_features` in `{key}` is unsupported, please switch to `default-features`");
             }
 
             let features = if let Some(value) = table.get("features") {
@@ -429,9 +429,6 @@ impl Dependency {
         if str_or_1_len_table(item) {
             // Nothing to preserve
             *item = self.to_toml(crate_root);
-        } else if !is_package_eq(item, &self.name, self.rename.as_deref()) {
-            // No existing keys are relevant when the package changes
-            *item = self.to_toml(crate_root);
         } else if let Some(table) = item.as_table_like_mut() {
             match &self.source {
                 Some(Source::Registry(src)) => {
@@ -563,16 +560,6 @@ fn path_field(crate_root: &Path, abs_path: &Path) -> String {
     relpath
 }
 
-fn is_package_eq(item: &mut toml_edit::Item, name: &str, rename: Option<&str>) -> bool {
-    if let Some(table) = item.as_table_like_mut() {
-        let existing_package = table.get("package").and_then(|i| i.as_str());
-        let new_package = rename.map(|_| name);
-        existing_package == new_package
-    } else {
-        false
-    }
-}
-
 /// Primary location of a dependency
 #[derive(Debug, Hash, PartialEq, Eq, Clone)]
 pub enum Source {

crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs

@@ -97,7 +97,8 @@ impl Manifest {
         self.data
             .as_table()
             .get("package")
-            .and_then(|m| m["name"].as_str())
+            .and_then(|m| m.get("name"))
+            .and_then(|m| m.as_str())
             .ok_or_else(parse_manifest_err)
     }
 
@@ -106,7 +107,8 @@ impl Manifest {
         self.data
             .as_table()
             .get("package")
-            .and_then(|m| m["version"].as_str())
+            .and_then(|m| m.get("version"))
+            .and_then(|m| m.as_str())
             .ok_or_else(parse_manifest_err)
     }
 
@@ -285,7 +287,7 @@ impl str::FromStr for Manifest {
 
     /// Read manifest data from string
     fn from_str(input: &str) -> ::std::result::Result<Self, Self::Err> {
-        let d: toml_edit::Document = input.parse().with_context(|| "Manifest not valid TOML")?;
+        let d: toml_edit::Document = input.parse().context("Manifest not valid TOML")?;
 
         Ok(Manifest { data: d })
     }
@@ -324,10 +326,9 @@ impl DerefMut for LocalManifest {
 impl LocalManifest {
     /// Construct the `LocalManifest` corresponding to the `Path` provided.
     pub fn try_new(path: &Path) -> CargoResult<Self> {
-        let path = dunce::canonicalize(path).with_context(|| "Failed to read manifest contents")?;
-        let data =
-            std::fs::read_to_string(&path).with_context(|| "Failed to read manifest contents")?;
-        let manifest = data.parse().with_context(|| "Unable to parse Cargo.toml")?;
+        let path = dunce::canonicalize(path).context("Failed to read manifest contents")?;
+        let data = cargo_util::paths::read(&path)?;
+        let manifest = data.parse().context("Unable to parse Cargo.toml")?;
         Ok(LocalManifest { manifest, path })
     }
 
@@ -353,8 +354,7 @@ impl LocalManifest {
         let s = self.manifest.data.to_string();
         let new_contents_bytes = s.as_bytes();
 
-        std::fs::write(&self.path, new_contents_bytes)
-            .with_context(|| "Failed to write updated Cargo.toml")
+        cargo_util::paths::write(&self.path, new_contents_bytes)
     }
 
     /// Lookup a dependency
@@ -426,7 +426,7 @@ impl LocalManifest {
                 if let toml_edit::Item::Value(toml_edit::Value::Array(feature_values)) =
                     &mut feature_values
                 {
-                    remove_feature_activation(
+                    fix_feature_activations(
                         feature_values,
                         dep_key,
                         status,
@@ -464,10 +464,11 @@ impl LocalManifest {
         for (_, tbl) in self.get_sections() {
             if let toml_edit::Item::Table(tbl) = tbl {
                 if let Some(dep_item) = tbl.get(dep_key) {
-                    let optional = dep_item.get("optional");
-                    let optional = optional.and_then(|i| i.as_value());
-                    let optional = optional.and_then(|i| i.as_bool());
-                    let optional = optional.unwrap_or(false);
+                    let optional = dep_item
+                        .get("optional")
+                        .and_then(|i| i.as_value())
+                        .and_then(|i| i.as_bool())
+                        .unwrap_or(false);
                     if optional {
                         return DependencyStatus::Optional;
                     } else {
@@ -487,7 +488,7 @@ enum DependencyStatus {
     Required,
 }
 
-fn remove_feature_activation(
+fn fix_feature_activations(
     feature_values: &mut toml_edit::Array,
     dep_key: &str,
     status: DependencyStatus,
@@ -498,30 +499,22 @@ fn remove_feature_activation(
         .enumerate()
         .filter_map(|(idx, value)| value.as_str().map(|s| (idx, s)))
         .filter_map(|(idx, value)| {
-            let value = cargo::core::FeatureValue::new(InternedString::new(value));
+            let parsed_value = cargo::core::FeatureValue::new(InternedString::new(value));
             match status {
-                DependencyStatus::None => {
-                    let dep_name = match (value, explicit_dep_activation) {
-                        (cargo::core::FeatureValue::Feature(dep_name), false)
-                        | (cargo::core::FeatureValue::Dep { dep_name }, _)
-                        | (cargo::core::FeatureValue::DepFeature { dep_name, .. }, _) => {
-                            Some(dep_name.as_str())
-                        }
-                        _ => None,
-                    };
-                    dep_name == Some(dep_key)
-                }
+                DependencyStatus::None => match (parsed_value, explicit_dep_activation) {
+                    (cargo::core::FeatureValue::Feature(dep_name), false)
+                    | (cargo::core::FeatureValue::Dep { dep_name }, _)
+                    | (cargo::core::FeatureValue::DepFeature { dep_name, .. }, _) => {
+                        dep_name == dep_key
+                    }
+                    _ => false,
+                },
                 DependencyStatus::Optional => false,
-                DependencyStatus::Required => {
-                    let dep_name = match (value, explicit_dep_activation) {
-                        (cargo::core::FeatureValue::Feature(dep_name), false)
-                        | (cargo::core::FeatureValue::Dep { dep_name }, _) => {
-                            Some(dep_name.as_str())
-                        }
-                        (cargo::core::FeatureValue::DepFeature { .. }, _) | _ => None,
-                    };
-                    dep_name == Some(dep_key)
-                }
+                DependencyStatus::Required => match (parsed_value, explicit_dep_activation) {
+                    (cargo::core::FeatureValue::Feature(dep_name), false)
+                    | (cargo::core::FeatureValue::Dep { dep_name }, _) => dep_name == dep_key,
+                    (cargo::core::FeatureValue::DepFeature { .. }, _) | _ => false,
+                },
             }
             .then(|| idx)
         })
@@ -531,6 +524,26 @@ fn remove_feature_activation(
     for idx in remove_list.iter().rev() {
         feature_values.remove(*idx);
     }
+
+    if status == DependencyStatus::Required {
+        for value in feature_values.iter_mut() {
+            let parsed_value = if let Some(value) = value.as_str() {
+                cargo::core::FeatureValue::new(InternedString::new(value))
+            } else {
+                continue;
+            };
+            if let cargo::core::FeatureValue::DepFeature {
+                dep_name,
+                dep_feature,
+                weak,
+            } = parsed_value
+            {
+                if dep_name == dep_key && weak {
+                    *value = format!("{dep_name}/{dep_feature}").into();
+                }
+            }
+        }
+    }
 }
 
 pub fn str_or_1_len_table(item: &toml_edit::Item) -> bool {

crates/cargo-add/src/cargo/ops/cargo_add/mod.rs

@@ -88,6 +88,15 @@ pub fn add(workspace: &cargo::core::Workspace<'_>, options: &AddOptions<'_>) ->
             table_option.map_or(true, |table| is_sorted(table.iter().map(|(name, _)| name)))
         });
     for dep in deps {
+        print_msg(&mut options.config.shell(), &dep, &dep_table)?;
+        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",
+                    manifest.package_name()?
+                )
+            }
+        }
         if let Some(req_feats) = dep.features.as_ref() {
             let req_feats: BTreeSet<_> = req_feats.iter().map(|s| s.as_str()).collect();
 
@@ -108,16 +117,6 @@ pub fn add(workspace: &cargo::core::Workspace<'_>, options: &AddOptions<'_>) ->
                     .warn(format!("unrecognized features: {unknown_features:?}"))?;
             };
         }
-
-        print_msg(&mut options.config.shell(), &dep, &dep_table)?;
-        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",
-                    manifest.package_name()?
-                )
-            }
-        }
         manifest.insert_into_table(&dep_table, &dep)?;
         manifest.gc_dep(dep.toml_key());
     }
@@ -186,12 +185,20 @@ fn resolve_dependency(
     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() {
-            // Overwrite with `crate_spec`
-            old_dep.source = spec_dep.source;
+        if old_dep.name != spec_dep.name {
+            // Assuming most existing keys are not relevant when the package changes
+            if spec_dep.optional.is_none() {
+                spec_dep.optional = old_dep.optional;
+            }
+            spec_dep
+        } else {
+            if spec_dep.source().is_some() {
+                // Overwrite with `crate_spec`
+                old_dep.source = spec_dep.source;
+            }
+            old_dep = populate_dependency(old_dep, arg);
+            old_dep
         }
-        old_dep = populate_dependency(old_dep, arg);
-        old_dep
     } else {
         spec_dep
     };

crates/cargo-add/tests/snapshots/add/change_rename_target.in/Cargo.toml

@@ -0,0 +1,8 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"
+
+[dependencies]
+some-package = { package = "my-package1", version = "0.1.1", optional = true }

crates/cargo-add/tests/snapshots/add/change_rename_target.out/Cargo.toml

@@ -0,0 +1,8 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"
+
+[dependencies]
+some-package = { package = "my-package2", version = "99999.0.0", optional = true }

crates/cargo-add/tests/snapshots/add/change_rename_target.stderr

@@ -0,0 +1,2 @@
+    Updating `dummy-registry` index
+      Adding my-package2 v99999.0.0 to optional dependencies.

crates/cargo-add/tests/snapshots/add/deprecated_default_features.stderr

@@ -1 +1 @@
-error: Use of `default_features` in `my-package` is unsupported
+error: Use of `default_features` in `my-package` is unsupported, please switch to `default-features`

crates/cargo-add/tests/snapshots/add/features_unknown.stderr

@@ -1,9 +1,9 @@
     Updating `dummy-registry` index
-warning: unrecognized features: ["noze"]
       Adding your-face v99999.0.0 to dependencies.
              Features:
              + noze
              - ears
              - eyes
              - mouth
              - nose
+warning: unrecognized features: ["noze"]

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`

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`

crates/cargo-add/tests/snapshots/add/overwrite_name_dev_noop.stderr

@@ -1,5 +1,5 @@
-warning: unrecognized features: ["one", "two"]
       Adding cargo-list-test-fixture-dependency (local) to dev-dependencies.
              Features:
              + one
              + two
+warning: unrecognized features: ["one", "two"]

crates/cargo-add/tests/snapshots/add/overwrite_name_noop.stderr

@@ -1,5 +1,5 @@
-warning: unrecognized features: ["one", "two"]
       Adding cargo-list-test-fixture-dependency (local) to optional dependencies.
              Features:
              + one
              + two
+warning: unrecognized features: ["one", "two"]

crates/cargo-add/tests/snapshots/add/overwrite_path_noop.stderr

@@ -1,5 +1,5 @@
-warning: unrecognized features: ["one", "two"]
       Adding cargo-list-test-fixture-dependency (local) to optional dependencies.
              Features:
              + one
              + two
+warning: unrecognized features: ["one", "two"]

crates/cargo-add/tests/snapshots/add/require_weak.in/Cargo.toml

@@ -0,0 +1,11 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"
+
+[features]
+eyes = ["your-face?/eyes"]
+
+[dependencies]
+your-face = { version = "99999.0.0", optional = true }