Fix variable expansion issue #145

benches/parse.rs

@@ -1,5 +1,6 @@
 use divan::Bencher;
-use std::{io::Write, path::PathBuf, str::FromStr};
+use std::io::Write;
+use std::path::PathBuf;
 
 fn generate_build_ninja(statement_count: usize) -> Vec<u8> {
     let mut buf: Vec<u8> = Vec::new();
@@ -55,8 +56,10 @@ fn load_synthetic(bencher: Bencher) {
     input.push(0);
     bencher.bench_local(|| {
         let mut loader = n2::load::Loader::new();
+        let mut parser = n2::parse::Parser::new(&input);
+
         loader
-            .parse(PathBuf::from_str("build.ninja").unwrap(), &input)
+            .parse_with_parser(&mut parser, PathBuf::from("<synthetic>"), &[])
             .unwrap();
     });
 }

src/eval.rs

@@ -128,10 +128,16 @@ impl<'text> Vars<'text> {
     pub fn insert(&mut self, key: &'text str, val: String) {
         self.0.insert(key, val);
     }
+
     pub fn get(&self, key: &str) -> Option<&String> {
         self.0.get(key)
     }
+
+    pub fn get_all(&self) -> &FxHashMap<&'text str, String> {
+        &self.0
+    }
 }
+
 impl<'a> Env for Vars<'a> {
     fn get_var(&self, var: &str) -> Option<EvalString<Cow<'_, str>>> {
         Some(EvalString::new(vec![EvalPart::Literal(

src/load.rs

@@ -173,29 +173,23 @@ impl Loader {
         self.graph.add_build(build)
     }
 
-    fn read_file(&mut self, id: FileId) -> anyhow::Result<()> {
+    pub fn read_file_by_id(&self, id: FileId) -> anyhow::Result<(PathBuf, Vec<u8>)> {
         let path = self.graph.file(id).path().to_path_buf();
-        let bytes = match trace::scope("read file", || scanner::read_file_with_nul(&path)) {
-            Ok(b) => b,
+
+        match trace::scope("read file", || scanner::read_file_with_nul(&path)) {
+            Ok(b) => Ok((path, b)),
             Err(e) => bail!("read {}: {}", path.display(), e),
-        };
-        self.parse(path, &bytes)
+        }
     }
 
-    fn evaluate_and_read_file(
+    pub fn parse_with_parser(
         &mut self,
-        file: EvalString<&str>,
+        parser: &mut parse::Parser,
+        path: PathBuf,
         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()
@@ -204,18 +198,23 @@ impl Loader {
                 None => break,
                 Some(s) => s,
             };
+
             match stmt {
-                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.evaluate_and_read_file(id, &[&parser.vars])
-                })?,
+                Statement::Include(in_path) | Statement::Subninja(in_path) => {
+                    let id = self.evaluate_path(in_path, &[&parser.vars]);
+                    let (path, bytes) = self.read_file_by_id(id)?;
+                    let bytes = std::rc::Rc::new(bytes);
+                    let mut sub_parser = parse::Parser::new(&bytes);
+
+                    sub_parser.inherit(&parser);
+                    self.parse_with_parser(&mut sub_parser, path, envs)?;
+                }
+
                 Statement::Default(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() {
@@ -226,12 +225,15 @@ impl Loader {
                     }
                     self.rules.insert(rule.name.to_owned(), vars);
                 }
+
                 Statement::Build(build) => self.add_build(filename.clone(), &parser.vars, build)?,
+
                 Statement::Pool(pool) => {
                     self.pools.insert(pool.name.to_string(), pool.depth);
                 }
             };
         }
+
         self.builddir = parser.vars.get("builddir").cloned();
         Ok(())
     }
@@ -254,8 +256,12 @@ pub fn read(build_filename: &str) -> anyhow::Result<State> {
             .graph
             .files
             .id_from_canonical(to_owned_canon_path(build_filename));
-        loader.read_file(id)
+        let (path, bytes) = loader.read_file_by_id(id)?;
+        let mut parser = parse::Parser::new(&bytes);
+
+        loader.parse_with_parser(&mut parser, path, &[])
     })?;
+
     let mut hashes = graph::Hashes::default();
     let db = trace::scope("db::open", || {
         let mut db_path = PathBuf::from(".n2_db");
@@ -268,6 +274,7 @@ pub fn read(build_filename: &str) -> anyhow::Result<State> {
         db::open(&db_path, &mut loader.graph, &mut hashes)
     })
     .map_err(|err| anyhow!("load .n2_db: {}", err))?;
+
     Ok(State {
         graph: loader.graph,
         db,
@@ -282,8 +289,11 @@ pub fn read(build_filename: &str) -> anyhow::Result<State> {
 pub fn parse(name: &str, mut content: Vec<u8>) -> anyhow::Result<graph::Graph> {
     content.push(0);
     let mut loader = Loader::new();
+
     trace::scope("loader.read_file", || {
-        loader.parse(PathBuf::from(name), &content)
+        let mut parser = parse::Parser::new(&content);
+        loader.parse_with_parser(&mut parser, PathBuf::from(name), &[])
     })?;
+
     Ok(loader.graph)
 }

src/parse.rs

@@ -66,6 +66,13 @@ impl<'text> Parser<'text> {
         }
     }
 
+    pub fn inherit<'b>(&mut self, from: &'b Self) {
+        // TODO: should use from parser as a scope rather than copying.
+        for (k, v) in from.vars.get_all() {
+            self.vars.insert(k, v.clone());
+        }
+    }
+
     pub fn format_parse_error(&self, filename: &Path, err: ParseError) -> String {
         self.scanner.format_parse_error(filename, err)
     }

tests/e2e/bindings.rs

@@ -153,12 +153,13 @@ build foo: write_file
 #[test]
 fn across_files() -> anyhow::Result<()> {
     let space = TestSpace::new()?;
+    space.write("world.txt", "<t>")?;
     space.write(
         "build.ninja",
         &[
             ECHO_RULE,
             "
-var = hello
+ext = txt
 include other.ninja
 ",
         ]
@@ -167,15 +168,13 @@ include other.ninja
     space.write(
         "other.ninja",
         "
-build out: echo $var/world
+build hello: echo world.$ext
+    text = what a beautiful day
 ",
     )?;
 
-    let out = space.run(&mut n2_command(vec!["out"]))?;
-    assert_output_contains(&out, "input /world missing");
+    let out = space.run_expect(&mut n2_command(vec!["hello"]))?;
+    assert_output_contains(&out, "what a beautiful day");
 
-    // TODO: should instead be something like:
-    // let out = space.run_expect(&mut n2_command(vec!["out"]))?;
-    // assert_output_contains(&out, "echo hello/world");
     Ok(())
 }