Auto merge of #2821 - bennofs:no-transitive-dep-feature, r=alexcrichton

Disallow specifying features of transitive deps

Before this commit, it was possible to activate a feature in a transtive
dependency, using a Cargo.toml like the following one:

    ...
    [features]
    # this will enable feature fast in package bar, which is a
    # dependency of foo
    default = [ foo/bar/fast ]

This is a bug, and was never intended, and it is checked in other places
already. The behavior was possible because `build_features::add_feature`
treats the specification "foo/bar/fast" as just another feature. So when
we require the feature "foo/bar/fast", add_feature for foo will generate a
dependency on "foo" requiring that feature "bar/fast" is enabled. Then,
when resolving foo, add_feature will find that "bar/fast" is a required
feature, so it'll happily add "fast" as the required feature for the
dependency "foo".

The fix for this is to make sure that the `add_feature` function does
not treat `a/b` specifications as just another feature. Instead, it now
handles that case without recursion directly when it encounters it.

We can see how this resolves the above problem: when resolving foo,
add_feature for the required feature "bar/fast" will be called.
Because add_feature no longer treats such specifciations differently at
the top level, it will try to enable a feature with the exact name
"bar/fast", and Context::resolve_features will later find that no such
feature exists for package foo.

To give a friendlier error message, we also check in
Context::resolve_features that we never ever require a feature with a
slash in a name from a dependency.

src/cargo/core/resolver/mod.rs

@@ -658,41 +658,53 @@ fn build_features(s: &Summary, method: &Method)
                    visited: &mut HashSet<String>) -> CargoResult<()> {
         if feat.is_empty() { return Ok(()) }
 
-        // If this feature is of the form `foo/bar`, then we just lookup package
-        // `foo` and enable its feature `bar`. Otherwise this feature is of the
-        // form `foo` and we need to recurse to enable the feature `foo` for our
-        // own package, which may end up enabling more features or just enabling
-        // a dependency.
-        let mut parts = feat.splitn(2, '/');
-        let feat_or_package = parts.next().unwrap();
-        match parts.next() {
-            Some(feat) => {
-                let package = feat_or_package;
-                used.insert(package.to_string());
-                deps.entry(package.to_string())
-                    .or_insert(Vec::new())
-                    .push(feat.to_string());
-            }
-            None => {
-                let feat = feat_or_package;
-                if !visited.insert(feat.to_string()) {
-                    bail!("Cyclic feature dependency: feature `{}` depends \
-                           on itself", feat)
-                }
-                used.insert(feat.to_string());
-                match s.features().get(feat) {
-                    Some(recursive) => {
-                        for f in recursive {
-                            try!(add_feature(s, f, deps, used, visited));
+        if !visited.insert(feat.to_string()) {
+            bail!("Cyclic feature dependency: feature `{}` depends \
+                   on itself", feat)
+        }
+
+        used.insert(feat.to_string());
+
+        // The lookup of here may fail if the feature corresponds to an optional
+        // dependency. If that is the case, we simply enable the optional dependency.
+        //
+        // Otherwise, we check what other features the feature wants and recursively
+        // add them.
+        match s.features().get(feat) {
+            Some(wanted_features) => {
+                for entry in wanted_features {
+                    // If the entry is of the form `foo/bar`, then we just lookup package 
+                    // `foo` and enable its feature `bar`. We also add `foo` to the used
+                    // set because `foo` might have been an optional dependency.
+                    //
+                    // Otherwise the entry refers to another feature of our current package,
+                    // so we recurse by calling add_feature again, which may end up enabling
+                    // more features or just enabling a dependency (remember, optional
+                    // dependencies create an implicit feature with the same name).
+                    let mut parts = entry.splitn(2, '/');
+                    let feat_or_package = parts.next().unwrap();
+                    match parts.next() {
+                        Some(feat) => {
+                            let package = feat_or_package;
+                            used.insert(package.to_string());
+                            deps.entry(package.to_string())
+                                .or_insert(Vec::new())
+                                .push(feat.to_string());
+                        }
+                        None => {
+                            let feat = feat_or_package;
+                            try!(add_feature(s, feat, deps, used, visited));
                         }
-                    }
-                    None => {
-                        deps.entry(feat.to_string()).or_insert(Vec::new());
                     }
                 }
-                visited.remove(&feat.to_string());
+            }
+            None => {
+                deps.entry(feat.to_string()).or_insert(Vec::new());
             }
         }
+
+        visited.remove(&feat.to_string());
+
         Ok(())
     }
 }
@@ -843,11 +855,10 @@ impl<'a> Context<'a> {
                 continue
             }
             let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]);
-            for feature in dep.features().iter() {
-                base.push(feature.clone());
+            base.extend(dep.features().iter().map(|x| x.clone()));
+            for feature in base.iter() {
                 if feature.contains("/") {
-                    bail!("features in dependencies cannot enable features in \
-                           other dependencies: `{}`", feature)
+                    bail!("feature names may not contain slashes: `{}`", feature);
                 }
             }
             ret.push((dep.clone(), base));

tests/features.rs

@@ -220,7 +220,58 @@ fn invalid8() {
 
     assert_that(p.cargo_process("build").arg("--features").arg("foo"),
                 execs().with_status(101).with_stderr("\
-[ERROR] features in dependencies cannot enable features in other dependencies: `foo/bar`
+[ERROR] feature names may not contain slashes: `foo/bar`
+"));
+}
+
+#[test]
+fn no_transitive_dep_feature_requirement() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.derived]
+            path = "derived"
+
+            [features]
+            default = ["derived/bar/qux"]
+        "#)
+        .file("src/main.rs", r#"
+            extern crate derived;
+            fn main() { derived::test(); }
+        "#)
+        .file("derived/Cargo.toml", r#"
+            [package]
+            name = "derived"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.bar]
+            path = "../bar"
+        "#)
+        .file("derived/src/lib.rs", r#"
+            extern crate bar;
+            pub use bar::test;
+        "#)
+        .file("bar/Cargo.toml", r#"
+            [package]
+            name = "bar"
+            version = "0.0.1"
+            authors = []
+
+            [features]
+            qux = []
+        "#)
+        .file("bar/src/lib.rs", r#"
+            #[cfg(feature = "qux")]
+            pub fn test() { print!("test"); }
+        "#);
+    assert_that(p.cargo_process("build"),
+                execs().with_status(101).with_stderr("\
+[ERROR] feature names may not contain slashes: `bar/qux`
 "));
 }