Skip to content

Commit

Permalink
Merge pull request #826 from vsbogd/no-module-duplication
Browse files Browse the repository at this point in the history 
Don't copy imported module to filter out its dependencies
  • Loading branch information
@vsbogd
vsbogd authored Dec 20, 2024
2 parents 2f95fd8 + cbe0086 commit b083e09
Show file tree
Hide file tree
Showing 8 changed files with 194 additions and 153 deletions.
63 changes: 18 additions & 45 deletions c/src/space.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use hyperon::matcher::*;
use crate::atom::*;

use std::os::raw::*;
use std::borrow::Cow;

// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
// Space Client Interface
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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 }
}
Expand All @@ -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
}
}
Expand Down
50 changes: 11 additions & 39 deletions lib/src/metta/runner/modules/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::cell::RefCell;

use crate::metta::*;
use crate::metta::runner::*;
use crate::space::module::ModuleSpace;

use regex::Regex;

Expand Down Expand Up @@ -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>>,
Expand All @@ -75,6 +76,7 @@ impl MettaMod {
}
}
}
let space = Rc::new(RefCell::new(ModuleSpace::new(space)));

let new_mod = Self {
mod_path,
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(())
Expand All @@ -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"
Expand All @@ -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> {
Expand Down
2 changes: 1 addition & 1 deletion lib/src/metta/runner/stdlib/module.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
},
_ => {
Expand Down
22 changes: 15 additions & 7 deletions lib/src/metta/runner/stdlib/space.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down Expand Up @@ -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("
Expand All @@ -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"))]);
}

Expand All @@ -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"))]);
}
Expand All @@ -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());
}

Expand All @@ -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"))]);
}

Expand Down
10 changes: 5 additions & 5 deletions lib/src/space/grounding.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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
}
}
Expand Down
Loading

0 comments on commit b083e09

Please sign in to comment.