Repository: rust-lang/cargo (rust)

Issue:
2024 implicit feature migration deletes dependency features
### Problem

Migrating to 2024 causes `foo/feature_name` features to get removed for optional dependencies. Those need to be preserved.

### Steps

Example manifest:

```toml
cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
serde = { version = "1.0.203", optional = true }

[features]
serde = ["serde/derive"]
```

Running `cargo fix --edition` on this will result in:

```toml
serde = ["dep:serde"]
```

Note that the `/derive` feature got removed.


### Possible Solution(s)

When there is a `["foo/feature_name"]` syntax in the `[features]` table, and `dep:foo` isn't found anywhere in the `[features]` table, it should be rewritten to be `["dep:foo", "foo/feature_name"]`. Some things to be careful of:

* Adjust all places that `foo/xxx` is found (such as `x = ["foo/a"]` and `y = ["foo/b"]`, both x and y need a "`dep:foo`")
* Should not disturb any other features (like `["foo/feature_name", "bar"] would be rewritten to ["dep:foo", "foo/feature_name", "bar"]`)
* Should avoid duplicates (`["foo/a", "foo/b"]` should not have two `dep:foo` entries added).


### Notes

_No response_

### Version

```text
cargo 1.80.0-nightly (7a6fad098 2024-05-31)
release: 1.80.0-nightly
commit-hash: 7a6fad0984d28c8330974636972aa296b67c4513
commit-date: 2024-05-31
host: aarch64-apple-darwin
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0 (sys:0.4.72+curl-8.6.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.5.0 [64-bit]
```


Pull request:
fix(fix): Address problems with implicit -> explicit feature migration
### What does this PR try to resolve?

Within the scope of `cargo fix` there  are two problems
- We wipe out existing feature activations if it has the same name as an optional dependency
- The `Cargo.toml` isn't parseable because the unused optional dependency won't "exist" if just `dep_name/feature_name` is used

Fixes #14010

### How should we test and review this PR?

As for the unused optional dependency not "existing" error,
- #14015 is for improving the message for weak dep features
- #14016 is for re-evaluating how we handle this for strong dep features

Depending on what solution we go with for #14016, we might want to revisit the second migration within this PR.  This is one reason I made the commit separate (in addition to just making it clearer whats happening as this gets into some finer details of features).

### Additional information



Patch:
diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs
index 24dd8b635..df354b36e 100644
--- a/src/cargo/ops/fix.rs
+++ b/src/cargo/ops/fix.rs
@@ -285,7 +285,7 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
                 fixes += rename_dep_fields_2024(workspace, "dependencies");
             }
 
-            fixes += add_feature_for_unused_deps(pkg, root);
+            fixes += add_feature_for_unused_deps(pkg, root, ws.gctx());
             fixes += rename_table(root, "project", "package");
             if let Some(target) = root.get_mut("lib").and_then(|t| t.as_table_like_mut()) {
                 fixes += rename_target_fields_2024(target);
@@ -435,7 +435,11 @@ fn rename_table(parent: &mut dyn toml_edit::TableLike, old: &str, new: &str) ->
     1
 }
 
-fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableLike) -> usize {
+fn add_feature_for_unused_deps(
+    pkg: &Package,
+    parent: &mut dyn toml_edit::TableLike,
+    gctx: &GlobalContext,
+) -> usize {
     let manifest = pkg.manifest();
 
     let activated_opt_deps = manifest
@@ -456,18 +460,48 @@ fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableL
     for dep in manifest.dependencies() {
         let dep_name_in_toml = dep.name_in_toml();
         if dep.is_optional() && !activated_opt_deps.contains(dep_name_in_toml.as_str()) {
-            fixes += 1;
             if let Some(features) = parent
                 .entry("features")
                 .or_insert(toml_edit::table())
                 .as_table_like_mut()
             {
-                features.insert(
-                    dep_name_in_toml.as_str(),
-                    toml_edit::Item::Value(toml_edit::Value::Array(toml_edit::Array::from_iter(
-                        &[format!("dep:{}", dep_name_in_toml)],
-                    ))),
-                );
+                let activate_dep = format!("dep:{dep_name_in_toml}");
+                let strong_dep_feature_prefix = format!("{dep_name_in_toml}/");
+                features
+                    .entry(dep_name_in_toml.as_str())
+                    .or_insert_with(|| {
+                        fixes += 1;
+                        toml_edit::Item::Value(toml_edit::Value::Array(
+                            toml_edit::Array::from_iter([&activate_dep]),
+                        ))
+                    });
+                // Ensure `dep:dep_name` is present for `dep_name/feature_name` since `dep:` is the
+                // only way to guarantee an optional dependency is available for use.
+                //
+                // The way we avoid implicitly creating features in Edition2024 is we remove the
+                // dependency from `resolved_toml` if there is no `dep:` syntax as that is the only
+                // syntax that suppresses the creation of the implicit feature.
+                for (feature_name, activations) in features.iter_mut() {
+                    let Some(activations) = activations.as_array_mut() else {
+                        let _ = gctx.shell().warn(format_args!("skipping fix of feature `{feature_name}` in package `{}`: unsupported feature schema", pkg.name()));
+                        continue;
+                    };
+                    if activations
+                        .iter()
+                        .any(|a| a.as_str().map(|a| a == activate_dep).unwrap_or(false))
+                    {
+                        continue;
+                    }
+                    let Some(activate_dep_pos) = activations.iter().position(|a| {
+                        a.as_str()
+                            .map(|a| a.starts_with(&strong_dep_feature_prefix))
+                            .unwrap_or(false)
+                    }) else {
+                        continue;
+                    };
+                    fixes += 1;
+                    activations.insert(activate_dep_pos, &activate_dep);
+                }
             }
         }
     }


Test patch:
diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs
index 1a3499644..a86acb80e 100644
--- a/tests/testsuite/fix.rs
+++ b/tests/testsuite/fix.rs
@@ -1297,9 +1297,9 @@ fn fix_to_broken_code() {
         .with_stderr_contains("[WARNING] failed to automatically apply fixes [..]")
         .run();
 
-    assert_eq!(
+    assert_e2e().eq(
         p.read_file("bar/src/lib.rs"),
-        "pub fn foo() { let x = 3; let _ = x; }"
+        str!["pub fn foo() { let x = 3; let _ = x; }"],
     );
 }
 
@@ -1320,7 +1320,10 @@ fn fix_with_common() {
 
     p.cargo("fix --edition --allow-no-vcs").run();
 
-    assert_eq!(p.read_file("tests/common/mod.rs"), "pub fn r#try() {}");
+    assert_e2e().eq(
+        p.read_file("tests/common/mod.rs"),
+        str!["pub fn r#try() {}"],
+    );
 }
 
 #[cargo_test]
@@ -2312,9 +2315,10 @@ edition = "2021"
 ",
         )
         .run();
-    assert_eq!(
+    assert_e2e().eq(
         p.read_file("Cargo.toml"),
-        r#"
+        str![[r#"
+
 cargo-features = ["edition2024"]
 
 # Before project
@@ -2323,7 +2327,8 @@ cargo-features = ["edition2024"]
 name = "foo"
 edition = "2021"
 # After project table
-"#
+
+"#]],
     );
 }
 
@@ -2365,9 +2370,10 @@ edition = "2021"
 ",
         )
         .run();
-    assert_eq!(
+    assert_e2e().eq(
         p.read_file("Cargo.toml"),
-        r#"
+        str![[r#"
+
 cargo-features = ["edition2024"]
 
 # Before package
@@ -2376,7 +2382,8 @@ cargo-features = ["edition2024"]
 name = "foo"
 edition = "2021"
 # After project table
-"#
+
+"#]],
     );
 }
 
@@ -2469,9 +2476,10 @@ a = {path = "a", default_features = false}
 ",
         )
         .run();
-    assert_eq!(
+    assert_e2e().eq(
         p.read_file("Cargo.toml"),
-        r#"
+        str![[r#"
+
 cargo-features = ["edition2024"]
 
 [workspace.dependencies]
@@ -2519,14 +2527,15 @@ a = {path = "a", default-features = false}
 # After build_dependencies line
 a = {path = "a", default-features = false}
 # After build_dependencies table
-"#,
+
+"#]],
     );
 }
 
 #[cargo_test]
 fn add_feature_for_unused_dep() {
-    Package::new("bar", "0.1.0").publish();
-    Package::new("baz", "0.1.0").publish();
+    Package::new("regular-dep", "0.1.0").publish();
+    Package::new("build-dep", "0.1.0").publish();
     Package::new("target-dep", "0.1.0").publish();
     let p = project()
         .file(
@@ -2538,10 +2547,10 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
-bar = { version = "0.1.0", optional = true }
+regular-dep = { version = "0.1.0", optional = true }
 
 [build-dependencies]
-baz = { version = "0.1.0", optional = true }
+build-dep = { version = "0.1.0", optional = true }
 
 [target.'cfg(target_os = "linux")'.dependencies]
 target-dep = { version = "0.1.0", optional = true }
@@ -2564,36 +2573,36 @@ target-dep = { version = "0.1.0", optional = true }
 ",
         )
         .run();
-    assert_eq!(
+    assert_e2e().eq(
         p.read_file("Cargo.toml"),
-        r#"
+        str![[r#"
+
 [package]
 name = "foo"
 version = "0.1.0"
 edition = "2021"
 
 [dependencies]
-bar = { version = "0.1.0", optional = true }
+regular-dep = { version = "0.1.0", optional = true }
 
 [build-dependencies]
-baz = { version = "0.1.0", optional = true }
+build-dep = { version = "0.1.0", optional = true }
 
 [target.'cfg(target_os = "linux")'.dependencies]
 target-dep = { version = "0.1.0", optional = true }
 
 [features]
-bar = ["dep:bar"]
-baz = ["dep:baz"]
+regular-dep = ["dep:regular-dep"]
+build-dep = ["dep:build-dep"]
 target-dep = ["dep:target-dep"]
-"#
+
+"#]],
     );
 }
 
 #[cargo_test]
 fn add_feature_for_unused_dep_existing_table() {
-    Package::new("bar", "0.1.0").publish();
-    Package::new("baz", "0.1.0").publish();
-    Package::new("target-dep", "0.1.0").publish();
+    Package::new("dep", "0.1.0").publish();
     let p = project()
         .file(
             "Cargo.toml",
@@ -2604,16 +2613,10 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
-bar = { version = "0.1.0", optional = true }
-
-[build-dependencies]
-baz = { version = "0.1.0", optional = true }
-
-[target.'cfg(target_os = "linux")'.dependencies]
-target-dep = { version = "0.1.0", optional = true }
+dep = { version = "0.1.0", optional = true }
 
 [features]
-target-dep = ["dep:target-dep"]
+existing = []
 "#,
         )
         .file("src/lib.rs", "")
@@ -2624,37 +2627,117 @@ target-dep = ["dep:target-dep"]
         .with_stderr(
             "\
 [MIGRATING] Cargo.toml from 2021 edition to 2024
-[FIXED] Cargo.toml (2 fixes)
+[FIXED] Cargo.toml (1 fix)
 [UPDATING] `dummy-registry` index
-[LOCKING] 4 packages to latest compatible versions
+[LOCKING] 2 packages to latest compatible versions
 [CHECKING] foo v0.1.0 ([CWD])
 [MIGRATING] src/lib.rs from 2021 edition to 2024
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s
 ",
         )
         .run();
-    assert_eq!(
+    assert_e2e().eq(
         p.read_file("Cargo.toml"),
-        r#"
+        str![[r#"
+
 [package]
 name = "foo"
 version = "0.1.0"
 edition = "2021"
 
 [dependencies]
-bar = { version = "0.1.0", optional = true }
+dep = { version = "0.1.0", optional = true }
 
-[build-dependencies]
-baz = { version = "0.1.0", optional = true }
+[features]
+existing = []
+dep = ["dep:dep"]
 
-[target.'cfg(target_os = "linux")'.dependencies]
-target-dep = { version = "0.1.0", optional = true }
+"#]],
+    );
+}
+
+#[cargo_test]
+fn activate_dep_for_dep_feature() {
+    Package::new("dep-feature", "0.1.0")
+        .feature("a", &[])
+        .feature("b", &[])
+        .publish();
+    Package::new("dep-and-dep-feature", "0.1.0")
+        .feature("a", &[])
+        .feature("b", &[])
+        .publish();
+    Package::new("renamed-feature", "0.1.0")
+        .feature("a", &[])
+        .feature("b", &[])
+        .publish();
+    Package::new("unrelated-feature", "0.1.0")
+        .feature("a", &[])
+        .feature("b", &[])
+        .publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+[package]
+name = "foo"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+dep-feature = { version = "0.1.0", optional = true }
+dep-and-dep-feature = { version = "0.1.0", optional = true }
+renamed-feature = { version = "0.1.0", optional = true }
+unrelated-feature = { version = "0.1.0", optional = true }
 
 [features]
-target-dep = ["dep:target-dep"]
-bar = ["dep:bar"]
-baz = ["dep:baz"]
-"#
+dep-feature = ["dep-feature/a", "dep-feature/b"]
+dep-and-dep-feature = ["dep:dep-and-dep-feature", "dep-and-dep-feature/a", "dep-and-dep-feature/b"]
+renamed = ["renamed-feature/a", "renamed-feature/b"]
+unrelated-feature = []
+unrelated-dep-feature = ["unrelated-feature/a", "unrelated-feature/b"]
+"#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("fix --edition --allow-no-vcs")
+        .masquerade_as_nightly_cargo(&["edition2024"])
+        .with_stderr(
+            "\
+[MIGRATING] Cargo.toml from 2021 edition to 2024
+[FIXED] Cargo.toml (4 fixes)
+[UPDATING] `dummy-registry` index
+[LOCKING] 5 packages to latest compatible versions
+[CHECKING] foo v0.1.0 ([CWD])
+[MIGRATING] src/lib.rs from 2021 edition to 2024
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s
+",
+        )
+        .run();
+    assert_e2e().eq(
+        p.read_file("Cargo.toml"),
+        str![[r#"
+
+[package]
+name = "foo"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+dep-feature = { version = "0.1.0", optional = true }
+dep-and-dep-feature = { version = "0.1.0", optional = true }
+renamed-feature = { version = "0.1.0", optional = true }
+unrelated-feature = { version = "0.1.0", optional = true }
+
+[features]
+dep-feature = [ "dep:dep-feature","dep-feature/a", "dep-feature/b"]
+dep-and-dep-feature = ["dep:dep-and-dep-feature", "dep-and-dep-feature/a", "dep-and-dep-feature/b"]
+renamed = [ "dep:renamed-feature","renamed-feature/a", "renamed-feature/b"]
+unrelated-feature = []
+unrelated-dep-feature = [ "dep:unrelated-feature","unrelated-feature/a", "unrelated-feature/b"]
+renamed-feature = ["dep:renamed-feature"]
+
+"#]],
     );
 }
 
@@ -2777,11 +2860,12 @@ dep_df_false = { version = "0.1.0", default-features = false }
         )
         .run();
 
-    assert_eq!(p.read_file("pkg_default/Cargo.toml"), pkg_default);
-    assert_eq!(p.read_file("pkg_df_true/Cargo.toml"), pkg_df_true);
-    assert_eq!(
+    assert_e2e().eq(p.read_file("pkg_default/Cargo.toml"), pkg_default);
+    assert_e2e().eq(p.read_file("pkg_df_true/Cargo.toml"), pkg_df_true);
+    assert_e2e().eq(
         p.read_file("pkg_df_false/Cargo.toml"),
-        r#"
+        str![[r#"
+
 [package]
 name = "pkg_df_false"
 version = "0.1.0"
@@ -2801,6 +2885,7 @@ dep_df_false = { workspace = true, default-features = false }
 dep_simple = { workspace = true}
 dep_df_true = { workspace = true}
 dep_df_false = { workspace = true, default-features = false }
-"#
+
+"#]],
     );
 }