give targets an encapsulated set like macros have

3 files changed, 141 insertions, 63 deletions

diff --git a/src/makefile/input.rs b/src/makefile/input.rs
index 0e0506a..e2add0c 100644
--- a/src/makefile/input.rs
+++ b/src/makefile/input.rs
@@ -19,7 +19,7 @@ use super::inference_rules::InferenceRule;
 #[cfg(feature = "full")]
 use super::r#macro::ExportConfig;
 use super::r#macro::{Macro, Set as MacroSet};
-use super::target::Target;
+use super::target::{StaticTargetSet, Target};
 use super::token::{tokenize, Token, TokenString};
 use super::ItemSource;
 
@@ -166,7 +166,7 @@ pub struct MakefileReader<'a, 'parent, R: BufRead> {
     file_name: String,
     pub inference_rules: Vec<InferenceRule>,
     pub macros: MacroSet<'parent, 'static>,
-    pub targets: HashMap<String, Target>,
+    pub targets: StaticTargetSet,
     built_in_targets: HashMap<String, Target>,
     pub first_non_special_target: Option<String>,
     pub failed_includes: Vec<String>,
@@ -222,7 +222,7 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
             file_name: name.clone(),
             inference_rules: Vec::new(),
             macros,
-            targets: HashMap::new(),
+            targets: Default::default(),
             built_in_targets: HashMap::new(),
             first_non_special_target: None,
             failed_includes: Vec::new(),
@@ -675,7 +675,7 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
                                 .and_then(|x| x.get(1).map(|x| x.as_str().to_owned())),
                             already_updated: Cell::new(false),
                         };
-                        self.targets.insert(real_target.to_owned(), new_target);
+                        self.targets.put(new_target);
                     }
                 }
             } else {
@@ -728,34 +728,26 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
             });
             self.inference_rules.push(new_rule);
         } else {
+            log::trace!(
+                "{}:{}: new target {:?} based on {:?}",
+                &self.file_name,
+                line_number,
+                &targets,
+                &prerequisites
+            );
             for target in targets {
                 if self.first_non_special_target.is_none() && !target.starts_with('.') {
                     self.first_non_special_target = Some(target.into());
                 }
                 // TODO handle appending to built-in (it's Complicated)
-                match self.targets.get_mut(target) {
-                    Some(old_target)
-                        if commands.is_empty()
-                            && !(target == ".SUFFIXES" && prerequisites.is_empty()) =>
-                    {
-                        let new_prerequisites = prerequisites
-                            .iter()
-                            .filter(|x| !old_target.prerequisites.contains(x))
-                            .cloned()
-                            .collect::<Vec<_>>();
-                        old_target.prerequisites.extend(new_prerequisites);
-                    }
-                    _ => {
-                        let new_target = Target {
-                            name: target.into(),
-                            prerequisites: prerequisites.clone(),
-                            commands: commands.clone(),
-                            stem: None,
-                            already_updated: Cell::new(false),
-                        };
-                        self.targets.insert(target.into(), new_target);
-                    }
-                }
+                let new_target = Target {
+                    name: target.into(),
+                    prerequisites: prerequisites.clone(),
+                    commands: commands.clone(),
+                    stem: None,
+                    already_updated: Cell::new(false),
+                };
+                self.targets.put(new_target);
             }
         }
 
@@ -886,7 +878,7 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
             macros: self.macros.data,
             #[cfg(feature = "full")]
             macro_exports: self.macros.exported,
-            targets: self.targets,
+            targets: self.targets.into(),
             first_non_special_target: self.first_non_special_target,
             failed_includes: self.failed_includes,
         }
@@ -899,7 +891,9 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> {
             #[cfg(feature = "full")]
             new.macro_exports,
         );
-        self.targets.extend(new.targets);
+        for (_, target) in new.targets {
+            self.targets.put(target);
+        }
         if self.first_non_special_target.is_none() {
             self.first_non_special_target = new.first_non_special_target;
         }
@@ -976,6 +970,7 @@ endif
         ";
         let args = Args::empty();
         let makefile = MakefileReader::read(&args, MacroSet::new(), Cursor::new(file), "")?;
+        let makefile = makefile.finish();
         assert_eq!(makefile.targets["a"].commands.len(), 1);
         Ok(())
     }
@@ -1055,6 +1050,7 @@ clean:
 
         let args = Args::empty();
         let makefile = MakefileReader::read(&args, MacroSet::new(), Cursor::new(file), "")?;
+        let makefile = makefile.finish();
         assert!(makefile.targets.contains_key("server"));
         Ok(())
     }
@@ -1135,4 +1131,22 @@ cursed:
         assert_eq!(makefile.inference_rules.len(), 2);
         Ok(())
     }
+
+    #[test]
+    fn dependency_prepending_appending() -> R {
+        let file = "
+test: a
+test: b
+\techo hi
+test: c
+        ";
+        let args = Args::empty();
+        let makefile = MakefileReader::read(&args, MacroSet::new(), Cursor::new(file), "")?;
+        let makefile = makefile.finish();
+        assert_eq!(
+            makefile.targets["test"].prerequisites,
+            vec!["a".to_owned(), "b".to_owned(), "c".to_owned()]
+        );
+        Ok(())
+    }
 }
diff --git a/src/makefile/mod.rs b/src/makefile/mod.rs
index 1d4873e..5c38ab3 100644
--- a/src/makefile/mod.rs
+++ b/src/makefile/mod.rs
@@ -1,5 +1,4 @@
 use std::cell::{Cell, RefCell};
-use std::collections::HashMap;
 use std::env;
 use std::fmt;
 use std::path::{Path, PathBuf};
@@ -12,7 +11,7 @@ use inference_rules::InferenceRule;
 use input::FinishedMakefileReader;
 pub use input::MakefileReader;
 use r#macro::{Macro, Set as MacroSet};
-use target::Target;
+use target::{DynamicTargetSet, Target};
 use token::TokenString;
 
 use crate::args::Args;
@@ -42,7 +41,7 @@ pub struct Makefile<'a> {
     inference_rules: Vec<InferenceRule>,
     builtin_inference_rules: Vec<InferenceRule>,
     pub macros: MacroSet<'static, 'static>,
-    targets: RefCell<HashMap<String, Rc<RefCell<Target>>>>,
+    targets: RefCell<DynamicTargetSet>,
     pub first_non_special_target: Option<String>,
     args: &'a Args,
     // TODO borrow warnings from Python version
@@ -52,7 +51,7 @@ impl<'a> Makefile<'a> {
     pub fn new(args: &'a Args) -> Self {
         let mut inference_rules = vec![];
         let mut macros = MacroSet::new();
-        let mut targets = HashMap::new();
+        let mut targets = DynamicTargetSet::default();
         let first_non_special_target = None;
 
         macros.set(
@@ -70,11 +69,9 @@ impl<'a> Makefile<'a> {
         if !args.no_builtin_rules {
             inference_rules.extend(builtin_inference_rules());
             macros.add_builtins();
-            targets.extend(
-                builtin_targets()
-                    .into_iter()
-                    .map(|t| (t.name.clone(), Rc::new(RefCell::new(t)))),
-            );
+            for target in builtin_targets() {
+                targets.put(target);
+            }
         }
 
         macros.add_env();
@@ -136,11 +133,9 @@ impl<'a> Makefile<'a> {
             #[cfg(feature = "full")]
             new.macro_exports,
         );
-        self.targets.borrow_mut().extend(
-            new.targets
-                .into_iter()
-                .map(|(k, v)| (k, Rc::new(RefCell::new(v)))),
-        );
+        for (_, target) in new.targets {
+            self.targets.borrow_mut().put(target);
+        }
         if self.first_non_special_target.is_none() {
             self.first_non_special_target = new.first_non_special_target;
         }
@@ -216,7 +211,7 @@ impl<'a> Makefile<'a> {
                         // we can't build this based on itself! fuck outta here
                         return None;
                     }
-                    if self.targets.borrow().contains_key(&prereq_path_name) {
+                    if self.targets.borrow().has(&prereq_path_name) {
                         return Some(prereq_path_name);
                     }
                     let prereq_path = PathBuf::from(&prereq_path_name);
@@ -239,7 +234,7 @@ impl<'a> Makefile<'a> {
                             self.infer_target(&prereq_path_name, banned_rules, banned_names)
                                 .ok()
                                 .and_then(|_| {
-                                    if self.targets.borrow().contains_key(&prereq_path_name) {
+                                    if self.targets.borrow().has(&prereq_path_name) {
                                         Some(prereq_path_name)
                                     } else {
                                         None
@@ -264,9 +259,7 @@ impl<'a> Makefile<'a> {
         }
 
         if let Some(new_target) = new_target {
-            self.targets
-                .borrow_mut()
-                .insert(new_target.name.clone(), Rc::new(RefCell::new(new_target)));
+            self.targets.borrow_mut().put(new_target);
         }
 
         Ok(())
@@ -284,14 +277,14 @@ impl<'a> Makefile<'a> {
         } else {
             false
         };
-        if !self.targets.borrow().contains_key(name) || exists_but_infer_anyway {
+        if !self.targets.borrow().has(name) || exists_but_infer_anyway {
             log::trace!("trying to infer for {}", name);
             self.infer_target(name, vec![], vec![])?;
         }
 
         let mut new_target = None;
         let targets = self.targets.borrow();
-        if !targets.contains_key(name) {
+        if !targets.has(name) {
             // well, inference didn't work. is there a default?
             if let Some(default) = targets.get(".DEFAULT") {
                 let commands = default.borrow().commands.clone();
@@ -318,17 +311,13 @@ impl<'a> Makefile<'a> {
 
         drop(targets);
         if let Some(new_target) = new_target {
-            self.targets
-                .borrow_mut()
-                .insert(new_target.name.clone(), Rc::new(RefCell::new(new_target)));
+            self.targets.borrow_mut().put(new_target);
         }
 
         let targets = self.targets.borrow();
-        Ok(Rc::clone(
-            targets
-                .get(name)
-                .ok_or_else(|| eyre!("Target {:?} not found!", name))?,
-        ))
+        Ok(targets
+            .get(name)
+            .ok_or_else(|| eyre!("Target {:?} not found!", name))?)
     }
 
     pub fn update_target(&self, name: &str) -> Result<()> {
@@ -427,9 +416,7 @@ impl fmt::Display for Makefile<'_> {
         writeln!(f)?;
 
         header(f, "Targets")?;
-        for target in self.targets.borrow().values() {
-            writeln!(f, "{}", target.borrow())?;
-        }
+        writeln!(f, "{}", &self.targets.borrow())?;
 
         Ok(())
     }
diff --git a/src/makefile/target.rs b/src/makefile/target.rs
index 506bde8..5de280d 100644
--- a/src/makefile/target.rs
+++ b/src/makefile/target.rs
@@ -1,6 +1,8 @@
-use std::cell::Cell;
+use std::cell::{Cell, RefCell};
+use std::collections::HashMap;
 use std::fmt;
 use std::fs::metadata;
+use std::rc::Rc;
 use std::time::SystemTime;
 
 use eyre::{Result, WrapErr};
@@ -19,6 +21,17 @@ pub struct Target {
 }
 
 impl Target {
+    pub fn extend(&mut self, other: Target) {
+        assert_eq!(&self.name, &other.name);
+        self.prerequisites.extend(other.prerequisites);
+        assert!(self.commands.is_empty() || other.commands.is_empty());
+        self.commands.extend(other.commands);
+        assert!(self.stem.is_none() || other.stem.is_none());
+        self.stem = self.stem.take().or(other.stem);
+        let already_updated = self.already_updated.get() || other.already_updated.get();
+        self.already_updated.set(already_updated);
+    }
+
     fn modified_time(&self) -> Option<SystemTime> {
         metadata(&self.name)
             .and_then(|metadata| metadata.modified())
@@ -55,7 +68,7 @@ impl Target {
                 .unwrap_or(false)
         });
         log::trace!(
-            "{:>50} exists: {}, newer than dependencies: {}",
+            "{:} exists: {}, newer than dependencies: {}",
             self.name,
             exists,
             newer_than_all_dependencies
@@ -64,6 +77,7 @@ impl Target {
     }
 
     pub fn update(&self, file: &Makefile) -> Result<()> {
+        log::info!("{}: {:?}", &self.name, &self.prerequisites);
         for prereq in &self.prerequisites {
             file.update_target(prereq)
                 .wrap_err_with(|| format!("as a dependency for target {}", self.name))?;
@@ -101,3 +115,66 @@ impl fmt::Display for Target {
         Ok(())
     }
 }
+
+// for targets that won't change out from under us (don't need refcelling)
+#[derive(Clone, Default)]
+pub struct StaticTargetSet {
+    data: HashMap<String, Target>,
+}
+
+impl StaticTargetSet {
+    pub fn get(&self, name: &str) -> Option<&Target> {
+        self.data.get(name)
+    }
+
+    pub fn put(&mut self, target: Target) {
+        if target.name == ".SUFFIXES" && target.prerequisites.is_empty() {
+            self.data.remove(&target.name);
+        }
+        if let Some(existing_target) = self.data.get_mut(&target.name) {
+            existing_target.extend(target);
+        } else {
+            self.data.insert(target.name.clone(), target);
+        }
+    }
+}
+
+impl Into<HashMap<String, Target>> for StaticTargetSet {
+    fn into(self) -> HashMap<String, Target> {
+        self.data
+    }
+}
+
+// for targets that might become updated and so need refcelling
+#[derive(Clone, Default)]
+pub struct DynamicTargetSet {
+    data: HashMap<String, Rc<RefCell<Target>>>,
+}
+
+impl DynamicTargetSet {
+    pub fn get(&self, name: &str) -> Option<Rc<RefCell<Target>>> {
+        self.data.get(name).map(|x| Rc::clone(x))
+    }
+
+    pub fn put(&mut self, target: Target) {
+        if let Some(existing_target) = self.data.get_mut(&target.name) {
+            existing_target.borrow_mut().extend(target);
+        } else {
+            self.data
+                .insert(target.name.clone(), Rc::new(RefCell::new(target)));
+        }
+    }
+
+    pub fn has(&self, name: &str) -> bool {
+        self.data.contains_key(name)
+    }
+}
+
+impl fmt::Display for DynamicTargetSet {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        for target in self.data.values() {
+            writeln!(f, "{}", target.borrow())?;
+        }
+        Ok(())
+    }
+}