Required dependencies are not features

src/cargo/core/resolver/mod.rs

@@ -57,6 +57,7 @@ use url::Url;
 
 use core::{PackageId, Registry, SourceId, Summary, Dependency};
 use core::PackageIdSpec;
+use util::config::Config;
 use util::Graph;
 use util::errors::{CargoResult, CargoError};
 use util::profile;
@@ -330,20 +331,25 @@ struct Context<'a> {
     resolve_replacements: RcList<(PackageId, PackageId)>,
 
     replacements: &'a [(PackageIdSpec, Dependency)],
+
+    // These warnings are printed after resolution.
+    warnings: RcList<String>,
 }
 
 type Activations = HashMap<String, HashMap<SourceId, Vec<Summary>>>;
 
 /// Builds the list of all packages required to build the first argument.
 pub fn resolve(summaries: &[(Summary, Method)],
                replacements: &[(PackageIdSpec, Dependency)],
-               registry: &mut Registry) -> CargoResult<Resolve> {
+               registry: &mut Registry,
+               config: Option<&Config>) -> CargoResult<Resolve> {
     let cx = Context {
         resolve_graph: RcList::new(),
         resolve_features: HashMap::new(),
         resolve_replacements: RcList::new(),
         activations: HashMap::new(),
         replacements: replacements,
+        warnings: RcList::new(),
     };
     let _p = profile::start(format!("resolving"));
     let cx = activate_deps_loop(cx, registry, summaries)?;
@@ -368,8 +374,18 @@ pub fn resolve(summaries: &[(Summary, Method)],
     }
 
     check_cycles(&resolve, &cx.activations)?;
-
     trace!("resolved: {:?}", resolve);
+
+    // If we have a shell, emit warnings about required deps used as feature.
+    if let Some(config) = config {
+        let mut shell = config.shell();
+        let mut warnings = &cx.warnings;
+        while let Some(ref head) = warnings.head {
+            shell.warn(&head.0)?;
+            warnings = &head.1;
+        }
+    }
+
     Ok(resolve)
 }
 
@@ -827,13 +843,15 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool {
 //
 // The feature dependencies map is a mapping of package name to list of features
 // enabled. Each package should be enabled, and each package should have the
-// specified set of features enabled.
+// specified set of features enabled.  The boolean indicates whether this
+// package was specifically requested (rather than just requesting features
+// *within* this package).
 //
 // The all used features set is the set of features which this local package had
 // enabled, which is later used when compiling to instruct the code what
 // features were enabled.
 fn build_features<'a>(s: &'a Summary, method: &'a Method)
-                      -> CargoResult<(HashMap<&'a str, Vec<String>>, HashSet<&'a str>)> {
+                      -> CargoResult<(HashMap<&'a str, (bool, Vec<String>)>, HashSet<&'a str>)> {
     let mut deps = HashMap::new();
     let mut used = HashSet::new();
     let mut visited = HashSet::new();
@@ -867,7 +885,7 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method)
 
     fn add_feature<'a>(s: &'a Summary,
                        feat: &'a str,
-                       deps: &mut HashMap<&'a str, Vec<String>>,
+                       deps: &mut HashMap<&'a str, (bool, Vec<String>)>,
                        used: &mut HashSet<&'a str>,
                        visited: &mut HashSet<&'a str>) -> CargoResult<()> {
         if feat.is_empty() { return Ok(()) }
@@ -884,8 +902,8 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method)
                 let package = feat_or_package;
                 used.insert(package);
                 deps.entry(package)
-                    .or_insert(Vec::new())
-                    .push(feat.to_string());
+                    .or_insert((false, Vec::new()))
+                    .1.push(feat.to_string());
             }
             None => {
                 let feat = feat_or_package;
@@ -896,12 +914,14 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method)
                 used.insert(feat);
                 match s.features().get(feat) {
                     Some(recursive) => {
+                        // This is a feature, add it recursively.
                         for f in recursive {
                             add_feature(s, f, deps, used, visited)?;
                         }
                     }
                     None => {
-                        deps.entry(feat).or_insert(Vec::new());
+                        // This is a dependency, mark it as explicitly requested.
+                        deps.entry(feat).or_insert((false, Vec::new())).0 = true;
                     }
                 }
                 visited.remove(feat);
@@ -1057,8 +1077,9 @@ impl<'a> Context<'a> {
             .unwrap_or(&[])
     }
 
+    /// Return all dependencies and the features we want from them.
     fn resolve_features<'b>(&mut self,
-                            candidate: &'b Summary,
+                            s: &'b Summary,
                             method: &'b Method)
                             -> CargoResult<Vec<(Dependency, Vec<String>)>> {
         let dev_deps = match *method {
@@ -1067,21 +1088,31 @@ impl<'a> Context<'a> {
         };
 
         // First, filter by dev-dependencies
-        let deps = candidate.dependencies();
+        let deps = s.dependencies();
         let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);
 
-        let (mut feature_deps, used_features) = build_features(candidate,
-                                                               method)?;
+        let (mut feature_deps, used_features) = build_features(s, method)?;
         let mut ret = Vec::new();
 
-        // Next, sanitize all requested features by whitelisting all the
-        // requested features that correspond to optional dependencies
+        // Next, collect all actually enabled dependencies and their features.
         for dep in deps {
-            // weed out optional dependencies, but not those required
+            // Skip optional dependencies, but not those enabled through a feature
             if dep.is_optional() && !feature_deps.contains_key(dep.name()) {
                 continue
             }
-            let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]);
+            // So we want this dependency.  Move the features we want from `feature_deps`
+            // to `ret`.
+            let base = feature_deps.remove(dep.name()).unwrap_or((false, vec![]));
+            if !dep.is_optional() && base.0 {
+                self.warnings.push(
+                    format!("Package `{}` does not have feature `{}`. It has a required dependency \
+                       with that name, but only optional dependencies can be used as features. \
+                       This is currently a warning to ease the transition, but it will become an \
+                       error in the future.",
+                       s.package_id(), dep.name())
+                );
+            }
+            let mut base = base.1;
             base.extend(dep.features().iter().cloned());
             for feature in base.iter() {
                 if feature.contains("/") {
@@ -1091,23 +1122,20 @@ impl<'a> Context<'a> {
             ret.push((dep.clone(), base));
         }
 
-        // All features can only point to optional dependencies, in which case
-        // they should have all been weeded out by the above iteration. Any
-        // remaining features are bugs in that the package does not actually
-        // have those features.
+        // Any remaining entries in feature_deps are bugs in that the package does not actually
+        // have those dependencies.  We classified them as dependencies in the first place
+        // because there is no such feature, either.
         if !feature_deps.is_empty() {
             let unknown = feature_deps.keys().map(|s| &s[..])
                                       .collect::<Vec<&str>>();
-            if !unknown.is_empty() {
-                let features = unknown.join(", ");
-                bail!("Package `{}` does not have these features: `{}`",
-                      candidate.package_id(), features)
-            }
+            let features = unknown.join(", ");
+            bail!("Package `{}` does not have these features: `{}`",
+                    s.package_id(), features)
         }
 
         // Record what list of features is active for this package.
         if !used_features.is_empty() {
-            let pkgid = candidate.package_id();
+            let pkgid = s.package_id();
 
             let set = self.resolve_features.entry(pkgid.clone())
                               .or_insert_with(HashSet::new);

src/cargo/core/summary.rs

@@ -53,8 +53,8 @@ impl Summary {
                                feature, dep)
                     }
                     None if is_reexport => {
-                        bail!("Feature `{}` requires `{}` which is not an \
-                               optional dependency", feature, dep)
+                        bail!("Feature `{}` requires a feature of `{}` which is not a \
+                               dependency", feature, dep)
                     }
                     None => {
                         bail!("Feature `{}` includes `{}` which is neither \

src/cargo/ops/cargo_generate_lockfile.rs

@@ -19,7 +19,7 @@ pub fn generate_lockfile(ws: &Workspace) -> CargoResult<()> {
     let mut registry = PackageRegistry::new(ws.config())?;
     let resolve = ops::resolve_with_previous(&mut registry, ws,
                                              Method::Everything,
-                                             None, None, &[])?;
+                                             None, None, &[], true)?;
     ops::write_pkg_lockfile(ws, &resolve)?;
     Ok(())
 }
@@ -79,7 +79,8 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions)
                                                   Method::Everything,
                                                   Some(&previous_resolve),
                                                   Some(&to_avoid),
-                                                  &[])?;
+                                                  &[],
+                                                  true)?;
 
     // Summarize what is changing for the user.
     let print_change = |status: &str, msg: String| {

src/cargo/ops/resolve.rs

@@ -15,7 +15,7 @@ use util::errors::{CargoResult, CargoResultExt};
 /// lockfile.
 pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolve)> {
     let mut registry = PackageRegistry::new(ws.config())?;
-    let resolve = resolve_with_registry(ws, &mut registry)?;
+    let resolve = resolve_with_registry(ws, &mut registry, true)?;
     let packages = get_resolved_packages(&resolve, registry);
     Ok((packages, resolve))
 }
@@ -44,7 +44,7 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>,
     let resolve = if ws.require_optional_deps() {
         // First, resolve the root_package's *listed* dependencies, as well as
         // downloading and updating all remotes and such.
-        let resolve = resolve_with_registry(ws, &mut registry)?;
+        let resolve = resolve_with_registry(ws, &mut registry, false)?;
 
         // Second, resolve with precisely what we're doing. Filter out
         // transitive dependencies if necessary, specify features, handle
@@ -79,19 +79,19 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>,
     let resolved_with_overrides =
     ops::resolve_with_previous(&mut registry, ws,
                                method, resolve.as_ref(), None,
-                               specs)?;
+                               specs, true)?;
 
     let packages = get_resolved_packages(&resolved_with_overrides, registry);
 
     Ok((packages, resolved_with_overrides))
 }
 
-fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry)
+fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry, warn: bool)
                          -> CargoResult<Resolve> {
     let prev = ops::load_pkg_lockfile(ws)?;
     let resolve = resolve_with_previous(registry, ws,
                                         Method::Everything,
-                                        prev.as_ref(), None, &[])?;
+                                        prev.as_ref(), None, &[], warn)?;
 
     if !ws.is_ephemeral() {
         ops::write_pkg_lockfile(ws, &resolve)?;
@@ -114,7 +114,8 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
                                  method: Method,
                                  previous: Option<&'a Resolve>,
                                  to_avoid: Option<&HashSet<&'a PackageId>>,
-                                 specs: &[PackageIdSpec])
+                                 specs: &[PackageIdSpec],
+                                 warn: bool)
                                  -> CargoResult<Resolve> {
     // Here we place an artificial limitation that all non-registry sources
     // cannot be locked at more than one revision. This means that if a git
@@ -256,9 +257,15 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
         None => root_replace.to_vec(),
     };
 
+    let config = if warn {
+        Some(ws.config())
+    } else {
+        None
+    };
     let mut resolved = resolver::resolve(&summaries,
                                          &replace,
-                                         registry)?;
+                                         registry,
+                                         config)?;
     resolved.register_used_patches(registry.patches());
     if let Some(previous) = previous {
         resolved.merge_from(previous)?;

tests/features.rs

@@ -169,7 +169,7 @@ fn invalid6() {
 [ERROR] failed to parse manifest at `[..]`
 
 Caused by:
-  Feature `foo` requires `bar` which is not an optional dependency
+  Feature `foo` requires a feature of `bar` which is not a dependency
 "));
 }
 
@@ -193,7 +193,7 @@ fn invalid7() {
 [ERROR] failed to parse manifest at `[..]`
 
 Caused by:
-  Feature `foo` requires `bar` which is not an optional dependency
+  Feature `foo` requires a feature of `bar` which is not a dependency
 "));
 }
 
@@ -225,6 +225,80 @@ fn invalid8() {
 "));
 }
 
+#[test]
+fn invalid9() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.bar]
+            path = "bar"
+        "#)
+        .file("src/main.rs", "fn main() {}")
+        .file("bar/Cargo.toml", r#"
+            [package]
+            name = "bar"
+            version = "0.0.1"
+            authors = []
+        "#)
+        .file("bar/src/lib.rs", "");
+
+    assert_that(p.cargo_process("build").arg("--features").arg("bar"),
+                execs().with_status(0).with_stderr("\
+warning: Package `foo v0.0.1 ([..])` does not have feature `bar`. It has a required dependency with \
+that name, but only optional dependencies can be used as features. [..]
+   Compiling bar v0.0.1 ([..])
+   Compiling foo v0.0.1 ([..])
+    Finished dev [unoptimized + debuginfo] target(s) in [..] secs
+"));
+}
+
+#[test]
+fn invalid10() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.bar]
+            path = "bar"
+            features = ["baz"]
+        "#)
+        .file("src/main.rs", "fn main() {}")
+        .file("bar/Cargo.toml", r#"
+            [package]
+            name = "bar"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.baz]
+            path = "baz"
+        "#)
+        .file("bar/src/lib.rs", "")
+        .file("bar/baz/Cargo.toml", r#"
+            [package]
+            name = "baz"
+            version = "0.0.1"
+            authors = []
+        "#)
+        .file("bar/baz/src/lib.rs", "");
+
+    assert_that(p.cargo_process("build"),
+                execs().with_status(0).with_stderr("\
+warning: Package `bar v0.0.1 ([..])` does not have feature `baz`. It has a required dependency with \
+that name, but only optional dependencies can be used as features. [..]
+   Compiling baz v0.0.1 ([..])
+   Compiling bar v0.0.1 ([..])
+   Compiling foo v0.0.1 ([..])
+    Finished dev [unoptimized + debuginfo] target(s) in [..] secs
+"));
+}
+
 #[test]
 fn no_transitive_dep_feature_requirement() {
     let p = project("foo")

tests/resolve.rs

@@ -32,7 +32,7 @@ fn resolve(pkg: PackageId, deps: Vec<Dependency>, registry: &[Summary])
     let mut registry = MyRegistry(registry);
     let summary = Summary::new(pkg.clone(), deps, HashMap::new()).unwrap();
     let method = Method::Everything;
-    let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry)?;
+    let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry, None)?;
     let res = resolve.iter().cloned().collect();
     Ok(res)
 }