From 0f1f6c4ad3e8e5614c24a64b25ff1b3231764be5 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 9 Jan 2021 00:24:55 +0800 Subject: [PATCH 1/4] Add Engine::compile_to_self_contained. --- RELEASES.md | 5 +++ src/ast.rs | 110 +++++++++++++++++++++++++++++++++++++++++++++- src/engine.rs | 22 ++++++++-- src/engine_api.rs | 57 ++++++++++++++++++++++++ tests/modules.rs | 19 +++++++- 5 files changed, 206 insertions(+), 7 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index c014d5e6..debb5010 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -11,6 +11,11 @@ Breaking changes * `ParseErrorType::WrongFnDefinition` is renamed `FnWrongDefinition`. * Redefining an existing function within the same script now throws a new `ParseErrorType::FnDuplicatedDefinition`. This is to prevent accidental overwriting an earlier function definition. +New features +------------ + +* `Engine::compile_to_self_contained` compiles a script into an `AST` and _eagerly_ resolves all `import` statements with string literal paths. The resolved modules are directly embedded into the `AST`. When the `AST` is later evaluated, `import` statements directly yield the pre-resolved modules without going through the resolution process once again. + Enhancements ------------ diff --git a/src/ast.rs b/src/ast.rs index 4891878b..9be4301b 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -2,7 +2,7 @@ use crate::dynamic::{AccessMode, Union}; use crate::fn_native::shared_make_mut; -use crate::module::NamespaceRef; +use crate::module::{resolvers::StaticModuleResolver, NamespaceRef}; use crate::stdlib::{ borrow::Cow, boxed::Box, @@ -170,6 +170,8 @@ pub struct AST { statements: Vec, /// Script-defined functions. functions: Shared, + /// Embedded module resolver, if any. + resolver: Option>, } impl Default for AST { @@ -179,6 +181,7 @@ impl Default for AST { source: None, statements: Vec::with_capacity(16), functions: Default::default(), + resolver: None, } } } @@ -194,6 +197,7 @@ impl AST { source: None, statements: statements.into_iter().collect(), functions: functions.into(), + resolver: None, } } /// Create a new [`AST`] with a source name. @@ -207,6 +211,7 @@ impl AST { source: Some(source.into()), statements: statements.into_iter().collect(), functions: functions.into(), + resolver: None, } } /// Get the source. @@ -269,6 +274,28 @@ impl AST { pub fn lib(&self) -> &Module { &self.functions } + /// Get the embedded [module resolver][`ModuleResolver`]. + #[cfg(not(feature = "internals"))] + #[inline(always)] + pub(crate) fn shared_resolver(&self) -> Option> { + self.resolver.clone() + } + /// _(INTERNALS)_ Get the embedded [module resolver][`ModuleResolver`]. + /// Exported under the `internals` feature only. + #[cfg(feature = "internals")] + #[inline(always)] + pub fn resolver(&self) -> Option<&dyn crate::ModuleResolver> { + self.resolver.map(|r| &*r) + } + /// Set the embedded [module resolver][`ModuleResolver`]. + #[inline(always)] + pub(crate) fn set_resolver( + &mut self, + resolver: impl Into>, + ) -> &mut Self { + self.resolver = Some(resolver.into()); + self + } /// Clone the [`AST`]'s functions into a new [`AST`]. /// No statements are cloned. /// @@ -298,6 +325,7 @@ impl AST { source: self.source.clone(), statements: Default::default(), functions: functions.into(), + resolver: self.resolver.clone(), } } /// Clone the [`AST`]'s script statements into a new [`AST`]. @@ -308,6 +336,7 @@ impl AST { source: self.source.clone(), statements: self.statements.clone(), functions: Default::default(), + resolver: self.resolver.clone(), } } /// Merge two [`AST`] into one. Both [`AST`]'s are untouched and a new, merged, version @@ -605,6 +634,16 @@ impl AST { /// Not available under [`no_function`]. #[cfg(not(feature = "no_function"))] #[inline(always)] + pub(crate) fn iter_fn_def(&self) -> impl Iterator { + self.functions + .iter_script_fn() + .map(|(_, _, _, _, fn_def)| fn_def) + } + /// Iterate through all function definitions. + /// + /// Not available under [`no_function`]. + #[cfg(not(feature = "no_function"))] + #[inline(always)] pub fn iter_functions<'a>(&'a self) -> impl Iterator + 'a { self.functions .iter_script_fn() @@ -888,6 +927,53 @@ impl Stmt { Self::Share(_) => false, } } + /// Recursively walk this statement. + #[inline(always)] + pub fn walk(&self, process_stmt: &mut impl FnMut(&Stmt), process_expr: &mut impl FnMut(&Expr)) { + process_stmt(self); + + match self { + Self::Let(_, Some(e), _, _) | Self::Const(_, Some(e), _, _) => { + e.walk(process_stmt, process_expr) + } + Self::If(e, x, _) => { + e.walk(process_stmt, process_expr); + x.0.walk(process_stmt, process_expr); + if let Some(ref s) = x.1 { + s.walk(process_stmt, process_expr); + } + } + Self::Switch(e, x, _) => { + e.walk(process_stmt, process_expr); + x.0.values() + .for_each(|s| s.walk(process_stmt, process_expr)); + if let Some(ref s) = x.1 { + s.walk(process_stmt, process_expr); + } + } + Self::While(e, s, _) | Self::Do(s, e, _, _) => { + e.walk(process_stmt, process_expr); + s.walk(process_stmt, process_expr); + } + Self::For(e, x, _) => { + e.walk(process_stmt, process_expr); + x.1.walk(process_stmt, process_expr); + } + Self::Assignment(x, _) => { + x.0.walk(process_stmt, process_expr); + x.2.walk(process_stmt, process_expr); + } + Self::Block(x, _) => x.iter().for_each(|s| s.walk(process_stmt, process_expr)), + Self::TryCatch(x, _, _) => { + x.0.walk(process_stmt, process_expr); + x.2.walk(process_stmt, process_expr); + } + Self::Expr(e) | Self::Return(_, Some(e), _) | Self::Import(e, _, _) => { + e.walk(process_stmt, process_expr) + } + _ => (), + } + } } /// _(INTERNALS)_ A custom syntax definition. @@ -1308,6 +1394,28 @@ impl Expr { Self::Custom(_, _) => false, } } + /// Recursively walk this expression. + #[inline(always)] + pub fn walk(&self, process_stmt: &mut impl FnMut(&Stmt), process_expr: &mut impl FnMut(&Expr)) { + process_expr(self); + + match self { + Self::Stmt(x, _) => x.iter().for_each(|s| s.walk(process_stmt, process_expr)), + Self::Array(x, _) => x.iter().for_each(|e| e.walk(process_stmt, process_expr)), + Self::Map(x, _) => x + .iter() + .for_each(|(_, e)| e.walk(process_stmt, process_expr)), + Self::Index(x, _) | Expr::In(x, _) | Expr::And(x, _) | Expr::Or(x, _) => { + x.lhs.walk(process_stmt, process_expr); + x.rhs.walk(process_stmt, process_expr); + } + Self::Custom(x, _) => x + .keywords + .iter() + .for_each(|e| e.walk(process_stmt, process_expr)), + _ => (), + } + } } #[cfg(test)] diff --git a/src/engine.rs b/src/engine.rs index 37f279d2..6b220376 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -7,7 +7,7 @@ use crate::fn_native::{ CallableFunction, IteratorFn, OnDebugCallback, OnPrintCallback, OnProgressCallback, OnVarCallback, }; -use crate::module::NamespaceRef; +use crate::module::{resolvers::StaticModuleResolver, NamespaceRef}; use crate::optimize::OptimizationLevel; use crate::packages::{Package, StandardPackage}; use crate::r#unsafe::unsafe_cast_var_name_to_lifetime; @@ -26,8 +26,8 @@ use crate::stdlib::{ use crate::syntax::CustomSyntax; use crate::utils::{get_hasher, StraightHasherBuilder}; use crate::{ - calc_native_fn_hash, Dynamic, EvalAltResult, FnPtr, ImmutableString, Module, Position, Scope, - Shared, StaticVec, + calc_native_fn_hash, Dynamic, EvalAltResult, FnPtr, ImmutableString, Module, ModuleResolver, + Position, Scope, Shared, StaticVec, }; #[cfg(not(feature = "no_index"))] @@ -516,6 +516,8 @@ pub struct State { pub operations: u64, /// Number of modules loaded. pub modules: usize, + /// Embedded module resolver. + pub resolver: Option>, /// Cached lookup values for function hashes. pub functions_cache: HashMap< NonZeroU64, @@ -2328,7 +2330,19 @@ impl Engine { .eval_expr(scope, mods, state, lib, this_ptr, &expr, level)? .try_cast::() { - let module = self.module_resolver.resolve(self, &path, expr.position())?; + let expr_pos = expr.position(); + + let module = state + .resolver + .as_ref() + .and_then(|r| match r.resolve(self, &path, expr_pos) { + Ok(m) => return Some(Ok(m)), + Err(err) => match *err { + EvalAltResult::ErrorModuleNotFound(_, _) => None, + _ => return Some(Err(err)), + }, + }) + .unwrap_or_else(|| self.module_resolver.resolve(self, &path, expr_pos))?; if let Some(name_def) = alias { if !module.is_indexed() { diff --git a/src/engine_api.rs b/src/engine_api.rs index fed074bf..bb28b8c0 100644 --- a/src/engine_api.rs +++ b/src/engine_api.rs @@ -889,6 +889,61 @@ impl Engine { pub fn compile_with_scope(&self, scope: &Scope, script: &str) -> Result { self.compile_scripts_with_scope(scope, &[script]) } + /// Compile a string into an [`AST`] using own scope, which can be used later for evaluation, + /// embedding all imported modules. + /// + /// Modules referred by `import` statements containing literal string paths are eagerly resolved + /// via the current [module resolver][crate::ModuleResolver] and embedded into the resultant + /// [`AST`]. When it is evaluated later, `import` statement directly recall pre-resolved + /// [modules][Module] and the resolution process is not performed again. + /// + /// Not available under `no_module`. + #[cfg(not(feature = "no_module"))] + pub fn compile_into_self_contained( + &self, + scope: &Scope, + script: &str, + ) -> Result> { + use crate::{ + ast::{Expr, Stmt}, + fn_native::shared_take_or_clone, + module::resolvers::StaticModuleResolver, + stdlib::collections::HashMap, + ImmutableString, + }; + + let mut ast = self.compile_scripts_with_scope(scope, &[script])?; + let mut imports = HashMap::::new(); + + ast.statements() + .iter() + .chain(ast.iter_fn_def().map(|f| &f.body)) + .for_each(|stmt| { + stmt.walk( + &mut |stmt| match stmt { + Stmt::Import(Expr::StringConstant(s, pos), _, _) + if !imports.contains_key(s) => + { + imports.insert(s.clone(), *pos); + } + _ => (), + }, + &mut |_| {}, + ) + }); + + if !imports.is_empty() { + let mut resolver = StaticModuleResolver::new(); + for (path, pos) in imports { + let module = self.module_resolver.resolve(self, &path, pos)?; + let module = shared_take_or_clone(module); + resolver.insert(path, module); + } + ast.set_resolver(resolver); + } + + Ok(ast) + } /// When passed a list of strings, first join the strings into one large script, /// and then compile them into an [`AST`] using own scope, which can be used later for evaluation. /// @@ -1457,6 +1512,7 @@ impl Engine { ) -> Result> { let state = &mut State { source: ast.clone_source(), + resolver: ast.shared_resolver(), ..Default::default() }; self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], level) @@ -1524,6 +1580,7 @@ impl Engine { let mods = &mut (&self.global_sub_modules).into(); let state = &mut State { source: ast.clone_source(), + resolver: ast.shared_resolver(), ..Default::default() }; self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], 0)?; diff --git a/tests/modules.rs b/tests/modules.rs index eee696fb..96733e3f 100644 --- a/tests/modules.rs +++ b/tests/modules.rs @@ -1,7 +1,8 @@ #![cfg(not(feature = "no_module"))] use rhai::{ - module_resolvers::StaticModuleResolver, Dynamic, Engine, EvalAltResult, FnNamespace, - ImmutableString, Module, ParseError, ParseErrorType, Scope, INT, + module_resolvers::{DummyModuleResolver, StaticModuleResolver}, + Dynamic, Engine, EvalAltResult, FnNamespace, ImmutableString, Module, ParseError, + ParseErrorType, Scope, INT, }; #[test] @@ -246,6 +247,20 @@ fn test_module_resolver() -> Result<(), Box> { )?; } + let script = r#" + import "hello" as h; + h::answer + "#; + let mut scope = Scope::new(); + + let ast = engine.compile_into_self_contained(&mut scope, script)?; + + engine.set_module_resolver(DummyModuleResolver::new()); + + assert_eq!(engine.eval_ast::(&ast)?, 42); + + assert!(engine.eval::(script).is_err()); + Ok(()) } From 9f71e5b155c9f4edac49ac653fcb122bcb18cab7 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 9 Jan 2021 00:26:49 +0800 Subject: [PATCH 2/4] Revert "Delete benchmark.yml" This reverts commit 277eef4be6ccd179e4ef08b825681ee17ef68bc9. --- .github/workflows/benchmark.yml | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .github/workflows/benchmark.yml diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml new file mode 100644 index 00000000..df310705 --- /dev/null +++ b/.github/workflows/benchmark.yml @@ -0,0 +1,29 @@ +name: Benchmark +on: + push: + branches: + - master + +jobs: + benchmark: + name: Run Rust benchmark + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - run: rustup toolchain update nightly && rustup default nightly + - name: Run benchmark + run: cargo +nightly bench | tee output.txt + - name: Store benchmark result + uses: rhysd/github-action-benchmark@v1 + with: + name: Rust Benchmark + tool: 'cargo' + output-file-path: output.txt + # Use personal access token instead of GITHUB_TOKEN due to https://github.community/t5/GitHub-Actions/Github-action-not-triggering-gh-pages-upon-push/td-p/26869/highlight/false + github-token: ${{ secrets.RHAI }} + auto-push: true + # Show alert with commit comment on detecting possible performance regression + alert-threshold: '200%' + comment-on-alert: true + fail-on-alert: true + alert-comment-cc-users: '@schungx' From bfe9ac2188300d362184b767966927ef2d33c22f Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 9 Jan 2021 00:40:44 +0800 Subject: [PATCH 3/4] Fix feature builds. --- src/ast.rs | 27 +++++++++++++++++++-------- src/engine.rs | 11 +++++++---- src/engine_api.rs | 15 ++++++++++++--- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/ast.rs b/src/ast.rs index 9be4301b..87aeaeb5 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -2,7 +2,7 @@ use crate::dynamic::{AccessMode, Union}; use crate::fn_native::shared_make_mut; -use crate::module::{resolvers::StaticModuleResolver, NamespaceRef}; +use crate::module::NamespaceRef; use crate::stdlib::{ borrow::Cow, boxed::Box, @@ -171,7 +171,8 @@ pub struct AST { /// Script-defined functions. functions: Shared, /// Embedded module resolver, if any. - resolver: Option>, + #[cfg(not(feature = "no_module"))] + resolver: Option>, } impl Default for AST { @@ -181,6 +182,7 @@ impl Default for AST { source: None, statements: Vec::with_capacity(16), functions: Default::default(), + #[cfg(not(feature = "no_module"))] resolver: None, } } @@ -197,6 +199,7 @@ impl AST { source: None, statements: statements.into_iter().collect(), functions: functions.into(), + #[cfg(not(feature = "no_module"))] resolver: None, } } @@ -211,6 +214,7 @@ impl AST { source: Some(source.into()), statements: statements.into_iter().collect(), functions: functions.into(), + #[cfg(not(feature = "no_module"))] resolver: None, } } @@ -275,23 +279,27 @@ impl AST { &self.functions } /// Get the embedded [module resolver][`ModuleResolver`]. - #[cfg(not(feature = "internals"))] + #[cfg(not(feature = "no_module"))] #[inline(always)] - pub(crate) fn shared_resolver(&self) -> Option> { + pub(crate) fn shared_resolver( + &self, + ) -> Option> { self.resolver.clone() } /// _(INTERNALS)_ Get the embedded [module resolver][`ModuleResolver`]. /// Exported under the `internals` feature only. + #[cfg(not(feature = "no_module"))] #[cfg(feature = "internals")] #[inline(always)] pub fn resolver(&self) -> Option<&dyn crate::ModuleResolver> { self.resolver.map(|r| &*r) } /// Set the embedded [module resolver][`ModuleResolver`]. + #[cfg(not(feature = "no_module"))] #[inline(always)] pub(crate) fn set_resolver( &mut self, - resolver: impl Into>, + resolver: impl Into>, ) -> &mut Self { self.resolver = Some(resolver.into()); self @@ -325,6 +333,7 @@ impl AST { source: self.source.clone(), statements: Default::default(), functions: functions.into(), + #[cfg(not(feature = "no_module"))] resolver: self.resolver.clone(), } } @@ -336,6 +345,7 @@ impl AST { source: self.source.clone(), statements: self.statements.clone(), functions: Default::default(), + #[cfg(not(feature = "no_module"))] resolver: self.resolver.clone(), } } @@ -633,6 +643,7 @@ impl AST { /// /// Not available under [`no_function`]. #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_module"))] #[inline(always)] pub(crate) fn iter_fn_def(&self) -> impl Iterator { self.functions @@ -968,9 +979,9 @@ impl Stmt { x.0.walk(process_stmt, process_expr); x.2.walk(process_stmt, process_expr); } - Self::Expr(e) | Self::Return(_, Some(e), _) | Self::Import(e, _, _) => { - e.walk(process_stmt, process_expr) - } + Self::Expr(e) | Self::Return(_, Some(e), _) => e.walk(process_stmt, process_expr), + #[cfg(not(feature = "no_module"))] + Self::Import(e, _, _) => e.walk(process_stmt, process_expr), _ => (), } } diff --git a/src/engine.rs b/src/engine.rs index 6b220376..77f0b8aa 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -7,7 +7,7 @@ use crate::fn_native::{ CallableFunction, IteratorFn, OnDebugCallback, OnPrintCallback, OnProgressCallback, OnVarCallback, }; -use crate::module::{resolvers::StaticModuleResolver, NamespaceRef}; +use crate::module::NamespaceRef; use crate::optimize::OptimizationLevel; use crate::packages::{Package, StandardPackage}; use crate::r#unsafe::unsafe_cast_var_name_to_lifetime; @@ -26,8 +26,8 @@ use crate::stdlib::{ use crate::syntax::CustomSyntax; use crate::utils::{get_hasher, StraightHasherBuilder}; use crate::{ - calc_native_fn_hash, Dynamic, EvalAltResult, FnPtr, ImmutableString, Module, ModuleResolver, - Position, Scope, Shared, StaticVec, + calc_native_fn_hash, Dynamic, EvalAltResult, FnPtr, ImmutableString, Module, Position, Scope, + Shared, StaticVec, }; #[cfg(not(feature = "no_index"))] @@ -517,7 +517,8 @@ pub struct State { /// Number of modules loaded. pub modules: usize, /// Embedded module resolver. - pub resolver: Option>, + #[cfg(not(feature = "no_module"))] + pub resolver: Option>, /// Cached lookup values for function hashes. pub functions_cache: HashMap< NonZeroU64, @@ -2330,6 +2331,8 @@ impl Engine { .eval_expr(scope, mods, state, lib, this_ptr, &expr, level)? .try_cast::() { + use crate::ModuleResolver; + let expr_pos = expr.position(); let module = state diff --git a/src/engine_api.rs b/src/engine_api.rs index bb28b8c0..02775b00 100644 --- a/src/engine_api.rs +++ b/src/engine_api.rs @@ -896,8 +896,6 @@ impl Engine { /// via the current [module resolver][crate::ModuleResolver] and embedded into the resultant /// [`AST`]. When it is evaluated later, `import` statement directly recall pre-resolved /// [modules][Module] and the resolution process is not performed again. - /// - /// Not available under `no_module`. #[cfg(not(feature = "no_module"))] pub fn compile_into_self_contained( &self, @@ -917,7 +915,16 @@ impl Engine { ast.statements() .iter() - .chain(ast.iter_fn_def().map(|f| &f.body)) + .chain({ + #[cfg(not(feature = "no_function"))] + { + ast.iter_fn_def().map(|f| &f.body) + } + #[cfg(feature = "no_function")] + { + crate::stdlib::iter::empty() + } + }) .for_each(|stmt| { stmt.walk( &mut |stmt| match stmt { @@ -1512,6 +1519,7 @@ impl Engine { ) -> Result> { let state = &mut State { source: ast.clone_source(), + #[cfg(not(feature = "no_module"))] resolver: ast.shared_resolver(), ..Default::default() }; @@ -1580,6 +1588,7 @@ impl Engine { let mods = &mut (&self.global_sub_modules).into(); let state = &mut State { source: ast.clone_source(), + #[cfg(not(feature = "no_module"))] resolver: ast.shared_resolver(), ..Default::default() }; From 1513e6ab6a48c2c90f46bdbd8320af375503bb2b Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 9 Jan 2021 00:49:50 +0800 Subject: [PATCH 4/4] Fix internals build. --- src/ast.rs | 7 ++++--- src/engine_api.rs | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ast.rs b/src/ast.rs index 87aeaeb5..72eedf1e 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -280,8 +280,9 @@ impl AST { } /// Get the embedded [module resolver][`ModuleResolver`]. #[cfg(not(feature = "no_module"))] + #[cfg(not(feature = "internals"))] #[inline(always)] - pub(crate) fn shared_resolver( + pub(crate) fn resolver( &self, ) -> Option> { self.resolver.clone() @@ -291,8 +292,8 @@ impl AST { #[cfg(not(feature = "no_module"))] #[cfg(feature = "internals")] #[inline(always)] - pub fn resolver(&self) -> Option<&dyn crate::ModuleResolver> { - self.resolver.map(|r| &*r) + pub fn resolver(&self) -> Option> { + self.resolver.clone() } /// Set the embedded [module resolver][`ModuleResolver`]. #[cfg(not(feature = "no_module"))] diff --git a/src/engine_api.rs b/src/engine_api.rs index 02775b00..d2af0960 100644 --- a/src/engine_api.rs +++ b/src/engine_api.rs @@ -1520,7 +1520,7 @@ impl Engine { let state = &mut State { source: ast.clone_source(), #[cfg(not(feature = "no_module"))] - resolver: ast.shared_resolver(), + resolver: ast.resolver(), ..Default::default() }; self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], level) @@ -1589,7 +1589,7 @@ impl Engine { let state = &mut State { source: ast.clone_source(), #[cfg(not(feature = "no_module"))] - resolver: ast.shared_resolver(), + resolver: ast.resolver(), ..Default::default() }; self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], 0)?;