Auto merge of #12454 - alcolmenar:alcolmenar/fix-invalid-rm, r=epage

Fix cargo remove incorrectly removing used patches

### What does this PR try to resolve?

Fixes an issue where patches are being removed when member dependencies don't explicitly contain the patched crate.

Fixes #12419

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

- Created a test for the failing use case
- Verify passing test

<!--
### Additional information

Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.

-->

Author: bors

src/bin/cargo/commands/remove.rs

@@ -1,5 +1,6 @@
 use cargo::core::dependency::DepKind;
 use cargo::core::PackageIdSpec;
+use cargo::core::Resolve;
 use cargo::core::Workspace;
 use cargo::ops::cargo_remove::remove;
 use cargo::ops::cargo_remove::RemoveOptions;
@@ -109,9 +110,24 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
 
         // Reload the workspace since we've changed dependencies
         let ws = args.workspace(config)?;
-        resolve_ws(&ws)?;
-    }
+        let resolve = {
+            // HACK: Avoid unused patch warnings by temporarily changing the verbosity.
+            // In rare cases, this might cause index update messages to not show up
+            let verbosity = ws.config().shell().verbosity();
+            ws.config()
+                .shell()
+                .set_verbosity(cargo::core::Verbosity::Quiet);
+            let resolve = resolve_ws(&ws);
+            ws.config().shell().set_verbosity(verbosity);
+            resolve?.1
+        };
 
+        // Attempt to gc unused patches and re-resolve if anything is removed
+        if gc_unused_patches(&workspace, &resolve)? {
+            let ws = args.workspace(config)?;
+            resolve_ws(&ws)?;
+        }
+    }
     Ok(())
 }
 
@@ -229,31 +245,6 @@ fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {
         }
     }
 
-    // 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);
@@ -310,35 +301,46 @@ fn spec_has_match(
     Ok(false)
 }
 
-/// 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;
-        }
+/// Removes unused patches from the manifest
+fn gc_unused_patches(workspace: &Workspace<'_>, resolve: &Resolve) -> CargoResult<bool> {
+    let mut manifest: toml_edit::Document =
+        cargo_util::paths::read(workspace.root_manifest())?.parse()?;
+    let mut modified = false;
 
-        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
+    // 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);
+
+        for (_, 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 dep = Dependency::from_toml(&workspace.root_manifest(), key.get(), item)?;
+
+                    // Generate a PackageIdSpec url for querying
+                    let url = if let MaybeWorkspace::Other(source_id) =
+                        dep.source_id(workspace.config())?
                     {
-                        return Ok(true);
-                    }
-                } else if source_id.is_git() {
-                    if source_id.url().as_str() == source {
-                        return Ok(true);
+                        format!("{}#{}", source_id.url(), dep.name)
+                    } else {
+                        continue;
+                    };
+
+                    if PackageIdSpec::query_str(&url, resolve.unused_patches().iter().cloned())
+                        .is_ok()
+                    {
+                        *item = toml_edit::Item::None;
+                        modified = true;
                     }
                 }
             }
-            MaybeWorkspace::Workspace(_) => {}
         }
     }
 
-    Ok(false)
+    if modified {
+        cargo_util::paths::write(workspace.root_manifest(), manifest.to_string().as_bytes())?;
+    }
+
+    Ok(modified)
 }

tests/testsuite/cargo_remove/avoid_empty_tables/stderr.log

@@ -1,2 +1 @@
     Removing clippy from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/build/stderr.log

@@ -1,2 +1 @@
     Removing semver from build-dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/dev/stderr.log

@@ -1,2 +1 @@
     Removing regex from dev-dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_keep_used_patch/in/Cargo.toml

@@ -0,0 +1,8 @@
+# Cargo.toml
+
+[workspace]
+members = ["serde", "serde_derive"]
+
+[patch.crates-io]
+serde = { path = "serde" }
+

tests/testsuite/cargo_remove/gc_keep_used_patch/in/serde/Cargo.toml

@@ -0,0 +1,9 @@
+# serde/Cargo.toml
+
+[package]
+name = "serde"
+version = "1.0.0"
+
+[dependencies]
+serde_derive = { path = "../serde_derive" }
+

tests/testsuite/cargo_remove/gc_keep_used_patch/in/serde/src/lib.rs

@@ -0,0 +1,8 @@
+# serde_derive/Cargo.toml
+
+[package]
+name = "serde_derive"
+version = "1.0.0"
+
+[dev-dependencies]
+serde_json = "1.0.0"

tests/testsuite/cargo_remove/gc_keep_used_patch/in/serde_derive/Cargo.toml

@@ -0,0 +1,27 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::curr_dir;
+use cargo_test_support::CargoCommand;
+use cargo_test_support::Project;
+
+#[cargo_test]
+fn case() {
+    cargo_test_support::registry::init();
+    cargo_test_support::registry::Package::new("serde", "1.0.0").publish();
+    cargo_test_support::registry::Package::new("serde_json", "1.0.0")
+        .dep("serde", "1.0.0")
+        .publish();
+
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = project.root();
+
+    snapbox::cmd::Command::cargo_ui()
+        .current_dir(&project_root)
+        .arg("remove")
+        .args(["--package", "serde", "serde_derive"])
+        .assert()
+        .code(0)
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), &project_root);
+}

tests/testsuite/cargo_remove/gc_keep_used_patch/in/serde_derive/src/lib.rs

@@ -0,0 +1,8 @@
+# Cargo.toml
+
+[workspace]
+members = ["serde", "serde_derive"]
+
+[patch.crates-io]
+serde = { path = "serde" }
+

tests/testsuite/cargo_remove/gc_keep_used_patch/mod.rs

@@ -0,0 +1,6 @@
+# serde/Cargo.toml
+
+[package]
+name = "serde"
+version = "1.0.0"
+

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

@@ -0,0 +1,8 @@
+# serde_derive/Cargo.toml
+
+[package]
+name = "serde_derive"
+version = "1.0.0"
+
+[dev-dependencies]
+serde_json = "1.0.0"

tests/testsuite/cargo_remove/gc_keep_used_patch/out/serde/Cargo.toml

@@ -0,0 +1 @@
+    Removing serde_derive from dependencies

tests/testsuite/cargo_remove/gc_keep_used_patch/out/serde/src/lib.rs

@@ -23,6 +23,13 @@ fn case() {
     })
     .url();
 
+    let git_project3 = git::new("bar3", |project| {
+        project
+            .file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
+            .file("src/lib.rs", "")
+    })
+    .url();
+
     let in_project = project()
         .file(
             "Cargo.toml",
@@ -38,7 +45,7 @@ fn case() {
                  bar = {{ git = \"{git_project1}\" }}\n\
                  \n\
                  [patch.\"{git_project1}\"]\n\
-                 bar = {{ git = \"{git_project2}\" }}\n\
+                 bar = {{ git = \"{git_project3}\" }}\n\
                  \n\
                  [patch.crates-io]\n\
                  bar = {{ git = \"{git_project2}\" }}\n",

tests/testsuite/cargo_remove/gc_keep_used_patch/out/serde_derive/Cargo.toml

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

tests/testsuite/cargo_remove/gc_keep_used_patch/out/serde_derive/src/lib.rs

@@ -1,2 +1 @@
     Removing toml from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_keep_used_patch/stderr.log

@@ -1,2 +1 @@
     Removing toml from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_keep_used_patch/stdout.log

@@ -2,6 +2,7 @@ mod avoid_empty_tables;
 mod build;
 mod dev;
 mod dry_run;
+mod gc_keep_used_patch;
 mod gc_patch;
 mod gc_profile;
 mod gc_replace;

tests/testsuite/cargo_remove/gc_patch/mod.rs

@@ -1,3 +1,2 @@
     Removing docopt from dependencies
     Removing semver from dependencies
-    Updating `dummy-registry` index

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

@@ -1,3 +1,2 @@
     Removing regex from dev-dependencies
     Removing serde from dev-dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_patch/stderr.log

@@ -1,2 +1 @@
     Removing serde from dev-dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_profile/stderr.log

@@ -1,2 +1 @@
     Removing semver from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/gc_replace/stderr.log

@@ -1,2 +1 @@
     Removing docopt from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/mod.rs

@@ -1,2 +1 @@
     Removing docopt from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/multiple_deps/stderr.log

@@ -1,2 +1 @@
     Removing dbus from dependencies for target `x86_64-unknown-linux-gnu`
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/multiple_dev/stderr.log

@@ -1,2 +1 @@
     Removing semver from build-dependencies for target `x86_64-unknown-linux-gnu`
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/optional_dep_feature/stderr.log

@@ -1,2 +1 @@
     Removing ncurses from dev-dependencies for target `x86_64-unknown-linux-gnu`
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/optional_feature/stderr.log

@@ -1,2 +1 @@
     Removing rustc-serialize from dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/package/stderr.log

@@ -1,2 +1 @@
     Removing semver from build-dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/remove_basic/stderr.log

@@ -1,2 +1 @@
     Removing semver from build-dependencies
-    Updating `dummy-registry` index

tests/testsuite/cargo_remove/target/stderr.log

@@ -1,2 +1 @@
     Removing semver from build-dependencies
-    Updating `dummy-registry` index