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

This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.

Author: Colecf

src/eval.rs

@@ -2,13 +2,14 @@
 //! `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.
@@ -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,20 @@ 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(),
+        )
+    }
+}
+
 /// A single scope's worth of variable definitions.
 #[derive(Debug, Default)]
 pub struct Vars<'text>(HashMap<&'text str, String>);
@@ -70,36 +94,28 @@ 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 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<String>> {
+    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,
         }
     }
@@ -108,21 +111,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");

src/parse.rs

@@ -131,7 +131,10 @@ impl<'text> Parser<'text> {
         self.skip_spaces();
         self.scanner.expect('=')?;
         self.skip_spaces();
-        self.read_eval()
+        let result = self.read_eval(&[]);
+        self.scanner.skip('\r');
+        self.scanner.expect('\n')?;
+        result
     }
 
     /// Read a collection of `  foo = bar` variables, with leading indent.
@@ -195,57 +198,85 @@ impl<'text> Parser<'text> {
         Ok(())
     }
 
+    fn read_unevaluated_paths_to(
+        &mut self,
+        v: &mut Vec<EvalString<&'text str>>,
+    ) -> ParseResult<()> {
+        self.skip_spaces();
+        while self.scanner.peek() != ':'
+            && self.scanner.peek() != '|'
+            && !self.scanner.peek_newline()
+        {
+            v.push(self.read_eval(&[':', '|', ' '])?);
+            self.skip_spaces();
+        }
+        Ok(())
+    }
+
     fn read_build<L: Loader>(&mut self, loader: &mut L) -> ParseResult<Build<'text, L::Path>> {
         let line = self.scanner.line;
-        let mut outs = Vec::new();
-        self.read_paths_to(loader, &mut outs)?;
-        let explicit_outs = outs.len();
+        let mut unevaluated_outs = Vec::new();
+        self.read_unevaluated_paths_to(&mut unevaluated_outs)?;
+        let explicit_outs = unevaluated_outs.len();
 
         if self.scanner.peek() == '|' {
             self.scanner.next();
-            self.read_paths_to(loader, &mut outs)?;
+            self.read_unevaluated_paths_to(&mut unevaluated_outs)?;
         }
 
         self.scanner.expect(':')?;
         self.skip_spaces();
         let rule = self.read_ident()?;
 
-        let mut ins = Vec::new();
-        self.read_paths_to(loader, &mut ins)?;
-        let explicit_ins = ins.len();
+        let mut unevaluated_ins = Vec::new();
+        self.read_unevaluated_paths_to(&mut unevaluated_ins)?;
+        let explicit_ins = unevaluated_ins.len();
 
         if self.scanner.peek() == '|' {
             self.scanner.next();
             let peek = self.scanner.peek();
             if peek == '|' || peek == '@' {
                 self.scanner.back();
             } else {
-                self.read_paths_to(loader, &mut ins)?;
+                self.read_unevaluated_paths_to(&mut unevaluated_ins)?;
             }
         }
-        let implicit_ins = ins.len() - explicit_ins;
+        let implicit_ins = unevaluated_ins.len() - explicit_ins;
 
         if self.scanner.peek() == '|' {
             self.scanner.next();
             if self.scanner.peek() == '@' {
                 self.scanner.back();
             } else {
                 self.scanner.expect('|')?;
-                self.read_paths_to(loader, &mut ins)?;
+                self.read_unevaluated_paths_to(&mut unevaluated_ins)?;
             }
         }
-        let order_only_ins = ins.len() - implicit_ins - explicit_ins;
+        let order_only_ins = unevaluated_ins.len() - implicit_ins - explicit_ins;
 
         if self.scanner.peek() == '|' {
             self.scanner.next();
             self.scanner.expect('@')?;
-            self.read_paths_to(loader, &mut ins)?;
+            self.read_unevaluated_paths_to(&mut unevaluated_ins)?;
         }
-        let validation_ins = ins.len() - order_only_ins - implicit_ins - explicit_ins;
+        let validation_ins = unevaluated_ins.len() - order_only_ins - implicit_ins - explicit_ins;
 
         self.scanner.skip('\r');
         self.scanner.expect('\n')?;
         let vars = self.read_scoped_vars()?;
+
+        let env: &[&dyn crate::eval::Env] = &[&vars, &self.vars];
+
+        let mut outs = Vec::new();
+        for unevaluated_out in unevaluated_outs {
+            outs.push(loader.path(&mut unevaluated_out.evaluate(env)));
+        }
+
+        let mut ins = Vec::new();
+        for unevaluated_in in unevaluated_ins {
+            ins.push(loader.path(&mut unevaluated_in.evaluate(env)));
+        }
+
         Ok(Build {
             rule,
             line,
@@ -299,17 +330,26 @@ impl<'text> Parser<'text> {
         Ok(self.scanner.slice(start, end))
     }
 
-    fn read_eval(&mut self) -> ParseResult<EvalString<&'text str>> {
+    /// Reads an EvalString. Stops at either a newline, or any of the characters
+    /// in stop_at, without consuming the character that caused it to stop.
+    fn read_eval(&mut self, stop_at: &[char]) -> ParseResult<EvalString<&'text str>> {
         // Guaranteed at least one part.
         let mut parts = Vec::with_capacity(1);
         let mut ofs = self.scanner.ofs;
         let end = loop {
             match self.scanner.read() {
                 '\0' => return self.scanner.parse_error("unexpected EOF"),
-                '\n' => break self.scanner.ofs - 1,
+                x if stop_at.contains(&x) => {
+                    self.scanner.back();
+                    break self.scanner.ofs;
+                }
+                '\n' => {
+                    self.scanner.back();
+                    break self.scanner.ofs;
+                }
                 '\r' if self.scanner.peek() == '\n' => {
-                    self.scanner.next();
-                    break self.scanner.ofs - 2;
+                    self.scanner.back();
+                    break self.scanner.ofs;
                 }
                 '$' => {
                     let end = self.scanner.ofs - 1;

src/scanner.rs

@@ -31,6 +31,16 @@ impl<'a> Scanner<'a> {
     pub fn peek(&self) -> char {
         unsafe { *self.buf.get_unchecked(self.ofs) as char }
     }
+    pub fn peek_newline(&self) -> bool {
+        if self.peek() == '\n' {
+            return true;
+        }
+        if self.ofs >= self.buf.len() - 1 {
+            return false;
+        }
+        let peek2 = unsafe { *self.buf.get_unchecked(self.ofs + 1) as char };
+        self.peek() == '\r' && peek2 == '\n'
+    }
     pub fn next(&mut self) {
         if self.peek() == '\n' {
             self.line += 1;