Evaluate build paths in the context of the build's bindings

This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes #91 and #39.

benches/parse.rs

@@ -22,14 +22,6 @@ pub fn bench_canon(c: &mut Criterion) {
     });
 }
 
-struct NoOpLoader {}
-impl n2::parse::Loader for NoOpLoader {
-    type Path = ();
-    fn path(&mut self, _path: &mut str) -> Self::Path {
-        ()
-    }
-}
-
 fn generate_build_ninja() -> Vec<u8> {
     let mut buf: Vec<u8> = Vec::new();
     write!(buf, "rule cc\n    command = touch $out",).unwrap();
@@ -46,14 +38,13 @@ fn generate_build_ninja() -> Vec<u8> {
 }
 
 fn bench_parse_synthetic(c: &mut Criterion) {
-    let mut loader = NoOpLoader {};
     let mut input = generate_build_ninja();
     input.push(0);
     c.bench_function("parse synthetic build.ninja", |b| {
         b.iter(|| {
             let mut parser = n2::parse::Parser::new(&input);
             loop {
-                if parser.read(&mut loader).unwrap().is_none() {
+                if parser.read().unwrap().is_none() {
                     break;
                 }
             }
@@ -62,13 +53,12 @@ fn bench_parse_synthetic(c: &mut Criterion) {
 }
 
 fn bench_parse_file(c: &mut Criterion, mut input: Vec<u8>) {
-    let mut loader = NoOpLoader {};
     input.push(0);
     c.bench_function("parse benches/build.ninja", |b| {
         b.iter(|| {
             let mut parser = n2::parse::Parser::new(&input);
             loop {
-                if parser.read(&mut loader).unwrap().is_none() {
+                if parser.read().unwrap().is_none() {
                     break;
                 }
             }

src/eval.rs

@@ -2,17 +2,18 @@
 //! `c++ $in -o $out`, and mechanisms for expanding those into plain strings.
 
 use crate::smallmap::SmallMap;
+use std::borrow::Borrow;
 use std::{borrow::Cow, collections::HashMap};
 
 /// An environment providing a mapping of variable name to variable value.
-/// A given EvalString may be expanded with multiple environments as possible
-/// context.
+/// This represents one "frame" of evaluation context, a given EvalString may
+/// need multiple environments in order to be fully expanded.
 pub trait Env {
-    fn get_var(&self, var: &str) -> Option<Cow<str>>;
+    fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>>;
 }
 
 /// One token within an EvalString, either literal text or a variable reference.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub enum EvalPart<T: AsRef<str>> {
     Literal(T),
     VarRef(T),
@@ -29,22 +30,31 @@ impl<T: AsRef<str>> EvalString<T> {
         EvalString(parts)
     }
 
-    pub fn evaluate(&self, envs: &[&dyn Env]) -> String {
-        let mut val = String::new();
+    fn evaluate_inner(&self, result: &mut String, envs: &[&dyn Env]) {
         for part in &self.0 {
             match part {
-                EvalPart::Literal(s) => val.push_str(s.as_ref()),
+                EvalPart::Literal(s) => result.push_str(s.as_ref()),
                 EvalPart::VarRef(v) => {
-                    for env in envs {
+                    for (i, env) in envs.iter().enumerate() {
                         if let Some(v) = env.get_var(v.as_ref()) {
-                            val.push_str(&v);
+                            v.evaluate_inner(result, &envs[i + 1..]);
                             break;
                         }
                     }
                 }
             }
         }
-        val
+    }
+
+    /// evalulate turns the EvalString into a regular String, looking up the
+    /// values of variable references in the provided Envs. It will look up
+    /// its variables in the earliest Env that has them, and then those lookups
+    /// will be recursively expanded starting from the env after the one that
+    /// had the first successful lookup.
+    pub fn evaluate(&self, envs: &[&dyn Env]) -> String {
+        let mut result = String::new();
+        self.evaluate_inner(&mut result, envs);
+        result
     }
 }
 
@@ -62,6 +72,34 @@ impl EvalString<&str> {
     }
 }
 
+impl EvalString<String> {
+    pub fn as_cow(&self) -> EvalString<Cow<str>> {
+        EvalString(
+            self.0
+                .iter()
+                .map(|part| match part {
+                    EvalPart::Literal(s) => EvalPart::Literal(Cow::Borrowed(s.as_ref())),
+                    EvalPart::VarRef(s) => EvalPart::VarRef(Cow::Borrowed(s.as_ref())),
+                })
+                .collect(),
+        )
+    }
+}
+
+impl EvalString<&str> {
+    pub fn as_cow(&self) -> EvalString<Cow<str>> {
+        EvalString(
+            self.0
+                .iter()
+                .map(|part| match part {
+                    EvalPart::Literal(s) => EvalPart::Literal(Cow::Borrowed(*s)),
+                    EvalPart::VarRef(s) => EvalPart::VarRef(Cow::Borrowed(*s)),
+                })
+                .collect(),
+        )
+    }
+}
+
 /// A single scope's worth of variable definitions.
 #[derive(Debug, Default)]
 pub struct Vars<'text>(HashMap<&'text str, String>);
@@ -70,36 +108,34 @@ impl<'text> Vars<'text> {
     pub fn insert(&mut self, key: &'text str, val: String) {
         self.0.insert(key, val);
     }
-    pub fn get(&self, key: &'text str) -> Option<&String> {
+    pub fn get(&self, key: &str) -> Option<&String> {
         self.0.get(key)
     }
 }
 impl<'a> Env for Vars<'a> {
-    fn get_var(&self, var: &str) -> Option<Cow<str>> {
-        self.0.get(var).map(|str| Cow::Borrowed(str.as_str()))
+    fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
+        Some(EvalString::new(vec![EvalPart::Literal(
+            std::borrow::Cow::Borrowed(self.get(var)?),
+        )]))
+    }
+}
+
+impl<K: Borrow<str> + PartialEq> Env for SmallMap<K, EvalString<String>> {
+    fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
+        Some(self.get(var)?.as_cow())
     }
 }
 
-// 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 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<K: Borrow<str> + PartialEq> Env for SmallMap<K, EvalString<&str>> {
+    fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
+        Some(self.get(var)?.as_cow())
     }
 }
 
-// Impl for the variables attached to a build.
 impl Env for SmallMap<&str, String> {
-    fn get_var(&self, var: &str) -> Option<Cow<str>> {
-        self.get(var).map(|val| Cow::Borrowed(val.as_str()))
+    fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
+        Some(EvalString::new(vec![EvalPart::Literal(
+            std::borrow::Cow::Borrowed(self.get(var)?),
+        )]))
     }
 }

src/load.rs

@@ -2,6 +2,7 @@
 
 use crate::{
     canon::{canon_path, canon_path_fast},
+    eval::{EvalPart, EvalString},
     graph::{FileId, RspFile},
     parse::Statement,
     smallmap::SmallMap,
@@ -30,12 +31,14 @@ impl<'a> BuildImplicitVars<'a> {
     }
 }
 impl<'a> eval::Env for BuildImplicitVars<'a> {
-    fn get_var(&self, var: &str) -> Option<Cow<str>> {
+    fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
+        let string_to_evalstring =
+            |s: String| Some(EvalString::new(vec![EvalPart::Literal(Cow::Owned(s))]));
         match var {
-            "in" => Some(Cow::Owned(self.file_list(self.build.explicit_ins(), ' '))),
-            "in_newline" => Some(Cow::Owned(self.file_list(self.build.explicit_ins(), '\n'))),
-            "out" => Some(Cow::Owned(self.file_list(self.build.explicit_outs(), ' '))),
-            "out_newline" => Some(Cow::Owned(self.file_list(self.build.explicit_outs(), '\n'))),
+            "in" => string_to_evalstring(self.file_list(self.build.explicit_ins(), ' ')),
+            "in_newline" => string_to_evalstring(self.file_list(self.build.explicit_ins(), '\n')),
+            "out" => string_to_evalstring(self.file_list(self.build.explicit_outs(), ' ')),
+            "out_newline" => string_to_evalstring(self.file_list(self.build.explicit_outs(), '\n')),
             _ => None,
         }
     }
@@ -72,21 +75,38 @@ impl Loader {
         loader
     }
 
+    fn evaluate_path(&mut self, path: EvalString<&str>, envs: &[&dyn eval::Env]) -> FileId {
+        use parse::Loader;
+        let mut evaluated = path.evaluate(envs);
+        self.path(&mut evaluated)
+    }
+
+    fn evaluate_paths(
+        &mut self,
+        paths: Vec<EvalString<&str>>,
+        envs: &[&dyn eval::Env],
+    ) -> Vec<FileId> {
+        paths
+            .into_iter()
+            .map(|path| self.evaluate_path(path, envs))
+            .collect()
+    }
+
     fn add_build(
         &mut self,
         filename: std::rc::Rc<PathBuf>,
         env: &eval::Vars,
-        b: parse::Build<FileId>,
+        b: parse::Build,
     ) -> anyhow::Result<()> {
         let ins = graph::BuildIns {
-            ids: b.ins,
+            ids: self.evaluate_paths(b.ins, &[&b.vars, env]),
             explicit: b.explicit_ins,
             implicit: b.implicit_ins,
             order_only: b.order_only_ins,
             // validation is implied by the other counts
         };
         let outs = graph::BuildOuts {
-            ids: b.outs,
+            ids: self.evaluate_paths(b.outs, &[&b.vars, env]),
             explicit: b.explicit_outs,
         };
         let mut build = graph::Build::new(
@@ -108,21 +128,14 @@ impl Loader {
             build: &build,
         };
 
-        // 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];
+        // temp variable in order to not move all of b into the closure
+        let build_vars = &b.vars;
         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)
+            Some(match rule.get(key) {
+                Some(val) => val.evaluate(&[&implicit_vars, build_vars, env]),
+                None => build_vars.get(key)?.evaluate(&[env]),
+            })
         };
 
         let cmdline = lookup("command");
@@ -167,29 +180,47 @@ impl Loader {
         self.parse(path, &bytes)
     }
 
+    fn evaluate_and_read_file(
+        &mut self,
+        file: EvalString<&str>,
+        envs: &[&dyn eval::Env],
+    ) -> anyhow::Result<()> {
+        let evaluated = self.evaluate_path(file, envs);
+        self.read_file(evaluated)
+    }
+
     pub fn parse(&mut self, path: PathBuf, bytes: &[u8]) -> anyhow::Result<()> {
         let filename = std::rc::Rc::new(path);
 
         let mut parser = parse::Parser::new(&bytes);
+
         loop {
             let stmt = match parser
-                .read(self)
+                .read()
                 .map_err(|err| anyhow!(parser.format_parse_error(&filename, err)))?
             {
                 None => break,
                 Some(s) => s,
             };
             match stmt {
-                Statement::Include(id) => trace::scope("include", || self.read_file(id))?,
+                Statement::Include(id) => trace::scope("include", || {
+                    self.evaluate_and_read_file(id, &[&parser.vars])
+                })?,
                 // TODO: implement scoping for subninja
-                Statement::Subninja(id) => trace::scope("subninja", || self.read_file(id))?,
+                Statement::Subninja(id) => trace::scope("subninja", || {
+                    self.evaluate_and_read_file(id, &[&parser.vars])
+                })?,
                 Statement::Default(defaults) => {
-                    self.default.extend(defaults);
+                    let evaluated = self.evaluate_paths(defaults, &[&parser.vars]);
+                    self.default.extend(evaluated);
                 }
                 Statement::Rule(rule) => {
                     let mut vars: SmallMap<String, eval::EvalString<String>> = SmallMap::default();
                     for (name, val) in rule.vars.into_iter() {
-                        vars.insert(name.to_owned(), val);
+                        // TODO: We should not need to call .into_owned() here
+                        // if we keep the contents of all included files in
+                        // memory.
+                        vars.insert(name.to_owned(), val.into_owned());
                     }
                     self.rules.insert(rule.name.to_owned(), vars);
                 }