Auto merge of #5180 - alexcrichton:transitive-update, r=matklad

Don't abort resolution on transitive updates

This commit is directed at fixing #4127, allowing the resolver to automatically
perform transitive updates when required. A few use casese and tagged links are
hanging off #4127 itself, but the crux of the issue happens when you either add
a dependency or update a version requirement in `Cargo.toml` which conflicts
with something listed in your `Cargo.lock`. In this case Cargo would previously
provide an obscure "cannot resolve" error whereas this commit updates Cargo to
automatically perform a conservative re-resolution of the dependency graph.

It's hoped that this commit will help reduce the number of "unresolvable"
dependency graphs we've seen in the wild and otherwise make Cargo a little more
ergonomic to use as well. More details can be found in the source's comments!

Closes #4127

Author: bors

src/bin/commands/read_manifest.rs

@@ -1,6 +1,5 @@
 use command_prelude::*;
 
-use cargo::core::Package;
 use cargo::print_json;
 
 pub fn cli() -> App {
@@ -13,8 +12,7 @@ Print a JSON representation of a Cargo.toml manifest.",
 }
 
 pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
-    let root = args.root_manifest(config)?;
-    let pkg = Package::for_path(&root, config)?;
-    print_json(&pkg);
+    let ws = args.workspace(config)?;
+    print_json(&ws.current()?);
     Ok(())
 }

src/cargo/core/dependency.rs

@@ -343,9 +343,8 @@ impl Dependency {
     }
 
     /// Returns true if the package (`sum`) can fulfill this dependency request.
-    pub fn matches_ignoring_source(&self, sum: &Summary) -> bool {
-        self.name() == sum.package_id().name()
-            && self.version_req().matches(sum.package_id().version())
+    pub fn matches_ignoring_source(&self, id: &PackageId) -> bool {
+        self.name() == id.name() && self.version_req().matches(id.version())
     }
 
     /// Returns true if the package (`id`) can fulfill this dependency request.

src/cargo/core/package.rs

@@ -12,7 +12,6 @@ use lazycell::LazyCell;
 use core::{Dependency, Manifest, PackageId, SourceId, Target};
 use core::{SourceMap, Summary};
 use core::interning::InternedString;
-use ops;
 use util::{internal, lev_distance, Config};
 use util::errors::{CargoResult, CargoResultExt};
 
@@ -81,14 +80,6 @@ impl Package {
         }
     }
 
-    /// Calculate the Package from the manifest path (and cargo configuration).
-    pub fn for_path(manifest_path: &Path, config: &Config) -> CargoResult<Package> {
-        let path = manifest_path.parent().unwrap();
-        let source_id = SourceId::for_path(path)?;
-        let (pkg, _) = ops::read_package(manifest_path, &source_id, config)?;
-        Ok(pkg)
-    }
-
     /// Get the manifest dependencies
     pub fn dependencies(&self) -> &[Dependency] {
         self.manifest.dependencies()

src/cargo/core/registry.rs

@@ -434,7 +434,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                 patches.extend(
                     extra
                         .iter()
-                        .filter(|s| dep.matches_ignoring_source(s))
+                        .filter(|s| dep.matches_ignoring_source(s.package_id()))
                         .cloned(),
                 );
             }

src/cargo/core/resolver/encode.rs

@@ -6,7 +6,7 @@ use serde::ser;
 use serde::de;
 
 use core::{Dependency, Package, PackageId, SourceId, Workspace};
-use util::{internal, Config, Graph};
+use util::{internal, Graph};
 use util::errors::{CargoError, CargoResult, CargoResultExt};
 
 use super::Resolve;
@@ -207,33 +207,33 @@ fn build_path_deps(ws: &Workspace) -> HashMap<String, SourceId> {
         visited.insert(member.package_id().source_id().clone());
     }
     for member in members.iter() {
-        build_pkg(member, ws.config(), &mut ret, &mut visited);
+        build_pkg(member, ws, &mut ret, &mut visited);
     }
     for deps in ws.root_patch().values() {
         for dep in deps {
-            build_dep(dep, ws.config(), &mut ret, &mut visited);
+            build_dep(dep, ws, &mut ret, &mut visited);
         }
     }
     for &(_, ref dep) in ws.root_replace() {
-        build_dep(dep, ws.config(), &mut ret, &mut visited);
+        build_dep(dep, ws, &mut ret, &mut visited);
     }
 
     return ret;
 
     fn build_pkg(
         pkg: &Package,
-        config: &Config,
+        ws: &Workspace,
         ret: &mut HashMap<String, SourceId>,
         visited: &mut HashSet<SourceId>,
     ) {
         for dep in pkg.dependencies() {
-            build_dep(dep, config, ret, visited);
+            build_dep(dep, ws, ret, visited);
         }
     }
 
     fn build_dep(
         dep: &Dependency,
-        config: &Config,
+        ws: &Workspace,
         ret: &mut HashMap<String, SourceId>,
         visited: &mut HashSet<SourceId>,
     ) {
@@ -245,13 +245,13 @@ fn build_path_deps(ws: &Workspace) -> HashMap<String, SourceId> {
             Ok(p) => p.join("Cargo.toml"),
             Err(_) => return,
         };
-        let pkg = match Package::for_path(&path, config) {
+        let pkg = match ws.load(&path) {
             Ok(p) => p,
             Err(_) => return,
         };
         ret.insert(pkg.name().to_string(), pkg.package_id().source_id().clone());
         visited.insert(pkg.package_id().source_id().clone());
-        build_pkg(&pkg, config, ret, visited);
+        build_pkg(&pkg, ws, ret, visited);
     }
 }
 

src/cargo/core/resolver/mod.rs

@@ -384,10 +384,39 @@ struct Context {
 type Activations = HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;
 
 /// Builds the list of all packages required to build the first argument.
+///
+/// * `summaries` - the list of package summaries along with how to resolve
+///   their features. This is a list of all top-level packages that are intended
+///   to be part of the lock file (resolve output). These typically are a list
+///   of all workspace members.
+///
+/// * `replacements` - this is a list of `[replace]` directives found in the
+///   root of the workspace. The list here is a `PackageIdSpec` of what to
+///   replace and a `Dependency` to replace that with. In general it's not
+///   recommended to use `[replace]` any more and use `[patch]` instead, which
+///   is supported elsewhere.
+///
+/// * `registry` - this is the source from which all package summaries are
+///   loaded. It's expected that this is extensively configured ahead of time
+///   and is idempotent with our requests to it (aka returns the same results
+///   for the same query every time). Typically this is an instance of a
+///   `PackageRegistry`.
+///
+/// * `try_to_use` - this is a list of package ids which were previously found
+///   in the lock file. We heuristically prefer the ids listed in `try_to_use`
+///   when sorting candidates to activate, but otherwise this isn't used
+///   anywhere else.
+///
+/// * `config` - a location to print warnings and such, or `None` if no warnings
+///   should be printed
+///
+/// * `print_warnings` - whether or not to print backwards-compatibility
+///   warnings and such
 pub fn resolve(
     summaries: &[(Summary, Method)],
     replacements: &[(PackageIdSpec, Dependency)],
     registry: &mut Registry,
+    try_to_use: &[&PackageId],
     config: Option<&Config>,
     print_warnings: bool,
 ) -> CargoResult<Resolve> {
@@ -400,12 +429,8 @@ pub fn resolve(
         warnings: RcList::new(),
     };
     let _p = profile::start("resolving");
-    let cx = activate_deps_loop(
-        cx,
-        &mut RegistryQueryer::new(registry, replacements),
-        summaries,
-        config,
-    )?;
+    let mut registry = RegistryQueryer::new(registry, replacements, try_to_use);
+    let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
 
     let mut resolve = Resolve {
         graph: cx.graph(),
@@ -637,16 +662,22 @@ impl ConflictReason {
 struct RegistryQueryer<'a> {
     registry: &'a mut (Registry + 'a),
     replacements: &'a [(PackageIdSpec, Dependency)],
+    try_to_use: HashSet<&'a PackageId>,
     // TODO: with nll the Rc can be removed
     cache: HashMap<Dependency, Rc<Vec<Candidate>>>,
 }
 
 impl<'a> RegistryQueryer<'a> {
-    fn new(registry: &'a mut Registry, replacements: &'a [(PackageIdSpec, Dependency)]) -> Self {
+    fn new(
+        registry: &'a mut Registry,
+        replacements: &'a [(PackageIdSpec, Dependency)],
+        try_to_use: &'a [&'a PackageId],
+    ) -> Self {
         RegistryQueryer {
             registry,
             replacements,
             cache: HashMap::new(),
+            try_to_use: try_to_use.iter().cloned().collect(),
         }
     }
 
@@ -739,9 +770,17 @@ impl<'a> RegistryQueryer<'a> {
             candidate.replace = replace;
         }
 
-        // When we attempt versions for a package, we'll want to start at
-        // the maximum version and work our way down.
-        ret.sort_unstable_by(|a, b| b.summary.version().cmp(a.summary.version()));
+        // When we attempt versions for a package we'll want to do so in a
+        // sorted fashion to pick the "best candidates" first. Currently we try
+        // prioritized summaries (those in `try_to_use`) and failing that we
+        // list everything from the maximum version to the lowest version.
+        ret.sort_unstable_by(|a, b| {
+            let a_in_previous = self.try_to_use.contains(a.summary.package_id());
+            let b_in_previous = self.try_to_use.contains(b.summary.package_id());
+            let a = (a_in_previous, a.summary.version());
+            let b = (b_in_previous, b.summary.version());
+            a.cmp(&b).reverse()
+        });
 
         let out = Rc::new(ret);
 

src/cargo/core/workspace.rs

@@ -1,17 +1,21 @@
-use std::collections::hash_map::{Entry, HashMap};
+use std::cell::RefCell;
 use std::collections::BTreeMap;
+use std::collections::hash_map::{Entry, HashMap};
 use std::path::{Path, PathBuf};
 use std::slice;
 
 use glob::glob;
 use url::Url;
 
+use core::registry::PackageRegistry;
 use core::{EitherManifest, Package, SourceId, VirtualManifest};
 use core::{Dependency, PackageIdSpec, Profile, Profiles};
-use util::{Config, Filesystem};
+use ops;
+use sources::PathSource;
 use util::errors::{CargoResult, CargoResultExt};
 use util::paths;
 use util::toml::read_manifest;
+use util::{Config, Filesystem};
 
 /// The core abstraction in Cargo for working with a workspace of crates.
 ///
@@ -67,6 +71,10 @@ pub struct Workspace<'cfg> {
     // needed by the current configuration (such as in cargo install). In some
     // cases `false` also results in the non-enforcement of dev-dependencies.
     require_optional_deps: bool,
+
+    // A cache of lodaed packages for particular paths which is disjoint from
+    // `packages` up above, used in the `load` method down below.
+    loaded_packages: RefCell<HashMap<PathBuf, Package>>,
 }
 
 // Separate structure for tracking loaded packages (to avoid loading anything
@@ -137,6 +145,7 @@ impl<'cfg> Workspace<'cfg> {
             default_members: Vec::new(),
             is_ephemeral: false,
             require_optional_deps: true,
+            loaded_packages: RefCell::new(HashMap::new()),
         };
         ws.root_manifest = ws.find_root(manifest_path)?;
         ws.find_members()?;
@@ -172,6 +181,7 @@ impl<'cfg> Workspace<'cfg> {
             default_members: Vec::new(),
             is_ephemeral: true,
             require_optional_deps,
+            loaded_packages: RefCell::new(HashMap::new()),
         };
         {
             let key = ws.current_manifest.parent().unwrap();
@@ -669,11 +679,63 @@ impl<'cfg> Workspace<'cfg> {
 
         Ok(())
     }
+
+    pub fn load(&self, manifest_path: &Path) -> CargoResult<Package> {
+        match self.packages.maybe_get(manifest_path) {
+            Some(&MaybePackage::Package(ref p)) => return Ok(p.clone()),
+            Some(&MaybePackage::Virtual(_)) => bail!("cannot load workspace root"),
+            None => {}
+        }
+
+        let mut loaded = self.loaded_packages.borrow_mut();
+        if let Some(p) = loaded.get(manifest_path).cloned() {
+            return Ok(p);
+        }
+        let source_id = SourceId::for_path(manifest_path.parent().unwrap())?;
+        let (package, _nested_paths) = ops::read_package(manifest_path, &source_id, self.config)?;
+        loaded.insert(manifest_path.to_path_buf(), package.clone());
+        Ok(package)
+    }
+
+    /// Preload the provided registry with already loaded packages.
+    ///
+    /// A workspace may load packages during construction/parsing/early phases
+    /// for various operations, and this preload step avoids doubly-loading and
+    /// parsing crates on the filesystem by inserting them all into the registry
+    /// with their in-memory formats.
+    pub fn preload(&self, registry: &mut PackageRegistry<'cfg>) {
+        // These can get weird as this generally represents a workspace during
+        // `cargo install`. Things like git repositories will actually have a
+        // `PathSource` with multiple entries in it, so the logic below is
+        // mostly just an optimization for normal `cargo build` in workspaces
+        // during development.
+        if self.is_ephemeral {
+            return;
+        }
+
+        for pkg in self.packages.packages.values() {
+            let pkg = match *pkg {
+                MaybePackage::Package(ref p) => p.clone(),
+                MaybePackage::Virtual(_) => continue,
+            };
+            let mut src = PathSource::new(
+                pkg.manifest_path(),
+                pkg.package_id().source_id(),
+                self.config,
+            );
+            src.preload_with(pkg);
+            registry.add_preloaded(Box::new(src));
+        }
+    }
 }
 
 impl<'cfg> Packages<'cfg> {
     fn get(&self, manifest_path: &Path) -> &MaybePackage {
-        &self.packages[manifest_path.parent().unwrap()]
+        self.maybe_get(manifest_path).unwrap()
+    }
+
+    fn maybe_get(&self, manifest_path: &Path) -> Option<&MaybePackage> {
+        self.packages.get(manifest_path.parent().unwrap())
     }
 
     fn load(&mut self, manifest_path: &Path) -> CargoResult<&MaybePackage> {

src/cargo/ops/cargo_rustc/mod.rs

@@ -1082,7 +1082,7 @@ fn build_deps_args<'a, 'cfg>(
                 .pkg
                 .dependencies()
                 .iter()
-                .filter(|d| d.matches_ignoring_source(dep.pkg.summary()))
+                .filter(|d| d.matches_ignoring_source(dep.pkg.package_id()))
                 .filter_map(|d| d.rename())
                 .next();
 

src/cargo/ops/registry.rs

@@ -447,8 +447,8 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
         Some(ref name) => name.clone(),
         None => {
             let manifest_path = find_root_manifest_for_wd(None, config.cwd())?;
-            let pkg = Package::for_path(&manifest_path, config)?;
-            pkg.name().to_string()
+            let ws = Workspace::new(&manifest_path, config)?;
+            ws.current()?.package_id().name().to_string()
         }
     };
 
@@ -508,8 +508,8 @@ pub fn yank(
         Some(name) => name,
         None => {
             let manifest_path = find_root_manifest_for_wd(None, config.cwd())?;
-            let pkg = Package::for_path(&manifest_path, config)?;
-            pkg.name().to_string()
+            let ws = Workspace::new(&manifest_path, config)?;
+            ws.current()?.package_id().name().to_string()
         }
     };
     let version = match version {