From 7b92a80c32a68b52000a7745ac0e278d8eca6d6d Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sun, 30 Jan 2022 17:27:13 +0800 Subject: [PATCH] Fix encapsulated environment in module functions. --- CHANGELOG.md | 1 + src/ast/mod.rs | 3 ++ src/ast/script_fn.rs | 31 +++++++++++++++---- src/bin/rhai-repl.rs | 1 + src/eval/global_state.rs | 10 ++++-- src/eval/mod.rs | 3 ++ src/func/call.rs | 5 +-- src/func/script.rs | 67 +++++++++++++++++++++++++++------------- src/lib.rs | 5 +++ src/module/mod.rs | 34 +++++++------------- src/optimizer.rs | 3 +- src/parser.rs | 6 ++-- tests/modules.rs | 44 ++++++++++++++++++++++++++ 13 files changed, 150 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c9de46..8595e784 100644 --- a/CHANGELOG.md +++ b/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 ----------------------- diff --git a/src/ast/mod.rs b/src/ast/mod.rs index aebf79fc..009b1199 100644 --- a/src/ast/mod.rs +++ b/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}; diff --git a/src/ast/script_fn.rs b/src/ast/script_fn.rs index a9d924e7..0daf388f 100644 --- a/src/ast/script_fn.rs +++ b/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, + /// Imported [modules][crate::Module]. + pub imports: Box<[(Identifier, crate::Shared)]>, + /// Globally-defined constants. + pub constants: Option, +} + /// _(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. - pub lib: Option>, - /// Encapsulated stack of imported modules, if any. + /// Encapsulated AST environment, if any. /// - /// Not available under `no_module`. + /// Not available under `no_module` or `no_function`. #[cfg(not(feature = "no_module"))] - pub global: Option)]>>, + #[cfg(not(feature = "no_function"))] + pub environ: Option, /// Function name. pub name: Identifier, /// Function access mode. diff --git a/src/bin/rhai-repl.rs b/src/bin/rhai-repl.rs index 6e964182..9a6f68d3 100644 --- a/src/bin/rhai-repl.rs +++ b/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| { diff --git a/src/eval/global_state.rs b/src/eval/global_state.rs index fa7979b7..22f2574f 100644 --- a/src/eval/global_state.rs +++ b/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>>; + /// _(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< - crate::Shared>>, - >, + pub(crate) constants: Option, /// Debugging interface. #[cfg(feature = "debugging")] pub debugger: super::Debugger, diff --git a/src/eval/mod.rs b/src/eval/mod.rs index b76619e4..0609c9b8 100644 --- a/src/eval/mod.rs +++ b/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}; diff --git a/src/func/call.rs b/src/func/call.rs index f7c45e27..95ea4758 100644 --- a/src/func/call.rs +++ b/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, ); diff --git a/src/func/script.rs b/src/func/script.rs index c087053e..32d13f4d 100644 --- a/src/func/script.rs +++ b/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 - .lib - .as_ref() - .and_then(|m| m.id().map(str::to_string)) - .unwrap_or_else(|| global.source.to_string()), + source.unwrap_or_else(|| global.source.to_string()), err, pos, ) @@ -98,28 +104,39 @@ 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 { - if fn_lib.is_empty() { - lib - } else { - state.push_fn_resolution_cache(); - lib_merged.push(fn_lib.as_ref()); - lib_merged.extend(lib.iter().cloned()); - &lib_merged - } - } else { - lib - }; + #[cfg(not(feature = "no_module"))] + let mut lib_merged = crate::StaticVec::with_capacity(lib.len() + 1); #[cfg(not(feature = "no_module"))] - if let Some(ref modules) = fn_def.global { - for (n, m) in modules.iter().cloned() { + 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 { + state.push_fn_resolution_cache(); + 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, None) + }; // Evaluate the function let result = self @@ -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); diff --git a/src/lib.rs b/src/lib.rs index 3e514dd8..e7d347d0 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/module/mod.rs b/src/module/mod.rs index e842fc1d..d9b16a17 100644 --- a/src/module/mod.rs +++ b/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 func_global = None; + let mut imports = StaticVec::new_const(); if result.is_ok() { global .scan_imports_raw() .skip(orig_imports_len) .for_each(|(k, m)| { - #[cfg(not(feature = "no_function"))] - 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())); - + imports.push((k.clone(), m.clone())); module.set_sub_module(k.clone(), m.clone()); }); } // Restore global state #[cfg(not(feature = "no_function"))] - { - global.constants = orig_constants; - } + let constants = std::mem::replace(&mut 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); }); } diff --git a/src/optimizer.rs b/src/optimizer.rs index 7e2cb6e7..d430ac76 100644 --- a/src/optimizer.rs +++ b/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, diff --git a/src/parser.rs b/src/parser.rs index 21d12147..a24af289 100644 --- a/src/parser.rs +++ b/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, diff --git a/tests/modules.rs b/tests/modules.rs index 9b3e62d4..db0038e7 100644 --- a/tests/modules.rs +++ b/tests/modules.rs @@ -514,3 +514,47 @@ fn test_module_file() -> Result<(), Box> { Module::eval_ast_as_new(Scope::new(), &ast, &engine)?; Ok(()) } + +#[cfg(not(feature = "no_function"))] +#[test] +fn test_module_environ() -> Result<(), Box> { + 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::( + r#" + const SECRET = "hello"; + + fn foo(x) { + print(global::SECRET); + global::SECRET + x + } + + let t = test::foo(0); + + foo(t) + "# + )?, + "hello42" + ); + + Ok(()) +}