From 2fd18762ea9f1e021abc4a490573a956b39a1316 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:34:41 -0500 Subject: [PATCH 01/25] style: Use implicit format arguments --- .../cargo-add/src/bin/cargo/commands/add.rs | 3 +-- .../src/cargo/ops/cargo_add/crate_spec.rs | 7 +++--- .../src/cargo/ops/cargo_add/manifest.rs | 5 ++-- .../cargo-add/src/cargo/ops/cargo_add/mod.rs | 25 ++++++------------- 4 files changed, 13 insertions(+), 27 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 7d089e0a32..76991117ef 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -199,8 +199,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, )); 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..af377c72c0 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 @@ -47,18 +47,17 @@ impl CrateSpec { .collect(); if !invalid.is_empty() { anyhow::bail!( - "Name `{}` contains invalid characters: {}", - name, + "Name `{name}` contains invalid characters: {}", invalid.join(", ") ); } if name.is_empty() { - anyhow::bail!("pkg id has empty name: `{}`", pkg_id); + anyhow::bail!("pkg id has empty name: `{pkg_id}`"); } 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 { 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..d50dcd3ea6 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -300,9 +300,8 @@ impl LocalManifest { Some(dep) => (table_path, Ok(dep)), None => { let message = anyhow::format_err!( - "Invalid dependency {}.{}", + "Invalid dependency {}.{dep_key}", table_path.join("."), - dep_key ); (table_path, Err(message)) } @@ -465,7 +464,7 @@ fn parse_manifest_err() -> anyhow::Error { } 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 { 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..38b5da8961 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -96,7 +96,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:?}"))?; }; } @@ -215,8 +215,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 +224,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: _, @@ -259,7 +254,7 @@ fn resolve_dependency( // dev-dependencies do not need the version populated if section != DepTable::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); @@ -377,10 +372,7 @@ fn get_latest_dependency( (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() { @@ -464,10 +456,7 @@ fn populate_available_features( (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 +497,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(); From 70e801faf8df9471c7be2daa3b636aaa1dd326fa Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:38:36 -0500 Subject: [PATCH 02/25] fix(error): Start errors with lowercase like cargo --- crates/cargo-add/src/bin/cargo/commands/add.rs | 8 ++++---- .../src/cargo/ops/cargo_add/crate_spec.rs | 4 ++-- .../cargo-add/src/cargo/ops/cargo_add/manifest.rs | 12 ++++++------ crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 14 +++++++------- .../add/add_normalized_name_external.stderr | 4 ++-- .../tests/snapshots/add/features_unknown.stderr | 2 +- .../snapshots/add/git_conflicts_namever.stderr | 2 +- .../snapshots/add/invalid_inline_path_self.stderr | 2 +- .../snapshots/add/invalid_name_external.stderr | 2 +- .../tests/snapshots/add/invalid_vers.stderr | 2 +- .../add/multiple_conflicts_with_features.stderr | 2 +- .../add/multiple_conflicts_with_git.stderr | 2 +- .../add/multiple_conflicts_with_rename.stderr | 2 +- .../snapshots/add/overwrite_name_dev_noop.stderr | 2 +- .../tests/snapshots/add/overwrite_name_noop.stderr | 2 +- .../tests/snapshots/add/overwrite_path_noop.stderr | 2 +- 16 files changed, 32 insertions(+), 32 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 76991117ef..149a3c7a3f 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -191,7 +191,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, )); } @@ -242,18 +242,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(); 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 af377c72c0..fc03a7287a 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 @@ -47,7 +47,7 @@ impl CrateSpec { .collect(); if !invalid.is_empty() { anyhow::bail!( - "Name `{name}` contains invalid characters: {}", + "name `{name}` contains invalid characters: {}", invalid.join(", ") ); } @@ -57,7 +57,7 @@ impl CrateSpec { 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 { 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 d50dcd3ea6..6129453952 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -241,13 +241,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() ); } @@ -300,7 +300,7 @@ impl LocalManifest { Some(dep) => (table_path, Ok(dep)), None => { let message = anyhow::format_err!( - "Invalid dependency {}.{dep_key}", + "invalid dependency {}.{dep_key}", table_path.join("."), ); (table_path, Err(message)) @@ -460,13 +460,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 `{table}` could not be found.") + 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 38b5da8961..43b89a7d13 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -96,7 +96,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 +104,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()? ) } @@ -215,7 +215,7 @@ fn resolve_dependency( match &crate_spec { CrateSpec::Path(path) => { anyhow::bail!( - "Cannot specify a git URL (`{url}`) with a path (`{}`).", + "cannot specify a git URL (`{url}`) with a path (`{}`).", path.display() ) } @@ -224,7 +224,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 (`{url}`) with a version (`{v}`).",) + anyhow::bail!("cannot specify a git URL (`{url}`) with a version (`{v}`).",) } CrateSpec::PkgId { name: _, @@ -263,7 +263,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 @@ -372,7 +372,7 @@ fn get_latest_dependency( (stable, s.version()) }) .ok_or_else(|| { - anyhow::format_err!("The crate `{dependency}` could not be found in registry index.") + 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() { @@ -456,7 +456,7 @@ fn populate_available_features( (is_pre, s.version()) }) .ok_or_else(|| { - anyhow::format_err!("The crate `{dependency}` could not be found in registry index.") + anyhow::format_err!("the crate `{dependency}` could not be found in registry index.") })?; dependency = dependency.set_available_features_from_cargo(lowest_common_denominator.features()); 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/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/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 From 06a3a60d144cbb2374768f37c6c5dfcfb77dd36f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:40:32 -0500 Subject: [PATCH 03/25] refactor: Remove dead long_help calls --- crates/cargo-add/src/bin/cargo/commands/add.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 149a3c7a3f..a6fc7261de 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -46,12 +46,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 +92,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 +101,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,7 +134,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") @@ -157,7 +151,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 +158,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") From bbb035b0a2b58814dfaa604c00869095071043f3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:41:16 -0500 Subject: [PATCH 04/25] fix(cli): Fix grammar error in '--help' --- crates/cargo-add/src/bin/cargo/commands/add.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index a6fc7261de..1a267a9646 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -37,7 +37,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/` From ce59a446b322ca586202c22639988f2e443e5028 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:44:08 -0500 Subject: [PATCH 05/25] refactor: Reuse package name validation --- .../src/cargo/ops/cargo_add/crate_spec.rs | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) 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 fc03a7287a..61e058bb81 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 @@ -40,20 +40,7 @@ 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 `{name}` contains invalid characters: {}", - 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) @@ -103,10 +90,6 @@ impl std::str::FromStr for CrateSpec { } } -fn is_name_char(c: char) -> bool { - c.is_alphanumeric() || ['-', '_'].contains(&c) -} - fn is_path_like(s: &str) -> bool { s.contains('/') || s.contains('\\') } From ef60aec48ad6d525e7e9b14f234a672e84de7218 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:44:34 -0500 Subject: [PATCH 06/25] refactor: Remove dead FromStr --- crates/cargo-add/src/cargo/ops/cargo_add/crate_spec.rs | 8 -------- 1 file changed, 8 deletions(-) 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 61e058bb81..29b9595956 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 @@ -82,14 +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_path_like(s: &str) -> bool { s.contains('/') || s.contains('\\') } From 8fdc49dfbf28c4d4449e92825a05a6fcbfc2944e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:45:33 -0500 Subject: [PATCH 07/25] refactor: Don't pollute scope with Context --- crates/cargo-add/src/cargo/ops/cargo_add/crate_spec.rs | 2 +- crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs | 2 +- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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 29b9595956..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; 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 6129453952..b71fa6f510 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -3,7 +3,7 @@ use std::ops::{Deref, DerefMut}; use std::path::{Path, PathBuf}; use std::str; -use anyhow::Context; +use anyhow::Context as _; use cargo::util::interning::InternedString; use cargo::CargoResult; 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 43b89a7d13..2c5465be4f 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,7 @@ use std::collections::BTreeSet; use std::collections::VecDeque; use std::path::Path; -use anyhow::Context; +use anyhow::Context as _; use cargo::core::Registry; use cargo::CargoResult; use cargo::Config; From 674bf20b8e0322bbabe1506db0b4914daaa311c5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:46:12 -0500 Subject: [PATCH 08/25] refactor: Remove unused assignment --- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 2c5465be4f..20f3bb7bdb 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -47,8 +47,7 @@ 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 mut registry = cargo::core::registry::PackageRegistry::new(options.config)?; From 69cc62846cff61fbe425b4d4a531d88994087b9c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:48:07 -0500 Subject: [PATCH 09/25] refactor: Remove redundant feature parse --- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) 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 20f3bb7bdb..51eeadbe17 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -381,13 +381,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; @@ -409,8 +402,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 { From 70c9bc07a8a8b2bc76aaf10639881dd3b8f00112 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:49:08 -0500 Subject: [PATCH 10/25] refactor: Move parse_feature to where to smallest scope --- crates/cargo-add/src/bin/cargo/commands/add.rs | 10 +++++++++- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 9 --------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 1a267a9646..88156133d7 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -3,7 +3,6 @@ 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; @@ -318,3 +317,12 @@ fn parse_section(matches: &ArgMatches) -> DepTable<'_> { DepTable::Normal } } + +/// 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/mod.rs b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs index 51eeadbe17..2f9b1916f1 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -413,15 +413,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, From 769166cb5fdf591f8dd914ca3b2b1eda28c04820 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:52:29 -0500 Subject: [PATCH 11/25] docs: Clarify pre-release shenanigans --- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) 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 2f9b1916f1..b1bdd5a98f 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -367,6 +367,8 @@ 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()) }) @@ -435,6 +437,8 @@ fn populate_available_features( 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()) }) From bf8d11c69cbd1e24399bd2dd5116854e1a2ce130 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:52:50 -0500 Subject: [PATCH 12/25] docs: Clarify why features are read from oldest version --- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 2 ++ 1 file changed, 2 insertions(+) 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 b1bdd5a98f..4e04352787 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -434,6 +434,8 @@ 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| { From e67e9b729c3720bfcbcf4c78089f85438d025e68 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 13:53:59 -0500 Subject: [PATCH 13/25] chore: Remove references to clippy --- crates/cargo-add/src/bin/cargo/commands/add.rs | 2 -- crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 88156133d7..c05f2c004f 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -1,5 +1,3 @@ -#![allow(clippy::bool_assert_comparison)] - use cargo::util::command_prelude::*; use cargo::CargoResult; use cargo_add::ops::add; 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..820c8a2078 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs @@ -390,7 +390,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); From 2ead775d6c090460a965775a3e8995e33d44bf27 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 14:25:03 -0500 Subject: [PATCH 14/25] refactor: Simplify dep table lookup --- crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) 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 b71fa6f510..dd41c4346d 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -265,16 +265,6 @@ impl LocalManifest { &'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, - { let crate_root = self.path.parent().expect("manifest path is absolute"); self.get_sections() .into_iter() @@ -284,7 +274,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 From 2a383f99089102b82a36fe6c287ecb7cfbd6b70c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 16:05:24 -0500 Subject: [PATCH 15/25] refactor: Allow more error reports --- .../src/cargo/ops/cargo_add/dependency.rs | 105 ++++++++++++------ .../src/cargo/ops/cargo_add/manifest.rs | 11 +- 2 files changed, 72 insertions(+), 44 deletions(-) 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 820c8a2078..1094067840 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs @@ -216,44 +216,72 @@ 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 }; @@ -263,10 +291,15 @@ impl Dependency { 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 +319,9 @@ impl Dependency { available_features, optional, }; - Some(dep) + Ok(dep) } else { - None + anyhow::bail!("Unrecognized` dependency entry format for `{key}"); } } @@ -493,6 +526,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 dd41c4346d..d5e6f1098e 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -286,16 +286,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 {}.{dep_key}", - table_path.join("."), - ); - (table_path, Err(message)) - } - } + (table_path, dep) }) } From 67eee4f9b328417448de5cdb09488d7a9580a51f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 16:10:02 -0500 Subject: [PATCH 16/25] fix: Don't ignore manifest errors --- crates/cargo-add/src/cargo/ops/cargo_add/mod.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) 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 4e04352787..08954a8c06 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -197,7 +197,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() { @@ -297,9 +297,10 @@ fn get_existing_dependency( manifest: &LocalManifest, dep_key: &str, section: DepTable<'_>, -) -> Option { +) -> CargoResult> { #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)] enum Key { + Error, Dev, Build, Target, @@ -310,10 +311,11 @@ fn get_existing_dependency( 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 + } else if dep.is_err() { + Key::Error } else { match path[0].as_str() { "dependencies" => Key::Runtime, @@ -327,7 +329,12 @@ fn get_existing_dependency( }) .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 { // When the dep comes from a different section, we only care about the source and not any @@ -346,7 +353,7 @@ fn get_existing_dependency( } } - Some(dep) + Ok(Some(dep)) } fn get_latest_dependency( From 0d30afaa272bd46fd5c7efe070a2bc2d96722a3f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 16:18:31 -0500 Subject: [PATCH 17/25] fix: Error on default_features --- .../src/cargo/ops/cargo_add/dependency.rs | 3 +++ .../deprecated_default_features.in/Cargo.toml | 8 +++++++ .../deprecated_default_features.in/src/lib.rs | 0 .../Cargo.toml | 8 +++++++ .../add/deprecated_default_features.stderr | 1 + .../add/deprecated_default_features.stdout | 0 crates/cargo-add/tests/testsuite/cargo_add.rs | 21 +++++++++++++++++++ 7 files changed, 41 insertions(+) create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/Cargo.toml create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_default_features.in/src/lib.rs create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_default_features.out/Cargo.toml create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_default_features.stderr create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_default_features.stdout 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 1094067840..95931e51eb 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/dependency.rs @@ -287,6 +287,9 @@ 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"); + } let features = if let Some(value) = table.get("features") { Some( 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/testsuite/cargo_add.rs b/crates/cargo-add/tests/testsuite/cargo_add.rs index 11daae25ce..9d064f0c33 100644 --- a/crates/cargo-add/tests/testsuite/cargo_add.rs +++ b/crates/cargo-add/tests/testsuite/cargo_add.rs @@ -1826,3 +1826,24 @@ fn workspace_name() { assert().subset_matches("tests/snapshots/add/workspace_name.out", &project_root); } + +#[cargo_test] +fn deprecated_default_feature() { + 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, + ); +} From 111f9030ece82a5ec2cb9d1817b5150487b45810 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 16:34:38 -0500 Subject: [PATCH 18/25] fix: Error on deprecated dependency sections --- .../src/cargo/ops/cargo_add/manifest.rs | 28 +++++++++++++++++++ .../cargo-add/src/cargo/ops/cargo_add/mod.rs | 7 +++++ .../add/deprecated_section.in/Cargo.toml | 11 ++++++++ .../add/deprecated_section.in/src/lib.rs | 0 .../add/deprecated_section.out/Cargo.toml | 11 ++++++++ .../snapshots/add/deprecated_section.stderr | 1 + .../snapshots/add/deprecated_section.stdout | 0 crates/cargo-add/tests/testsuite/cargo_add.rs | 20 ++++++++++++- 8 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_section.in/Cargo.toml create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_section.in/src/lib.rs create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_section.out/Cargo.toml create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_section.stderr create mode 100644 crates/cargo-add/tests/snapshots/add/deprecated_section.stdout 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 d5e6f1098e..3c8ae7ad78 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -135,6 +135,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") 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 08954a8c06..38a547c052 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -49,6 +49,13 @@ pub fn add(workspace: &cargo::core::Workspace, options: &AddOptions<'_>) -> Carg 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)?; 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/testsuite/cargo_add.rs b/crates/cargo-add/tests/testsuite/cargo_add.rs index 9d064f0c33..d9cd3d6222 100644 --- a/crates/cargo-add/tests/testsuite/cargo_add.rs +++ b/crates/cargo-add/tests/testsuite/cargo_add.rs @@ -1828,7 +1828,7 @@ fn workspace_name() { } #[cargo_test] -fn deprecated_default_feature() { +fn deprecated_default_features() { init_registry(); let project_root = project_from_template("tests/snapshots/add/deprecated_default_features.in"); let cwd = &project_root; @@ -1847,3 +1847,21 @@ fn deprecated_default_feature() { &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); +} From 18265f2cd418695687121093ca0596a27e5ce4a4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 20:17:44 -0500 Subject: [PATCH 19/25] refactor: Make DepTable owned --- .../cargo-add/src/bin/cargo/commands/add.rs | 4 ++-- .../cargo-add/src/cargo/ops/cargo_add/mod.rs | 24 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index c05f2c004f..fcffa72caf 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -303,14 +303,14 @@ fn resolve_bool_arg(yes: bool, no: bool) -> Option { } } -fn parse_section(matches: &ArgMatches) -> DepTable<'_> { +fn parse_section(matches: &ArgMatches) -> DepTable { if matches.is_present("dev") { DepTable::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) + DepTable::Target(target.to_owned()) } else { DepTable::Normal } 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 38a547c052..707b198ea0 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -33,7 +33,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, } @@ -70,7 +70,7 @@ pub fn add(workspace: &cargo::core::Workspace, options: &AddOptions<'_>) -> Carg &manifest, raw, workspace, - options.section, + &options.section, options.config, &mut registry, ) @@ -168,8 +168,8 @@ pub struct DepOp { } /// Dependency table to add dep to -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum DepTable<'m> { +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum DepTable { /// Used for building final artifact Normal, /// Used for testing @@ -177,11 +177,11 @@ pub enum DepTable<'m> { /// Used for build.rs Build, /// Used for building final artifact only on specific target platforms - Target(&'m str), + Target(String), } -impl<'m> DepTable<'m> { - fn to_table(self) -> Vec<&'m str> { +impl DepTable { + fn to_table(&self) -> Vec<&str> { match self { Self::Normal => vec!["dependencies"], Self::Development => vec!["dev-dependencies"], @@ -195,7 +195,7 @@ fn resolve_dependency( manifest: &LocalManifest, arg: &DepOp, ws: &cargo::core::Workspace, - section: DepTable<'_>, + section: &DepTable, config: &Config, registry: &mut cargo::core::registry::PackageRegistry, ) -> CargoResult { @@ -258,7 +258,7 @@ 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 != DepTable::Development { let op = ""; let v = format!("{op}{version}", version = package.version()); src = src.set_version(v); @@ -281,7 +281,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 == DepTable::Development; let preserve_existing_version = old_dep .as_ref() .map(|d| d.version().is_some()) @@ -303,7 +303,7 @@ fn resolve_dependency( fn get_existing_dependency( manifest: &LocalManifest, dep_key: &str, - section: DepTable<'_>, + section: &DepTable, ) -> CargoResult> { #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)] enum Key { @@ -354,7 +354,7 @@ 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 == DepTable::Development; if !version_required && version_optional_in_section { dep = dep.clear_version(); } From 46f670250a79aa5e151906778ddc8c4c6849a53b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 20:33:40 -0500 Subject: [PATCH 20/25] refactor: Move DepTable to be with Manifest --- .../src/cargo/ops/cargo_add/manifest.rs | 25 ++++++++++++++++++ .../cargo-add/src/cargo/ops/cargo_add/mod.rs | 26 ++----------------- 2 files changed, 27 insertions(+), 24 deletions(-) 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 3c8ae7ad78..dbc8163766 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -11,6 +11,31 @@ 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 enum DepTable { + /// 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(String), +} + +impl DepTable { + /// Keys to the table + pub fn to_table(&self) -> Vec<&str> { + match self { + Self::Normal => vec!["dependencies"], + Self::Development => vec!["dev-dependencies"], + Self::Build => vec!["build-dependencies"], + Self::Target(target) => vec!["target", target, "dependencies"], + } + } +} + /// A Cargo manifest #[derive(Debug, Clone)] pub struct Manifest { 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 707b198ea0..6a9763bf9f 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -23,6 +23,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> { @@ -167,30 +169,6 @@ pub struct DepOp { pub tag: Option, } -/// Dependency table to add dep to -#[derive(Clone, Debug, PartialEq, Eq)] -pub enum DepTable { - /// 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(String), -} - -impl DepTable { - fn to_table(&self) -> Vec<&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, From 5aa3de96f81b0c41e87d8c0a0043027705919eac Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 20:46:02 -0500 Subject: [PATCH 21/25] refactor: Decouple Kind and Target for DepTable --- .../cargo-add/src/bin/cargo/commands/add.rs | 21 ++++-- .../src/cargo/ops/cargo_add/manifest.rs | 66 +++++++++++++++---- .../cargo-add/src/cargo/ops/cargo_add/mod.rs | 7 +- 3 files changed, 70 insertions(+), 24 deletions(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index fcffa72caf..967c6f0cdd 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -1,3 +1,4 @@ +use cargo::core::dependency::DepKind; use cargo::util::command_prelude::*; use cargo::CargoResult; use cargo_add::ops::add; @@ -304,16 +305,22 @@ fn resolve_bool_arg(yes: bool, no: bool) -> Option { } fn parse_section(matches: &ArgMatches) -> DepTable { - if matches.is_present("dev") { - DepTable::Development + 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.to_owned()) + 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 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 dbc8163766..f4490206c2 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -4,6 +4,7 @@ use std::path::{Path, PathBuf}; use std::str; use anyhow::Context as _; +use cargo::core::dependency::DepKind; use cargo::util::interning::InternedString; use cargo::CargoResult; @@ -13,27 +14,64 @@ const DEP_TABLES: &[&str] = &["dependencies", "dev-dependencies", "build-depende /// Dependency table to add dep to #[derive(Clone, Debug, PartialEq, Eq)] -pub enum DepTable { - /// 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(String), +pub struct DepTable { + kind: DepKind, + target: Option, } impl DepTable { + /// Reference to a Dependency Table + pub fn new() -> Self { + Self { + kind: DepKind::Normal, + target: None, + } + } + + /// Choose the type of dependency + pub 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> { - match self { - Self::Normal => vec!["dependencies"], - Self::Development => vec!["dev-dependencies"], - Self::Build => vec!["build-dependencies"], - Self::Target(target) => vec!["target", target, "dependencies"], + 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() + } } /// A Cargo manifest 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 6a9763bf9f..6b67009332 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -9,6 +9,7 @@ use std::collections::VecDeque; use std::path::Path; use anyhow::Context as _; +use cargo::core::dependency::DepKind; use cargo::core::Registry; use cargo::CargoResult; use cargo::Config; @@ -236,7 +237,7 @@ 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}", version = package.version()); src = src.set_version(v); @@ -259,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()) @@ -332,7 +333,7 @@ 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(); } From f248c8539fe8cfd5f39941b4589ea0a2a0b2ad5e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 20:47:01 -0500 Subject: [PATCH 22/25] fix: Allow target-specific dev/build deps --- crates/cargo-add/src/bin/cargo/commands/add.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/cargo-add/src/bin/cargo/commands/add.rs b/crates/cargo-add/src/bin/cargo/commands/add.rs index 967c6f0cdd..caf5854720 100644 --- a/crates/cargo-add/src/bin/cargo/commands/add.rs +++ b/crates/cargo-add/src/bin/cargo/commands/add.rs @@ -132,7 +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") - .group("section"), ]) .next_help_heading("UNSTABLE") .args([ From 0f5869bef4c53a6459f4a3268dfbf6e8083f2cea Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 20:51:57 -0500 Subject: [PATCH 23/25] refactor: Consolidate kind table names --- .../src/cargo/ops/cargo_add/manifest.rs | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) 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 f4490206c2..3effaf52c9 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -10,8 +10,6 @@ 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 { @@ -20,8 +18,14 @@ pub struct DepTable { } 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 fn new() -> Self { + pub const fn new() -> Self { Self { kind: DepKind::Normal, target: None, @@ -29,7 +33,7 @@ impl DepTable { } /// Choose the type of dependency - pub fn set_kind(mut self, kind: DepKind) -> Self { + pub const fn set_kind(mut self, kind: DepKind) -> Self { self.kind = kind; self } @@ -74,6 +78,12 @@ impl Default for DepTable { } } +impl From for DepTable { + fn from(other: DepKind) -> Self { + Self::new().set_kind(other) + } +} + /// A Cargo manifest #[derive(Debug, Clone)] pub struct Manifest { @@ -156,7 +166,8 @@ impl Manifest { pub fn get_sections(&self) -> Vec<(Vec, 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 @@ -165,7 +176,7 @@ impl Manifest { .unwrap_or(false) { sections.push(( - vec![String::from(*dependency_type)], + vec![String::from(dependency_type)], self.data[dependency_type].clone(), )) } @@ -185,7 +196,7 @@ impl Manifest { vec![ "target".to_string(), target_name.to_string(), - String::from(*dependency_type), + String::from(dependency_type), ], dependency_table.clone(), ) From 1e738a01c5b3e3b729c3056e9694f87915cdd66b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 20:56:16 -0500 Subject: [PATCH 24/25] refactor: Always operate on DepTable --- .../src/cargo/ops/cargo_add/manifest.rs | 15 +++-------- .../cargo-add/src/cargo/ops/cargo_add/mod.rs | 25 ++++++++----------- 2 files changed, 15 insertions(+), 25 deletions(-) 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 3effaf52c9..411b5a333d 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/manifest.rs @@ -163,7 +163,7 @@ 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 table in DepTable::KINDS { @@ -175,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`. @@ -193,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(), ) }) @@ -366,7 +359,7 @@ impl LocalManifest { pub fn get_dependency_versions<'s>( &'s self, dep_key: &'s str, - ) -> impl Iterator, CargoResult)> + 's { + ) -> impl Iterator)> + 's { let crate_root = self.path.parent().expect("manifest path is absolute"); self.get_sections() .into_iter() 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 6b67009332..70059cc190 100644 --- a/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs +++ b/crates/cargo-add/src/cargo/ops/cargo_add/mod.rs @@ -289,27 +289,24 @@ fn get_existing_dependency( Error, Dev, Build, - Target, - Runtime, + Normal, Existing, } - let target_section = section.to_table(); let mut possible: Vec<_> = manifest .get_dependency_versions(dep_key) .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 + (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) }) @@ -322,7 +319,7 @@ fn get_existing_dependency( }; 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; From 84af19bd1cd6757dfca4024f886db8ddbf42a2ce Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 14 Mar 2022 21:09:23 -0500 Subject: [PATCH 25/25] test: Ensure we can add dep when only pre-releases exist --- .../tests/snapshots/add/infer_prerelease.in | 1 + .../add/infer_prerelease.out/Cargo.toml | 8 +++++++ .../snapshots/add/infer_prerelease.stderr | 2 ++ .../snapshots/add/infer_prerelease.stdout | 0 crates/cargo-add/tests/testsuite/cargo_add.rs | 21 +++++++++++++++++++ 5 files changed, 32 insertions(+) create mode 120000 crates/cargo-add/tests/snapshots/add/infer_prerelease.in create mode 100644 crates/cargo-add/tests/snapshots/add/infer_prerelease.out/Cargo.toml create mode 100644 crates/cargo-add/tests/snapshots/add/infer_prerelease.stderr create mode 100644 crates/cargo-add/tests/snapshots/add/infer_prerelease.stdout 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/testsuite/cargo_add.rs b/crates/cargo-add/tests/testsuite/cargo_add.rs index d9cd3d6222..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();