Merge pull request #129 from togglebyte/feature/variable-scoping

Feature/variable scoping

Author: togglebyte

CHANGES.md

@@ -6,6 +6,7 @@
     * BUGFIX: ctrl+c works with the error display
     * Trying to use a component twice will now include the component name in the
       error
+    * Global definitions will raise an error if it's already assigned
 * 0.2.7
     * BUGFIX: use correct truthiness check in control flow update
 * 0.2.6

anathema-templates/src/error/mod.rs

@@ -54,6 +54,7 @@ pub enum ErrorKind {
     EmptyBody,
     InvalidStatement(String),
     Io(std::io::Error),
+    GlobalAlreadyAssigned(String),
 }
 
 impl ErrorKind {
@@ -72,6 +73,7 @@ impl Display for ErrorKind {
             ErrorKind::EmptyBody => write!(f, "if or else node has no children"),
             ErrorKind::InvalidStatement(stmt) => write!(f, "invalid statement: {stmt}"),
             ErrorKind::Io(err) => write!(f, "{err}"),
+            ErrorKind::GlobalAlreadyAssigned(name) => write!(f, "global value `{name}` already assigned"),
         }
     }
 }

anathema-templates/src/lexer.rs

@@ -332,6 +332,7 @@ mod test {
             | crate::error::ErrorKind::MissingComponent(_)
             | crate::error::ErrorKind::EmptyTemplate
             | crate::error::ErrorKind::EmptyBody
+            | crate::error::ErrorKind::GlobalAlreadyAssigned(_)
             | crate::error::ErrorKind::Io(_) => panic!("invalid error"),
         }
     }

anathema-templates/src/statements/eval.rs

@@ -61,7 +61,10 @@ impl Scope {
                     }
                     match is_global {
                         false => _ = ctx.variables.define_local(binding, value),
-                        true => ctx.variables.define_global(binding, value),
+                        true => match ctx.variables.define_global(binding, value) {
+                            Ok(()) => (),
+                            Err(kind) => return Err(kind.to_error(ctx.template.path())),
+                        },
                     }
                 }
                 Statement::ComponentSlot(slot_id) => {
@@ -89,7 +92,10 @@ impl Scope {
         let ident = ctx.strings.get_unchecked(ident);
         let attributes = self.eval_attributes(ctx)?;
         let value = self.statements.take_value().and_then(|v| const_eval(v, ctx));
+
+        ctx.variables.push();
         let children = self.consume_scope(ctx)?;
+        ctx.variables.pop();
 
         let node = Blueprint::Single(Single {
             ident,
@@ -199,8 +205,6 @@ impl Scope {
     }
 
     fn eval_component(&mut self, component_id: ComponentBlueprintId, ctx: &mut Context<'_>) -> Result<Blueprint> {
-        ctx.variables.push();
-
         let parent = ctx.component_parent();
 
         // Associated functions
@@ -209,6 +213,9 @@ impl Scope {
         // Attributes
         let attributes = self.eval_attributes(ctx)?;
 
+        // Scope variables to the component
+        ctx.variables.push_scope_boundary();
+
         // Slots
         let mut slots = SmallMap::empty();
         let mut scope = self.statements.take_scope();
@@ -243,7 +250,7 @@ impl Scope {
             parent,
         };
 
-        ctx.variables.pop();
+        ctx.variables.pop_scope_boundary();
         Ok(Blueprint::Component(component))
     }
 }

anathema-templates/src/variables.rs

@@ -1,7 +1,9 @@
 use std::collections::HashMap;
+use std::sync::OnceLock;
 
 use anathema_store::slab::{Slab, SlabIndex};
 
+use crate::error::ErrorKind;
 use crate::expressions::Expression;
 
 #[derive(Debug, Default, Clone)]
@@ -12,6 +14,10 @@ impl Globals {
         Self(HashMap::new())
     }
 
+    pub fn contains(&self, ident: &str) -> bool {
+        self.0.contains_key(ident)
+    }
+
     pub fn get(&self, ident: &str) -> Option<&Expression> {
         self.0.get(ident)
     }
@@ -66,6 +72,11 @@ impl Variable {
 pub struct ScopeId(Box<[u16]>);
 
 impl ScopeId {
+    fn root() -> &'static Self {
+        static ROOT: OnceLock<ScopeId> = OnceLock::new();
+        ROOT.get_or_init(|| ScopeId(Box::new([])))
+    }
+
     // Create the next child id.
     fn next(&self, index: u16) -> Self {
         let mut scope_id = Vec::with_capacity(self.0.len() + 1);
@@ -98,6 +109,16 @@ impl ScopeId {
     fn as_slice(&self) -> &[u16] {
         &self.0
     }
+
+    fn contains(&self, other: impl AsRef<[u16]>) -> Option<&ScopeId> {
+        let other = other.as_ref();
+        let len = self.0.len();
+
+        match other.len() >= len {
+            true => (*self.0 == other[..len]).then_some(self),
+            false => None,
+        }
+    }
 }
 
 impl AsRef<[u16]> for ScopeId {
@@ -182,13 +203,14 @@ impl Declarations {
     }
 
     // Get the scope id that is closest to the argument
-    fn get(&self, ident: &str, scope_id: impl AsRef<[u16]>) -> Option<VarId> {
+    fn get(&self, ident: &str, scope_id: impl AsRef<[u16]>, boundary: &ScopeId) -> Option<VarId> {
         self.0
             .get(ident)?
             .iter()
             .rev()
-            .filter(|(scope, _)| scope.as_ref() == scope_id.as_ref())
-            .map(|(_, var)| *var)
+            // here we need to look up closest scope that is still within the last boundary
+            .filter(|(scope, _)| boundary.contains(scope).is_some())
+            .filter_map(|(scope, var)| scope.contains(&scope_id).map(|_| *var))
             .next()
     }
 }
@@ -200,6 +222,7 @@ pub struct Variables {
     globals: Globals,
     root: RootScope,
     current: ScopeId,
+    boundary: Vec<ScopeId>,
     store: Slab<VarId, Variable>,
     declarations: Declarations,
 }
@@ -210,6 +233,7 @@ impl Default for Variables {
         Self {
             globals: Globals::empty(),
             current: root.0.id.clone(),
+            boundary: vec![],
             root,
             store: Slab::empty(),
             declarations: Declarations::new(),
@@ -232,9 +256,15 @@ impl Variables {
         var_id
     }
 
-    pub fn define_global(&mut self, ident: impl Into<String>, value: impl Into<Expression>) {
+    pub fn define_global(&mut self, ident: impl Into<String>, value: impl Into<Expression>) -> Result<(), ErrorKind> {
+        let ident = ident.into();
+        if self.globals.contains(&ident) {
+            return Err(ErrorKind::GlobalAlreadyAssigned(ident));
+        }
+
         let value = value.into();
-        self.globals.set(ident.into(), value)
+        self.globals.set(ident, value);
+        Ok(())
     }
 
     pub fn define_local(&mut self, ident: impl Into<String>, value: impl Into<Expression>) -> VarId {
@@ -254,7 +284,21 @@ impl Variables {
 
     /// Fetch a value starting from the current path.
     pub fn fetch(&self, ident: &str) -> Option<VarId> {
-        self.declarations.get(ident, &self.current)
+        self.declarations.get(ident, &self.current, self.boundary())
+    }
+
+    /// Create a new scope and set that scope as a boundary.
+    /// This prevents inner components from accessing values
+    /// declared outside of the component.
+    pub(crate) fn push_scope_boundary(&mut self) {
+        self.push();
+        self.boundary.push(self.current.clone());
+    }
+
+    /// Pop the scope boundary.
+    pub(crate) fn pop_scope_boundary(&mut self) {
+        self.pop();
+        self.boundary.pop();
     }
 
     /// Create a new child and set the new childs id as the `current` id.
@@ -281,13 +325,17 @@ impl Variables {
     // Fetch and load a value from its ident
     #[cfg(test)]
     fn fetch_load(&self, ident: &str) -> Option<&Expression> {
-        let id = self.declarations.get(ident, &self.current)?;
+        let id = self.declarations.get(ident, &self.current, self.boundary())?;
         self.load(id)
     }
 
     pub fn global_lookup(&self, ident: &str) -> Option<&Expression> {
         self.globals.get(ident)
     }
+
+    fn boundary(&self) -> &ScopeId {
+        self.boundary.last().unwrap_or(ScopeId::root())
+    }
 }
 
 impl From<Variables> for HashMap<String, Variable> {
@@ -309,6 +357,7 @@ impl From<Variables> for HashMap<String, Variable> {
 #[cfg(test)]
 mod test {
     use super::*;
+    use crate::expressions::num;
 
     impl From<usize> for VarId {
         fn from(value: usize) -> Self {
@@ -393,15 +442,15 @@ mod test {
     fn declaration_lookup() {
         let mut dec = Declarations::new();
         dec.add("var", [0, 0], 0);
-        let root = dec.get("var", [0, 0]).unwrap();
+        let root = dec.get("var", [0, 0], ScopeId::root()).unwrap();
         assert_eq!(root, 0.into());
     }
 
     #[test]
     fn declaration_failed_lookup() {
         let mut dec = Declarations::new();
         dec.add("var", [0], 0);
-        let root = dec.get("var", [1, 0]);
+        let root = dec.get("var", [1, 0], ScopeId::root());
         assert!(root.is_none());
     }
 
@@ -413,15 +462,40 @@ mod test {
         dec.add(ident, [0, 0], 1);
         dec.add(ident, [0, 0, 0], 2);
 
-        assert_eq!(dec.get(ident, [0]).unwrap().0, 0);
-        assert_eq!(dec.get(ident, [0, 0]).unwrap().0, 1);
-        assert!(dec.get(ident, [0, 0, 0, 1, 1]).is_none());
+        assert_eq!(dec.get(ident, [0], ScopeId::root()).unwrap().0, 0);
+        assert_eq!(dec.get(ident, [0, 0], ScopeId::root()).unwrap().0, 1);
+        assert_eq!(dec.get(ident, [0, 0, 0, 1, 1], ScopeId::root()).unwrap().0, 2);
     }
 
     #[test]
     fn unreachable_declaration() {
         let mut dec = Declarations::new();
         dec.add("var", [0, 1], 0);
-        assert!(dec.get("var", [0, 0, 1]).is_none());
+        assert!(dec.get("var", [0, 0, 1], ScopeId::root()).is_none());
+    }
+
+    #[test]
+    fn get_inside_boundary() {
+        let mut vars = Variables::new();
+
+        // Define a varialbe in the root scope
+        _ = vars.define_local("var", 1);
+
+        // Create a new unique scope and boundary.
+        // * `var` should be inaccessible from within the new scope boundary
+        // * `outer_var` should be inaccessible to the root scope
+        vars.push_scope_boundary();
+        assert!(vars.fetch("var").is_none());
+        _ = vars.define_local("var", 2);
+        _ = vars.define_local("other_var", 3);
+        assert_eq!(vars.fetch_load("var").unwrap(), &*num(2));
+        vars.push();
+        assert_eq!(vars.fetch_load("other_var").unwrap(), &*num(3));
+        vars.pop();
+
+        // Return to root scope
+        vars.pop_scope_boundary();
+        assert_eq!(vars.fetch_load("var").unwrap(), &*num(1));
+        assert!(vars.fetch("other_var").is_none());
     }
 }

anathema-value-resolver/src/value.rs

@@ -573,7 +573,7 @@ pub(crate) mod test {
 
         let mut states = States::new();
         let mut globals = Variables::new();
-        globals.define_global("index", 0);
+        globals.define_global("index", 0).unwrap();
 
         setup(&mut states, globals, |test| {
             let value = test.eval(&expr);
@@ -881,7 +881,7 @@ pub(crate) mod test {
         // state[empty|full]
         let mut states = States::new();
         let mut globals = Variables::new();
-        globals.define_global("full", "string");
+        globals.define_global("full", "string").unwrap();
         setup(&mut states, globals, |test| {
             let expr = index(ident("state"), either(ident("empty"), ident("full")));
             test.with_state(|state| state.string.set("a string"));
@@ -934,7 +934,7 @@ pub(crate) mod test {
     fn test_either() {
         let mut states = States::new();
         let mut globals = Variables::new();
-        globals.define_global("missing", 111);
+        globals.define_global("missing", 111).unwrap();
         setup(&mut states, globals, |test| {
             let expr = either(ident("missings"), num(2));
             let value = test.eval(&expr);