Merge pull request #346 from schungx/master

Fix bugs.

RELEASES.md

@@ -4,6 +4,12 @@ Rhai Release Notes
 Version 0.19.12
 ===============
 
+Bug fixes
+---------
+
+* Empty statements (i.e. statements with only one `;`) now parse correctly and no longer hang.
+* `continue`, `break` and `return` statements no longer panic inside a `try .. catch` block.
+
 Breaking changes
 ----------------
 

codegen/ui_tests/export_fn_raw_return.stderr

@@ -2,9 +2,9 @@ error[E0308]: mismatched types
   --> $DIR/export_fn_raw_return.rs:10:8
    |
 9  | #[export_fn(return_raw)]
-   | ------------------------ expected `std::result::Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>` because of return type
+   | ------------------------ expected `Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>` because of return type
 10 | pub fn test_fn(input: Point) -> bool {
-   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `std::result::Result`, found `bool`
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `Result`, found `bool`
    |
-   = note: expected enum `std::result::Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>`
+   = note: expected enum `Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>`
               found type `bool`

codegen/ui_tests/export_mod_raw_return.stderr

@@ -2,10 +2,10 @@ error[E0308]: mismatched types
   --> $DIR/export_mod_raw_return.rs:12:8
    |
 9  | #[export_module]
-   | ---------------- expected `std::result::Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>` because of return type
+   | ---------------- expected `Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>` because of return type
 ...
 12 | pub fn test_fn(input: Point) -> bool {
-   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `std::result::Result`, found `bool`
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `Result`, found `bool`
    |
-   = note: expected enum `std::result::Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>`
+   = note: expected enum `Result<rhai::Dynamic, std::boxed::Box<rhai::EvalAltResult>>`
               found type `bool`

src/engine.rs

@@ -519,8 +519,8 @@ pub struct State {
     /// Embedded module resolver.
     #[cfg(not(feature = "no_module"))]
     pub resolver: Option<Shared<crate::module::resolvers::StaticModuleResolver>>,
-    /// Cached lookup values for function hashes.
+    /// Functions resolution cache.
-    pub functions_caches: StaticVec<
+    fn_resolution_caches: StaticVec<
         HashMap<
             NonZeroU64,
             Option<(CallableFunction, Option<ImmutableString>)>,
@@ -535,6 +535,48 @@ impl State {
     pub fn is_global(&self) -> bool {
         self.scope_level == 0
     }
+    /// Get the current functions resolution cache.
+    pub fn fn_resolution_cache(
+        &self,
+    ) -> Option<
+        &HashMap<
+            NonZeroU64,
+            Option<(CallableFunction, Option<ImmutableString>)>,
+            StraightHasherBuilder,
+        >,
+    > {
+        self.fn_resolution_caches.last()
+    }
+    /// Get a mutable reference to the current functions resolution cache.
+    pub fn fn_resolution_cache_mut(
+        &mut self,
+    ) -> &mut HashMap<
+        NonZeroU64,
+        Option<(CallableFunction, Option<ImmutableString>)>,
+        StraightHasherBuilder,
+    > {
+        if self.fn_resolution_caches.is_empty() {
+            self.fn_resolution_caches
+                .push(HashMap::with_capacity_and_hasher(16, StraightHasherBuilder));
+        }
+        self.fn_resolution_caches.last_mut().unwrap()
+    }
+    /// Push an empty functions resolution cache onto the stack and make it current.
+    pub fn push_fn_resolution_cache(&mut self) {
+        self.fn_resolution_caches.push(Default::default());
+    }
+    /// Remove the current functions resolution cache and make the last one current.
+    pub fn pop_fn_resolution_cache(&mut self) {
+        self.fn_resolution_caches.pop();
+    }
+    /// Clear the current functions resolution cache.
+    ///
+    /// # Panics
+    ///
+    /// Panics if there is no current functions resolution cache.
+    pub fn clear_fn_resolution_cache(&mut self) {
+        self.fn_resolution_caches.last_mut().unwrap().clear();
+    }
 }
 
 /// _(INTERNALS)_ A type containing all the limits imposed by the [`Engine`].
@@ -1712,7 +1754,7 @@ impl Engine {
 
             // Statement block
             Expr::Stmt(x, _) => {
-                self.eval_stmt_block(scope, mods, state, lib, this_ptr, x.as_ref(), level)
+                self.eval_stmt_block(scope, mods, state, lib, this_ptr, x.as_ref(), true, level)
             }
 
             // lhs[idx_expr]
@@ -1856,45 +1898,49 @@ impl Engine {
         lib: &[&Module],
         this_ptr: &mut Option<&mut Dynamic>,
         statements: impl IntoIterator<Item = &'a Stmt>,
+        restore: bool,
         level: usize,
     ) -> Result<Dynamic, Box<EvalAltResult>> {
-        let mut has_imports = false;
+        let mut _has_imports = false;
         let prev_always_search = state.always_search;
         let prev_scope_len = scope.len();
         let prev_mods_len = mods.len();
-        state.scope_level += 1;
 
-        let result = statements
+        if restore {
-            .into_iter()
+            state.scope_level += 1;
-            .try_fold(Default::default(), |_, stmt| {
-                match stmt {
-                    #[cfg(not(feature = "no_module"))]
-                    Stmt::Import(_, _, _) => {
-                        // When imports list is modified, clear the functions lookup cache
-                        if has_imports {
-                            state.functions_caches.last_mut().map(|c| c.clear());
-                        } else {
-                            state.functions_caches.push(Default::default());
-                        }
-                        has_imports = true;
-                    }
-                    _ => (),
-                }
-
-                self.eval_stmt(scope, mods, state, lib, this_ptr, stmt, level)
-            });
-
-        scope.rewind(prev_scope_len);
-        if has_imports {
-            // If imports list is modified, pop the functions lookup cache
-            state.functions_caches.pop();
         }
-        mods.truncate(prev_mods_len);
-        state.scope_level -= 1;
 
-        // The impact of new local variables goes away at the end of a block
+        let result = statements.into_iter().try_fold(Dynamic::UNIT, |_, stmt| {
-        // because any new variables introduced will go out of scope
+            #[cfg(not(feature = "no_module"))]
-        state.always_search = prev_always_search;
+            match stmt {
+                Stmt::Import(_, _, _) => {
+                    // When imports list is modified, clear the functions lookup cache
+                    if _has_imports {
+                        state.clear_fn_resolution_cache();
+                    } else if restore {
+                        state.push_fn_resolution_cache();
+                        _has_imports = true;
+                    }
+                }
+                _ => (),
+            }
+
+            self.eval_stmt(scope, mods, state, lib, this_ptr, stmt, level)
+        });
+
+        if restore {
+            scope.rewind(prev_scope_len);
+            if _has_imports {
+                // If imports list is modified, pop the functions lookup cache
+                state.pop_fn_resolution_cache();
+            }
+            mods.truncate(prev_mods_len);
+            state.scope_level -= 1;
+
+            // The impact of new local variables goes away at the end of a block
+            // because any new variables introduced will go out of scope
+            state.always_search = prev_always_search;
+        }
 
         result
     }
@@ -2088,7 +2134,7 @@ impl Engine {
 
             // Block scope
             Stmt::Block(statements, _) => {
-                self.eval_stmt_block(scope, mods, state, lib, this_ptr, statements, level)
+                self.eval_stmt_block(scope, mods, state, lib, this_ptr, statements, true, level)
             }
 
             // If statement
@@ -2246,6 +2292,7 @@ impl Engine {
 
                 match result {
                     Ok(_) => result,
+                    Err(err) if err.is_pseudo_error() => Err(err),
                     Err(err) if !err.is_catchable() => Err(err),
                     Err(mut err) => {
                         let value = match *err {

src/engine_api.rs

@@ -1527,13 +1527,16 @@ impl Engine {
         ast: &'a AST,
         level: usize,
     ) -> Result<Dynamic, Box<EvalAltResult>> {
-        let state = &mut State {
+        let mut state: State = Default::default();
-            source: ast.clone_source(),
+        state.source = ast.clone_source();
-            #[cfg(not(feature = "no_module"))]
+        #[cfg(not(feature = "no_module"))]
-            resolver: ast.resolver(),
+        {
-            ..Default::default()
+            state.resolver = ast.resolver();
-        };
+        }
-        self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], level)
+
+        let statements = ast.statements();
+        let lib = &[ast.lib()];
+        self.eval_global_statements(scope, mods, &mut state, statements, lib, level)
     }
     /// Evaluate a file, but throw away the result and only return error (if any).
     /// Useful for when you don't need the result, but still need to keep track of possible errors.
@@ -1596,13 +1599,15 @@ impl Engine {
         ast: &AST,
     ) -> Result<(), Box<EvalAltResult>> {
         let mods = &mut (&self.global_sub_modules).into();
-        let state = &mut State {
+        let mut state: State = Default::default();
-            source: ast.clone_source(),
+        state.source = ast.clone_source();
-            #[cfg(not(feature = "no_module"))]
+        #[cfg(not(feature = "no_module"))]
-            resolver: ast.resolver(),
+        {
-            ..Default::default()
+            state.resolver = ast.resolver();
-        };
+        }
-        self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], 0)?;
+        let statements = ast.statements();
+        let lib = &[ast.lib()];
+        self.eval_global_statements(scope, mods, &mut state, statements, lib, 0)?;
         Ok(())
     }
     /// Call a script function defined in an [`AST`] with multiple arguments.

src/fn_call.rs

@@ -175,15 +175,11 @@ impl Engine {
     ) -> Result<(Dynamic, bool), Box<EvalAltResult>> {
         self.inc_operations(state, pos)?;
 
-        // Check if function access already in the cache
+        let source = state.source.clone();
-        if state.functions_caches.is_empty() {
-            state.functions_caches.push(Default::default());
-        }
 
+        // Check if function access already in the cache
         let func = &*state
-            .functions_caches
+            .fn_resolution_cache_mut()
-            .last_mut()
-            .unwrap()
             .entry(hash_fn)
             .or_insert_with(|| {
                 // Search for the native function
@@ -208,7 +204,7 @@ impl Engine {
                     })
             });
 
-        if let Some((func, source)) = func {
+        if let Some((func, src)) = func {
             assert!(func.is_native());
 
             // Calling pure function but the first argument is a reference?
@@ -216,12 +212,7 @@ impl Engine {
             backup.change_first_arg_to_copy(is_ref && func.is_pure(), args);
 
             // Run external function
-            let source = if source.is_none() {
+            let source = src.as_ref().or_else(|| source.as_ref()).map(|s| s.as_str());
-                state.source.as_ref()
-            } else {
-                source.as_ref()
-            }
-            .map(|s| s.as_str());
             let result = if func.is_plugin_fn() {
                 func.get_plugin_fn()
                     .call((self, fn_name, source, mods, lib).into(), args)
@@ -405,7 +396,7 @@ impl Engine {
 
         let unified_lib = if let Some(ref env_lib) = fn_def.lib {
             unified = true;
-            state.functions_caches.push(Default::default());
+            state.push_fn_resolution_cache();
             lib_merged = Default::default();
             lib_merged.push(env_lib.as_ref());
             lib_merged.extend(lib.iter().cloned());
@@ -477,7 +468,7 @@ impl Engine {
         state.scope_level = orig_scope_level;
 
         if unified {
-            state.functions_caches.pop();
+            state.pop_fn_resolution_cache();
         }
 
         result
@@ -515,13 +506,13 @@ impl Engine {
         // Check if it is already in the cache
         if let Some(state) = state.as_mut() {
             if let Some(hash) = hash_script {
-                match state.functions_caches.last().map_or(None, |c| c.get(&hash)) {
+                match state.fn_resolution_cache().map_or(None, |c| c.get(&hash)) {
                     Some(v) => return v.is_some(),
                     None => (),
                 }
             }
             if let Some(hash) = hash_fn {
-                match state.functions_caches.last().map_or(None, |c| c.get(&hash)) {
+                match state.fn_resolution_cache().map_or(None, |c| c.get(&hash)) {
                     Some(v) => return v.is_some(),
                     None => (),
                 }
@@ -545,24 +536,10 @@ impl Engine {
         if !r {
             if let Some(state) = state.as_mut() {
                 if let Some(hash) = hash_script {
-                    if state.functions_caches.is_empty() {
+                    state.fn_resolution_cache_mut().insert(hash, None);
-                        state.functions_caches.push(Default::default());
-                    }
-                    state
-                        .functions_caches
-                        .last_mut()
-                        .unwrap()
-                        .insert(hash, None);
                 }
                 if let Some(hash) = hash_fn {
-                    if state.functions_caches.is_empty() {
+                    state.fn_resolution_cache_mut().insert(hash, None);
-                        state.functions_caches.push(Default::default());
-                    }
-                    state
-                        .functions_caches
-                        .last_mut()
-                        .unwrap()
-                        .insert(hash, None);
                 }
             }
         }
@@ -653,14 +630,8 @@ impl Engine {
                 let hash_script = hash_script.unwrap();
 
                 // Check if function access already in the cache
-                if state.functions_caches.is_empty() {
-                    state.functions_caches.push(Default::default());
-                }
-
                 let (func, source) = state
-                    .functions_caches
+                    .fn_resolution_cache_mut()
-                    .last_mut()
-                    .unwrap()
                     .entry(hash_script)
                     .or_insert_with(|| {
                         lib.iter()
@@ -768,10 +739,10 @@ impl Engine {
         }
     }
 
-    /// Evaluate a list of statements with an empty state and no `this` pointer.
+    /// Evaluate a list of statements with no `this` pointer.
     /// This is commonly used to evaluate a list of statements in an [`AST`] or a script function body.
     #[inline]
-    pub(crate) fn eval_statements_raw<'a>(
+    pub(crate) fn eval_global_statements<'a>(
         &self,
         scope: &mut Scope,
         mods: &mut Imports,
@@ -780,11 +751,7 @@ impl Engine {
         lib: &[&Module],
         level: usize,
     ) -> Result<Dynamic, Box<EvalAltResult>> {
-        statements
+        self.eval_stmt_block(scope, mods, state, lib, &mut None, statements, false, level)
-            .into_iter()
-            .try_fold(Dynamic::UNIT, |_, stmt| {
-                self.eval_stmt(scope, mods, state, lib, &mut None, stmt, level)
-            })
             .or_else(|err| match *err {
                 EvalAltResult::Return(out, _) => Ok(out),
                 EvalAltResult::LoopBreak(_, _) => {
@@ -826,14 +793,12 @@ impl Engine {
         }
 
         // Evaluate the AST
-        let mut new_state = State {
+        let mut new_state: State = Default::default();
-            source: state.source.clone(),
+        new_state.source = state.source.clone();
-            operations: state.operations,
+        new_state.operations = state.operations;
-            ..Default::default()
-        };
 
         let result =
-            self.eval_statements_raw(scope, mods, &mut new_state, ast.statements(), lib, level);
+            self.eval_global_statements(scope, mods, &mut new_state, ast.statements(), lib, level);
 
         state.operations = new_state.operations;
         result

src/module/mod.rs

@@ -374,7 +374,7 @@ impl Module {
 
     /// Generate signatures for all the functions in the [`Module`].
     #[inline(always)]
-    pub fn gen_fn_signatures<'a>(&'a self) -> impl Iterator<Item = String> + 'a {
+    pub fn gen_fn_signatures(&self) -> impl Iterator<Item = String> + '_ {
         self.functions
             .values()
             .filter(|FuncInfo { access, .. }| !access.is_private())
@@ -1691,9 +1691,9 @@ impl Module {
     /// 5) Shared reference to function definition [`ScriptFnDef`][crate::ast::ScriptFnDef].
     #[cfg(not(feature = "no_function"))]
     #[inline(always)]
-    pub(crate) fn iter_script_fn<'a>(
+    pub(crate) fn iter_script_fn(
-        &'a self,
+        &self,
-    ) -> impl Iterator<Item = (FnNamespace, FnAccess, &str, usize, &ScriptFnDef)> + 'a {
+    ) -> impl Iterator<Item = (FnNamespace, FnAccess, &str, usize, &ScriptFnDef)> + '_ {
         self.functions.values().filter(|f| f.func.is_script()).map(
             |FuncInfo {
                  namespace,

src/parser.rs

@@ -2571,7 +2571,10 @@ fn parse_stmt(
 
     match token {
         // ; - empty statement
-        Token::SemiColon => Ok(Stmt::Noop(settings.pos)),
+        Token::SemiColon => {
+            eat_token(input, Token::SemiColon);
+            Ok(Stmt::Noop(settings.pos))
+        }
 
         // { - statements block
         Token::LeftBrace => Ok(parse_block(input, state, lib, settings.level_up())?),

src/result.rs

@@ -272,6 +272,15 @@ impl<T: AsRef<str>> From<T> for Box<EvalAltResult> {
 }
 
 impl EvalAltResult {
+    /// Is this a pseudo error?  A pseudo error is one that does not occur naturally.
+    ///
+    /// [`LoopBreak`][EvalAltResult::LoopBreak] or [`Return`][EvalAltResult::Return] are pseudo errors.
+    pub fn is_pseudo_error(&self) -> bool {
+        match self {
+            Self::LoopBreak(_, _) | Self::Return(_, _) => true,
+            _ => false,
+        }
+    }
     /// Can this error be caught?
     ///
     /// # Panics

src/scope.rs

@@ -449,14 +449,14 @@ impl<'a> Scope<'a> {
     ///
     /// let mut iter = my_scope.iter();
     ///
-    /// let (name, constant, value) = iter.next().unwrap();
+    /// let (name, is_constant, value) = iter.next().unwrap();
     /// assert_eq!(name, "x");
-    /// assert!(!constant);
+    /// assert!(!is_constant);
     /// assert_eq!(value.cast::<i64>(), 42);
     ///
-    /// let (name, constant, value) = iter.next().unwrap();
+    /// let (name, is_constant, value) = iter.next().unwrap();
     /// assert_eq!(name, "foo");
-    /// assert!(constant);
+    /// assert!(is_constant);
     /// assert_eq!(value.cast::<String>(), "hello");
     /// ```
     #[inline(always)]
@@ -467,7 +467,7 @@ impl<'a> Scope<'a> {
     /// Get an iterator to entries in the [`Scope`].
     /// Shared values are not expanded.
     #[inline(always)]
-    pub fn iter_raw<'x: 'a>(&'x self) -> impl Iterator<Item = (&'a str, bool, &'x Dynamic)> + 'x {
+    pub fn iter_raw(&self) -> impl Iterator<Item = (&str, bool, &Dynamic)> {
         self.names
             .iter()
             .zip(self.values.iter())

tests/eval.rs

@@ -4,14 +4,7 @@ use rhai::{Engine, EvalAltResult, LexError, ParseErrorType, RegisterFn, Scope, I
 fn test_eval() -> Result<(), Box<EvalAltResult>> {
     let engine = Engine::new();
 
-    assert_eq!(
+    assert_eq!(engine.eval::<INT>(r#"eval("40 + 2")"#)?, 42);
-        engine.eval::<INT>(
-            r#"
-                eval("40 + 2")
-            "#
-        )?,
-        42
-    );
 
     Ok(())
 }
@@ -62,7 +55,7 @@ fn test_eval_function() -> Result<(), Box<EvalAltResult>> {
                 script +=    "x + y";
 
                 eval(script) + x + y
-    "#
+            "#
         )?,
         84
     );