From 3a39030a50203b363f4f32eeb8727d9f8d061965 Mon Sep 17 00:00:00 2001
From: Evan Martin <evan.martin@gmail.com>
Date: Wed, 30 Aug 2023 12:43:08 -0700
Subject: [PATCH] rule expansion can refer to build vars that refer to
 top-level vars

Fixes #83.
---
 src/eval.rs        | 27 ++++++++++++++++++---------
 src/load.rs        | 24 ++++++++++++++++--------
 src/smallmap.rs    |  4 ++++
 tests/e2e/basic.rs |  3 +--
 4 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/src/eval.rs b/src/eval.rs
index db4ee02..c3f296d 100644
--- a/src/eval.rs
+++ b/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()))
     }
 }
diff --git a/src/load.rs b/src/load.rs
index 05f75cc..d786c03 100644
--- a/src/load.rs
+++ b/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");
diff --git a/src/smallmap.rs b/src/smallmap.rs
index 49e12b8..37cc424 100644
--- a/src/smallmap.rs
+++ b/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()
     }
diff --git a/tests/e2e/basic.rs b/tests/e2e/basic.rs
index 93c0cf4..0058ec7 100644
--- a/tests/e2e/basic.rs
+++ b/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(())
 }