rule expansion can refer to build vars that refer to top-level vars

Fixes #83.

src/eval.rs

@@ -28,6 +28,7 @@ impl<T: AsRef<str>> EvalString<T> {
     pub fn new(parts: Vec<EvalPart<T>>) -> Self {
         EvalString(parts)
     }
+
     pub fn evaluate(&self, envs: &[&dyn Env]) -> String {
         let mut val = String::new();
         for part in &self.0 {
@@ -46,6 +47,7 @@ impl<T: AsRef<str>> EvalString<T> {
         val
     }
 }
+
 impl EvalString<&str> {
     pub fn into_owned(self) -> EvalString<String> {
         EvalString(
@@ -78,19 +80,26 @@ impl<'a> Env for Vars<'a> {
     }
 }
 
-/// A single scope's worth of variable definitions, before $-expansion.
-/// For variables attached to a rule we keep them unexpanded in memory because
-/// they may be expanded in multiple different ways depending on which rule uses
-/// them.
-impl Env for SmallMap<&str, EvalString<String>> {
+// Impl for Loader.rules
+impl Env for SmallMap<String, EvalString<String>> {
     fn get_var(&self, var: &str) -> Option<Cow<str>> {
-        // TODO(#83): this is wrong, it should consider envs.
+        // TODO(#83): this is wrong in that it doesn't include envs.
+        // This can occur when you have e.g.
+        //   rule foo
+        //     bar = $baz
+        //   build ...: foo
+        //     x = $bar
+        // When evaluating the value of `x`, we find `bar` in the rule but
+        // then need to pick the right env to evaluate $baz.  But we also don't
+        // wanna generically always use all available envs because we don't
+        // wanna get into evaluation cycles.
         self.get(var).map(|val| Cow::Owned(val.evaluate(&[])))
     }
 }
-impl Env for SmallMap<String, EvalString<String>> {
+
+// Impl for the variables attached to a build.
+impl Env for SmallMap<&str, String> {
     fn get_var(&self, var: &str) -> Option<Cow<str>> {
-        // TODO(#83): this is wrong, it should consider envs.
-        self.get(var).map(|val| Cow::Owned(val.evaluate(&[])))
+        self.get(var).map(|val| Cow::Borrowed(val.as_str()))
     }
 }

src/load.rs

@@ -105,14 +105,22 @@ impl Loader {
             graph: &self.graph,
             build: &build,
         };
-        let build_vars = &b.vars;
-        let envs: [&dyn eval::Env; 4] = [&implicit_vars, build_vars, rule, env];
-
-        let lookup = |key: &str| {
-            build_vars
-                .get(key)
-                .or_else(|| rule.get(key))
-                .map(|var| var.evaluate(&envs))
+
+        // Expand all build-scoped variable values, as they may be referred to in rules.
+        let mut build_vars = SmallMap::default();
+        for &(name, ref val) in b.vars.iter() {
+            let val = val.evaluate(&[&implicit_vars, &build_vars, env]);
+            build_vars.insert(name, val);
+        }
+
+        let envs: [&dyn eval::Env; 4] = [&implicit_vars, &build_vars, rule, env];
+        let lookup = |key: &str| -> Option<String> {
+            // Look up `key = ...` binding in build and rule block.
+            let val = match build_vars.get(key) {
+                Some(val) => val.clone(),
+                None => rule.get(key)?.evaluate(&envs),
+            };
+            Some(val)
         };
 
         let cmdline = lookup("command");

src/smallmap.rs

@@ -38,6 +38,10 @@ impl<K: PartialEq, V> SmallMap<K, V> {
         None
     }
 
+    pub fn iter(&self) -> std::slice::Iter<(K, V)> {
+        self.0.iter()
+    }
+
     pub fn iter_mut(&mut self) -> std::slice::IterMut<(K, V)> {
         self.0.iter_mut()
     }

tests/e2e/basic.rs

@@ -289,7 +289,6 @@ build out: custom
     )?;
 
     let out = space.run_expect(&mut n2_command(vec!["out"]))?;
-    // WRONG(#83): this should be `echo 123 hello 123`.
-    assert_output_contains(&out, "echo  hello 123");
+    assert_output_contains(&out, "echo 123 hello 123");
     Ok(())
 }