From 5d7f66b063f1e6df364c7158a4d870b306b802b0 Mon Sep 17 00:00:00 2001 From: Vitaly Bogdanov Date: Thu, 19 Sep 2024 18:35:00 +0300 Subject: [PATCH 1/4] Use ModuleSpace to exclude transitive deps from search ModuleSpace doesn't keep imported spaces as atoms. It keeps them in a separate collection instead. This allows constructing new instances of the ModuleSpace without filtering original imported space. --- lib/src/metta/runner/modules/mod.rs | 50 ++++----------- lib/src/metta/runner/stdlib.rs | 2 +- lib/src/space/mod.rs | 4 ++ lib/src/space/module.rs | 91 +++++++++++++++++++++++++++ python/tests/scripts/f1_imports.metta | 18 +++--- 5 files changed, 116 insertions(+), 49 deletions(-) create mode 100644 lib/src/space/module.rs diff --git a/lib/src/metta/runner/modules/mod.rs b/lib/src/metta/runner/modules/mod.rs index e52295140..88b7893e4 100644 --- a/lib/src/metta/runner/modules/mod.rs +++ b/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, - space: DynSpace, + space: Rc>, tokenizer: Shared, imported_deps: Mutex>, loader: Option>, @@ -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>) { + fn stripped_space(&self) -> (Rc>, Option>) { 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::() { - - // 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::() { - 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 { diff --git a/lib/src/metta/runner/stdlib.rs b/lib/src/metta/runner/stdlib.rs index 4bc7714dd..48da07958 100644 --- a/lib/src/metta/runner/stdlib.rs +++ b/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::(other_atom) == Some(context.module().space()) => { + Atom::Grounded(_) if Atom::as_gnd::(other_atom) == Some(&context.module().space()) => { context.import_all_from_dependency(mod_id)?; }, _ => { diff --git a/lib/src/space/mod.rs b/lib/src/space/mod.rs index 0bd604ba1..59dd9dd42 100644 --- a/lib/src/space/mod.rs +++ b/lib/src/space/mod.rs @@ -2,6 +2,7 @@ //! This module is intended to keep different space implementations. pub mod grounding; +pub mod module; use std::fmt::Display; use std::rc::{Rc, Weak}; @@ -282,6 +283,9 @@ pub trait SpaceMut: Space { pub struct DynSpace(Rc>); impl DynSpace { + pub fn with_rc(space: Rc>) -> Self { + Self(space) + } pub fn new(space: T) -> Self { let shared = Rc::new(RefCell::new(space)); DynSpace(shared) diff --git a/lib/src/space/module.rs b/lib/src/space/module.rs new file mode 100644 index 000000000..6a07a7c6c --- /dev/null +++ b/lib/src/space/module.rs @@ -0,0 +1,91 @@ +use super::*; + +use std::fmt::Debug; + +pub struct ModuleSpace { + main: Box, + deps: Vec, +} + +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(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::() { + 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 { + &self.deps + } +} + +impl Space for ModuleSpace { + fn common(&self) -> FlexRef { + self.main.common() + } + fn query(&self, query: &Atom) -> BindingsSet { + ModuleSpace::query(self, query) + } + fn atom_count(&self) -> Option { + self.main.atom_count() + } + fn atom_iter(&self) -> Option { + 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(&self) -> &dyn Space { + self + } +} + diff --git a/python/tests/scripts/f1_imports.metta b/python/tests/scripts/f1_imports.metta index 6f32c95db..a8c74dbf6 100644 --- a/python/tests/scripts/f1_imports.metta +++ b/python/tests/scripts/f1_imports.metta @@ -8,9 +8,9 @@ ; is corelib, which was a dependency of stdlib that has been promoted ; These atoms are both wrapped spaces, as is `&self` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -!(assertEqual - ((let $x (get-atoms &self) (get-type $x))) - (superpose (((get-type &self)) ((get-type &self))))) +;!(assertEqual + ;((let $x (get-atoms &self) (get-type $x))) + ;(superpose (((get-type &self)) ((get-type &self))))) ; stdlib is already loaded !(assertEqual @@ -37,9 +37,9 @@ (let* (($type (get-type $atom)) ($space (get-type &self))) (== $type $space))) ; It's first atom is a space -!(assertEqual - (let $x (collapse (get-atoms &m)) (contains $x is-space)) - True) +;!(assertEqual + ;(let $x (collapse (get-atoms &m)) (contains $x is-space)) + ;True) ; FIXME? Now, it is moduleC space. ; Should it be `stdlib` atom for a separately imported space @@ -93,9 +93,9 @@ !(import! &corelib top:corelib) (: is-corelib (-> Atom Bool)) (= (is-corelib $atom) (== $atom &corelib)) -!(assertEqual - (let $a (collapse (get-atoms &self)) (contains $a is-corelib)) - True) +;!(assertEqual + ;(let $a (collapse (get-atoms &self)) (contains $a is-corelib)) + ;True) ; Let's check that `if` from stdlib is not duplicated and gives only one result !(assertEqual From de48f6f881bfa4c917b8b25408988ebf1b3ea561 Mon Sep 17 00:00:00 2001 From: Vitaly Bogdanov Date: Tue, 24 Sep 2024 08:10:32 +0300 Subject: [PATCH 2/4] Replace SpaceIter by boxed Iterator trait --- c/src/space.rs | 4 ++-- lib/src/space/grounding.rs | 8 ++++---- lib/src/space/mod.rs | 25 +++---------------------- lib/src/space/module.rs | 2 +- 4 files changed, 10 insertions(+), 29 deletions(-) diff --git a/c/src/space.rs b/c/src/space.rs index 87c60f8ae..15194a611 100644 --- a/c/src/space.rs +++ b/c/src/space.rs @@ -736,7 +736,7 @@ impl Space for CSpace { None } } - fn atom_iter(&self) -> Option { + fn atom_iter(&self) -> Option + '_>> { 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 diff --git a/lib/src/space/grounding.rs b/lib/src/space/grounding.rs index dd4235ea8..caa31b4e4 100644 --- a/lib/src/space/grounding.rs +++ b/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 { Some(self.iter().count()) } - fn atom_iter(&self) -> Option { - Some(self.iter()) + fn atom_iter(&self) -> Option + '_>> { + Some(Box::new(self.iter())) } fn as_any(&self) -> Option<&dyn std::any::Any> { Some(self) diff --git a/lib/src/space/mod.rs b/lib/src/space/mod.rs index 59dd9dd42..7a664d6b4 100644 --- a/lib/src/space/mod.rs +++ b/lib/src/space/mod.rs @@ -88,25 +88,6 @@ impl From>> for SpaceObserverRef { } } -/// Space iterator. -pub struct SpaceIter<'a> { - iter: Box + 'a> -} - -impl<'a> SpaceIter<'a> { - pub fn new + '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 { @@ -204,7 +185,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 { + fn atom_iter(&self) -> Option + '_>> { None } @@ -342,7 +323,7 @@ impl Space for DynSpace { fn atom_count(&self) -> Option { self.0.borrow().atom_count() } - fn atom_iter(&self) -> Option { + fn atom_iter(&self) -> Option + '_>> { None } fn as_any(&self) -> Option<&dyn std::any::Any> { @@ -388,7 +369,7 @@ impl Space for &T { fn atom_count(&self) -> Option { T::atom_count(*self) } - fn atom_iter(&self) -> Option { + fn atom_iter(&self) -> Option + '_>> { T::atom_iter(*self) } fn as_any(&self) -> Option<&dyn std::any::Any> { diff --git a/lib/src/space/module.rs b/lib/src/space/module.rs index 6a07a7c6c..de407b565 100644 --- a/lib/src/space/module.rs +++ b/lib/src/space/module.rs @@ -63,7 +63,7 @@ impl Space for ModuleSpace { fn atom_count(&self) -> Option { self.main.atom_count() } - fn atom_iter(&self) -> Option { + fn atom_iter(&self) -> Option + '_>> { self.main.atom_iter() } fn as_any(&self) -> Option<&dyn std::any::Any> { From 479afb1463007fa0a157b3bb0c38afb1b82b5090 Mon Sep 17 00:00:00 2001 From: Vitaly Bogdanov Date: Tue, 24 Sep 2024 10:43:33 +0300 Subject: [PATCH 3/4] Fix SpaceMut::as_space signature to reflect lifetimes properly --- c/src/space.rs | 2 +- lib/src/space/grounding.rs | 2 +- lib/src/space/mod.rs | 4 ++-- lib/src/space/module.rs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/c/src/space.rs b/c/src/space.rs index 15194a611..8518aa808 100644 --- a/c/src/space.rs +++ b/c/src/space.rs @@ -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 } } diff --git a/lib/src/space/grounding.rs b/lib/src/space/grounding.rs index caa31b4e4..b3a7d3ea9 100644 --- a/lib/src/space/grounding.rs +++ b/lib/src/space/grounding.rs @@ -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 } } diff --git a/lib/src/space/mod.rs b/lib/src/space/mod.rs index 7a664d6b4..5930a6ef4 100644 --- a/lib/src/space/mod.rs +++ b/lib/src/space/mod.rs @@ -257,7 +257,7 @@ 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)] @@ -305,7 +305,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 } } diff --git a/lib/src/space/module.rs b/lib/src/space/module.rs index de407b565..f01eb229c 100644 --- a/lib/src/space/module.rs +++ b/lib/src/space/module.rs @@ -84,7 +84,7 @@ impl SpaceMut for ModuleSpace { fn replace(&mut self, from: &Atom, to: Atom) -> bool { self.main.replace(from, to) } - fn as_space(&self) -> &dyn Space { + fn as_space<'a>(&self) -> &(dyn Space + 'a) { self } } From ab9c0c15a8a6019cf228f1d767f10fa970c4d440 Mon Sep 17 00:00:00 2001 From: Vitaly Bogdanov Date: Tue, 24 Sep 2024 11:23:44 +0300 Subject: [PATCH 4/4] Implement Space::atom_iter for DynSpace --- lib/src/space/mod.rs | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/lib/src/space/mod.rs b/lib/src/space/mod.rs index 5930a6ef4..1662aadb6 100644 --- a/lib/src/space/mod.rs +++ b/lib/src/space/mod.rs @@ -7,6 +7,7 @@ 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::*; @@ -324,7 +325,7 @@ impl Space for DynSpace { self.0.borrow().atom_count() } fn atom_iter(&self) -> Option + '_>> { - None + DynSpaceIter::new(self) } fn as_any(&self) -> Option<&dyn std::any::Any> { None @@ -334,6 +335,37 @@ impl Space for DynSpace { } } +struct DynSpaceIter<'a> { + space_ref: Ref<'a, dyn Space>, + space_iter: Option + 'a>>, +} + +impl<'a> DynSpaceIter<'a> { + fn new(space: &'a DynSpace) -> Option + '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.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))