Fix encapsulated environment in module functions.

CHANGELOG.md

@@ -12,6 +12,7 @@ Bug fixes
 * In `Scope::clone_visible`, constants are now properly cloned as constants.
 * Variables introduced inside `try` blocks are now properly cleaned up upon an exception.
 * Off-by-one error in character positions after a comment line is now fixed.
+* Globally-defined constants are now encapsulated correctly inside a loaded module and no longer spill across call boundaries.
 
 Script-breaking changes
 -----------------------

src/ast/mod.rs

@@ -11,6 +11,9 @@ pub use ast::{ASTNode, AST};
 pub use expr::{BinaryExpr, CustomExpr, Expr, FnCallExpr, FnCallHashes};
 pub use flags::{FnAccess, OptionFlags, AST_OPTION_FLAGS};
 pub use ident::Ident;
+#[cfg(not(feature = "no_module"))]
+#[cfg(not(feature = "no_function"))]
+pub use script_fn::EncapsulatedEnviron;
 #[cfg(not(feature = "no_function"))]
 pub use script_fn::{ScriptFnDef, ScriptFnMetadata};
 pub use stmt::{ConditionalStmtBlock, OpAssignment, Stmt, StmtBlock, SwitchCases, TryCatchBlock};

src/ast/script_fn.rs

@@ -2,24 +2,43 @@
 #![cfg(not(feature = "no_function"))]
 
 use super::{FnAccess, StmtBlock};
-use crate::{Identifier, Module, Shared, StaticVec};
+use crate::{Identifier, StaticVec};
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
 use std::{fmt, hash::Hash};
 
+/// _(internals)_ Encapsulated AST environment.
+/// Exported under the `internals` feature only.
+///
+/// 1) other functions defined within the same AST
+/// 2) the stack of imported [modules][crate::Module]
+/// 3) global constants
+///
+/// Not available under `no_module` or `no_function`.
+#[cfg(not(feature = "no_module"))]
+#[cfg(not(feature = "no_function"))]
+#[derive(Debug, Clone)]
+pub struct EncapsulatedEnviron {
+    /// Functions defined within the same [`AST`][crate::AST].
+    pub lib: crate::Shared<crate::Module>,
+    /// Imported [modules][crate::Module].
+    pub imports: Box<[(Identifier, crate::Shared<crate::Module>)]>,
+    /// Globally-defined constants.
+    pub constants: Option<crate::eval::GlobalConstants>,
+}
+
 /// _(internals)_ A type containing information on a script-defined function.
 /// Exported under the `internals` feature only.
 #[derive(Debug, Clone)]
 pub struct ScriptFnDef {
     /// Function body.
     pub body: StmtBlock,
-    /// Encapsulated running environment, if any.
+    /// Encapsulated AST environment, if any.
-    pub lib: Option<Shared<Module>>,
-    /// Encapsulated stack of imported modules, if any.
     ///
-    /// Not available under `no_module`.
+    /// Not available under `no_module` or `no_function`.
     #[cfg(not(feature = "no_module"))]
-    pub global: Option<Box<[(Identifier, Shared<Module>)]>>,
+    #[cfg(not(feature = "no_function"))]
+    pub environ: Option<EncapsulatedEnviron>,
     /// Function name.
     pub name: Identifier,
     /// Function access mode.

src/bin/rhai-repl.rs

@@ -295,6 +295,7 @@ fn main() {
         engine.set_module_resolver(resolver);
     }
 
+    // Register sample functions
     engine
         .register_fn("test", |x: INT, y: INT| format!("{} {}", x, y))
         .register_fn("test", |x: &mut INT, y: INT, z: &str| {

src/eval/global_state.rs

@@ -5,6 +5,12 @@ use crate::{Engine, Identifier};
 use std::prelude::v1::*;
 use std::{fmt, marker::PhantomData};
 
+/// Collection of globally-defined constants.
+#[cfg(not(feature = "no_module"))]
+#[cfg(not(feature = "no_function"))]
+pub type GlobalConstants =
+    crate::Shared<crate::Locked<std::collections::BTreeMap<Identifier, crate::Dynamic>>>;
+
 /// _(internals)_ Global runtime states.
 /// Exported under the `internals` feature only.
 //
@@ -44,9 +50,7 @@ pub struct GlobalRuntimeState<'a> {
     /// Interior mutability is needed because it is shared in order to aid in cloning.
     #[cfg(not(feature = "no_module"))]
     #[cfg(not(feature = "no_function"))]
-    pub(crate) constants: Option<
+    pub(crate) constants: Option<GlobalConstants>,
-        crate::Shared<crate::Locked<std::collections::BTreeMap<Identifier, crate::Dynamic>>>,
-    >,
     /// Debugging interface.
     #[cfg(feature = "debugging")]
     pub debugger: super::Debugger,

src/eval/mod.rs

@@ -17,5 +17,8 @@ pub use debugger::CallStackFrame;
 pub use debugger::{BreakPoint, Debugger, DebuggerCommand, OnDebuggerCallback, OnDebuggingInit};
 pub use eval_context::EvalContext;
 pub use eval_state::EvalState;
+#[cfg(not(feature = "no_module"))]
+#[cfg(not(feature = "no_function"))]
+pub use global_state::GlobalConstants;
 pub use global_state::GlobalRuntimeState;
 pub use target::{calc_index, calc_offset_len, Target};

src/func/call.rs

@@ -646,13 +646,12 @@ impl Engine {
             };
 
             mem::swap(&mut global.source, &mut source);
+            let level = _level + 1;
 
             let result = if _is_method_call {
                 // Method call of script function - map first argument to `this`
                 let (first_arg, rest_args) = args.split_first_mut().unwrap();
 
-                let level = _level + 1;
-
                 let result = self.call_script_fn(
                     scope,
                     global,
@@ -679,8 +678,6 @@ impl Engine {
                         .change_first_arg_to_copy(args);
                 }
 
-                let level = _level + 1;
-
                 let result = self.call_script_fn(
                     scope, global, state, lib, &mut None, func, args, pos, true, level,
                 );

src/func/script.rs

@@ -4,7 +4,7 @@
 use super::call::FnCallArgs;
 use crate::ast::ScriptFnDef;
 use crate::eval::{EvalState, GlobalRuntimeState};
-use crate::{Dynamic, Engine, Module, Position, RhaiError, RhaiResult, Scope, StaticVec, ERR};
+use crate::{Dynamic, Engine, Module, Position, RhaiError, RhaiResult, Scope, ERR};
 use std::mem;
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
@@ -41,13 +41,19 @@ impl Engine {
             err: RhaiError,
             pos: Position,
         ) -> RhaiResult {
+            let _fn_def = fn_def;
+
+            #[cfg(not(feature = "no_module"))]
+            let source = _fn_def
+                .environ
+                .as_ref()
+                .and_then(|environ| environ.lib.id().map(str::to_string));
+            #[cfg(feature = "no_module")]
+            let source = None;
+
             Err(ERR::ErrorInFunctionCall(
                 name,
-                fn_def
+                source.unwrap_or_else(|| global.source.to_string()),
-                    .lib
-                    .as_ref()
-                    .and_then(|m| m.id().map(str::to_string))
-                    .unwrap_or_else(|| global.source.to_string()),
                 err,
                 pos,
             )
@@ -98,10 +104,23 @@ impl Engine {
         );
 
         // Merge in encapsulated environment, if any
-        let mut lib_merged = StaticVec::with_capacity(lib.len() + 1);
         let orig_fn_resolution_caches_len = state.fn_resolution_caches_len();
 
-        let lib = if let Some(ref fn_lib) = fn_def.lib {
+        #[cfg(not(feature = "no_module"))]
+        let mut lib_merged = crate::StaticVec::with_capacity(lib.len() + 1);
+
+        #[cfg(not(feature = "no_module"))]
+        let (lib, constants) = if let Some(crate::ast::EncapsulatedEnviron {
+            lib: ref fn_lib,
+            ref imports,
+            #[cfg(not(feature = "no_function"))]
+            ref constants,
+        }) = fn_def.environ
+        {
+            for (n, m) in imports.iter().cloned() {
+                global.push_import(n, m)
+            }
+            (
                 if fn_lib.is_empty() {
                     lib
                 } else {
@@ -109,18 +128,16 @@ impl Engine {
                     lib_merged.push(fn_lib.as_ref());
                     lib_merged.extend(lib.iter().cloned());
                     &lib_merged
-            }
+                },
+                #[cfg(not(feature = "no_function"))]
+                Some(mem::replace(&mut global.constants, constants.clone())),
+                #[cfg(feature = "no_function")]
+                None,
+            )
         } else {
-            lib
+            (lib, None)
         };
 
-        #[cfg(not(feature = "no_module"))]
-        if let Some(ref modules) = fn_def.global {
-            for (n, m) in modules.iter().cloned() {
-                global.push_import(n, m)
-            }
-        }
-
         // Evaluate the function
         let result = self
             .eval_stmt_block(
@@ -165,6 +182,12 @@ impl Engine {
         #[cfg(not(feature = "no_module"))]
         global.truncate_imports(orig_imports_len);
 
+        // Restore constants
+        #[cfg(not(feature = "no_function"))]
+        if let Some(constants) = constants {
+            global.constants = constants;
+        }
+
         // Restore state
         state.rewind_fn_resolution_caches(orig_fn_resolution_caches_len);
 

src/lib.rs

@@ -259,6 +259,11 @@ pub use ast::{
     AST_OPTION_FLAGS::*,
 };
 
+#[cfg(feature = "internals")]
+#[cfg(not(feature = "no_module"))]
+#[cfg(not(feature = "no_function"))]
+pub use ast::EncapsulatedEnviron;
+
 #[cfg(feature = "internals")]
 #[cfg(not(feature = "no_float"))]
 pub use ast::FloatWrapper;

src/module/mod.rs

@@ -1696,33 +1696,21 @@ impl Module {
         let mut module = Module::new();
 
         // Extra modules left become sub-modules
-        #[cfg(not(feature = "no_function"))]
+        let mut imports = StaticVec::new_const();
-        let mut func_global = None;
 
         if result.is_ok() {
             global
                 .scan_imports_raw()
                 .skip(orig_imports_len)
                 .for_each(|(k, m)| {
-                    #[cfg(not(feature = "no_function"))]
+                    imports.push((k.clone(), m.clone()));
-                    if func_global.is_none() {
-                        func_global = Some(StaticVec::new());
-                    }
-                    #[cfg(not(feature = "no_function"))]
-                    func_global
-                        .as_mut()
-                        .expect("`Some`")
-                        .push((k.clone(), m.clone()));
-
                     module.set_sub_module(k.clone(), m.clone());
                 });
         }
 
         // Restore global state
         #[cfg(not(feature = "no_function"))]
-        {
+        let constants = std::mem::replace(&mut global.constants, orig_constants);
-            global.constants = orig_constants;
-        }
         global.truncate_imports(orig_imports_len);
         global.source = orig_source;
 
@@ -1747,12 +1735,9 @@ impl Module {
             }
         }
 
-        #[cfg(not(feature = "no_function"))]
-        let func_global = func_global.map(|v| v.into_boxed_slice());
-
         // Non-private functions defined become module functions
         #[cfg(not(feature = "no_function"))]
-        if ast.has_functions() {
+        {
             ast.shared_lib()
                 .iter_fn()
                 .filter(|&f| match f.metadata.access {
@@ -1761,15 +1746,20 @@ impl Module {
                 })
                 .filter(|&f| f.func.is_script())
                 .for_each(|f| {
-                    // Encapsulate AST environment
                     let mut func = f
                         .func
                         .get_script_fn_def()
                         .expect("script-defined function")
                         .as_ref()
                         .clone();
-                    func.lib = Some(ast.shared_lib().clone());
+
-                    func.global = func_global.clone();
+                    // Encapsulate AST environment
+                    func.environ = Some(crate::ast::EncapsulatedEnviron {
+                        lib: ast.shared_lib().clone(),
+                        imports: imports.clone().into_boxed_slice(),
+                        constants: constants.clone(),
+                    });
+
                     module.set_script_fn(func);
                 });
         }

src/optimizer.rs

@@ -1211,9 +1211,8 @@ pub fn optimize_into_ast(
                     access: fn_def.access,
                     body: crate::ast::StmtBlock::NONE,
                     params: fn_def.params.clone(),
-                    lib: None,
                     #[cfg(not(feature = "no_module"))]
-                    global: None,
+                    environ: None,
                     #[cfg(not(feature = "no_function"))]
                     #[cfg(feature = "metadata")]
                     comments: None,

src/parser.rs

@@ -3185,9 +3185,8 @@ fn parse_fn(
         access,
         params,
         body,
-        lib: None,
         #[cfg(not(feature = "no_module"))]
-        global: None,
+        environ: None,
         #[cfg(not(feature = "no_function"))]
         #[cfg(feature = "metadata")]
         comments: if comments.is_empty() {
@@ -3341,9 +3340,8 @@ fn parse_anon_fn(
         access: crate::FnAccess::Public,
         params,
         body: body.into(),
-        lib: None,
         #[cfg(not(feature = "no_module"))]
-        global: None,
+        environ: None,
         #[cfg(not(feature = "no_function"))]
         #[cfg(feature = "metadata")]
         comments: None,

tests/modules.rs

@@ -514,3 +514,47 @@ fn test_module_file() -> Result<(), Box<EvalAltResult>> {
     Module::eval_ast_as_new(Scope::new(), &ast, &engine)?;
     Ok(())
 }
+
+#[cfg(not(feature = "no_function"))]
+#[test]
+fn test_module_environ() -> Result<(), Box<EvalAltResult>> {
+    let mut engine = Engine::new();
+
+    let ast = engine.compile(
+        r#"
+            const SECRET = 42;
+
+            fn foo(x) {
+                print(global::SECRET);
+                global::SECRET + x
+            }
+        "#,
+    )?;
+
+    let mut m = Module::eval_ast_as_new(Scope::new(), &ast, &engine)?;
+
+    m.set_id("test");
+    m.build_index();
+
+    engine.register_static_module("test", m.into());
+
+    assert_eq!(
+        engine.eval::<String>(
+            r#"
+                const SECRET = "hello";
+
+                fn foo(x) {
+                    print(global::SECRET);
+                    global::SECRET + x
+                }
+
+                let t = test::foo(0);
+
+                foo(t)
+            "#
+        )?,
+        "hello42"
+    );
+
+    Ok(())
+}