From c009b3208de8ecf7edb6f7874ccd8656ce38bb2f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 5 Jun 2024 11:59:50 -0500 Subject: [PATCH 1/6] tests(fix): Snapshot fixes --- tests/testsuite/fix.rs | 61 ++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 1a34996449b..90c6711a805 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,7 +2527,8 @@ a = {path = "a", default-features = false} # After build_dependencies line a = {path = "a", default-features = false} # After build_dependencies table -"#, + +"#]], ); } @@ -2564,9 +2573,10 @@ 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" @@ -2585,7 +2595,8 @@ target-dep = { version = "0.1.0", optional = true } bar = ["dep:bar"] baz = ["dep:baz"] target-dep = ["dep:target-dep"] -"# + +"#]], ); } @@ -2633,9 +2644,10 @@ target-dep = ["dep:target-dep"] ", ) .run(); - assert_eq!( + assert_e2e().eq( p.read_file("Cargo.toml"), - r#" + str![[r#" + [package] name = "foo" version = "0.1.0" @@ -2654,7 +2666,8 @@ target-dep = { version = "0.1.0", optional = true } target-dep = ["dep:target-dep"] bar = ["dep:bar"] baz = ["dep:baz"] -"# + +"#]], ); } @@ -2777,11 +2790,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 +2815,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 } -"# + +"#]], ); } From 1b51d170fefc72f9479c44ff6355faf234a63c72 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 5 Jun 2024 12:03:24 -0500 Subject: [PATCH 2/6] tests(fix): Clarify role of dependencies --- tests/testsuite/fix.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 90c6711a805..889b5c5e7f5 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2534,8 +2534,8 @@ a = {path = "a", default-features = false} #[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( @@ -2547,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 } @@ -2583,17 +2583,17 @@ 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"] "#]], From fa14a1361835a0ae8bfa6efa65cf541ac87b3582 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 5 Jun 2024 15:08:22 -0500 Subject: [PATCH 3/6] test(fix): Simplify existing-table test We already have a test that covers each dep type; we don't need to cover that here. --- tests/testsuite/fix.rs | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 889b5c5e7f5..fd924a20328 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2602,9 +2602,7 @@ 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", @@ -2615,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", "") @@ -2635,9 +2627,9 @@ 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 @@ -2654,18 +2646,11 @@ 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"] -bar = ["dep:bar"] -baz = ["dep:baz"] +existing = [] +dep = ["dep:dep"] "#]], ); From fbe1cd1ab7dfd90e9c515e63b1ccea1ee76060da Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 5 Jun 2024 12:37:38 -0500 Subject: [PATCH 4/6] test(fix): Show the dep-feature behavior --- tests/testsuite/fix.rs | 85 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index fd924a20328..606ecc39f07 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2656,6 +2656,91 @@ dep = ["dep:dep"] ); } +#[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] +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 (3 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-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 = ["dep:unrelated-feature"] +unrelated-dep-feature = ["unrelated-feature/a", "unrelated-feature/b"] +renamed-feature = ["dep:renamed-feature"] + +"#]], + ); +} + #[cargo_test] fn remove_ignored_default_features() { Package::new("dep_simple", "0.1.0").publish(); From 078383bd0fe628d5b48e0abc3aa09456afa3e118 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 5 Jun 2024 15:27:00 -0500 Subject: [PATCH 5/6] fix(fix): Dont remove features when making implicit features explicit --- src/cargo/ops/fix.rs | 15 ++++++++------- tests/testsuite/fix.rs | 6 +++--- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 24dd8b63500..0e45e164418 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -456,18 +456,19 @@ 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)], - ))), - ); + 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(&[format!("dep:{}", dep_name_in_toml)]), + )) + }); } } } diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 606ecc39f07..0c3d7793ade 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2705,7 +2705,7 @@ unrelated-dep-feature = ["unrelated-feature/a", "unrelated-feature/b"] .with_stderr( "\ [MIGRATING] Cargo.toml from 2021 edition to 2024 -[FIXED] Cargo.toml (3 fixes) +[FIXED] Cargo.toml (1 fix) [UPDATING] `dummy-registry` index [LOCKING] 5 packages to latest compatible versions [CHECKING] foo v0.1.0 ([CWD]) @@ -2730,10 +2730,10 @@ renamed-feature = { version = "0.1.0", optional = true } unrelated-feature = { version = "0.1.0", optional = true } [features] -dep-feature = ["dep:dep-feature"] +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 = ["dep:unrelated-feature"] +unrelated-feature = [] unrelated-dep-feature = ["unrelated-feature/a", "unrelated-feature/b"] renamed-feature = ["dep:renamed-feature"] From 21c09286383a3aca7701771f2c0801e29f470d20 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 5 Jun 2024 16:00:39 -0500 Subject: [PATCH 6/6] fix(fix): Ensure optional dep is available for dep-features --- src/cargo/ops/fix.rs | 39 ++++++++++++++++++++++++++++++++++++--- tests/testsuite/fix.rs | 8 ++++---- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 0e45e164418..df354b36e4b 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 @@ -461,14 +465,43 @@ fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableL .or_insert(toml_edit::table()) .as_table_like_mut() { + 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(&[format!("dep:{}", dep_name_in_toml)]), + 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); + } } } } diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 0c3d7793ade..a86acb80ea8 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2705,7 +2705,7 @@ unrelated-dep-feature = ["unrelated-feature/a", "unrelated-feature/b"] .with_stderr( "\ [MIGRATING] Cargo.toml from 2021 edition to 2024 -[FIXED] Cargo.toml (1 fix) +[FIXED] Cargo.toml (4 fixes) [UPDATING] `dummy-registry` index [LOCKING] 5 packages to latest compatible versions [CHECKING] foo v0.1.0 ([CWD]) @@ -2730,11 +2730,11 @@ renamed-feature = { version = "0.1.0", optional = true } unrelated-feature = { version = "0.1.0", optional = true } [features] -dep-feature = ["dep-feature/a", "dep-feature/b"] +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 = ["renamed-feature/a", "renamed-feature/b"] +renamed = [ "dep:renamed-feature","renamed-feature/a", "renamed-feature/b"] unrelated-feature = [] -unrelated-dep-feature = ["unrelated-feature/a", "unrelated-feature/b"] +unrelated-dep-feature = [ "dep:unrelated-feature","unrelated-feature/a", "unrelated-feature/b"] renamed-feature = ["dep:renamed-feature"] "#]],