[guppy] handle platform dependencies + update feature graph construction

This is a complete overhaul of the way platform-specific dependencies are
handled. Based on my experimentation and reading the Cargo source code, I
believe that this is now correct.

Doing so also required feature graph construction to be updated, so the
feature graph construction is ready for the new feature resolver, and is now
platform-sensitive as well. The APIs for the feature graph are still to
come, but making the data model be full-fidelity allows for both the current
and new feature resolvers to be implemented.

Now that target logic is internalized in guppy, cargo-guppy no longer needs
to do its own evaluation.

For more about the new feature resolver, see:

* https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#features
* rust-lang/cargo#7914
* rust-lang/cargo#7915
* rust-lang/cargo#7916

cargo-guppy/Cargo.toml

@@ -13,4 +13,3 @@ itertools = "0.9.0"
 serde = "1.0.40"
 serde_json = "1.0.40"
 structopt = "0.3.0"
-target-spec = { version = "0.1.0", path = "../target-spec" }

cargo-guppy/src/lib.rs

@@ -3,9 +3,10 @@
 
 use anyhow;
 use clap::arg_enum;
+use guppy::graph::EnabledStatus;
 use guppy::{
     graph::{DependencyLink, DotWrite, PackageDotVisitor, PackageGraph, PackageMetadata},
-    MetadataCommand, PackageId,
+    MetadataCommand, PackageId, Platform, TargetFeatures,
 };
 use itertools;
 use std::cmp;
@@ -15,7 +16,6 @@ use std::fs;
 use std::io::Write;
 use std::iter;
 use structopt::StructOpt;
-use target_spec;
 
 mod diff;
 
@@ -305,6 +305,13 @@ fn narrow_graph(pkg_graph: &mut PackageGraph, options: &FilterOptions) {
         }
     }
 
+    let platform = if let Some(ref target) = options.target {
+        // The features are unknown.
+        Some(Platform::new(target, TargetFeatures::Unknown).unwrap())
+    } else {
+        None
+    };
+
     pkg_graph.retain_edges(|_, DependencyLink { from, to, edge }| {
         // filter by the kind of dependency (--kind)
         // NOTE: We always retain all workspace deps in the graph, otherwise
@@ -316,14 +323,12 @@ fn narrow_graph(pkg_graph: &mut PackageGraph, options: &FilterOptions) {
         };
 
         // filter out irrelevant dependencies for a specific target (--target)
-        let include_target = if let Some(ref target) = options.target {
+        let include_target = if let Some(platform) = &platform {
             edge.normal()
-                .and_then(|meta| meta.target())
-                .and_then(|edge_target| {
-                    let res = target_spec::eval(edge_target, target)
-                        .unwrap_or(Some(true))
-                        .unwrap_or(true);
-                    Some(res)
+                .map(|meta| {
+                    // Include this dependency if it's optional or mandatory or if the status is
+                    // unknown.
+                    meta.enabled_on(platform) != EnabledStatus::Never
                 })
                 .unwrap_or(true)
         } else {

guppy/Cargo.toml

@@ -28,6 +28,7 @@ maintenance = { status = "actively-developed" }
 
 [dependencies]
 cargo_metadata = "0.9"
+cfg-expr = { git = "https://github.com/sunshowers/cfg-expr", branch = "logic" }
 derivative = "2"
 fixedbitset = { version = "0.2.0", default-features = false }
 nested = "0.1.1"
@@ -39,6 +40,7 @@ proptest-derive = { version = "0.1.2", optional = true }
 semver = "0.9.0"
 serde = { version = "1.0.99", features = ["derive"] }
 serde_json = "1.0.40"
+target-spec = { version = "0.1.0", path = "../target-spec" }
 
 [dev-dependencies]
 assert_matches = "1.3.0"

guppy/src/errors.rs

@@ -25,6 +25,8 @@ pub enum Error {
     UnknownPackageId(PackageId),
     /// A feature ID was unknown to this `FeatureGraph`.
     UnknownFeatureId(PackageId, Option<String>),
+    /// The platform `guppy` is running on is unknown.
+    UnknownCurrentPlatform,
     /// An internal error occurred within this `PackageGraph`.
     PackageGraphInternalError(String),
 }
@@ -46,6 +48,7 @@ impl fmt::Display for Error {
                 Some(feature) => write!(f, "Unknown feature ID: '{}' '{}'", package_id, feature),
                 None => write!(f, "Unknown feature ID: '{}' (base)", package_id),
             },
+            UnknownCurrentPlatform => write!(f, "Unknown current platform"),
             PackageGraphInternalError(msg) => write!(f, "Internal error in package graph: {}", msg),
         }
     }
@@ -59,6 +62,7 @@ impl error::Error for Error {
             PackageGraphConstructError(_) => None,
             UnknownPackageId(_) => None,
             UnknownFeatureId(_, _) => None,
+            UnknownCurrentPlatform => None,
             PackageGraphInternalError(_) => None,
         }
     }

guppy/src/graph/build.rs

@@ -2,16 +2,18 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
 use crate::graph::{
-    cargo_version_matches, kind_str, DependencyEdge, DependencyMetadata, PackageGraph,
-    PackageGraphData, PackageIx, PackageMetadata, Workspace,
+    cargo_version_matches, DependencyEdge, DependencyMetadata, DependencyReq, DependencyReqImpl,
+    PackageGraph, PackageGraphData, PackageIx, PackageMetadata, TargetPredicate, Workspace,
 };
-use crate::{Error, Metadata, PackageId};
+use crate::{Error, Metadata, PackageId, Platform};
 use cargo_metadata::{Dependency, DependencyKind, NodeDep, Package, Resolve};
 use once_cell::sync::OnceCell;
 use petgraph::prelude::*;
-use semver::Version;
+use semver::{Version, VersionReq};
 use std::collections::{BTreeMap, HashMap, HashSet};
+use std::mem;
 use std::path::{Path, PathBuf};
+use target_spec::TargetSpec;
 
 impl PackageGraph {
     /// Constructs a new `PackageGraph` instances from the given metadata.
@@ -437,10 +439,9 @@ impl DependencyEdge {
         resolved_name: &str,
         deps: impl IntoIterator<Item = &'a Dependency>,
     ) -> Result<Self, Error> {
-        // deps should have at most 1 normal dependency, 1 build dep and 1 dev dep.
-        let mut normal: Option<DependencyMetadata> = None;
-        let mut build: Option<DependencyMetadata> = None;
-        let mut dev: Option<DependencyMetadata> = None;
+        let mut normal = DependencyBuildState::default();
+        let mut build = DependencyBuildState::default();
+        let mut dev = DependencyBuildState::default();
         for dep in deps {
             // Dev dependencies cannot be optional.
             if dep.kind == DependencyKind::Development && dep.optional {
@@ -450,71 +451,207 @@ impl DependencyEdge {
                 )));
             }
 
-            let to_set = match dep.kind {
-                DependencyKind::Normal => &mut normal,
-                DependencyKind::Build => &mut build,
-                DependencyKind::Development => &mut dev,
+            match dep.kind {
+                DependencyKind::Normal => normal.add_instance(from_id, dep)?,
+                DependencyKind::Build => build.add_instance(from_id, dep)?,
+                DependencyKind::Development => dev.add_instance(from_id, dep)?,
                 _ => {
                     // unknown dependency kind -- can't do much with this!
                     continue;
                 }
             };
-            let metadata = DependencyMetadata {
-                version_req: dep.req.clone(),
-                optional: dep.optional,
-                uses_default_features: dep.uses_default_features,
-                features: dep.features.clone(),
-                target: dep.target.as_ref().map(|t| format!("{}", t)),
-            };
-
-            // It is typically an error for the same dependency to be listed multiple times for
-            // the same kind, but there are some situations in which it's possible. The main one
-            // is if there's a custom 'target' field -- one real world example is at
-            // https://github.com/alexcrichton/flate2-rs/blob/5751ad9/Cargo.toml#L29-L33:
-            //
-            // [dependencies]
-            // miniz_oxide = { version = "0.3.2", optional = true}
-            //
-            // [target.'cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))'.dependencies]
-            // miniz_oxide = "0.3.2"
-            //
-            // For now, prefer target = null (the more general target) in such cases, and error out
-            // if both sides are null.
-            //
-            // TODO: Handle this better, probably through some sort of target resolution.
-            let write_to_set = match to_set {
-                Some(old) => match (old.target(), metadata.target()) {
-                    (Some(_), None) => true,
-                    (None, Some(_)) => false,
-                    (Some(_), Some(_)) => {
-                        // Both targets are set. We don't yet know if they are mutually exclusive,
-                        // so take the first one.
-                        // XXX This is wrong and needs to be fixed along with target resolution
-                        // in general.
-                        false
-                    }
-                    (None, None) => {
-                        return Err(Error::PackageGraphConstructError(format!(
-                            "{}: duplicate dependencies found for '{}' (kind: {})",
-                            from_id,
-                            name,
-                            kind_str(dep.kind)
-                        )))
-                    }
-                },
-                None => true,
-            };
-            if write_to_set {
-                to_set.replace(metadata);
-            }
         }
 
         Ok(DependencyEdge {
             dep_name: name.into(),
             resolved_name: resolved_name.into(),
-            normal,
-            build,
-            dev,
+            normal: normal.finish()?,
+            build: build.finish()?,
+            dev: dev.finish()?,
         })
     }
 }
+
+/// It is possible to specify a dependency several times within the same section through
+/// platform-specific dependencies and the [target] section. For example:
+/// https://github.com/alexcrichton/flate2-rs/blob/5751ad9/Cargo.toml#L29-L33
+///
+/// ```toml
+/// [dependencies]
+/// miniz_oxide = { version = "0.3.2", optional = true}
+///
+/// [target.'cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))'.dependencies]
+/// miniz_oxide = "0.3.2"
+/// ```
+///
+/// (From here on, each separate time a particular version of a dependency
+/// is listed, it is called an "instance".)
+///
+/// For such situations, there are two separate analyses that happen:
+///
+/// 1. Whether the dependency is included at all. This is a union of all instances, conditional on
+///    the specifics of the `[target]` lines.
+/// 2. What features are enabled. As of cargo 1.42, this is unified across all instances but
+///    separately for mandatory/optional instances.
+///
+/// Note that the new feature resolver
+/// (https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#features)'s `itarget` setting
+/// causes this union-ing to *not* happen, so that's why we store all the features enabled by
+/// each target separately.
+#[derive(Debug, Default)]
+struct DependencyBuildState {
+    // This is the `req` field from the first instance seen if there are any, or `None` if none are
+    // seen.
+    version_req: Option<VersionReq>,
+    dependency_req: DependencyReq,
+    // Set if there's a single target -- mostly there for backwards compat support.
+    single_target: Option<String>,
+}
+
+impl DependencyBuildState {
+    fn add_instance(&mut self, from_id: &PackageId, dep: &Dependency) -> Result<(), Error> {
+        match &self.version_req {
+            Some(_) => {
+                // There's more than one instance, so mark the single target `None`.
+                self.single_target = None;
+            }
+            None => {
+                self.version_req = Some(dep.req.clone());
+                self.single_target = dep.target.as_ref().map(|platform| format!("{}", platform));
+            }
+        }
+        self.dependency_req.add_instance(from_id, dep)?;
+
+        Ok(())
+    }
+
+    fn finish(self) -> Result<Option<DependencyMetadata>, Error> {
+        let version_req = match self.version_req {
+            Some(version_req) => version_req,
+            None => {
+                // No instances seen.
+                return Ok(None);
+            }
+        };
+
+        let dependency_req = self.dependency_req;
+
+        // Evaluate this dependency against the current platform.
+        let current_platform = Platform::current().ok_or(Error::UnknownCurrentPlatform)?;
+        let current_enabled = dependency_req.enabled_on(&current_platform);
+        let current_default_features = dependency_req.default_features_on(&current_platform);
+
+        // Collect all features from both the optional and mandatory instances.
+        let all_features: HashSet<_> = dependency_req.all_features().collect();
+        let all_features: Vec<_> = all_features
+            .into_iter()
+            .map(|feature| feature.to_string())
+            .collect();
+
+        // Collect the status of every feature on this platform.
+        let current_feature_statuses = all_features
+            .iter()
+            .map(|feature| {
+                (
+                    feature.clone(),
+                    dependency_req.feature_enabled_on(feature, &current_platform),
+                )
+            })
+            .collect();
+
+        Ok(Some(DependencyMetadata {
+            version_req,
+            dependency_req,
+            current_enabled,
+            current_default_features,
+            all_features,
+            current_feature_statuses,
+            single_target: self.single_target,
+        }))
+    }
+}
+
+impl DependencyReq {
+    fn add_instance(&mut self, from_id: &PackageId, dep: &Dependency) -> Result<(), Error> {
+        if dep.optional {
+            self.optional.add_instance(from_id, dep)
+        } else {
+            self.mandatory.add_instance(from_id, dep)
+        }
+    }
+
+    fn all_features(&self) -> impl Iterator<Item = &str> {
+        self.mandatory
+            .all_features()
+            .chain(self.optional.all_features())
+    }
+}
+
+impl DependencyReqImpl {
+    fn add_instance(&mut self, from_id: &PackageId, dep: &Dependency) -> Result<(), Error> {
+        // target_spec is None if this is not a platform-specific dependency.
+        let target_spec = match dep.target.as_ref() {
+            Some(spec_or_triple) => {
+                // This is a platform-specific dependency, so add it to the list of specs.
+                let spec_or_triple = format!("{}", spec_or_triple);
+                let target_spec: TargetSpec = spec_or_triple.parse().map_err(|err| {
+                    Error::PackageGraphConstructError(format!(
+                        "for package '{}': for dependency '{}', parsing target '{}' failed: {}",
+                        from_id, dep.name, spec_or_triple, err
+                    ))
+                })?;
+                Some(target_spec)
+            }
+            None => None,
+        };
+
+        self.build_if.add_spec(target_spec.as_ref());
+        if dep.uses_default_features {
+            self.default_features_if.add_spec(target_spec.as_ref());
+        }
+        self.target_features
+            .push((target_spec, dep.features.clone()));
+        Ok(())
+    }
+}
+
+impl TargetPredicate {
+    pub(super) fn extend(&mut self, other: &TargetPredicate) {
+        // &mut *self is a reborrow to allow mem::replace to work below.
+        match (&mut *self, other) {
+            (TargetPredicate::Always, _) => {
+                // Always stays the same since it means all specs are included.
+            }
+            (TargetPredicate::Specs(_), TargetPredicate::Always) => {
+                // Mark self as Always.
+                mem::replace(self, TargetPredicate::Always);
+            }
+            (TargetPredicate::Specs(specs), TargetPredicate::Specs(other)) => {
+                specs.extend_from_slice(other.as_slice());
+            }
+        }
+    }
+
+    pub(super) fn add_spec(&mut self, spec: Option<&TargetSpec>) {
+        // &mut *self is a reborrow to allow mem::replace to work below.
+        match (&mut *self, spec) {
+            (TargetPredicate::Always, _) => {
+                // Always stays the same since it means all specs are included.
+            }
+            (TargetPredicate::Specs(_), None) => {
+                // Mark self as Always.
+                mem::replace(self, TargetPredicate::Always);
+            }
+            (TargetPredicate::Specs(specs), Some(spec)) => {
+                specs.push(spec.clone());
+            }
+        }
+    }
+}
+
+impl Default for TargetPredicate {
+    fn default() -> Self {
+        // Empty vector means never.
+        TargetPredicate::Specs(vec![])
+    }
+}