cleanup handling of symbols

werbolg-compile/src/bindings.rs

@@ -18,12 +18,25 @@ impl<T> Default for Bindings<T> {
     }
 }
 
+pub struct BindingInsertError {
+    pub name: Ident,
+}
+
 impl<T> Bindings<T> {
     pub fn new() -> Self {
         Self(HashMap::new())
     }
 
-    pub fn add(&mut self, name: Ident, value: T) {
+    pub fn add(&mut self, name: Ident, value: T) -> Result<(), BindingInsertError> {
+        if self.0.get(&name).is_some() {
+            Err(BindingInsertError { name })
+        } else {
+            self.0.insert(name, value);
+            Ok(())
+        }
+    }
+
+    pub fn add_replace(&mut self, name: Ident, value: T) {
         self.0.insert(name, value);
     }
 
@@ -56,9 +69,11 @@ impl<T: Clone> GlobalBindings<T> {
             self.0.add_ns_hier(namespace.clone()).unwrap()
         }
 
-        self.0.on_mut(&namespace, |bindings| {
-            bindings.add(ident.clone(), value.clone())
-        })
+        self.0
+            .on_mut(&namespace, |bindings| {
+                bindings.add(ident.clone(), value.clone())
+            })
+            .map_err(|_| ())
     }
 
     #[allow(unused)]
@@ -89,7 +104,7 @@ impl<T> BindingsStack<T> {
                 // fall through to the global
             }
             Some(bindings) => {
-                bindings.add(name.clone(), value);
+                bindings.add_replace(name.clone(), value);
             }
         }
     }

werbolg-compile/src/environ.rs

@@ -1,5 +1,5 @@
 use super::symbols::IdVec;
-use crate::symbols::{NamespaceError, SymbolsTableData};
+use crate::symbols::{NamespaceError, SymbolInsertError, SymbolsTable};
 use werbolg_core::{AbsPath, GlobalId, Namespace, NifId};
 
 /// Environment of the compilation
@@ -13,43 +13,78 @@ use werbolg_core::{AbsPath, GlobalId, Namespace, NifId};
 /// * G is the type for global
 ///
 pub struct Environment<N, G> {
+    pub(crate) symbols: SymbolsTable<EnvironmentId>,
     /// All the global values defined
-    pub(crate) globals: SymbolsTableData<GlobalId, G>,
+    pub(crate) globals: IdVec<GlobalId, G>,
     /// The symbols
-    pub(crate) symbols: SymbolsTableData<NifId, N>,
+    pub(crate) nifs: IdVec<NifId, N>,
+}
+
+#[derive(Debug, Clone, Copy)]
+pub enum EnvironmentId {
+    Global(GlobalId),
+    Nif(NifId),
+}
+
+#[derive(Debug, Clone)]
+pub enum EnvironmentError {
+    DuplicateSymbol(AbsPath, EnvironmentId),
+    SymbolInsertError(SymbolInsertError),
 }
 
 impl<N, G> Environment<N, G> {
     /// Create a new empty environment
     pub fn new() -> Self {
         Self {
-            symbols: SymbolsTableData::new(),
-            globals: SymbolsTableData::new(),
+            symbols: SymbolsTable::new(),
+            nifs: IdVec::new(),
+            globals: IdVec::new(),
         }
     }
 
     /// Create a namespace in the environment
     pub fn create_namespace(&mut self, namespace: &Namespace) -> Result<(), NamespaceError> {
         self.symbols.create_namespace(namespace.clone())?;
-        self.globals.create_namespace(namespace.clone())?;
         Ok(())
     }
 
     /// Add NIF to the environment
-    pub fn add_nif(&mut self, path: &AbsPath, t: N) -> NifId {
-        let nif_id = self.symbols.add(&path, t).expect("unique NIF");
-        nif_id
+    pub fn add_nif(&mut self, path: &AbsPath, t: N) -> Result<NifId, EnvironmentError> {
+        let nif_id = self.nifs.next_id();
+        if let Some(id) = self.symbols.get(&path) {
+            return Err(EnvironmentError::DuplicateSymbol(path.clone(), id));
+        }
+
+        self.symbols
+            .insert(&path, EnvironmentId::Nif(nif_id))
+            .map_err(|e| EnvironmentError::SymbolInsertError(e))?;
+
+        let id = self.nifs.push(t);
+        assert_eq!(nif_id, id);
+
+        Ok(nif_id)
     }
 
     /// Add global to the environment
-    pub fn add_global(&mut self, path: &AbsPath, p: G) -> GlobalId {
-        let global_id = self.globals.add(&path, p).expect("unique Global");
-        global_id
+    pub fn add_global(&mut self, path: &AbsPath, p: G) -> Result<GlobalId, EnvironmentError> {
+        let global_id = self.globals.next_id();
+        if let Some(id) = self.symbols.get(&path) {
+            return Err(EnvironmentError::DuplicateSymbol(path.clone(), id));
+        }
+
+        self.symbols
+            .insert(&path, EnvironmentId::Global(global_id))
+            .map_err(|e| EnvironmentError::SymbolInsertError(e))?;
+
+        let id = self.globals.push(p);
+        assert_eq!(global_id, id);
+
+        Ok(global_id)
     }
 
     /// Finalize the environment and keep only the execution relevant information
     #[must_use]
     pub fn finalize(self) -> (IdVec<GlobalId, G>, IdVec<NifId, N>) {
-        (self.globals.vecdata, self.symbols.vecdata)
+        (self.globals, self.nifs)
     }
 }

werbolg-compile/src/hier.rs

@@ -2,6 +2,7 @@ use alloc::vec::Vec;
 use hashbrown::HashMap;
 use werbolg_core::{Ident, Namespace};
 
+/// A hierarchical T with recursives namespaces as Ident
 pub struct Hier<T> {
     current: T,
     ns: HashMap<Ident, Hier<T>>,
@@ -13,6 +14,11 @@ impl<T: Default> Default for Hier<T> {
     }
 }
 
+pub struct HierError<E> {
+    pub namespace: Namespace,
+    pub err: Option<E>,
+}
+
 impl<T: Default> Hier<T> {
     pub fn new(current: T) -> Self {
         Self {
@@ -64,37 +70,25 @@ impl<T: Default> Hier<T> {
         &self.current
     }
 
-    pub fn on_mut<F>(&mut self, namespace: &Namespace, mut f: F) -> Result<(), ()>
+    pub fn on_mut<E, F>(&mut self, namespace: &Namespace, mut f: F) -> Result<(), HierError<E>>
     where
-        F: FnMut(&mut T) -> (),
+        F: FnMut(&mut T) -> Result<(), E>,
     {
         if namespace.is_root() {
-            f(&mut self.current);
-            Ok(())
+            f(&mut self.current).map_err(|e| HierError {
+                namespace: namespace.clone(),
+                err: Some(e),
+            })
         } else {
             let (id, next) = namespace.clone().drop_first();
             if let Some(r) = self.ns.get_mut(&id) {
                 r.on_mut(&next, f)
             } else {
-                Err(())
-            }
-            /*
-            loop {
-                let (id, next) = namespace.drop_first();
-
-                if let Some(mut r) = hier_pointer.ns.get_mut(&id) {
-                    hier_pointer = &mut r;
-                    if next.is_root() {
-                        f(&mut hier_pointer.current);
-                        return Ok(());
-                    } else {
-                        namespace = next;
-                    }
-                } else {
-                    return Err(());
-                }
+                Err(HierError {
+                    namespace: namespace.clone(),
+                    err: None,
+                })
             }
-            */
         }
     }
 

werbolg-compile/src/prepare.rs

@@ -103,20 +103,16 @@ impl<L: Clone + Eq + core::hash::Hash> CompilationState<L> {
         let mut root_bindings = GlobalBindings::new();
 
         for (path, id) in environ.symbols.to_vec(Namespace::root()) {
-            root_bindings
-                .add(path.clone(), BindingType::Nif(id))
-                .map_err(|()| {
-                    CompilationError::DuplicateSymbolEnv(String::from("NIF"), path.clone())
-                        .context(format!("adding NIF"))
-                })?
-        }
-
-        for (path, id) in environ.globals.to_vec(Namespace::root()) {
-            root_bindings
-                .add(path.clone(), BindingType::Global(id))
-                .map_err(|()| {
-                    CompilationError::DuplicateSymbolEnv(String::from("global"), path.clone())
-                })?
+            // unwrap is ok here, the environment should check for duplicate symbol and
+            // missing namespace
+            match id {
+                crate::environ::EnvironmentId::Nif(nif_id) => root_bindings
+                    .add(path.clone(), BindingType::Nif(nif_id))
+                    .unwrap(),
+                crate::environ::EnvironmentId::Global(global_id) => root_bindings
+                    .add(path.clone(), BindingType::Global(global_id))
+                    .unwrap(),
+            }
         }
 
         for (path, fun_id) in table.to_vec(Namespace::root()) {

werbolg-compile/src/symbols.rs

@@ -1,4 +1,4 @@
-use crate::hier::Hier;
+use crate::hier::{Hier, HierError};
 use alloc::vec::Vec;
 use core::hash::Hash;
 use core::marker::PhantomData;
@@ -11,27 +11,32 @@ use werbolg_core::{AbsPath, Ident, Namespace};
 ///
 /// this is a flat table (only use 1 Ident for lookup/insertion),
 /// for hierarchical table use `SymbolsTable`
-pub struct SymbolsTableFlat<ID: IdF> {
+pub struct SymbolsTableFlat<ID> {
     pub(crate) tbl: HashMap<Ident, ID>,
     phantom: PhantomData<ID>,
 }
 
-impl<ID: IdF> Default for SymbolsTableFlat<ID> {
+impl<ID: Copy> Default for SymbolsTableFlat<ID> {
     fn default() -> Self {
         Self::new()
     }
 }
 
-impl<ID: IdF> SymbolsTableFlat<ID> {
+impl<ID: Copy> SymbolsTableFlat<ID> {
     pub fn new() -> Self {
         Self {
             tbl: Default::default(),
             phantom: PhantomData,
         }
     }
 
-    pub fn insert(&mut self, ident: Ident, id: ID) {
-        self.tbl.insert(ident, id);
+    pub fn insert(&mut self, ident: Ident, id: ID) -> Result<(), SymbolInsertFlatError> {
+        if self.tbl.get(&ident).is_some() {
+            Err(SymbolInsertFlatError { ident })
+        } else {
+            self.tbl.insert(ident, id);
+            Ok(())
+        }
     }
 
     pub fn get(&self, ident: &Ident) -> Option<ID> {
@@ -43,7 +48,7 @@ impl<ID: IdF> SymbolsTableFlat<ID> {
     }
 }
 
-pub struct SymbolsTable<ID: IdF>(pub(crate) Hier<SymbolsTableFlat<ID>>);
+pub struct SymbolsTable<ID>(pub(crate) Hier<SymbolsTableFlat<ID>>);
 
 #[derive(Clone, Debug)]
 pub enum NamespaceError {
@@ -55,7 +60,17 @@ pub enum NamespaceError {
     Missing(Namespace, Ident),
 }
 
-impl<ID: IdF> SymbolsTable<ID> {
+pub struct SymbolInsertFlatError {
+    ident: Ident,
+}
+
+#[derive(Clone, Debug)]
+pub enum SymbolInsertError {
+    AlreadyExist(Namespace, Ident),
+    NamespaceNotPresent(Namespace),
+}
+
+impl<ID: Copy> SymbolsTable<ID> {
     pub fn new() -> Self {
         Self(Hier::default())
     }
@@ -70,9 +85,9 @@ impl<ID: IdF> SymbolsTable<ID> {
     }
     */
 
-    fn on_flat_table_mut<F>(&mut self, namespace: &Namespace, f: F) -> Result<(), ()>
+    fn on_flat_table_mut<F, E>(&mut self, namespace: &Namespace, f: F) -> Result<(), HierError<E>>
     where
-        F: FnMut(&mut SymbolsTableFlat<ID>) -> (),
+        F: FnMut(&mut SymbolsTableFlat<ID>) -> Result<(), E>,
     {
         self.0.on_mut(namespace, f)
     }
@@ -83,13 +98,17 @@ impl<ID: IdF> SymbolsTable<ID> {
             .map_err(|()| NamespaceError::DuplicateLeaf(namespace))
     }
 
-    pub fn insert(&mut self, path: &AbsPath, id: ID) {
+    pub fn insert(&mut self, path: &AbsPath, id: ID) -> Result<(), SymbolInsertError> {
         let (namespace, ident) = path.split();
-        if let Ok(()) = self.on_flat_table_mut(&namespace, |table| table.insert(ident.clone(), id))
-        {
-            ()
-        } else {
-            panic!("unknown namespace {:?}", namespace);
+        match self.on_flat_table_mut(&namespace, |table| table.insert(ident.clone(), id)) {
+            Ok(()) => Ok(()),
+            Err(e) => {
+                if let Some(err) = e.err {
+                    Err(SymbolInsertError::AlreadyExist(namespace, err.ident))
+                } else {
+                    Err(SymbolInsertError::NamespaceNotPresent(namespace))
+                }
+            }
         }
     }
 
@@ -121,7 +140,7 @@ impl<ID: IdF> SymbolsTable<ID> {
 }
 
 /// Symbol Table Data maps Ident to ID and store the ID to T
-pub struct SymbolsTableData<ID: IdF, T> {
+pub struct SymbolsTableData<ID, T> {
     pub table: SymbolsTable<ID>,
     pub vecdata: IdVec<ID, T>,
 }
@@ -143,7 +162,7 @@ impl<ID: IdF, T> SymbolsTableData<ID, T> {
             return None;
         }
         let id = self.vecdata.push(v);
-        self.table.insert(path, id);
+        self.table.insert(path, id).unwrap();
         Some(id)
     }
 

werbolg-tales/src/environ.rs

@@ -93,7 +93,7 @@ pub fn create_env() -> Environment<NIF<crate::DummyAlloc, MyLiteral, (), Value>,
         ($env:ident, $i:literal, $arity:literal, $e:expr) => {
             let nif = NIFCall::Pure($e).info($i, CallArity::try_from($arity as usize).unwrap());
             let path = AbsPath::new(&Namespace::root(), &Ident::from($i));
-            $env.add_nif(&path, nif);
+            $env.add_nif(&path, nif).unwrap();
         };
     }
 

werbolg-tests/src/lib.rs

@@ -86,7 +86,7 @@ pub fn execute(mod1: werbolg_core::Module) -> Result<Value, ExecutionError> {
         ($env:ident, $i:literal, $arity:literal, $e:expr) => {
             let nif = NIFCall::Pure($e).info($i, CallArity::try_from($arity as usize).unwrap());
             let path = AbsPath::new(&Namespace::root(), &Ident::from($i));
-            $env.add_nif(&path, nif);
+            $env.add_nif(&path, nif).unwrap();
         };
     }
     let module_ns = Namespace::root().append(Ident::from("main"));