Auto merge of #11194 - cassaundra:cargo-remove-gc, r=epage

Clean profile, patch, and replace in cargo remove

### What does this PR try to resolve?

This PR is part of the continued work on cargo remove (#11099, see deferred work).

After a successful removal of a dependency, clean up the profile, patch, and replace sections to remove all references to it.

**Note** the GC process was expanded to clean up not just references to the dependencies just removed, but also references of all dependencies. This was because there's not an easy way to determine which dependencies correspond to the given TOML keys, without either 1) figuring that out before the removal (therefore having to predict the behavior), or 2) returning that information from the remove function (somewhat unorthodox for an op).

### How should we review and test this PR?

Verify that the implementation makes sense and that the tests are sufficient.

Author: bors

src/bin/cargo/commands/remove.rs

@@ -1,10 +1,14 @@
 use cargo::core::dependency::DepKind;
+use cargo::core::PackageIdSpec;
 use cargo::core::Workspace;
 use cargo::ops::cargo_remove::remove;
 use cargo::ops::cargo_remove::RemoveOptions;
 use cargo::ops::resolve_ws;
 use cargo::util::command_prelude::*;
 use cargo::util::print_available_packages;
+use cargo::util::toml_mut::dependency::Dependency;
+use cargo::util::toml_mut::dependency::MaybeWorkspace;
+use cargo::util::toml_mut::dependency::Source;
 use cargo::util::toml_mut::manifest::DepTable;
 use cargo::util::toml_mut::manifest::LocalManifest;
 use cargo::CargoResult;
@@ -86,7 +90,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
         .get_many::<String>("dependencies")
         .expect("required(true)")
         .cloned()
-        .collect();
+        .collect::<Vec<_>>();
 
     let section = parse_section(args);
 
@@ -100,8 +104,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     remove(&options)?;
 
     if !dry_run {
-        // Clean up workspace dependencies
-        gc_workspace(&workspace, &options.dependencies)?;
+        // Clean up the workspace
+        gc_workspace(&workspace)?;
 
         // Reload the workspace since we've changed dependencies
         let ws = args.workspace(config)?;
@@ -133,49 +137,208 @@ fn parse_section(args: &ArgMatches) -> DepTable {
     table
 }
 
-/// Clean up workspace dependencies which no longer have a reference to them.
-fn gc_workspace(workspace: &Workspace<'_>, dependencies: &[String]) -> CargoResult<()> {
+/// Clean up the workspace.dependencies, profile, patch, and replace sections of the root manifest
+/// by removing dependencies which no longer have a reference to them.
+fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {
     let mut manifest: toml_edit::Document =
         cargo_util::paths::read(workspace.root_manifest())?.parse()?;
+    let mut is_modified = true;
 
     let members = workspace
         .members()
         .map(|p| LocalManifest::try_new(p.manifest_path()))
         .collect::<CargoResult<Vec<_>>>()?;
 
-    for dep in dependencies {
-        if !dep_in_workspace(dep, &members) {
-            remove_workspace_dep(dep, &mut manifest);
+    let mut dependencies = members
+        .iter()
+        .flat_map(|manifest| {
+            manifest.get_sections().into_iter().flat_map(|(_, table)| {
+                table
+                    .as_table_like()
+                    .unwrap()
+                    .iter()
+                    .map(|(key, item)| Dependency::from_toml(&manifest.path, key, item))
+                    .collect::<Vec<_>>()
+            })
+        })
+        .collect::<CargoResult<Vec<_>>>()?;
+
+    // Clean up the workspace.dependencies section and replace instances of
+    // workspace dependencies with their definitions
+    if let Some(toml_edit::Item::Table(deps_table)) = manifest
+        .get_mut("workspace")
+        .and_then(|t| t.get_mut("dependencies"))
+    {
+        deps_table.set_implicit(true);
+        for (key, item) in deps_table.iter_mut() {
+            let ws_dep = Dependency::from_toml(&workspace.root(), key.get(), item)?;
+
+            // search for uses of this workspace dependency
+            let mut is_used = false;
+            for dep in dependencies.iter_mut().filter(|d| {
+                d.toml_key() == key.get() && matches!(d.source(), Some(Source::Workspace(_)))
+            }) {
+                // HACK: Replace workspace references in `dependencies` to simplify later GC steps:
+                // 1. Avoid having to look it up again to determine the dependency source / spec
+                // 2. The entry might get deleted, preventing us from looking it up again
+                //
+                // This does lose extra information, like features enabled, but that shouldn't be a
+                // problem for GC
+                *dep = ws_dep.clone();
+
+                is_used = true;
+            }
+
+            if !is_used {
+                *item = toml_edit::Item::None;
+                is_modified = true;
+            }
         }
     }
 
-    cargo_util::paths::write(workspace.root_manifest(), manifest.to_string().as_bytes())?;
+    // Clean up the profile section
+    //
+    // Example tables:
+    // - profile.dev.package.foo
+    // - profile.release.package."*"
+    // - profile.release.package."foo:2.1.0"
+    if let Some(toml_edit::Item::Table(profile_section_table)) = manifest.get_mut("profile") {
+        profile_section_table.set_implicit(true);
+
+        for (_, item) in profile_section_table.iter_mut() {
+            if let toml_edit::Item::Table(profile_table) = item {
+                profile_table.set_implicit(true);
+
+                if let Some(toml_edit::Item::Table(package_table)) =
+                    profile_table.get_mut("package")
+                {
+                    package_table.set_implicit(true);
+
+                    for (key, item) in package_table.iter_mut() {
+                        if !spec_has_match(
+                            &PackageIdSpec::parse(key.get())?,
+                            &dependencies,
+                            workspace.config(),
+                        )? {
+                            *item = toml_edit::Item::None;
+                            is_modified = true;
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    // Clean up the patch section
+    if let Some(toml_edit::Item::Table(patch_section_table)) = manifest.get_mut("patch") {
+        patch_section_table.set_implicit(true);
+
+        // The key in each of the subtables is a source (either a registry or a URL)
+        for (source, item) in patch_section_table.iter_mut() {
+            if let toml_edit::Item::Table(patch_table) = item {
+                patch_table.set_implicit(true);
+
+                for (key, item) in patch_table.iter_mut() {
+                    let package_name =
+                        Dependency::from_toml(&workspace.root_manifest(), key.get(), item)?.name;
+                    if !source_has_match(
+                        &package_name,
+                        source.get(),
+                        &dependencies,
+                        workspace.config(),
+                    )? {
+                        *item = toml_edit::Item::None;
+                    }
+                }
+            }
+        }
+    }
+
+    // Clean up the replace section
+    if let Some(toml_edit::Item::Table(table)) = manifest.get_mut("replace") {
+        table.set_implicit(true);
+
+        for (key, item) in table.iter_mut() {
+            if !spec_has_match(
+                &PackageIdSpec::parse(key.get())?,
+                &dependencies,
+                workspace.config(),
+            )? {
+                *item = toml_edit::Item::None;
+                is_modified = true;
+            }
+        }
+    }
+
+    if is_modified {
+        cargo_util::paths::write(workspace.root_manifest(), manifest.to_string().as_bytes())?;
+    }
 
     Ok(())
 }
 
-/// Get whether or not a dependency is depended upon in a workspace.
-fn dep_in_workspace(dep: &str, members: &[LocalManifest]) -> bool {
-    members.iter().any(|manifest| {
-        manifest.get_sections().iter().any(|(_, table)| {
-            table
-                .as_table_like()
-                .unwrap()
-                .get(dep)
-                .and_then(|t| t.get("workspace"))
-                .and_then(|v| v.as_bool())
-                .unwrap_or(false)
-        })
-    })
+/// Check whether or not a package ID spec matches any non-workspace dependencies.
+fn spec_has_match(
+    spec: &PackageIdSpec,
+    dependencies: &[Dependency],
+    config: &Config,
+) -> CargoResult<bool> {
+    for dep in dependencies {
+        if spec.name().as_str() != &dep.name {
+            continue;
+        }
+
+        let version_matches = match (spec.version(), dep.version()) {
+            (Some(v), Some(vq)) => semver::VersionReq::parse(vq)?.matches(v),
+            (Some(_), None) => false,
+            (None, None | Some(_)) => true,
+        };
+        if !version_matches {
+            continue;
+        }
+
+        match dep.source_id(config)? {
+            MaybeWorkspace::Other(source_id) => {
+                if spec.url().map(|u| u == source_id.url()).unwrap_or(true) {
+                    return Ok(true);
+                }
+            }
+            MaybeWorkspace::Workspace(_) => {}
+        }
+    }
+
+    Ok(false)
 }
 
-/// Remove a dependency from a workspace manifest.
-fn remove_workspace_dep(dep: &str, ws_manifest: &mut toml_edit::Document) {
-    if let Some(toml_edit::Item::Table(table)) = ws_manifest
-        .get_mut("workspace")
-        .and_then(|t| t.get_mut("dependencies"))
-    {
-        table.set_implicit(true);
-        table.remove(dep);
+/// Check whether or not a source (URL or registry name) matches any non-workspace dependencies.
+fn source_has_match(
+    name: &str,
+    source: &str,
+    dependencies: &[Dependency],
+    config: &Config,
+) -> CargoResult<bool> {
+    for dep in dependencies {
+        if &dep.name != name {
+            continue;
+        }
+
+        match dep.source_id(config)? {
+            MaybeWorkspace::Other(source_id) => {
+                if source_id.is_registry() {
+                    if source_id.display_registry_name() == source
+                        || source_id.url().as_str() == source
+                    {
+                        return Ok(true);
+                    }
+                } else if source_id.is_git() {
+                    if source_id.url().as_str() == source {
+                        return Ok(true);
+                    }
+                }
+            }
+            MaybeWorkspace::Workspace(_) => {}
+        }
     }
+
+    Ok(false)
 }

tests/testsuite/cargo_remove/gc_patch/mod.rs

@@ -0,0 +1,72 @@
+use cargo_test_support::basic_manifest;
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::curr_dir;
+use cargo_test_support::git;
+use cargo_test_support::project;
+use cargo_test_support::CargoCommand;
+
+use crate::cargo_remove::init_registry;
+
+#[cargo_test]
+fn case() {
+    init_registry();
+
+    let git_project1 = git::new("bar1", |project| {
+        project
+            .file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
+            .file("src/lib.rs", "")
+    })
+    .url();
+
+    let git_project2 = git::new("bar2", |project| {
+        project
+            .file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
+            .file("src/lib.rs", "")
+    })
+    .url();
+
+    let in_project = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                "[workspace]\n\
+                 members = [ \"my-member\" ]\n\
+                 \n\
+                 [package]\n\
+                 name = \"my-project\"\n\
+                 version = \"0.1.0\"\n\
+                 \n\
+                 [dependencies]\n\
+                 bar = {{ git = \"{git_project1}\" }}\n\
+                 \n\
+                 [patch.\"{git_project1}\"]\n\
+                 bar = {{ git = \"{git_project2}\" }}\n\
+                 \n\
+                 [patch.crates-io]\n\
+                 bar = {{ git = \"{git_project2}\" }}\n",
+            ),
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "my-member/Cargo.toml",
+            "[package]\n\
+               name = \"my-member\"\n\
+               version = \"0.1.0\"\n\
+               \n\
+               [dependencies]\n\
+               bar = \"0.1.0\"\n",
+        )
+        .file("my-member/src/lib.rs", "")
+        .build();
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg("remove")
+        .args(["bar"])
+        .current_dir(&in_project.root())
+        .assert()
+        .success()
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), &in_project.root());
+}

tests/testsuite/cargo_remove/gc_patch/out/Cargo.toml

@@ -0,0 +1,9 @@
+[workspace]
+members = [ "my-member" ]
+
+[package]
+name = "my-project"
+version = "0.1.0"
+
+[patch.crates-io]
+bar = { git = "[ROOTURL]/bar2" }

tests/testsuite/cargo_remove/gc_patch/out/src/lib.rs

@@ -0,0 +1,3 @@
+    Removing bar from dependencies
+    Updating git repository `[ROOTURL]/bar2`
+    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_patch/stderr.log

@@ -0,0 +1,36 @@
+[package]
+name = "cargo-remove-test-fixture"
+version = "0.1.0"
+
+[[bin]]
+name = "main"
+path = "src/main.rs"
+
+[build-dependencies]
+semver = "0.1.0"
+
+[dependencies]
+docopt = "0.6"
+rustc-serialize = "0.4"
+semver = "0.1"
+toml = "0.1"
+clippy = "0.4"
+
+[dev-dependencies]
+regex = "0.1.1"
+serde = "1.0.90"
+toml = "0.2.3"
+docopt = "0.6"
+
+[features]
+std = ["serde/std", "semver/std"]
+
+[profile.dev.package.docopt]
+opt-level = 3
+
+[profile.dev.package."toml@0.1.0"]
+opt-level = 3
+
+[profile.release.package.toml]
+opt-level = 1
+overflow-checks = false

tests/testsuite/cargo_remove/gc_patch/stdout.log

@@ -0,0 +1 @@
+