Merge pull request #826 from vsbogd/no-module-duplication

Don't copy imported module to filter out its dependencies

Author: vsbogd

c/src/space.rs

@@ -7,6 +7,7 @@ use hyperon::matcher::*;
 use crate::atom::*;
 
 use std::os::raw::*;
+use std::borrow::Cow;
 
 // =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
 // Space Client Interface
@@ -238,14 +239,9 @@ pub extern "C" fn space_atom_count(space: *const space_t) -> isize {
 pub extern "C" fn space_iterate(space: *const space_t,
         callback: c_atom_callback_t, context: *mut c_void) -> bool {
     let dyn_space = unsafe{ &*space }.borrow();
-    match dyn_space.borrow().atom_iter() {
-        Some(atom_iter) => {
-            for atom in atom_iter {
-                callback(atom.into(), context);
-            }
-            true
-        },
-        None => false
+    match dyn_space.visit(&mut |atom: Cow<Atom>| callback(atom.as_ref().into(), context)) {
+        Ok(()) => true,
+        Err(()) => false,
     }
 }
 
@@ -736,46 +732,22 @@ impl Space for CSpace {
             None
         }
     }
-    fn atom_iter(&self) -> Option<SpaceIter> {
-        struct CSpaceIterator<'a>(&'a CSpace, *mut c_void);
-        impl<'a> Iterator for CSpaceIterator<'a> {
-            type Item = &'a Atom;
-            fn next(&mut self) -> Option<&'a Atom> {
-                let api = unsafe{ &*self.0.api };
-                if let Some(next_atom) = api.next_atom {
-                    let atom_ref = next_atom(&self.0.params, self.1);
+    fn visit(&self, v: &mut dyn SpaceVisitor) -> Result<(), ()> {
+        let api = unsafe{ &*self.api };
+        match (api.new_atom_iterator_state, api.next_atom) {
+            (Some(new_atom_iterator_state), Some(next_atom)) => {
+                let ctx = new_atom_iterator_state(&self.params);
+                loop {
+                    let atom_ref = next_atom(&self.params, ctx);
                     if atom_ref.is_null() {
-                        None
+                        break;
                     } else {
-                        Some(atom_ref.into_ref())
+                        v.accept(Cow::Borrowed(atom_ref.into_ref()));
                     }
-                } else {
-                    panic!("next_atom function must be implemented if new_atom_iterator_state is implemented");
-                }
-            }
-        }
-        impl Drop for CSpaceIterator<'_> {
-            fn drop(&mut self) {
-                let api = unsafe{ &*self.0.api };
-                if let Some(free_atom_iterator_state) = api.free_atom_iterator_state {
-                    free_atom_iterator_state(&self.0.params, self.1);
-                } else {
-                    panic!("free_atom_iterator_state function must be implemented if new_atom_iterator_state is implemented");
                 }
-            }
-        }
-
-        let api = unsafe{ &*self.api };
-        if let Some(new_atom_iterator_state) = api.new_atom_iterator_state {
-            let ctx = new_atom_iterator_state(&self.params);
-            if ctx.is_null() {
-                None
-            } else {
-                let new_iter = CSpaceIterator(self, ctx);
-                Some(SpaceIter::new(new_iter))
-            }
-        } else {
-            None
+                Ok(())
+            },
+            _ => Err(()),
         }
     }
     fn as_any(&self) -> Option<&dyn std::any::Any> {
@@ -803,6 +775,7 @@ struct DefaultSpace<'a>(&'a CSpace);
 impl Space for DefaultSpace<'_> {
     fn common(&self) -> FlexRef<SpaceCommon> { self.0.common() }
     fn query(&self, query: &Atom) -> BindingsSet { self.0.query(query) }
+    fn visit(&self, v: &mut dyn SpaceVisitor) -> Result<(), ()> { self.0.visit(v) }
     fn as_any(&self) -> Option<&dyn std::any::Any> { Some(self.0) }
     fn as_any_mut(&mut self) -> Option<&mut dyn std::any::Any> { None }
 }
@@ -827,7 +800,7 @@ impl SpaceMut for CSpace {
         let from: atom_ref_t = from.into();
         (api.replace)(&self.params, &from, to.into())
     }
-    fn as_space(&self) -> &dyn Space {
+    fn as_space<'a>(&self) -> &(dyn Space + 'a) {
         self
     }
 }

lib/src/metta/runner/modules/mod.rs

@@ -4,6 +4,7 @@ use std::cell::RefCell;
 
 use crate::metta::*;
 use crate::metta::runner::*;
+use crate::space::module::ModuleSpace;
 
 use regex::Regex;
 
@@ -56,7 +57,7 @@ impl ModId {
 pub struct MettaMod {
     mod_path: String,
     resource_dir: Option<PathBuf>,
-    space: DynSpace,
+    space: Rc<RefCell<ModuleSpace>>,
     tokenizer: Shared<Tokenizer>,
     imported_deps: Mutex<HashMap<ModId, DynSpace>>,
     loader: Option<Box<dyn ModuleLoader>>,
@@ -75,6 +76,7 @@ impl MettaMod {
                 }
             }
         }
+        let space = Rc::new(RefCell::new(ModuleSpace::new(space)));
 
         let new_mod = Self {
             mod_path,
@@ -86,8 +88,8 @@ impl MettaMod {
         };
 
         // Load the base tokens for the module's new Tokenizer
-        register_runner_tokens(&mut *new_mod.tokenizer().borrow_mut(), new_mod.tokenizer().clone(), &new_mod.space, metta);
-        register_common_tokens(&mut *new_mod.tokenizer().borrow_mut(), new_mod.tokenizer().clone(), &new_mod.space, metta);
+        register_runner_tokens(&mut *new_mod.tokenizer().borrow_mut(), new_mod.tokenizer().clone(), &DynSpace::with_rc(new_mod.space.clone()), metta);
+        register_common_tokens(&mut *new_mod.tokenizer().borrow_mut(), new_mod.tokenizer().clone(), &DynSpace::with_rc(new_mod.space.clone()), metta);
 
         //Load the stdlib unless this module is no_std
         if !no_stdlib {
@@ -192,7 +194,7 @@ impl MettaMod {
         let (dep_space, transitive_deps) = mod_ptr.stripped_space();
 
         // Add a new Grounded Space atom to the &self space, so we can access the dependent module
-        self.insert_dep(mod_id, dep_space)?;
+        self.insert_dep(mod_id, DynSpace::with_rc(dep_space.clone()))?;
 
         // Add all the transitive deps from the dependency
         if let Some(transitive_deps) = transitive_deps {
@@ -228,7 +230,7 @@ impl MettaMod {
     fn insert_dep(&self, mod_id: ModId, dep_space: DynSpace) -> Result<(), String> {
         let mut deps_table = self.imported_deps.lock().unwrap();
         if !deps_table.contains_key(&mod_id) {
-            self.add_atom(Atom::gnd(dep_space.clone()), false).map_err(|a| a.to_string())?;
+            self.space.borrow_mut().add_dep(dep_space.clone());
             deps_table.insert(mod_id, dep_space);
         }
         Ok(())
@@ -251,39 +253,9 @@ impl MettaMod {
 
     /// Private function that returns a deep copy of a module's space, with the module's dependency
     /// sub-spaces stripped out and returned separately
-    //
-    // HACK.  This is a terrible design.  It is a stop-gap to get around problems caused by transitive
-    // imports described above, but it has some serious downsides, To name a few:
-    //  - It means we must copy every atom in the space for every import
-    //  - It only works when the dep module's space is a GroundingSpace
-    fn stripped_space(&self) -> (DynSpace, Option<HashMap<ModId, DynSpace>>) {
+    fn stripped_space(&self) -> (Rc<RefCell<ModuleSpace>>, Option<HashMap<ModId, DynSpace>>) {
         let deps_table = self.imported_deps.lock().unwrap();
-        if deps_table.len() == 0 {
-            (self.space.clone(), None)
-        } else {
-            if let Some(any_space) = self.space.borrow().as_any() {
-                if let Some(dep_g_space) = any_space.downcast_ref::<GroundingSpace>() {
-
-                    // Do a deep-clone of the dep-space atom-by-atom, because `space.remove()` doesn't recognize
-                    // two GroundedAtoms wrapping DynSpaces as being the same, even if the underlying space is
-                    let mut new_space = GroundingSpace::new();
-                    new_space.set_name(self.path().to_string());
-                    for atom in dep_g_space.atom_iter().unwrap() {
-                        if let Some(sub_space) = atom.as_gnd::<DynSpace>() {
-                            if !deps_table.values().any(|space| space == sub_space) {
-                                new_space.add(atom.clone());
-                            }
-                        } else {
-                            new_space.add(atom.clone());
-                        }
-                    }
-
-                    return (DynSpace::new(new_space), Some(deps_table.clone()));
-                }
-            }
-            log::warn!("import_all_from_dependency: Importing from module based on a non-GroundingSpace is currently unsupported");
-            (self.space.clone(), None)
-        }
+        (self.space.clone(), Some(deps_table.clone()))
     }
 
     /// Returns the full path of a loaded module.  For example: "top:parent_mod:this_mod"
@@ -302,8 +274,8 @@ impl MettaMod {
         self.loader.as_ref().and_then(|loader| loader.pkg_info())
     }
 
-    pub fn space(&self) -> &DynSpace {
-        &self.space
+    pub fn space(&self) -> DynSpace {
+        DynSpace::with_rc(self.space.clone())
     }
 
     pub fn tokenizer(&self) -> &Shared<Tokenizer> {

lib/src/metta/runner/stdlib/module.rs

@@ -90,7 +90,7 @@ impl CustomExecute for ImportOp {
             }
             other_atom => {
                 match &other_atom {
-                    Atom::Grounded(_) if Atom::as_gnd::<DynSpace>(other_atom) == Some(context.module().space()) => {
+                    Atom::Grounded(_) if Atom::as_gnd::<DynSpace>(other_atom) == Some(&context.module().space()) => {
                         context.import_all_from_dependency(mod_id)?;
                     },
                     _ => {

lib/src/metta/runner/stdlib/space.rs

@@ -160,9 +160,10 @@ impl CustomExecute for GetAtomsOp {
         let arg_error = || ExecError::from("get-atoms expects one argument: space");
         let space = args.get(0).ok_or_else(arg_error)?;
         let space = Atom::as_gnd::<DynSpace>(space).ok_or("get-atoms expects a space as its argument")?;
-        space.borrow().as_space().atom_iter()
-            .map(|iter| iter.cloned().map(|a| make_variables_unique(a)).collect())
-            .ok_or(ExecError::Runtime("Unsupported Operation. Can't traverse atoms in this space".to_string()))
+        let mut result = Vec::new();
+        space.borrow().as_space().visit(&mut |atom: std::borrow::Cow<Atom>| {
+            result.push(make_variables_unique(atom.into_owned()))
+        }).map_or(Err(ExecError::Runtime("Unsupported Operation. Can't traverse atoms in this space".to_string())), |_| Ok(result))
     }
 }
 
@@ -259,6 +260,13 @@ mod tests {
         assert_eq!(result[2], vec![Atom::gnd(stdlib_space)]);
     }
 
+    fn collect_atoms(space: &dyn Space) -> Vec<Atom> {
+        let mut atoms = Vec::new();
+        space.visit(&mut |atom: std::borrow::Cow<Atom>| atoms.push(atom.into_owned()))
+            .expect("Space::visit is not implemented");
+        atoms
+    }
+
     #[test]
     fn remove_atom_op() {
         let space = DynSpace::new(metta_space("
@@ -269,7 +277,7 @@ mod tests {
         let res = RemoveAtomOp{}.execute(&mut vec![satom, expr!(("foo" "bar"))]).expect("No result returned");
         // REM: can return Bool in future
         assert_eq!(res, vec![UNIT_ATOM]);
-        let space_atoms: Vec<Atom> = space.borrow().as_space().atom_iter().unwrap().cloned().collect();
+        let space_atoms = collect_atoms(space.borrow().as_space());
         assert_eq_no_order!(space_atoms, vec![expr!(("bar" "foo"))]);
     }
 
@@ -281,7 +289,7 @@ mod tests {
         "));
         let satom = Atom::gnd(space.clone());
         let res = GetAtomsOp{}.execute(&mut vec![satom]).expect("No result returned");
-        let space_atoms: Vec<Atom> = space.borrow().as_space().atom_iter().unwrap().cloned().collect();
+        let space_atoms = collect_atoms(space.borrow().as_space());
         assert_eq_no_order!(res, space_atoms);
         assert_eq_no_order!(res, vec![expr!(("foo" "bar")), expr!(("bar" "foo"))]);
     }
@@ -291,7 +299,7 @@ mod tests {
         let res = NewSpaceOp{}.execute(&mut vec![]).expect("No result returned");
         let space = res.get(0).expect("Result is empty");
         let space = space.as_gnd::<DynSpace>().expect("Result is not space");
-        let space_atoms: Vec<Atom> = space.borrow().as_space().atom_iter().unwrap().cloned().collect();
+        let space_atoms = collect_atoms(space.borrow().as_space());
         assert_eq_no_order!(space_atoms, Vec::<Atom>::new());
     }
 
@@ -301,7 +309,7 @@ mod tests {
         let satom = Atom::gnd(space.clone());
         let res = AddAtomOp{}.execute(&mut vec![satom, expr!(("foo" "bar"))]).expect("No result returned");
         assert_eq!(res, vec![UNIT_ATOM]);
-        let space_atoms: Vec<Atom> = space.borrow().as_space().atom_iter().unwrap().cloned().collect();
+        let space_atoms = collect_atoms(space.borrow().as_space());
         assert_eq_no_order!(space_atoms, vec![expr!(("foo" "bar"))]);
     }
 

lib/src/space/grounding.rs

@@ -291,8 +291,8 @@ impl GroundingSpace {
     }
 
     /// Returns the iterator over content of the space.
-    pub fn iter(&self) -> SpaceIter {
-        SpaceIter::new(GroundingSpaceIter::new(self))
+    fn iter(&self) -> GroundingSpaceIter<'_> {
+        GroundingSpaceIter::new(self)
     }
 
     /// Sets the name property for the `GroundingSpace` which can be useful for debugging
@@ -316,8 +316,8 @@ impl Space for GroundingSpace {
     fn atom_count(&self) -> Option<usize> {
         Some(self.iter().count())
     }
-    fn atom_iter(&self) -> Option<SpaceIter> {
-        Some(self.iter())
+    fn visit(&self, v: &mut dyn SpaceVisitor) -> Result<(), ()> {
+        Ok(self.iter().for_each(|atom| v.accept(Cow::Borrowed(atom))))
     }
     fn as_any(&self) -> Option<&dyn std::any::Any> {
         Some(self)
@@ -337,7 +337,7 @@ impl SpaceMut for GroundingSpace {
     fn replace(&mut self, from: &Atom, to: Atom) -> bool {
         GroundingSpace::replace(self, from, to)
     }
-    fn as_space(&self) -> &dyn Space {
+    fn as_space<'a>(&self) -> &(dyn Space + 'a) {
         self
     }
 }