Don't copy imported module to filter out its dependencies

c/src/space.rs

@@ -736,7 +736,7 @@ impl Space for CSpace {
             None
         }
     }
-    fn atom_iter(&self) -> Option<SpaceIter> {
+    fn atom_iter(&self) -> Option<Box<dyn Iterator<Item=&Atom> + '_>> {
         struct CSpaceIterator<'a>(&'a CSpace, *mut c_void);
         impl<'a> Iterator for CSpaceIterator<'a> {
             type Item = &'a Atom;
@@ -772,7 +772,7 @@ impl Space for CSpace {
                 None
             } else {
                 let new_iter = CSpaceIterator(self, ctx);
-                Some(SpaceIter::new(new_iter))
+                Some(Box::new(new_iter))
             }
         } else {
             None
@@ -827,7 +827,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;
 
@@ -59,7 +60,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>>,
@@ -78,6 +79,7 @@ impl MettaMod {
                 }
             }
         }
+        let space = Rc::new(RefCell::new(ModuleSpace::new(space)));
 
         let new_mod = Self {
             mod_path,
@@ -89,8 +91,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 {
@@ -195,7 +197,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 {
@@ -231,7 +233,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(())
@@ -254,39 +256,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"
@@ -305,8 +277,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.rs

@@ -140,7 +140,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/space/grounding.rs

@@ -290,8 +290,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
@@ -315,8 +315,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 atom_iter(&self) -> Option<Box<dyn Iterator<Item=&Atom> + '_>> {
+        Some(Box::new(self.iter()))
     }
     fn as_any(&self) -> Option<&dyn std::any::Any> {
         Some(self)
@@ -336,7 +336,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
     }
 }

lib/src/space/mod.rs

@@ -2,10 +2,12 @@
 //! This module is intended to keep different space implementations.
 
 pub mod grounding;
+pub mod module;
 
 use std::fmt::Display;
 use std::rc::{Rc, Weak};
 use std::cell::{RefCell, Ref, RefMut};
+use std::ops::Deref;
 
 use crate::common::FlexRef;
 use crate::atom::*;
@@ -87,25 +89,6 @@ impl<T: SpaceObserver> From<Rc<RefCell<T>>> for SpaceObserverRef<T> {
     }
 }
 
-/// Space iterator.
-pub struct SpaceIter<'a> {
-    iter: Box<dyn Iterator<Item=&'a Atom> + 'a>
-}
-
-impl<'a> SpaceIter<'a> {
-    pub fn new<I: Iterator<Item=&'a Atom> + 'a>(iter: I) -> Self {
-        Self{ iter: Box::new(iter) }
-    }
-}
-
-impl<'a> Iterator for SpaceIter<'a> {
-    type Item = &'a Atom;
-
-    fn next(&mut self) -> Option<&'a Atom> {
-        self.iter.next()
-    }
-}
-
 /// A common object that needs to be maintained by all objects implementing the Space trait
 #[derive(Default)]
 pub struct SpaceCommon {
@@ -203,7 +186,7 @@ pub trait Space: std::fmt::Debug + std::fmt::Display {
     }
 
     /// Returns an iterator over every atom in the space, or None if that is not possible
-    fn atom_iter(&self) -> Option<SpaceIter> {
+    fn atom_iter(&self) -> Option<Box<dyn Iterator<Item=&Atom> + '_>> {
         None
     }
 
@@ -275,13 +258,16 @@ pub trait SpaceMut: Space {
 
     /// Turn a &dyn SpaceMut into an &dyn Space.  Obsolete when Trait Upcasting is stabilized.
     /// [Rust issue #65991](https://github.com/rust-lang/rust/issues/65991)  Any month now.
-    fn as_space(&self) -> &dyn Space;
+    fn as_space<'a>(&self) -> &(dyn Space + 'a);
 }
 
 #[derive(Clone)]
 pub struct DynSpace(Rc<RefCell<dyn SpaceMut>>);
 
 impl DynSpace {
+    pub fn with_rc(space: Rc<RefCell<dyn SpaceMut>>) -> Self {
+        Self(space)
+    }
     pub fn new<T: SpaceMut + 'static>(space: T) -> Self {
         let shared = Rc::new(RefCell::new(space));
         DynSpace(shared)
@@ -320,7 +306,7 @@ impl SpaceMut for DynSpace {
     fn replace(&mut self, from: &Atom, to: Atom) -> bool {
         self.0.borrow_mut().replace(from, to)
     }
-    fn as_space(&self) -> &dyn Space {
+    fn as_space<'a>(&self) -> &(dyn Space + 'a) {
         self
     }
 }
@@ -338,8 +324,8 @@ impl Space for DynSpace {
     fn atom_count(&self) -> Option<usize> {
         self.0.borrow().atom_count()
     }
-    fn atom_iter(&self) -> Option<SpaceIter> {
-        None
+    fn atom_iter(&self) -> Option<Box<dyn Iterator<Item=&Atom> + '_>> {
+        DynSpaceIter::new(self)
     }
     fn as_any(&self) -> Option<&dyn std::any::Any> {
         None
@@ -349,6 +335,37 @@ impl Space for DynSpace {
     }
 }
 
+struct DynSpaceIter<'a> {
+    space_ref: Ref<'a, dyn Space>,
+    space_iter: Option<Box<dyn Iterator<Item=&'a Atom> + 'a>>,
+}
+
+impl<'a> DynSpaceIter<'a> {
+    fn new(space: &'a DynSpace) -> Option<Box<dyn Iterator<Item=&Atom> + 'a>> {
+        let space_ref = Ref::map(space.borrow(), SpaceMut::as_space);
+        let mut iter = Self{
+            space_ref,
+            space_iter: None,
+        };
+        let space_ptr = iter.space_ref.deref() as *const dyn Space;
+        match unsafe{ (*space_ptr).atom_iter() } {
+            Some(it) => {
+                iter.space_iter = Some(it);
+                Some(Box::new(iter))
+            },
+            None => None,
+        }
+    }
+}
+
+impl<'a> Iterator for DynSpaceIter<'a> {
+    type Item=&'a Atom;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.space_iter.as_mut().unwrap().next()
+    }
+}
+
 impl PartialEq for DynSpace {
     fn eq(&self, other: &Self) -> bool {
         std::ptr::addr_eq(RefCell::as_ptr(&self.0), RefCell::as_ptr(&other.0))
@@ -384,7 +401,7 @@ impl<T: Space> Space for &T {
     fn atom_count(&self) -> Option<usize> {
         T::atom_count(*self)
     }
-    fn atom_iter(&self) -> Option<SpaceIter> {
+    fn atom_iter(&self) -> Option<Box<dyn Iterator<Item=&Atom> + '_>> {
         T::atom_iter(*self)
     }
     fn as_any(&self) -> Option<&dyn std::any::Any> {

lib/src/space/module.rs

@@ -0,0 +1,91 @@
+use super::*;
+
+use std::fmt::Debug;
+
+pub struct ModuleSpace {
+    main: Box<dyn SpaceMut>,
+    deps: Vec<DynSpace>,
+}
+
+impl Display for ModuleSpace {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        Display::fmt(&self.main, f)
+    }
+}
+
+impl Debug for ModuleSpace {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        Debug::fmt(&self.main, f)
+    }
+}
+
+impl ModuleSpace {
+    pub fn new<T: SpaceMut + 'static>(space: T) -> Self {
+        Self { main: Box::new(space), deps: Vec::new() }
+    }
+
+    pub fn query(&self, query: &Atom) -> BindingsSet {
+        let mut results = self.main.query(query);
+        for dep in &self.deps {
+            if let Some(space) = dep.borrow().as_any() {
+                if let Some(space) = space.downcast_ref::<Self>()  {
+                    results.extend(space.query_no_deps(query));
+                } else {
+                    panic!("Only ModuleSpace is expected inside dependencies collection");
+                }
+            } else {
+                panic!("Cannot get space as Any inside ModuleSpace dependencies: {}", dep);
+            }
+        }
+        results
+    }
+
+    fn query_no_deps(&self, query: &Atom) -> BindingsSet {
+        self.main.query(query)
+    }
+
+    pub fn add_dep(&mut self, space: DynSpace) {
+        self.deps.push(space)
+    }
+
+    pub fn deps(&self) -> &Vec<DynSpace> {
+        &self.deps
+    }
+}
+
+impl Space for ModuleSpace {
+    fn common(&self) -> FlexRef<SpaceCommon> {
+        self.main.common()
+    }
+    fn query(&self, query: &Atom) -> BindingsSet {
+        ModuleSpace::query(self, query)
+    }
+    fn atom_count(&self) -> Option<usize> {
+        self.main.atom_count()
+    }
+    fn atom_iter(&self) -> Option<Box<dyn Iterator<Item=&Atom> + '_>> {
+        self.main.atom_iter()
+    }
+    fn as_any(&self) -> Option<&dyn std::any::Any> {
+        Some(self)
+    }
+    fn as_any_mut(&mut self) -> Option<&mut dyn std::any::Any> {
+        Some(self)
+    }
+}
+
+impl SpaceMut for ModuleSpace {
+    fn add(&mut self, atom: Atom) {
+        self.main.add(atom)
+    }
+    fn remove(&mut self, atom: &Atom) -> bool {
+        self.main.remove(atom)
+    }
+    fn replace(&mut self, from: &Atom, to: Atom) -> bool {
+        self.main.replace(from, to)
+    }
+    fn as_space<'a>(&self) -> &(dyn Space + 'a) {
+        self
+    }
+}
+