Auto merge of #13518 - baby230211:fix/strip-feature-dev-dep, r=epage

fix: strip feature dep when dep is dev dep

### What does this PR try to resolve?
This change aims to strip features dependencies without a version key to be published.
If a dev-dependency is missing the version, it will be stripped from the packaged manifest.

The features table may contains the deps in following places.
- Target
  - normal
  - dev
  - build
  - normal-and-dev
- normal
- dev
- build
- normal-and-dev

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

See the initial commit, it shows current behavior that will cause error when feature has deps that point to dev_dep and doesn't have a version specified.

Title | orignal toml | published toml |
---- | ---- | ---- |
Before |   feature = ["dev-dep/feature"]  <br/> [dev-dependencies] <br/> dev-dep = { .., features: ["feature"] }   |   feature = ["dev-dep/feature"] <br/> [dev-dependencies] <br/> dev-dep = { .., features: ["feature"] } |
After |  feature = ["dev-dep/feature"]  <br/> [dev-dependencies] <br/> dev-dep = { .., features: ["feature"] }  | feature = [] <br/> [dev-dependencies] ```

Fix: #12225

src/cargo/ops/registry/publish.rs

@@ -3,6 +3,7 @@
 //! [1]: https://doc.rust-lang.org/nightly/cargo/reference/registry-web-api.html#publish
 
 use std::collections::BTreeMap;
+use std::collections::BTreeSet;
 use std::collections::HashSet;
 use std::fs::File;
 use std::time::Duration;
@@ -20,6 +21,7 @@ use crate::core::dependency::DepKind;
 use crate::core::manifest::ManifestMetadata;
 use crate::core::resolver::CliFeatures;
 use crate::core::Dependency;
+use crate::core::FeatureValue;
 use crate::core::Package;
 use crate::core::PackageIdSpecQuery;
 use crate::core::SourceId;
@@ -33,6 +35,7 @@ use crate::sources::CRATES_IO_REGISTRY;
 use crate::util::auth;
 use crate::util::cache_lock::CacheLockMode;
 use crate::util::context::JobsConfig;
+use crate::util::interning::InternedString;
 use crate::util::Progress;
 use crate::util::ProgressStyle;
 use crate::CargoResult;
@@ -412,13 +415,31 @@ fn transmit(
         return Ok(());
     }
 
+    let deps_set = deps
+        .iter()
+        .map(|dep| dep.name.clone())
+        .collect::<BTreeSet<String>>();
+
     let string_features = match manifest.original().features() {
         Some(features) => features
             .iter()
             .map(|(feat, values)| {
                 (
                     feat.to_string(),
-                    values.iter().map(|fv| fv.to_string()).collect(),
+                    values
+                        .iter()
+                        .filter(|fv| {
+                            let feature_value = FeatureValue::new(InternedString::new(fv));
+                            match feature_value {
+                                FeatureValue::Dep { dep_name }
+                                | FeatureValue::DepFeature { dep_name, .. } => {
+                                    deps_set.contains(&dep_name.to_string())
+                                }
+                                _ => true,
+                            }
+                        })
+                        .map(|fv| fv.to_string())
+                        .collect(),
                 )
             })
             .collect::<BTreeMap<String, Vec<String>>>(),

src/cargo/util/toml/mod.rs

@@ -9,8 +9,8 @@ use crate::AlreadyPrintedError;
 use anyhow::{anyhow, bail, Context as _};
 use cargo_platform::Platform;
 use cargo_util::paths;
-use cargo_util_schemas::manifest;
 use cargo_util_schemas::manifest::RustVersion;
+use cargo_util_schemas::manifest::{self, TomlManifest};
 use itertools::Itertools;
 use lazycell::LazyCell;
 use pathdiff::diff_paths;
@@ -21,7 +21,7 @@ use crate::core::compiler::{CompileKind, CompileTarget};
 use crate::core::dependency::{Artifact, ArtifactTarget, DepKind};
 use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings};
 use crate::core::resolver::ResolveBehavior;
-use crate::core::{find_workspace_root, resolve_relative_path, CliUnstable};
+use crate::core::{find_workspace_root, resolve_relative_path, CliUnstable, FeatureValue};
 use crate::core::{Dependency, Manifest, PackageId, Summary, Target};
 use crate::core::{Edition, EitherManifest, Feature, Features, VirtualManifest, Workspace};
 use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, WorkspaceRootConfig};
@@ -316,7 +316,7 @@ pub fn prepare_for_publish(
         }
     }
     let all = |_d: &manifest::TomlDependency| true;
-    return Ok(manifest::TomlManifest {
+    let mut manifest = manifest::TomlManifest {
         package: Some(package),
         project: None,
         profile: me.profile.clone(),
@@ -366,7 +366,52 @@ pub fn prepare_for_publish(
         badges: me.badges.clone(),
         cargo_features: me.cargo_features.clone(),
         lints: me.lints.clone(),
-    });
+    };
+    strip_features(&mut manifest);
+    return Ok(manifest);
+
+    fn strip_features(manifest: &mut TomlManifest) {
+        fn insert_dep_name(
+            dep_name_set: &mut BTreeSet<manifest::PackageName>,
+            deps: Option<&BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
+        ) {
+            let Some(deps) = deps else {
+                return;
+            };
+            deps.iter().for_each(|(k, _v)| {
+                dep_name_set.insert(k.clone());
+            });
+        }
+        let mut dep_name_set = BTreeSet::new();
+        insert_dep_name(&mut dep_name_set, manifest.dependencies.as_ref());
+        insert_dep_name(&mut dep_name_set, manifest.dev_dependencies());
+        insert_dep_name(&mut dep_name_set, manifest.build_dependencies());
+        if let Some(target_map) = manifest.target.as_ref() {
+            target_map.iter().for_each(|(_k, v)| {
+                insert_dep_name(&mut dep_name_set, v.dependencies.as_ref());
+                insert_dep_name(&mut dep_name_set, v.dev_dependencies());
+                insert_dep_name(&mut dep_name_set, v.build_dependencies());
+            });
+        }
+        let features = manifest.features.as_mut();
+
+        let Some(features) = features else {
+            return;
+        };
+
+        features.values_mut().for_each(|feature_deps| {
+            feature_deps.retain(|feature_dep| {
+                let feature_value = FeatureValue::new(InternedString::new(feature_dep));
+                match feature_value {
+                    FeatureValue::Dep { dep_name } | FeatureValue::DepFeature { dep_name, .. } => {
+                        let k = &manifest::PackageName::new(dep_name.to_string()).unwrap();
+                        dep_name_set.contains(k)
+                    }
+                    _ => true,
+                }
+            });
+        });
+    }
 
     fn map_deps(
         gctx: &GlobalContext,

tests/testsuite/publish.rs

@@ -1686,6 +1686,304 @@ repository = "foo"
     );
 }
 
+#[cargo_test]
+fn publish_with_feature_point_diff_kinds_dep() {
+    let registry = RegistryBuilder::new().http_api().http_index().build();
+    Package::new("normal-only", "1.0.0")
+        .feature("cat", &[])
+        .publish();
+    Package::new("build-only", "1.0.0")
+        .feature("cat", &[])
+        .publish();
+    Package::new("normal-and-dev", "1.0.0")
+        .feature("cat", &[])
+        .publish();
+    Package::new("target-normal-only", "1.0.0")
+        .feature("cat", &[])
+        .publish();
+    Package::new("target-build-only", "1.0.0")
+        .feature("cat", &[])
+        .publish();
+    Package::new("target-normal-and-dev", "1.0.0")
+        .feature("cat", &[])
+        .publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            edition = "2015"
+            authors = []
+            license = "MIT"
+            description = "foo"
+            documentation = "foo"
+            homepage = "foo"
+            repository = "foo"
+
+
+            [features]
+            foo_feature = [
+                "normal-only/cat",
+                "build-only/cat",
+                "dev-only/cat",
+                "normal-and-dev/cat",
+                "target-normal-only/cat",
+                "target-build-only/cat",
+                "target-dev-only/cat",
+                "target-normal-and-dev/cat",
+            ]
+
+            [dependencies]
+            normal-only = { version = "1.0", features = ["cat"] }
+            normal-and-dev = { version = "1.0", features = ["cat"] }
+
+            [build-dependencies]
+            build-only = { version = "1.0", features = ["cat"] }
+
+            [dev-dependencies]
+            dev-only = { path = "../dev-only", features = ["cat"] }
+            normal-and-dev = { version = "1.0", features = ["cat"] }
+
+            [target.'cfg(unix)'.dependencies]
+            target-normal-only = { version = "1.0", features = ["cat"] }
+            target-normal-and-dev = { version = "1.0", features = ["cat"] }
+
+            [target.'cfg(unix)'.build-dependencies]
+            target-build-only = { version = "1.0", features = ["cat"] }
+
+            [target.'cfg(unix)'.dev-dependencies]
+            target-dev-only = { path = "../dev-only", features = ["cat"] }
+            target-normal-and-dev = { version = "1.0", features = ["cat"] }
+            "#,
+        )
+        .file("src/main.rs", "")
+        .file(
+            "dev-only/Cargo.toml",
+            r#"
+            [package]
+            name = "dev-only"
+            version = "0.1.0"
+            edition = "2015"
+            authors = []
+
+            [features]
+            cat = []
+            "#,
+        )
+        .file(
+            "dev-only/src/lib.rs",
+            r#"
+                #[cfg(feature = "cat")]
+                pub fn cat() {}
+            "#,
+        )
+        .build();
+
+    p.cargo("publish --no-verify")
+        .env("RUSTFLAGS", "--cfg unix")
+        .replace_crates_io(registry.index_url())
+        .with_stderr(
+            "\
+[UPDATING] [..]
+[PACKAGING] foo v0.1.0 [..]
+[UPDATING] [..]
+[PACKAGED] [..] files, [..] ([..] compressed)
+[UPLOADING] foo v0.1.0 [..]
+[UPLOADED] foo v0.1.0 [..]
+[NOTE] waiting [..]
+You may press ctrl-c [..]
+[PUBLISHED] foo v0.1.0 [..]
+",
+        )
+        .run();
+
+    publish::validate_upload_with_contents(
+        r#"
+        {
+          "authors": [],
+          "badges": {},
+          "categories": [],
+          "deps": [
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "normal",
+              "name": "normal-and-dev",
+              "optional": false,
+              "target": null,
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "normal",
+              "name": "normal-only",
+              "optional": false,
+              "target": null,
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "dev",
+              "name": "normal-and-dev",
+              "optional": false,
+              "target": null,
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "build",
+              "name": "build-only",
+              "optional": false,
+              "target": null,
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "normal",
+              "name": "target-normal-and-dev",
+              "optional": false,
+              "target": "cfg(unix)",
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "normal",
+              "name": "target-normal-only",
+              "optional": false,
+              "target": "cfg(unix)",
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "build",
+              "name": "target-build-only",
+              "optional": false,
+              "target": "cfg(unix)",
+              "version_req": "^1.0"
+            },
+            {
+              "default_features": true,
+              "features": [
+                "cat"
+              ],
+              "kind": "dev",
+              "name": "target-normal-and-dev",
+              "optional": false,
+              "target": "cfg(unix)",
+              "version_req": "^1.0"
+            }
+          ],
+          "description": "foo",
+          "documentation": "foo",
+          "features": {
+            "foo_feature": [
+              "normal-only/cat",
+              "build-only/cat",
+              "normal-and-dev/cat",
+              "target-normal-only/cat",
+              "target-build-only/cat",
+              "target-normal-and-dev/cat"
+            ]
+          },
+          "homepage": "foo",
+          "keywords": [],
+          "license": "MIT",
+          "license_file": null,
+          "links": null,
+          "name": "foo",
+          "readme": null,
+          "readme_file": null,
+          "repository": "foo",
+          "rust_version": null,
+          "vers": "0.1.0"
+        }
+        "#,
+        "foo-0.1.0.crate",
+        &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
+        &[(
+            "Cargo.toml",
+            &format!(
+                r#"{}
+[package]
+edition = "2015"
+name = "foo"
+version = "0.1.0"
+authors = []
+description = "foo"
+homepage = "foo"
+documentation = "foo"
+license = "MIT"
+repository = "foo"
+
+[dependencies.normal-and-dev]
+version = "1.0"
+features = ["cat"]
+
+[dependencies.normal-only]
+version = "1.0"
+features = ["cat"]
+
+[dev-dependencies.normal-and-dev]
+version = "1.0"
+features = ["cat"]
+
+[build-dependencies.build-only]
+version = "1.0"
+features = ["cat"]
+
+[features]
+foo_feature = [
+    "normal-only/cat",
+    "build-only/cat",
+    "normal-and-dev/cat",
+    "target-normal-only/cat",
+    "target-build-only/cat",
+    "target-normal-and-dev/cat",
+]
+
+[target."cfg(unix)".dependencies.target-normal-and-dev]
+version = "1.0"
+features = ["cat"]
+
+[target."cfg(unix)".dependencies.target-normal-only]
+version = "1.0"
+features = ["cat"]
+
+[target."cfg(unix)".build-dependencies.target-build-only]
+version = "1.0"
+features = ["cat"]
+
+[target."cfg(unix)".dev-dependencies.target-normal-and-dev]
+version = "1.0"
+features = ["cat"]
+"#,
+                cargo::core::package::MANIFEST_PREAMBLE
+            ),
+        )],
+    );
+}
 #[cargo_test]
 fn credentials_ambiguous_filename() {
     // `publish` generally requires a remote registry