Run var def filter during parsing.

CHANGELOG.md

@@ -38,7 +38,7 @@ New features
 * A new package, `DebuggingPackage`, is added which contains the `back_trace` function to get the current call stack anywhere in a script.
 * `Engine::set_fail_on_invalid_map_property` is added to control whether to raise an error (new `EvalAltResult::ErrorPropertyNotFound`) when invalid properties are accessed on object maps.
 * `Engine::set_allow_shadowing` is added to allow/disallow variables _shadowing_, with new errors `EvalAltResult::ErrorVariableExists` and `ParseErrorType::VariableExists`.
-* `Engine::on_def_var` allows registering a closure which can decide whether a variable definition is allow to continue, or should fail with an error.
+* `Engine::on_def_var` allows registering a closure which can decide whether a variable definition is allow to continue, during compilation or runtime, or should fail with an error (`ParseErrorType::ForbiddenVariable` or `EvalAltResult::ErrorForbiddenVariable`).
 * A new syntax for defining custom packages is introduced that removes the need to specify the Rhai crate name (internally uses the `$crate` meta variable).
 
 Enhancements

src/api/events.rs

@@ -73,8 +73,9 @@ impl Engine {
     ///
     /// where:
     /// * `name`: name of the variable to be defined.
+    /// * `is_runtime`: `true` if the variable definition event happens during runtime, `false` if during compilation.
     /// * `is_const`: `true` if the statement is `const`, otherwise it is `let`.
-    /// * `block_level`: the current nesting level of statement blocks, with zero being the global level
+    /// * `block_level`: the current nesting level of statement blocks, with zero being the global level.
     /// * `will_shadow`: will the variable _shadow_ an existing variable?
     /// * `context`: the current [evaluation context][`EvalContext`].
     ///
@@ -96,7 +97,7 @@ impl Engine {
     /// let mut engine = Engine::new();
     ///
     /// // Register a variable definition filter.
-    /// engine.on_def_var(|name, is_const, _, _, _| {
+    /// engine.on_def_var(|name, _, is_const, _, _, _| {
     ///     // Disallow defining MYSTIC_NUMBER as a constant
     ///     if name == "MYSTIC_NUMBER" && is_const {
     ///         Ok(false)
@@ -117,7 +118,7 @@ impl Engine {
     #[inline(always)]
     pub fn on_def_var(
         &mut self,
-        callback: impl Fn(&str, bool, usize, bool, &EvalContext) -> RhaiResultOf<bool>
+        callback: impl Fn(&str, bool, bool, usize, bool, &EvalContext) -> RhaiResultOf<bool>
             + SendSync
             + 'static,
     ) -> &mut Self {

src/eval/stmt.rs

@@ -831,13 +831,13 @@ impl Engine {
                         level: level,
                     };
 
-                    match filter(var_name, is_const, scope_level, shadowing, &context) {
+                    match filter(var_name, true, is_const, scope_level, shadowing, &context) {
                         Ok(true) => None,
-                        Ok(false) => Some(Err(ERR::ErrorRuntime(
+                        Ok(false) => {
-                            format!("Variable cannot be defined: {}", var_name).into(),
+                            Some(Err(
-                            *pos,
+                                ERR::ErrorForbiddenVariable(var_name.to_string(), *pos).into()
-                        )
+                            ))
-                        .into())),
+                        }
                         err @ Err(_) => Some(err),
                     }
                 } else {

src/func/native.rs

@@ -447,8 +447,9 @@ pub type OnVarCallback =
 
 /// Callback function for variable definition.
 #[cfg(not(feature = "sync"))]
-pub type OnDefVarCallback = dyn Fn(&str, bool, usize, bool, &EvalContext) -> RhaiResultOf<bool>;
+pub type OnDefVarCallback =
+    dyn Fn(&str, bool, bool, usize, bool, &EvalContext) -> RhaiResultOf<bool>;
 /// Callback function for variable definition.
 #[cfg(feature = "sync")]
 pub type OnDefVarCallback =
-    dyn Fn(&str, bool, usize, bool, &EvalContext) -> RhaiResultOf<bool> + Send + Sync;
+    dyn Fn(&str, bool, bool, usize, bool, &EvalContext) -> RhaiResultOf<bool> + Send + Sync;

src/parser.rs

@@ -7,6 +7,7 @@ use crate::ast::{
     OpAssignment, ScriptFnDef, Stmt, SwitchCases, TryCatchBlock, AST_OPTION_FLAGS::*,
 };
 use crate::engine::{Precedence, KEYWORD_THIS, OP_CONTAINS};
+use crate::eval::{EvalState, GlobalRuntimeState};
 use crate::func::hashing::get_hasher;
 use crate::tokenizer::{
     is_keyword_function, is_valid_function_name, is_valid_identifier, Span, Token, TokenStream,
@@ -15,8 +16,9 @@ use crate::tokenizer::{
 use crate::types::dynamic::AccessMode;
 use crate::types::StringsInterner;
 use crate::{
-    calc_fn_hash, Dynamic, Engine, ExclusiveRange, Identifier, ImmutableString, InclusiveRange,
+    calc_fn_hash, Dynamic, Engine, EvalAltResult, EvalContext, ExclusiveRange, Identifier,
-    LexError, OptimizationLevel, ParseError, Position, Scope, Shared, StaticVec, AST, INT, PERR,
+    ImmutableString, InclusiveRange, LexError, OptimizationLevel, ParseError, Position, Scope,
+    Shared, StaticVec, AST, INT, PERR,
 };
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
@@ -47,7 +49,7 @@ pub struct ParseState<'e> {
     /// Interned strings.
     pub interned_strings: StringsInterner,
     /// Encapsulates a local stack with variable names to simulate an actual runtime scope.
-    pub stack: StaticVec<(Identifier, AccessMode)>,
+    pub stack: Scope<'e>,
     /// Size of the local variables stack upon entry of the current block scope.
     pub entry_stack_len: usize,
     /// Tracks a list of external variables (variables that are not explicitly declared in the scope).
@@ -89,7 +91,7 @@ impl<'e> ParseState<'e> {
             #[cfg(not(feature = "no_closure"))]
             allow_capture: true,
             interned_strings: StringsInterner::new(),
-            stack: StaticVec::new_const(),
+            stack: Scope::new(),
             entry_stack_len: 0,
             #[cfg(not(feature = "no_module"))]
             imports: StaticVec::new_const(),
@@ -112,10 +114,9 @@ impl<'e> ParseState<'e> {
 
         let index = self
             .stack
-            .iter()
+            .iter_rev_raw()
-            .rev()
             .enumerate()
-            .find(|(.., (n, ..))| {
+            .find(|&(.., (n, ..))| {
                 if n == SCOPE_SEARCH_BARRIER_MARKER {
                     // Do not go beyond the barrier
                     barrier = true;
@@ -1337,14 +1338,12 @@ fn parse_primary(
             }
 
             if segments.is_empty() {
-                segments.push(Expr::StringConstant(
+                Expr::StringConstant(state.get_interned_string("", ""), settings.pos)
-                    state.get_interned_string("", ""),
+            } else {
-                    settings.pos,
-                ));
-            }
                 segments.shrink_to_fit();
                 Expr::InterpolatedString(segments.into(), settings.pos)
             }
+        }
 
         // Array literal
         #[cfg(not(feature = "no_index"))]
@@ -1802,7 +1801,11 @@ fn make_assignment_stmt(
                 || index.expect("either long or short index is `None`").get(),
                 |n| n.get() as usize,
             );
-            match state.stack[state.stack.len() - index].1 {
+            match state
+                .stack
+                .get_mut_by_index(state.stack.len() - index)
+                .access_mode()
+            {
                 AccessMode::ReadWrite => Ok(Stmt::Assignment(
                     (op_info, (lhs, rhs).into()).into(),
                     op_pos,
@@ -2195,7 +2198,7 @@ fn parse_custom_syntax(
         // Add a barrier variable to the stack so earlier variables will not be matched.
         // Variable searches stop at the first barrier.
         let marker = state.get_identifier("", SCOPE_SEARCH_BARRIER_MARKER);
-        state.stack.push((marker, AccessMode::ReadWrite));
+        state.stack.push(marker, ());
     }
 
     let parse_func = syntax.parse.as_ref();
@@ -2562,21 +2565,24 @@ fn parse_for(
     let counter_var = counter_name.map(|name| {
         let name = state.get_identifier("", name);
         let pos = counter_pos.expect("`Some`");
-        state.stack.push((name.clone(), AccessMode::ReadWrite));
+        state.stack.push(name.clone(), ());
-        Ident { name, pos }
+        Ident {
+            name: name.clone(),
+            pos,
+        }
     });
 
     let loop_var = state.get_identifier("", name);
-    state.stack.push((loop_var.clone(), AccessMode::ReadWrite));
+    state.stack.push(loop_var.clone(), ());
     let loop_var = Ident {
-        name: loop_var,
+        name: loop_var.clone(),
         pos: name_pos,
     };
 
     settings.is_breakable = true;
     let body = parse_block(input, state, lib, settings.level_up())?;
 
-    state.stack.truncate(prev_stack_len);
+    state.stack.rewind(prev_stack_len);
 
     Ok(Stmt::For(
         expr,
@@ -2610,6 +2616,30 @@ fn parse_let(
         return Err(PERR::VariableExists(name.to_string()).into_err(pos));
     }
 
+    if let Some(ref filter) = state.engine.def_var_filter {
+        let shadowing = state.stack.iter().any(|(v, ..)| v == name.as_ref());
+        let level = settings.level;
+        let is_const = var_type == AccessMode::ReadOnly;
+        let context = EvalContext {
+            engine: state.engine,
+            scope: &mut state.stack,
+            global: &mut GlobalRuntimeState::new(state.engine),
+            state: &mut EvalState::new(),
+            lib: &[],
+            this_ptr: &mut None,
+            level,
+        };
+
+        match filter(&name, false, is_const, level, shadowing, &context) {
+            Ok(true) => (),
+            Ok(false) => return Err(PERR::ForbiddenVariable(name.to_string()).into_err(pos)),
+            Err(err) => match *err {
+                EvalAltResult::ErrorParsing(perr, pos) => return Err(perr.into_err(pos)),
+                _ => return Err(PERR::ForbiddenVariable(name.to_string()).into_err(pos)),
+            },
+        }
+    }
+
     let name = state.get_identifier("", name);
     let var_def = Ident {
         name: name.clone(),
@@ -2624,8 +2654,6 @@ fn parse_let(
         Expr::Unit(Position::NONE)
     };
 
-    state.stack.push((name, var_type));
-
     let export = if is_export {
         AST_OPTION_EXPORTED
     } else {
@@ -2634,14 +2662,20 @@ fn parse_let(
 
     match var_type {
         // let name = expr
-        AccessMode::ReadWrite => Ok(Stmt::Var(expr, var_def.into(), export, settings.pos)),
+        AccessMode::ReadWrite => {
+            state.stack.push(name, ());
+            Ok(Stmt::Var(expr, var_def.into(), export, settings.pos))
+        }
         // const name = { expr:constant }
-        AccessMode::ReadOnly => Ok(Stmt::Var(
+        AccessMode::ReadOnly => {
+            state.stack.push_constant(name, ());
+            Ok(Stmt::Var(
                 expr,
                 var_def.into(),
                 AST_OPTION_CONSTANT + export,
                 settings.pos,
-        )),
+            ))
+        }
     }
 }
 
@@ -2820,7 +2854,7 @@ fn parse_block(
         }
     };
 
-    state.stack.truncate(state.entry_stack_len);
+    state.stack.rewind(state.entry_stack_len);
     state.entry_stack_len = prev_entry_stack_len;
 
     #[cfg(not(feature = "no_module"))]
@@ -3106,7 +3140,7 @@ fn parse_try_catch(
         }
 
         let name = state.get_identifier("", name);
-        state.stack.push((name.clone(), AccessMode::ReadWrite));
+        state.stack.push(name.clone(), ());
         Some(Ident { name, pos })
     } else {
         None
@@ -3117,7 +3151,7 @@ fn parse_try_catch(
 
     if catch_var.is_some() {
         // Remove the error variable from the stack
-        state.stack.pop().unwrap();
+        state.stack.rewind(state.stack.len() - 1);
     }
 
     Ok(Stmt::TryCatch(
@@ -3176,7 +3210,7 @@ fn parse_fn(
                         );
                     }
                     let s = state.get_identifier("", s);
-                    state.stack.push((s.clone(), AccessMode::ReadWrite));
+                    state.stack.push(s.clone(), ());
                     params.push((s, pos))
                 }
                 (Token::LexError(err), pos) => return Err(err.into_err(pos)),
@@ -3319,7 +3353,7 @@ fn parse_anon_fn(
                         );
                     }
                     let s = state.get_identifier("", s);
-                    state.stack.push((s.clone(), AccessMode::ReadWrite));
+                    state.stack.push(s.clone(), ());
                     params_list.push(s)
                 }
                 (Token::LexError(err), pos) => return Err(err.into_err(pos)),

src/types/error.rs

@@ -36,6 +36,8 @@ pub enum EvalAltResult {
 
     /// Shadowing of an existing variable disallowed. Wrapped value is the variable name.
     ErrorVariableExists(String, Position),
+    /// Forbidden variable name. Wrapped value is the variable name.
+    ErrorForbiddenVariable(String, Position),
     /// Access of an unknown variable. Wrapped value is the variable name.
     ErrorVariableNotFound(String, Position),
     /// Access of an unknown object map property. Wrapped value is the property name.
@@ -148,6 +150,7 @@ impl fmt::Display for EvalAltResult {
             Self::ErrorInModule(s, err, ..) => write!(f, "Error in module {}: {}", s, err)?,
 
             Self::ErrorVariableExists(s, ..) => write!(f, "Variable is already defined: {}", s)?,
+            Self::ErrorForbiddenVariable(s, ..) => write!(f, "Forbidden variable name: {}", s)?,
             Self::ErrorVariableNotFound(s, ..) => write!(f, "Variable not found: {}", s)?,
             Self::ErrorPropertyNotFound(s, ..) => write!(f, "Property not found: {}", s)?,
             Self::ErrorFunctionNotFound(s, ..) => write!(f, "Function not found: {}", s)?,
@@ -285,6 +288,7 @@ impl EvalAltResult {
             | Self::ErrorIndexingType(..)
             | Self::ErrorFor(..)
             | Self::ErrorVariableExists(..)
+            | Self::ErrorForbiddenVariable(..)
             | Self::ErrorVariableNotFound(..)
             | Self::ErrorPropertyNotFound(..)
             | Self::ErrorModuleNotFound(..)
@@ -377,6 +381,7 @@ impl EvalAltResult {
                 map.insert("type".into(), t.into());
             }
             Self::ErrorVariableExists(v, ..)
+            | Self::ErrorForbiddenVariable(v, ..)
             | Self::ErrorVariableNotFound(v, ..)
             | Self::ErrorPropertyNotFound(v, ..)
             | Self::ErrorDataRace(v, ..)
@@ -440,6 +445,7 @@ impl EvalAltResult {
             | Self::ErrorIndexingType(.., pos)
             | Self::ErrorFor(pos)
             | Self::ErrorVariableExists(.., pos)
+            | Self::ErrorForbiddenVariable(.., pos)
             | Self::ErrorVariableNotFound(.., pos)
             | Self::ErrorPropertyNotFound(.., pos)
             | Self::ErrorModuleNotFound(.., pos)
@@ -490,6 +496,7 @@ impl EvalAltResult {
             | Self::ErrorIndexingType(.., pos)
             | Self::ErrorFor(pos)
             | Self::ErrorVariableExists(.., pos)
+            | Self::ErrorForbiddenVariable(.., pos)
             | Self::ErrorVariableNotFound(.., pos)
             | Self::ErrorPropertyNotFound(.., pos)
             | Self::ErrorModuleNotFound(.., pos)

src/types/parse_error.rs

@@ -109,6 +109,8 @@ pub enum ParseErrorType {
     PropertyExpected,
     /// Missing a variable name after the `let`, `const`, `for` or `catch` keywords.
     VariableExpected,
+    /// Forbidden variable name.  Wrapped value is the variable name.
+    ForbiddenVariable(String),
     /// An identifier is a reserved symbol.
     Reserved(String),
     /// An expression is of the wrong type.
@@ -240,6 +242,7 @@ impl fmt::Display for ParseErrorType {
             Self::WrongSwitchCaseCondition => f.write_str("This switch case cannot have a condition"),
             Self::PropertyExpected => f.write_str("Expecting name of a property"),
             Self::VariableExpected => f.write_str("Expecting name of a variable"),
+            Self::ForbiddenVariable(s) => write!(f, "Forbidden variable name: {}", s),
             Self::WrongFnDefinition => f.write_str("Function definitions must be at global level and cannot be inside a block or another function"),
             Self::FnMissingName => f.write_str("Expecting function name in function declaration"),
             Self::WrongDocComment => f.write_str("Doc-comment must be followed immediately by a function definition"),

src/types/scope.rs

@@ -595,6 +595,16 @@ impl Scope<'_> {
             .zip(self.values.iter())
             .map(|((name, ..), value)| (name.as_ref(), value.is_read_only(), value))
     }
+    /// Get a reverse iterator to entries in the [`Scope`].
+    /// Shared values are not expanded.
+    #[inline]
+    pub(crate) fn iter_rev_raw(&self) -> impl Iterator<Item = (&str, bool, &Dynamic)> {
+        self.names
+            .iter()
+            .rev()
+            .zip(self.values.iter().rev())
+            .map(|((name, ..), value)| (name.as_ref(), value.is_read_only(), value))
+    }
     /// Remove a range of entries within the [`Scope`].
     ///
     /// # Panics

tests/var_scope.rs

@@ -1,4 +1,4 @@
-use rhai::{Engine, EvalAltResult, Position, Scope, INT};
+use rhai::{Engine, EvalAltResult, ParseErrorType, Position, Scope, INT};
 
 #[test]
 fn test_var_scope() -> Result<(), Box<EvalAltResult>> {
@@ -125,7 +125,10 @@ fn test_var_resolver() -> Result<(), Box<EvalAltResult>> {
 fn test_var_def_filter() -> Result<(), Box<EvalAltResult>> {
     let mut engine = Engine::new();
 
-    engine.on_def_var(|name, _, scope_level, _, _| match (name, scope_level) {
+    let ast = engine.compile("let x = 42;")?;
+    engine.run_ast(&ast)?;
+
+    engine.on_def_var(|name, _, _, scope_level, _, _| match (name, scope_level) {
         ("x", 0 | 1) => Ok(false),
         _ => Ok(true),
     });
@@ -135,7 +138,14 @@ fn test_var_def_filter() -> Result<(), Box<EvalAltResult>> {
         124
     );
 
-    assert!(engine.run("let x = 42;").is_err());
+    assert!(matches!(
+        *engine.compile("let x = 42;").expect_err("should error").0,
+        ParseErrorType::ForbiddenVariable(s) if s == "x"
+    ));
+    assert!(matches!(
+        *engine.run_ast(&ast).expect_err("should err"),
+        EvalAltResult::ErrorForbiddenVariable(s, _) if s == "x"
+    ));
     assert!(engine.run("const x = 42;").is_err());
     assert!(engine.run("let y = 42; { let x = y + 1; }").is_err());
     assert!(engine.run("let y = 42; { let x = y + 1; }").is_err());