just give up and make it an error

Author: Eh2406

src/cargo/core/resolver/context.rs

@@ -9,7 +9,6 @@ use log::debug;
 
 use crate::core::interning::InternedString;
 use crate::core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
-use crate::util::config::Config;
 use crate::util::CargoResult;
 use crate::util::Graph;
 
@@ -158,7 +157,7 @@ impl Context {
         // First, figure out our set of dependencies based on the requested set
         // of features. This also calculates what features we're going to enable
         // for our own dependencies.
-        let deps = self.resolve_features(parent, candidate, method, None)?;
+        let deps = self.resolve_features(parent, candidate, method)?;
 
         // Next, transform all dependencies into a list of possible candidates
         // which can satisfy that dependency.
@@ -218,7 +217,6 @@ impl Context {
         parent: Option<&Summary>,
         s: &'b Summary,
         method: &'b Method<'_>,
-        config: Option<&Config>,
     ) -> ActivateResult<Vec<(Dependency, Vec<InternedString>)>> {
         let dev_deps = match *method {
             Method::Everything => true,
@@ -246,23 +244,26 @@ impl Context {
             // name.
             let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep);
             used_features.insert(dep.name_in_toml());
-            if let Some(config) = config {
-                let mut shell = config.shell();
-                let always_required = !dep.is_optional()
-                    && !s
-                        .dependencies()
-                        .iter()
-                        .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
-                if always_required && base.0 {
-                    shell.warn(&format!(
+            let always_required = !dep.is_optional()
+                && !s
+                    .dependencies()
+                    .iter()
+                    .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
+            if always_required && base.0 {
+                return Err(match parent {
+                    None => failure::format_err!(
                         "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.",
+                         with that name, but only optional dependencies can be used as features.",
                         s.package_id(),
                         dep.name_in_toml()
-                    ))?
-                }
+                    )
+                    .into(),
+                    Some(p) => (
+                        p.package_id(),
+                        ConflictReason::RequiredDependencyAsFeatures(dep.name_in_toml()),
+                    )
+                        .into(),
+                });
             }
             let mut base = base.1.clone();
             base.extend(dep.features().iter());

src/cargo/core/resolver/errors.rs

@@ -127,7 +127,7 @@ pub(super) fn activation_error(
             msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
         }
 
-        let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
+        let (features_errors, mut other_errors): (Vec<_>, Vec<_>) = other_errors
             .drain(..)
             .partition(|&(_, r)| r.is_missing_features());
 
@@ -146,6 +146,29 @@ pub(super) fn activation_error(
             // p == parent so the full path is redundant.
         }
 
+        let (required_dependency_as_features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
+            .drain(..)
+            .partition(|&(_, r)| r.is_required_dependency_as_features());
+
+        for &(p, r) in required_dependency_as_features_errors.iter() {
+            if let ConflictReason::RequiredDependencyAsFeatures(ref features) = *r {
+                msg.push_str("\n\nthe package `");
+                msg.push_str(&*p.name());
+                msg.push_str("` depends on `");
+                msg.push_str(&*dep.package_name());
+                msg.push_str("`, with features: `");
+                msg.push_str(features);
+                msg.push_str("` but `");
+                msg.push_str(&*dep.package_name());
+                msg.push_str("` does not have these features.\n");
+                msg.push_str(
+                    " It has a required dependency with that name, \
+                     but only optional dependencies can be used as features.\n",
+                );
+            }
+            // p == parent so the full path is redundant.
+        }
+
         if !other_errors.is_empty() {
             msg.push_str(
                 "\n\nall possible versions conflict with \

src/cargo/core/resolver/mod.rs

@@ -124,7 +124,6 @@ pub fn resolve(
     registry: &mut dyn Registry,
     try_to_use: &HashSet<PackageId>,
     config: Option<&Config>,
-    print_warnings: bool,
     check_public_visible_dependencies: bool,
 ) -> CargoResult<Resolve> {
     let cx = Context::new(check_public_visible_dependencies);
@@ -157,11 +156,6 @@ pub fn resolve(
     check_duplicate_pkgs_in_lockfile(&resolve)?;
     trace!("resolved: {:?}", resolve);
 
-    // If we have a shell, emit warnings about required deps used as feature.
-    if print_warnings && config.is_some() {
-        emit_warnings(&cx, &resolve, summaries, config)
-    }
-
     Ok(resolve)
 }
 
@@ -1087,65 +1081,3 @@ fn check_duplicate_pkgs_in_lockfile(resolve: &Resolve) -> CargoResult<()> {
     }
     Ok(())
 }
-
-/// Re-run all used `resolve_features` so it can print warnings.
-/// Within the resolution loop we do not pass a `config` to `resolve_features`
-/// this tells it not to print warnings. Hear we do pass `config` triggering it to print them.
-/// This is done as the resolution is NP-Hard so the loop can potentially call `resolve features`
-/// an exponential number of times, but this pass is linear in the number of dependencies.
-fn emit_warnings(
-    cx: &Context,
-    resolve: &Resolve,
-    summaries: &[(Summary, Method<'_>)],
-    config: Option<&Config>,
-) {
-    let mut new_cx = cx.clone();
-    new_cx.resolve_features = im_rc::HashMap::new();
-    let mut features_from_dep = HashMap::new();
-    for (summary, method) in summaries {
-        let replacement = &cx.activations[&resolve
-            .replacements()
-            .get(&summary.package_id())
-            .unwrap_or(&summary.package_id())
-            .as_activations_key()]
-            .0;
-        for (dep, features) in new_cx
-            .resolve_features(None, replacement, &method, config)
-            .expect("can not resolve_features for a required summery")
-        {
-            features_from_dep.insert((replacement.package_id(), dep), features);
-        }
-    }
-    // crates enable features for their dependencies.
-    // So by iterating reverse topologically we process all of the parents before each child.
-    // Then we know all the needed features for each crate.
-    let topological_sort = resolve.sort();
-    for id in topological_sort
-        .iter()
-        .rev()
-        .filter(|&id| !resolve.replacements().contains_key(id))
-    {
-        let summary = &cx.activations[&id.as_activations_key()].0;
-
-        for (parent, deps) in cx.parents.edges(&summary.package_id()) {
-            for dep in deps.iter() {
-                let features = features_from_dep
-                    .remove(&(*parent, dep.clone()))
-                    .expect("fulfilled a dep that was not needed");
-                let method = Method::Required {
-                    dev_deps: false,
-                    features: &features,
-                    all_features: false,
-                    uses_default_features: dep.uses_default_features(),
-                };
-                for (dep, features) in new_cx
-                    .resolve_features(None, summary, &method, config)
-                    .expect("can not resolve_features for a used dep")
-                {
-                    features_from_dep.insert((summary.package_id(), dep), features);
-                }
-            }
-        }
-    }
-    assert_eq!(cx.resolve_features, new_cx.resolve_features);
-}

src/cargo/core/resolver/types.rs

@@ -426,6 +426,12 @@ pub enum ConflictReason {
     /// candidate we're activating didn't actually have the feature `foo`.
     MissingFeatures(String),
 
+    /// A dependency listed features that ended up being a required dependency.
+    /// For example we tried to activate feature `foo` but the
+    /// candidate we're activating didn't actually have the feature `foo`
+    /// it had a dependency `foo` instead.
+    RequiredDependencyAsFeatures(InternedString),
+
     // TODO: needs more info for `activation_error`
     // TODO: needs more info for `find_candidate`
     /// pub dep error
@@ -446,6 +452,13 @@ impl ConflictReason {
         }
         false
     }
+
+    pub fn is_required_dependency_as_features(&self) -> bool {
+        if let ConflictReason::RequiredDependencyAsFeatures(_) = *self {
+            return true;
+        }
+        false
+    }
 }
 
 /// A list of packages that have gotten in the way of resolving a dependency.

src/cargo/ops/cargo_generate_lockfile.rs

@@ -21,16 +21,8 @@ pub struct UpdateOptions<'a> {
 
 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,
-        &[],
-        true,
-        true,
-    )?;
+    let resolve =
+        ops::resolve_with_previous(&mut registry, ws, Method::Everything, None, None, &[], true)?;
     ops::write_pkg_lockfile(ws, &resolve)?;
     Ok(())
 }
@@ -92,7 +84,6 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
         Some(&to_avoid),
         &[],
         true,
-        true,
     )?;
 
     // Summarize what is changing for the user.

src/cargo/ops/resolve.rs

@@ -23,7 +23,7 @@ version. This may also occur with an optional dependency that is not enabled.";
 /// lock file.
 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, true)?;
+    let resolve = resolve_with_registry(ws, &mut registry)?;
     let packages = get_resolved_packages(&resolve, registry)?;
     Ok((packages, resolve))
 }
@@ -64,7 +64,7 @@ pub fn resolve_ws_with_method<'a>(
     } else 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, false)?;
+        let resolve = resolve_with_registry(ws, &mut registry)?;
         add_patches = false;
 
         // Second, resolve with precisely what we're doing. Filter out
@@ -98,7 +98,6 @@ pub fn resolve_ws_with_method<'a>(
         None,
         specs,
         add_patches,
-        true,
     )?;
 
     let packages = get_resolved_packages(&resolved_with_overrides, registry)?;
@@ -109,7 +108,6 @@ pub fn resolve_ws_with_method<'a>(
 fn resolve_with_registry<'cfg>(
     ws: &Workspace<'cfg>,
     registry: &mut PackageRegistry<'cfg>,
-    warn: bool,
 ) -> CargoResult<Resolve> {
     let prev = ops::load_pkg_lockfile(ws)?;
     let resolve = resolve_with_previous(
@@ -120,7 +118,6 @@ fn resolve_with_registry<'cfg>(
         None,
         &[],
         true,
-        warn,
     )?;
 
     if !ws.is_ephemeral() {
@@ -146,7 +143,6 @@ pub fn resolve_with_previous<'cfg>(
     to_avoid: Option<&HashSet<PackageId>>,
     specs: &[PackageIdSpec],
     register_patches: bool,
-    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
@@ -334,7 +330,6 @@ pub fn resolve_with_previous<'cfg>(
         registry,
         &try_to_use,
         Some(ws.config()),
-        warn,
         false, // TODO: use "public and private dependencies" feature flag
     )?;
     resolved.register_used_patches(registry.patches());

tests/testsuite/features.rs

@@ -291,13 +291,12 @@ fn invalid9() {
         .file("bar/src/lib.rs", "")
         .build();
 
-    p.cargo("build --features bar").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 [..]s
-").run();
+    p.cargo("build --features bar")
+.with_stderr(
+            "\
+error: 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.
+",
+        ).with_status(101).run();
 }
 
 #[test]
@@ -335,13 +334,17 @@ fn invalid10() {
         .build();
 
     p.cargo("build").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 [..]s
-").run();
+error: failed to select a version for `bar`.
+    ... required by package `foo v0.0.1 ([..])`
+versions that meet the requirements `*` are: 0.0.1
+
+the package `foo` depends on `bar`, with features: `baz` but `bar` does not have these features.
+ It has a required dependency with that name, but only optional dependencies can be used as features.
+
+
+failed to select a version for `bar` which could resolve this conflict
+").with_status(101)
+        .run();
 }
 
 #[test]

tests/testsuite/support/resolver.rs

@@ -115,7 +115,6 @@ pub fn resolve_with_config_raw(
         &HashSet::new(),
         config,
         true,
-        true,
     );
 
     // The largest test in our suite takes less then 30 sec.