eagerly expand when appending to eagerly-expanded macros

4 files changed, 165 insertions, 57 deletions

diff --git a/src/makefile/functions.rs b/src/makefile/functions.rs
index 7bb0908..a16c268 100644
--- a/src/makefile/functions.rs
+++ b/src/makefile/functions.rs
@@ -6,7 +6,7 @@ use std::rc::Rc;
 use eyre::{bail, Result, WrapErr};
 
 use super::pattern::r#match;
-use super::r#macro::{Set as MacroSet, Source as MacroSource};
+use super::r#macro::{Macro, Set as MacroSet, Source as MacroSource};
 use super::token::TokenString;
 
 pub fn expand_call(
@@ -401,7 +401,15 @@ pub fn foreach(
     let mut macros = macros.with_overlay();
     let results = words
         .map(|word| {
-            macros.set(var.clone(), MacroSource::File, TokenString::text(word));
+            macros.set(
+                var.clone(),
+                Macro {
+                    source: MacroSource::File,
+                    text: TokenString::text(word),
+                    #[cfg(feature = "full")]
+                    eagerly_expanded: false,
+                },
+            );
             macros.expand(text)
         })
         .collect::<Result<Vec<_>, _>>()?;
@@ -417,7 +425,15 @@ pub fn call<'a>(macros: &MacroSet, args: impl Iterator<Item = &'a TokenString>)
 
     let mut macros = macros.with_overlay();
     for (i, x) in args.into_iter().enumerate() {
-        macros.set(i.to_string(), MacroSource::File, TokenString::text(x));
+        macros.set(
+            i.to_string(),
+            Macro {
+                source: MacroSource::File,
+                text: TokenString::text(x),
+                #[cfg(feature = "full")]
+                eagerly_expanded: false,
+            },
+        );
     }
     macros.expand(&TokenString::r#macro(function))
 }
@@ -579,10 +595,22 @@ mod test {
         let mut macros = MacroSet::new();
         macros.set(
             "test1".to_owned(),
-            MacroSource::File,
-            TokenString::text("something"),
+            Macro {
+                source: MacroSource::File,
+                text: TokenString::text("something"),
+                #[cfg(feature = "full")]
+                eagerly_expanded: false,
+            },
+        );
+        macros.set(
+            "test2".to_owned(),
+            Macro {
+                source: MacroSource::File,
+                text: TokenString::text(""),
+                #[cfg(feature = "full")]
+                eagerly_expanded: false,
+            },
         );
-        macros.set("test2".to_owned(), MacroSource::File, TokenString::text(""));
 
         assert_eq!(
             call(
@@ -668,8 +696,12 @@ mod test {
         let mut macros = MacroSet::new();
         macros.set(
             "test".to_owned(),
-            MacroSource::File,
-            "worked for $(item).".parse()?,
+            Macro {
+                source: MacroSource::File,
+                text: "worked for $(item).".parse()?,
+                #[cfg(feature = "full")]
+                eagerly_expanded: false,
+            },
         );
         assert_eq!(
             call(
@@ -691,8 +723,12 @@ mod test {
         let mut macros = MacroSet::new();
         macros.set(
             "reverse".to_owned(),
-            MacroSource::File,
-            "$(2) $(1)".parse()?,
+            Macro {
+                source: MacroSource::File,
+                text: "$(2) $(1)".parse()?,
+                #[cfg(feature = "full")]
+                eagerly_expanded: false,
+            },
         );
         assert_eq!(
             call(
diff --git a/src/makefile/input.rs b/src/makefile/input.rs
index 5dfaed7..5883590 100644
--- a/src/makefile/input.rs
+++ b/src/makefile/input.rs
@@ -16,7 +16,7 @@ use super::command_line::CommandLine;
 #[cfg(feature = "full")]
 use super::conditional::{Line as ConditionalLine, State as ConditionalState};
 use super::inference_rules::InferenceRule;
-use super::r#macro::{Set as MacroSet, Source as MacroSource};
+use super::r#macro::{Macro, Set as MacroSet, Source as MacroSource};
 use super::target::Target;
 use super::token::{tokenize, Token, TokenString};
 
@@ -162,21 +162,21 @@ impl<'a, 'parent> MakefileReader<'a, 'parent, BufReader<File>> {
         path: impl AsRef<Path>,
     ) -> Result<Self> {
         #[cfg(feature = "full")]
-        if let Some((_, mut old_makefile_list)) = macros.pop("MAKEFILE_LIST") {
-            old_makefile_list.extend(TokenString::text(format!(
+        if let Some(mut old_makefile_list) = macros.pop("MAKEFILE_LIST") {
+            old_makefile_list.text.extend(TokenString::text(format!(
                 " {}",
                 path.as_ref().to_string_lossy()
             )));
-            macros.set(
-                "MAKEFILE_LIST".to_owned(),
-                MacroSource::Builtin,
-                old_makefile_list,
-            );
+            macros.set("MAKEFILE_LIST".to_owned(), old_makefile_list);
         } else {
             macros.set(
                 "MAKEFILE_LIST".to_owned(),
-                MacroSource::Builtin,
-                TokenString::text(path.as_ref().to_string_lossy()),
+                Macro {
+                    source: MacroSource::Builtin,
+                    text: TokenString::text(path.as_ref().to_string_lossy()),
+                    #[cfg(feature = "full")]
+                    eagerly_expanded: false,
+                },
             );
         }
         let file = File::open(path);
@@ -606,23 +606,41 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
 
         match self.macros.get(name) {
             // We always let command line or MAKEFLAGS macros override macros from the file.
-            Some((MacroSource::CommandLineOrMakeflags, _)) => return Ok(()),
+            Some(Macro {
+                source: MacroSource::CommandLineOrMakeflags,
+                ..
+            }) => return Ok(()),
             // We let environment variables override macros from the file only if the command-line argument to do that was given
-            Some((MacroSource::Environment, _)) if self.args.environment_overrides => return Ok(()),
+            Some(Macro {
+                source: MacroSource::Environment,
+                ..
+            }) if self.args.environment_overrides => return Ok(()),
             Some(_) if skip_if_defined => return Ok(()),
             _ => {}
         }
 
         let value = match self.macros.pop(name) {
-            Some((_, mut old_value)) if append => {
-                // TODO eagerly expand if appending to eagerly-expanded macro
-                old_value.extend(TokenString::text(" "));
-                old_value.extend(value);
+            Some(mut old_value) if append => {
+                #[cfg(feature = "full")]
+                let value = if old_value.eagerly_expanded {
+                    TokenString::text(self.expand_macros(&value).wrap_err_with(|| {
+                        format!("while defining {} on line {}", name, line_number)
+                    })?)
+                } else {
+                    value
+                };
+                old_value.text.extend(TokenString::text(" "));
+                old_value.text.extend(value);
                 old_value
             }
-            _ => value,
+            _ => Macro {
+                source: MacroSource::File,
+                text: value,
+                #[cfg(feature = "full")]
+                eagerly_expanded: expand_value,
+            },
         };
-        self.macros.set(name.into(), MacroSource::File, value);
+        self.macros.set(name.into(), value);
 
         Ok(())
     }
@@ -652,7 +670,7 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
 
 pub struct FinishedMakefileReader {
     pub inference_rules: Vec<InferenceRule>,
-    pub macros: HashMap<String, (MacroSource, TokenString)>,
+    pub macros: HashMap<String, Macro>,
     pub targets: HashMap<String, Target>,
     pub first_non_special_target: Option<String>,
 }
diff --git a/src/makefile/macro.rs b/src/makefile/macro.rs
index a6dcb74..1c8bd01 100644
--- a/src/makefile/macro.rs
+++ b/src/makefile/macro.rs
@@ -21,6 +21,14 @@ pub enum Source {
     Builtin,
 }
 
+#[derive(Debug, Clone)]
+pub struct Macro {
+    pub source: Source,
+    pub text: TokenString,
+    #[cfg(feature = "full")]
+    pub eagerly_expanded: bool,
+}
+
 pub trait LookupInternal: for<'a> Fn(&'a str) -> Result<String> {}
 
 impl<F: for<'a> Fn(&'a str) -> Result<String>> LookupInternal for F {}
@@ -28,7 +36,7 @@ impl<F: for<'a> Fn(&'a str) -> Result<String>> LookupInternal for F {}
 #[derive(Clone)]
 pub struct Set<'parent, 'lookup> {
     parent: Option<&'parent Set<'parent, 'lookup>>,
-    pub data: HashMap<String, (Source, TokenString)>,
+    pub data: HashMap<String, Macro>,
     lookup_internal: Option<&'lookup dyn LookupInternal>,
     #[cfg(feature = "full")]
     pub to_eval: Rc<RefCell<Vec<String>>>,
@@ -47,15 +55,30 @@ impl<'parent, 'lookup> Set<'parent, 'lookup> {
 
     pub fn add_builtins(&mut self) {
         for (k, v) in builtins() {
-            self.data.insert(k.into(), (Source::Builtin, v));
+            self.data.insert(
+                k.into(),
+                Macro {
+                    source: Source::Builtin,
+                    text: v,
+                    #[cfg(feature = "full")]
+                    eagerly_expanded: false,
+                },
+            );
         }
     }
 
     pub fn add_env(&mut self) {
         for (k, v) in env::vars() {
             if k != "MAKEFLAGS" && k != "SHELL" {
-                self.data
-                    .insert(k, (Source::Environment, TokenString::text(v)));
+                self.data.insert(
+                    k,
+                    Macro {
+                        source: Source::Environment,
+                        text: TokenString::text(v),
+                        #[cfg(feature = "full")]
+                        eagerly_expanded: false,
+                    },
+                );
             }
         }
     }
@@ -73,27 +96,30 @@ impl<'parent, 'lookup> Set<'parent, 'lookup> {
         }
     }
 
-    pub fn get(&self, name: &str) -> Option<&(Source, TokenString)> {
+    pub fn get(&self, name: &str) -> Option<&Macro> {
         self.data
             .get(name)
             .or_else(|| self.parent.and_then(|parent| parent.get(name)))
     }
 
-    pub fn set(&mut self, name: String, source: Source, text: TokenString) {
-        self.data.insert(name, (source, text));
+    pub fn set(&mut self, name: String, r#macro: Macro) {
+        self.data.insert(name, r#macro);
     }
 
     #[cfg(feature = "full")]
     pub fn is_defined(&self, name: &str) -> bool {
-        self.data.get(name).map_or(false, |(_, x)| !x.is_empty())
+        self.data.get(name).map_or(false, |x| !x.text.is_empty())
     }
 
     // `remove` is fine, but I think for "remove-and-return" `pop` is better
-    pub fn pop(&mut self, name: &str) -> Option<(Source, TokenString)> {
-        self.data.remove(name)
+    pub fn pop(&mut self, name: &str) -> Option<Macro> {
+        // TODO figure out a better way to handle inheritance
+        self.data
+            .remove(name)
+            .or_else(|| self.parent.and_then(|p| p.get(name).cloned()))
     }
 
-    pub fn extend(&mut self, other: HashMap<String, (Source, TokenString)>) {
+    pub fn extend(&mut self, other: HashMap<String, Macro>) {
         self.data.extend(other);
     }
 
@@ -118,7 +144,7 @@ impl<'parent, 'lookup> Set<'parent, 'lookup> {
                                 log::warn!("undefined macro {}", name);
                                 Ok(String::new())
                             },
-                            |(_, macro_value)| self.expand(macro_value),
+                            |x| self.expand(&x.text),
                         )?
                     };
                     let macro_value = match replacement {
@@ -155,11 +181,23 @@ impl<'parent, 'lookup> Set<'parent, 'lookup> {
     pub fn origin(&self, name: &str) -> &'static str {
         match self.data.get(name) {
             None => self.parent.map_or("undefined", |p| p.origin(name)),
-            Some((Source::Builtin, _)) => "default",
-            Some((Source::Environment, _)) => "environment",
+            Some(Macro {
+                source: Source::Builtin,
+                ..
+            }) => "default",
+            Some(Macro {
+                source: Source::Environment,
+                ..
+            }) => "environment",
             // TODO figure out when to return "environment override"
-            Some((Source::File, _)) => "file",
-            Some((Source::CommandLineOrMakeflags, _)) => "command line",
+            Some(Macro {
+                source: Source::File,
+                ..
+            }) => "file",
+            Some(Macro {
+                source: Source::CommandLineOrMakeflags,
+                ..
+            }) => "command line",
             // TODO handle override and automatic
         }
     }
@@ -190,7 +228,7 @@ impl fmt::Display for Set<'_, '_> {
         let pieces = self
             .data
             .iter()
-            .map(|(k, (_, v))| format!("{}={}", k, v))
+            .map(|(k, x)| format!("{}={}", k, &x.text))
             .collect::<Vec<_>>();
         write!(f, "{}", pieces.join("\n"))
     }
@@ -259,8 +297,12 @@ mod test {
         let mut macros = Set::new();
         macros.set(
             "oof".to_owned(),
-            Source::File,
-            TokenString::text("bruh; swag; yeet;"),
+            Macro {
+                source: Source::File,
+                text: TokenString::text("bruh; swag; yeet;"),
+                #[cfg(feature = "full")]
+                eagerly_expanded: false,
+            },
         );
         assert_eq!(
             macros.expand(&"$(oof:;=?)".parse().unwrap())?,
diff --git a/src/makefile/mod.rs b/src/makefile/mod.rs
index 308d25d..aab5054 100644
--- a/src/makefile/mod.rs
+++ b/src/makefile/mod.rs
@@ -25,7 +25,7 @@ use command_line::CommandLine;
 use inference_rules::InferenceRule;
 use input::FinishedMakefileReader;
 pub use input::MakefileReader;
-use r#macro::{Set as MacroSet, Source as MacroSource};
+use r#macro::{Macro, Set as MacroSet, Source as MacroSource};
 use target::Target;
 use token::TokenString;
 
@@ -62,8 +62,12 @@ impl<'a> Makefile<'a> {
             if let [name, value] = *r#macro.splitn(2, '=').collect::<Vec<_>>() {
                 macros.set(
                     name.into(),
-                    MacroSource::CommandLineOrMakeflags,
-                    TokenString::text(value),
+                    Macro {
+                        source: MacroSource::CommandLineOrMakeflags,
+                        text: TokenString::text(value),
+                        #[cfg(feature = "full")]
+                        eagerly_expanded: false,
+                    },
                 );
             }
         }
@@ -73,15 +77,23 @@ impl<'a> Makefile<'a> {
             let make_cmd_goals = args.targets().collect::<Vec<_>>();
             macros.set(
                 "MAKECMDGOALS".to_owned(),
-                MacroSource::Builtin,
-                TokenString::text(make_cmd_goals.join(" ")),
+                Macro {
+                    source: MacroSource::Builtin,
+                    text: TokenString::text(make_cmd_goals.join(" ")),
+                    #[cfg(feature = "full")]
+                    eagerly_expanded: false,
+                },
             );
 
             if let Ok(curdir) = env::current_dir() {
                 macros.set(
                     "CURDIR".to_owned(),
-                    MacroSource::Builtin,
-                    TokenString::text(curdir.to_string_lossy()),
+                    Macro {
+                        source: MacroSource::Builtin,
+                        text: TokenString::text(curdir.to_string_lossy()),
+                        #[cfg(feature = "full")]
+                        eagerly_expanded: false,
+                    },
                 );
             }
         }
@@ -135,8 +147,8 @@ impl<'a> Makefile<'a> {
         let follow_gnu = cfg!(feature = "full");
 
         let vpath_options = match self.macros.get("VPATH") {
-            Some((_, vpath)) if follow_gnu => {
-                let vpath = self.expand_macros(vpath, None)?;
+            Some(Macro { text, .. }) if follow_gnu => {
+                let vpath = self.expand_macros(text, None)?;
                 env::split_paths(&vpath).collect()
             }
             _ => vec![],