Auto merge of #11331 - weihanglo:revert-11183, r=ehuss

Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102

Author: bors

src/cargo/core/compiler/unit_dependencies.rs

@@ -1040,7 +1040,6 @@ impl<'a, 'cfg> State<'a, 'cfg> {
         }
     }
 
-    /// See [`ResolvedFeatures::activated_features`].
     fn activated_features(
         &self,
         pkg_id: PackageId,
@@ -1050,9 +1049,14 @@ impl<'a, 'cfg> State<'a, 'cfg> {
         features.activated_features(pkg_id, features_for)
     }
 
-    /// See [`ResolvedFeatures::is_activated`].
-    fn is_activated(&self, pkg_id: PackageId, features_for: FeaturesFor) -> bool {
-        self.features().is_activated(pkg_id, features_for)
+    fn is_dep_activated(
+        &self,
+        pkg_id: PackageId,
+        features_for: FeaturesFor,
+        dep_name: InternedString,
+    ) -> bool {
+        self.features()
+            .is_dep_activated(pkg_id, features_for, dep_name)
     }
 
     fn get(&self, id: PackageId) -> &'a Package {
@@ -1067,7 +1071,7 @@ impl<'a, 'cfg> State<'a, 'cfg> {
         let kind = unit.kind;
         self.resolve()
             .deps(pkg_id)
-            .filter_map(|(dep_id, deps)| {
+            .filter_map(|(id, deps)| {
                 assert!(!deps.is_empty());
                 let deps: Vec<_> = deps
                     .iter()
@@ -1099,8 +1103,8 @@ impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let dep_features_for = unit_for.map_to_features_for(dep.artifact());
-                            if !self.is_activated(dep_id, dep_features_for) {
+                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
                         }
@@ -1113,7 +1117,7 @@ impl<'a, 'cfg> State<'a, 'cfg> {
                 if deps.is_empty() {
                     None
                 } else {
-                    Some((dep_id, deps))
+                    Some((id, deps))
                 }
             })
             .collect()

src/cargo/core/resolver/features.rs

@@ -56,12 +56,11 @@ type ActivateMap = HashMap<PackageFeaturesKey, BTreeSet<InternedString>>;
 
 /// Set of all activated features for all packages in the resolve graph.
 pub struct ResolvedFeatures {
-    /// Map of features activated for each package.
-    ///
-    /// The presence of each key also means the package itself is activated,
-    /// even its associated set contains no features.
     activated_features: ActivateMap,
-    /// Options that change how the feature resolver operates.
+    /// Optional dependencies that should be built.
+    ///
+    /// The value is the `name_in_toml` of the dependencies.
+    activated_dependencies: ActivateMap,
     opts: FeatureOpts,
 }
 
@@ -322,14 +321,21 @@ impl ResolvedFeatures {
             .expect("activated_features for invalid package")
     }
 
-    /// Asks the resolved features if the given package should be included.
+    /// Returns if the given dependency should be included.
     ///
-    /// One scenario to use this function is to deteremine a optional dependency
-    /// should be built or not.
-    pub fn is_activated(&self, pkg_id: PackageId, features_for: FeaturesFor) -> bool {
-        log::trace!("is_activated {} {features_for}", pkg_id.name());
-        self.activated_features_unverified(pkg_id, features_for.apply_opts(&self.opts))
-            .is_some()
+    /// This handles dependencies disabled via `cfg` expressions and optional
+    /// dependencies which are not enabled.
+    pub fn is_dep_activated(
+        &self,
+        pkg_id: PackageId,
+        features_for: FeaturesFor,
+        dep_name: InternedString,
+    ) -> bool {
+        let key = features_for.apply_opts(&self.opts);
+        self.activated_dependencies
+            .get(&(pkg_id, key))
+            .map(|deps| deps.contains(&dep_name))
+            .unwrap_or(false)
     }
 
     /// Variant of `activated_features` that returns `None` if this is
@@ -409,14 +415,8 @@ pub struct FeatureResolver<'a, 'cfg> {
     /// Options that change how the feature resolver operates.
     opts: FeatureOpts,
     /// Map of features activated for each package.
-    ///
-    /// The presence of each key also means the package itself is activated,
-    /// even its associated set contains no features.
     activated_features: ActivateMap,
     /// Map of optional dependencies activated for each package.
-    ///
-    /// The key is the package having their dependencies activated.
-    /// The value comes from `dep_name` part of the feature syntax `dep:dep_name`.
     activated_dependencies: ActivateMap,
     /// Keeps track of which packages have had its dependencies processed.
     /// Used to avoid cycles, and to speed up processing.
@@ -475,6 +475,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
         }
         Ok(ResolvedFeatures {
             activated_features: r.activated_features,
+            activated_dependencies: r.activated_dependencies,
             opts: r.opts,
         })
     }
@@ -506,24 +507,20 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
         Ok(())
     }
 
-    /// Activates a list of [`FeatureValue`] for a given package.
+    /// Activates [`FeatureValue`]s on the given package.
     ///
-    /// This is the main entrance into the recursion of feature activation for a package.
-    /// Other `activate_*` functions would be called inside this function accordingly.
+    /// This is the main entrance into the recursion of feature activation
+    /// for a package.
     fn activate_pkg(
         &mut self,
         pkg_id: PackageId,
         fk: FeaturesFor,
         fvs: &[FeatureValue],
     ) -> CargoResult<()> {
         log::trace!("activate_pkg {} {}", pkg_id.name(), fk);
-        // Cargo must insert an empty set here as the presence of an (empty) set
-        // also means that the dependency is activated.
-        // This `is_activated` behavior for dependencies was previously depends on field
-        // `activated_dependencies`, which is less useful after rust-lang/cargo#11183.
-        //
-        // That is, we may keep or remove `activated_dependencies` in the future
-        // if figuring out it can completely be replaced with `activated_features`.
+        // Add an empty entry to ensure everything is covered. This is intended for
+        // finding bugs where the resolver missed something it should have visited.
+        // Remove this in the future if `activated_features` uses an empty default.
         self.activated_features
             .entry((pkg_id, fk.apply_opts(&self.opts)))
             .or_insert_with(BTreeSet::new);

src/cargo/ops/tree/graph.rs

@@ -365,7 +365,11 @@ fn add_pkg(
                 if dep.is_optional() {
                     // If the new feature resolver does not enable this
                     // optional dep, then don't use it.
-                    if !resolved_features.is_activated(dep_id, features_for) {
+                    if !resolved_features.is_dep_activated(
+                        package_id,
+                        features_for,
+                        dep.name_in_toml(),
+                    ) {
                         return false;
                     }
                 }

tests/testsuite/artifact_dep.rs

@@ -20,7 +20,7 @@ fn check_with_invalid_artifact_dependency() {
                 version = "0.0.0"
                 authors = []
                 resolver = "2"
-
+                
                 [dependencies]
                 bar = { path = "bar/", artifact = "unknown" }
             "#,
@@ -2276,59 +2276,3 @@ fn build_script_features_for_shared_dependency() {
         .masquerade_as_nightly_cargo(&["bindeps"])
         .run();
 }
-
-#[cargo_test]
-fn build_with_target_and_optional() {
-    // This is a incorrect behaviour got to be fixed.
-    // See rust-lang/cargo#10526
-    if cross_compile::disabled() {
-        return;
-    }
-    let target = cross_compile::alternate();
-    let p = project()
-        .file(
-            "Cargo.toml",
-            &r#"
-                [package]
-                name = "foo"
-                version = "0.0.1"
-                edition = "2021"
-
-                [dependencies]
-                d1 = { path = "d1", artifact = "bin", optional = true, target = "$TARGET" }
-            "#
-            .replace("$TARGET", target),
-        )
-        .file(
-            "src/main.rs",
-            r#"
-                fn main() {
-                    let _b = include_bytes!(env!("CARGO_BIN_FILE_D1"));
-                }
-            "#,
-        )
-        .file(
-            "d1/Cargo.toml",
-            r#"
-                [package]
-                name = "d1"
-                version = "0.0.1"
-                edition = "2021"
-            "#,
-        )
-        .file("d1/src/main.rs", "fn main() {}")
-        .build();
-
-    p.cargo("build -Z bindeps -F d1 -v")
-        .masquerade_as_nightly_cargo(&["bindeps"])
-        .with_stderr(
-            "\
-[COMPILING] d1 v0.0.1 [..]
-[RUNNING] `rustc --crate-name d1 [..]--crate-type bin[..]
-[COMPILING] foo v0.0.1 [..]
-[RUNNING] `rustc --crate-name foo [..]--cfg[..]d1[..]
-[FINISHED] dev [..]
-",
-        )
-        .run();
-}