Auto merge of #8777 - ehuss:fix-doc-itarget, r=alexcrichton

Fix panic in `cargo doc` with -Zfeatures=itarget

There are some situations where `cargo doc -Zfeatures=itarget` can panic where an optional shared dependency is part of an inactive target. The issue is that the filtering logic in `compute_deps` should have been shared with `compute_deps_doc`.  I moved the common filtering into `State` to try to share the code.

Fixes #8774

Author: bors

src/cargo/core/compiler/unit_dependencies.rs

@@ -22,7 +22,7 @@ use crate::core::dependency::DepKind;
 use crate::core::profiles::{Profile, Profiles, UnitFor};
 use crate::core::resolver::features::{FeaturesFor, ResolvedFeatures};
 use crate::core::resolver::Resolve;
-use crate::core::{Package, PackageId, PackageSet, Target, Workspace};
+use crate::core::{Dependency, Package, PackageId, PackageSet, Target, Workspace};
 use crate::ops::resolve_all_features;
 use crate::util::interning::InternedString;
 use crate::util::Config;
@@ -223,50 +223,33 @@ fn compute_deps(
     }
 
     let id = unit.pkg.package_id();
-    let filtered_deps = state.resolve().deps(id).filter(|&(_id, deps)| {
-        assert!(!deps.is_empty());
-        deps.iter().any(|dep| {
-            // If this target is a build command, then we only want build
-            // dependencies, otherwise we want everything *other than* build
-            // dependencies.
-            if unit.target.is_custom_build() != dep.is_build() {
-                return false;
-            }
-
-            // If this dependency is **not** a transitive dependency, then it
-            // only applies to test/example targets.
-            if !dep.is_transitive()
-                && !unit.target.is_test()
-                && !unit.target.is_example()
-                && !unit.mode.is_any_test()
-            {
-                return false;
-            }
-
-            // If this dependency is only available for certain platforms,
-            // make sure we're only enabling it for that platform.
-            if !state.target_data.dep_platform_activated(dep, unit.kind) {
-                return false;
-            }
-
-            // 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();
+    let filtered_deps = state
+        .deps(unit, unit_for)
+        .into_iter()
+        .filter(|&(_id, deps)| {
+            deps.iter().any(|dep| {
+                // If this target is a build command, then we only want build
+                // dependencies, otherwise we want everything *other than* build
+                // dependencies.
+                if unit.target.is_custom_build() != dep.is_build() {
+                    return false;
+                }
 
-                let feats = state.activated_features(id, features_for);
-                if !feats.contains(&dep.name_in_toml()) {
+                // If this dependency is **not** a transitive dependency, then it
+                // only applies to test/example targets.
+                if !dep.is_transitive()
+                    && !unit.target.is_test()
+                    && !unit.target.is_example()
+                    && !unit.mode.is_any_test()
+                {
                     return false;
                 }
-            }
 
-            // If we've gotten past all that, then this dependency is
-            // actually used!
-            true
-        })
-    });
-    // Separate line to avoid rustfmt indentation. Must collect due to `state` capture.
-    let filtered_deps: Vec<_> = filtered_deps.collect();
+                // If we've gotten past all that, then this dependency is
+                // actually used!
+                true
+            })
+        });
 
     let mut ret = Vec::new();
     for (id, _) in filtered_deps {
@@ -410,16 +393,10 @@ fn compute_deps_custom_build(
 
 /// Returns the dependencies necessary to document a package.
 fn compute_deps_doc(unit: &Unit, state: &mut State<'_, '_>) -> CargoResult<Vec<UnitDep>> {
-    let target_data = state.target_data;
     let deps = state
-        .resolve()
-        .deps(unit.pkg.package_id())
-        .filter(|&(_id, deps)| {
-            deps.iter().any(|dep| match dep.kind() {
-                DepKind::Normal => target_data.dep_platform_activated(dep, unit.kind),
-                _ => false,
-            })
-        });
+        .deps(unit, UnitFor::new_normal())
+        .into_iter()
+        .filter(|&(_id, deps)| deps.iter().any(|dep| dep.kind() == DepKind::Normal));
 
     // To document a library, we depend on dependencies actually being
     // built. If we're documenting *all* libraries, then we also depend on
@@ -741,4 +718,36 @@ impl<'a, 'cfg> State<'a, 'cfg> {
             .get_one(id)
             .unwrap_or_else(|_| panic!("expected {} to be downloaded", id))
     }
+
+    /// Returns a filtered set of dependencies for the given unit.
+    fn deps(&self, unit: &Unit, unit_for: UnitFor) -> Vec<(PackageId, &HashSet<Dependency>)> {
+        let pkg_id = unit.pkg.package_id();
+        let kind = unit.kind;
+        self.resolve()
+            .deps(pkg_id)
+            .filter(|&(_id, deps)| {
+                assert!(!deps.is_empty());
+                deps.iter().any(|dep| {
+                    // If this dependency is only available for certain platforms,
+                    // make sure we're only enabling it for that platform.
+                    if !self.target_data.dep_platform_activated(dep, kind) {
+                        return false;
+                    }
+
+                    // 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();
+
+                        let feats = self.activated_features(pkg_id, features_for);
+                        if !feats.contains(&dep.name_in_toml()) {
+                            return false;
+                        }
+                    }
+
+                    true
+                })
+            })
+            .collect()
+    }
 }

tests/testsuite/features2.rs

@@ -1946,3 +1946,52 @@ fn test_proc_macro() {
         .masquerade_as_nightly_cargo()
         .run();
 }
+
+#[cargo_test]
+fn doc_optional() {
+    // Checks for a bug where `cargo doc` was failing with an inactive target
+    // that enables a shared optional dependency.
+    Package::new("spin", "1.0.0").publish();
+    Package::new("bar", "1.0.0")
+        .add_dep(Dependency::new("spin", "1.0").optional(true))
+        .publish();
+    // The enabler package enables the `spin` feature, which we don't want.
+    Package::new("enabler", "1.0.0")
+        .feature_dep("bar", "1.0", &["spin"])
+        .publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [target.'cfg(whatever)'.dependencies]
+                enabler = "1.0"
+
+                [dependencies]
+                bar = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("doc -Zfeatures=itarget")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_unordered(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] spin v1.0.0 [..]
+[DOWNLOADED] bar v1.0.0 [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] enabler v1.0.0 [..]
+[DOCUMENTING] bar v1.0.0
+[CHECKING] bar v1.0.0
+[DOCUMENTING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}