Fix caching features across backtracking

In the local loop during resolution all variables need to be reset whenever we
backtrack up a frame, but currently the `method` and `features` set are
accidentally not reset whenever we backtrack. Calculate the `method` later and
cache `features` in each frame so we can properly backtrack.

Closes #2472

Author: alexcrichton

src/cargo/core/resolver/mod.rs

@@ -275,6 +275,7 @@ struct BacktrackFrame {
     remaining_candidates: RcVecIter<Rc<Summary>>,
     parent: Rc<Summary>,
     dep: Dependency,
+    features: Vec<String>,
 }
 
 /// Recursively activates the dependencies for `top`, in depth-first order,
@@ -319,15 +320,9 @@ fn activate_deps_loop(mut cx: Context,
             }
             None => continue,
         };
-        let (mut parent, (mut cur, (mut dep, candidates, features))) = frame;
+        let (mut parent, (mut cur, (mut dep, candidates, mut features))) = frame;
         assert!(!remaining_deps.is_empty());
 
-        let method = Method::Required {
-            dev_deps: false,
-            features: &features,
-            uses_default_features: dep.uses_default_features(),
-        };
-
         let my_candidates = {
             let prev_active = cx.prev_active(&dep);
             trace!("{}[{}]>{} {} candidates", parent.name(), cur, dep.name(),
@@ -373,6 +368,7 @@ fn activate_deps_loop(mut cx: Context,
                     remaining_candidates: remaining_candidates,
                     parent: parent.clone(),
                     dep: dep.clone(),
+                    features: features.clone(),
                 });
                 candidate
             }
@@ -383,9 +379,13 @@ fn activate_deps_loop(mut cx: Context,
                 // their state at the found level of the `backtrack_stack`.
                 trace!("{}[{}]>{} -- no candidates", parent.name(), cur,
                        dep.name());
-                match find_candidate(&mut backtrack_stack, &mut cx,
-                                     &mut remaining_deps, &mut parent, &mut cur,
-                                     &mut dep) {
+                match find_candidate(&mut backtrack_stack,
+                                     &mut cx,
+                                     &mut remaining_deps,
+                                     &mut parent,
+                                     &mut cur,
+                                     &mut dep,
+                                     &mut features) {
                     None => return Err(activation_error(&cx, registry, &parent,
                                                         &dep,
                                                         &cx.prev_active(&dep),
@@ -395,6 +395,11 @@ fn activate_deps_loop(mut cx: Context,
             }
         };
 
+        let method = Method::Required {
+            dev_deps: false,
+            features: &features,
+            uses_default_features: dep.uses_default_features(),
+        };
         trace!("{}[{}]>{} trying {}", parent.name(), cur, dep.name(),
                candidate.version());
         cx.resolve.graph.link(parent.package_id().clone(),
@@ -414,14 +419,16 @@ fn find_candidate(backtrack_stack: &mut Vec<BacktrackFrame>,
                   remaining_deps: &mut BinaryHeap<DepsFrame>,
                   parent: &mut Rc<Summary>,
                   cur: &mut usize,
-                  dep: &mut Dependency) -> Option<Rc<Summary>> {
+                  dep: &mut Dependency,
+                  features: &mut Vec<String>) -> Option<Rc<Summary>> {
     while let Some(mut frame) = backtrack_stack.pop() {
         if let Some((_, candidate)) = frame.remaining_candidates.next() {
             *cx = frame.context_backup.clone();
             *remaining_deps = frame.deps_backup.clone();
             *parent = frame.parent.clone();
             *cur = remaining_deps.peek().unwrap().remaining_siblings.cur_index();
             *dep = frame.dep.clone();
+            *features = frame.features.clone();
             backtrack_stack.push(frame);
             return Some(candidate)
         }

tests/support/registry.rs

@@ -1,3 +1,4 @@
+use std::collections::HashMap;
 use std::fs::{self, File};
 use std::io::prelude::*;
 use std::path::{PathBuf, Path};
@@ -6,6 +7,7 @@ use flate2::Compression::Default;
 use flate2::write::GzEncoder;
 use git2;
 use rustc_serialize::hex::ToHex;
+use rustc_serialize::json::ToJson;
 use tar::{Builder, Header};
 use url::Url;
 
@@ -21,9 +23,18 @@ pub fn dl_url() -> Url { Url::from_file_path(&*dl_path()).ok().unwrap() }
 pub struct Package {
     name: String,
     vers: String,
-    deps: Vec<(String, String, &'static str, String)>,
+    deps: Vec<Dependency>,
     files: Vec<(String, String)>,
     yanked: bool,
+    features: HashMap<String, Vec<String>>,
+}
+
+struct Dependency {
+    name: String,
+    vers: String,
+    kind: String,
+    target: Option<String>,
+    features: Vec<String>,
 }
 
 fn init() {
@@ -55,6 +66,7 @@ impl Package {
             deps: Vec::new(),
             files: Vec::new(),
             yanked: false,
+            features: HashMap::new(),
         }
     }
 
@@ -64,23 +76,40 @@ impl Package {
     }
 
     pub fn dep(&mut self, name: &str, vers: &str) -> &mut Package {
-        self.deps.push((name.to_string(), vers.to_string(), "normal",
-                        "null".to_string()));
-        self
+        self.full_dep(name, vers, None, "normal", &[])
+    }
+
+    pub fn feature_dep(&mut self,
+                       name: &str,
+                       vers: &str,
+                       features: &[&str]) -> &mut Package {
+        self.full_dep(name, vers, None, "normal", features)
     }
 
     pub fn target_dep(&mut self,
                       name: &str,
                       vers: &str,
                       target: &str) -> &mut Package {
-        self.deps.push((name.to_string(), vers.to_string(), "normal",
-                        format!("\"{}\"", target)));
-        self
+        self.full_dep(name, vers, Some(target), "normal", &[])
     }
 
     pub fn dev_dep(&mut self, name: &str, vers: &str) -> &mut Package {
-        self.deps.push((name.to_string(), vers.to_string(), "dev",
-                        "null".to_string()));
+        self.full_dep(name, vers, None, "dev", &[])
+    }
+
+    fn full_dep(&mut self,
+                name: &str,
+                vers: &str,
+                target: Option<&str>,
+                kind: &str,
+                features: &[&str]) -> &mut Package {
+        self.deps.push(Dependency {
+            name: name.to_string(),
+            vers: vers.to_string(),
+            kind: kind.to_string(),
+            target: target.map(|s| s.to_string()),
+            features: features.iter().map(|s| s.to_string()).collect(),
+        });
         self
     }
 
@@ -89,30 +118,36 @@ impl Package {
         self
     }
 
-    #[allow(deprecated)] // connect => join in 1.3
     pub fn publish(&self) {
         self.make_archive();
 
         // Figure out what we're going to write into the index
-        let deps = self.deps.iter().map(|&(ref name, ref req, ref kind, ref target)| {
-            format!("{{\"name\":\"{}\",\
-                       \"req\":\"{}\",\
-                       \"features\":[],\
-                       \"default_features\":false,\
-                       \"target\":{},\
-                       \"optional\":false,\
-                       \"kind\":\"{}\"}}", name, req, target, kind)
-        }).collect::<Vec<_>>().connect(",");
+        let deps = self.deps.iter().map(|dep| {
+            let mut map = HashMap::new();
+            map.insert("name".to_string(), dep.name.to_json());
+            map.insert("req".to_string(), dep.vers.to_json());
+            map.insert("features".to_string(), dep.features.to_json());
+            map.insert("default_features".to_string(), false.to_json());
+            map.insert("target".to_string(), dep.target.to_json());
+            map.insert("optional".to_string(), false.to_json());
+            map.insert("kind".to_string(), dep.kind.to_json());
+            map
+        }).collect::<Vec<_>>();
         let cksum = {
             let mut c = Vec::new();
             File::open(&self.archive_dst()).unwrap()
                  .read_to_end(&mut c).unwrap();
             cksum(&c)
         };
-        let line = format!("{{\"name\":\"{}\",\"vers\":\"{}\",\
-                              \"deps\":[{}],\"cksum\":\"{}\",\"features\":{{}},\
-                              \"yanked\":{}}}",
-                         self.name, self.vers, deps, cksum, self.yanked);
+        let mut dep = HashMap::new();
+        dep.insert("name".to_string(), self.name.to_json());
+        dep.insert("vers".to_string(), self.vers.to_json());
+        dep.insert("deps".to_string(), deps.to_json());
+        dep.insert("cksum".to_string(), cksum.to_json());
+        dep.insert("features".to_string(), self.features.to_json());
+        dep.insert("yanked".to_string(), self.yanked.to_json());
+        let line = dep.to_json().to_string();
+
         let file = match self.name.len() {
             1 => format!("1/{}", self.name),
             2 => format!("2/{}", self.name),
@@ -152,20 +187,20 @@ impl Package {
             version = "{}"
             authors = []
         "#, self.name, self.vers);
-        for &(ref dep, ref req, kind, ref target) in self.deps.iter() {
-            let target = match &target[..] {
-                "null" => String::new(),
-                t => format!("target.{}.", t),
+        for dep in self.deps.iter() {
+            let target = match dep.target {
+                None => String::new(),
+                Some(ref s) => format!("target.{}.", s),
             };
-            let kind = match kind {
+            let kind = match &dep.kind[..] {
                 "build" => "build-",
                 "dev" => "dev-",
                 _ => ""
             };
             manifest.push_str(&format!(r#"
                 [{}{}dependencies.{}]
                 version = "{}"
-            "#, target, kind, dep, req));
+            "#, target, kind, dep.name, dep.vers));
         }
 
         let dst = self.archive_dst();

tests/test_cargo_cfg.rs

@@ -194,8 +194,8 @@ test!(works_through_the_registry {
 
     Package::new("foo", "0.1.0").publish();
     Package::new("bar", "0.1.0")
-            .target_dep("foo", "0.1.0", "cfg(unix)")
-            .target_dep("foo", "0.1.0", "cfg(windows)")
+            .target_dep("foo", "0.1.0", "'cfg(unix)'")
+            .target_dep("foo", "0.1.0", "'cfg(windows)'")
             .publish();
 
     let p = project("a")

tests/test_cargo_registry.rs

@@ -1015,3 +1015,26 @@ test!(only_download_relevant {
 {compiling} bar v0.5.0 ([..])
 ", downloading = DOWNLOADING, compiling = COMPILING, updating = UPDATING)));
 });
+
+test!(resolve_and_backtracking {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "bar"
+            version = "0.5.0"
+            authors = []
+
+            [dependencies]
+            foo = "*"
+        "#)
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    Package::new("foo", "0.1.1")
+            .feature_dep("bar", "0.1", &["a", "b"])
+            .publish();
+    Package::new("foo", "0.1.0").publish();
+
+    assert_that(p.cargo("build"),
+                execs().with_status(0));
+});