Auto merge of #4364 - RalfJung:feat, r=alexcrichton

bors · bors · commit 5485b253c32e · 2017-08-20T12:57:50.000Z
Required dependencies are not features Also, while I was at it, I fixed an error message which complained about something not being an optional dependency, when really what mattered was that it was not a dependency at all. I made a bunch of guesses about how things work. These guesses ended up as comments in the commit (so hopefully, the next reader of these files has to guess less). I am not totally certain these comments are all correct, so please yell if not. :) In particular, for resolve_features, I observed that dependencies get compiled even when they are not returned from that function. But judging from how the function used to behave, it actually returns all dependencies, even those that have nothing to do with any features. (Making the name rather misleading, TBH...) Fixes #4363
diff --git a/src/cargo/core/resolver/mod.rs b/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);
diff --git a/src/cargo/core/summary.rs b/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 \
diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/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| {
diff --git a/src/cargo/ops/resolve.rs b/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)?;
diff --git a/tests/features.rs b/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")
diff --git a/tests/resolve.rs b/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)
 }

Original file line number	Diff line number	Diff line change
`@@ -53,8 +53,8 @@ impl Summary {`
`53`	`53`	`feature, dep)`
`54`	`54`	`}`
`55`	`55`	`None if is_reexport => {`
`56`		- bail!("Feature `{}` requires `{}` which is not an \
`57`		`- optional dependency", feature, dep)`
	`56`	+ bail!("Feature `{}` requires a feature of `{}` which is not a \
	`57`	`+ dependency", feature, dep)`
`58`	`58`	`}`
`59`	`59`	`None => {`
`60`	`60`	bail!("Feature `{}` includes `{}` which is neither \
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ fn resolve(pkg: PackageId, deps: Vec<Dependency>, registry: &[Summary])`
`32`	`32`	`let mut registry = MyRegistry(registry);`
`33`	`33`	`let summary = Summary::new(pkg.clone(), deps, HashMap::new()).unwrap();`
`34`	`34`	`let method = Method::Everything;`
`35`		`- let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry)?;`
	`35`	`+ let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry, None)?;`
`36`	`36`	`let res = resolve.iter().cloned().collect();`
`37`	`37`	`Ok(res)`
`38`	`38`	`}`