diff --git a/CHANGELOG.md b/CHANGELOG.md index 01267a2e..ab32c6e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ Bug fixes * Invalid property or method access such as `a.b::c.d` or `a.b::func()` no longer panics but properly returns a syntax error. +Enhancements +------------ + +* Variable definitions are optimized so that shadowed variables are reused as much as possible to reduce memory consumption. + Version 1.5.0 ============= diff --git a/src/ast/ast.rs b/src/ast/ast.rs index b7603111..40119fe5 100644 --- a/src/ast/ast.rs +++ b/src/ast/ast.rs @@ -748,7 +748,7 @@ impl AST { if options.contains(AST_OPTION_CONSTANT) && include_constants || !options.contains(AST_OPTION_CONSTANT) && include_variables => { - let (name, expr) = x.as_ref(); + let (name, expr, ..) = x.as_ref(); if let Some(value) = expr.get_literal_value() { Some((name.as_str(), options.contains(AST_OPTION_CONSTANT), value)) } else { diff --git a/src/ast/stmt.rs b/src/ast/stmt.rs index 88aee7e0..fc45062f 100644 --- a/src/ast/stmt.rs +++ b/src/ast/stmt.rs @@ -11,6 +11,7 @@ use std::{ fmt, hash::Hash, mem, + num::NonZeroUsize, ops::{Deref, DerefMut}, }; @@ -345,14 +346,18 @@ pub enum Stmt { /// * [`AST_OPTION_NEGATED`] = `until` Do(Box<(Expr, StmtBlock)>, OptionFlags, Position), /// `for` `(` id `,` counter `)` `in` expr `{` stmt `}` - For(Box<(Ident, Expr, Option, StmtBlock)>, Position), + For(Box<(Ident, Option, Expr, StmtBlock)>, Position), /// \[`export`\] `let`|`const` id `=` expr /// /// ### Option Flags /// /// * [`AST_OPTION_EXPORTED`] = `export` /// * [`AST_OPTION_CONSTANT`] = `const` - Var(Box<(Ident, Expr)>, OptionFlags, Position), + Var( + Box<(Ident, Expr, Option)>, + OptionFlags, + Position, + ), /// expr op`=` expr Assignment(Box<(Option>, BinaryExpr)>, Position), /// func `(` expr `,` ... `)` @@ -380,12 +385,12 @@ pub enum Stmt { /// * [`AST_OPTION_NONE`] = `return` /// * [`AST_OPTION_BREAK`] = `throw` Return(Option>, OptionFlags, Position), - /// `import` expr `as` var + /// `import` expr `as` alias /// /// Not available under `no_module`. #[cfg(not(feature = "no_module"))] Import(Box<(Expr, Option)>, Position), - /// `export` var `as` var + /// `export` var `as` alias /// /// Not available under `no_module`. #[cfg(not(feature = "no_module"))] @@ -596,7 +601,7 @@ impl Stmt { // For loops can be pure because if the iterable is pure, it is finite, // so infinite loops can never occur. - Self::For(x, ..) => x.1.is_pure() && x.3.iter().all(Stmt::is_pure), + Self::For(x, ..) => x.2.is_pure() && x.3.iter().all(Stmt::is_pure), Self::Var(..) | Self::Assignment(..) | Self::FnCall(..) => false, Self::Block(block, ..) => block.iter().all(|stmt| stmt.is_pure()), @@ -765,7 +770,7 @@ impl Stmt { } } Self::For(x, ..) => { - if !x.1.walk(path, on_node) { + if !x.2.walk(path, on_node) { return false; } for s in x.3.iter() { diff --git a/src/eval/stmt.rs b/src/eval/stmt.rs index 8d7d7358..9ac8c048 100644 --- a/src/eval/stmt.rs +++ b/src/eval/stmt.rs @@ -574,7 +574,7 @@ impl Engine { // For loop Stmt::For(x, ..) => { - let (Ident { name: var_name, .. }, expr, counter, statements) = x.as_ref(); + let (Ident { name: var_name, .. }, counter, expr, statements) = x.as_ref(); let iter_result = self .eval_expr(scope, global, state, lib, this_ptr, expr, level) @@ -818,20 +818,20 @@ impl Engine { } // Let/const statement Stmt::Var(x, options, pos) => { - let var_name = &x.0.name; - let expr = &x.1; + let (Ident { name: var_name, .. }, expr, index) = x.as_ref(); - let entry_type = if options.contains(AST_OPTION_CONSTANT) { + let access = if options.contains(AST_OPTION_CONSTANT) { AccessMode::ReadOnly } else { AccessMode::ReadWrite }; let export = options.contains(AST_OPTION_EXPORTED); + // Check variable definition filter let result = if let Some(ref filter) = self.def_var_filter { let will_shadow = scope.contains(var_name); let nesting_level = state.scope_level; - let is_const = entry_type == AccessMode::ReadOnly; + let is_const = access == AccessMode::ReadOnly; let info = VarDefInfo { name: var_name, is_const, @@ -864,16 +864,18 @@ impl Engine { if let Some(result) = result { result.map(|_| Dynamic::UNIT) } else { + // Evaluate initial value let value_result = self .eval_expr(scope, global, state, lib, this_ptr, expr, level) .map(Dynamic::flatten); - if let Ok(value) = value_result { + if let Ok(mut value) = value_result { let _alias = if !rewind_scope { + // Put global constants into global module #[cfg(not(feature = "no_function"))] #[cfg(not(feature = "no_module"))] if state.scope_level == 0 - && entry_type == AccessMode::ReadOnly + && access == AccessMode::ReadOnly && lib.iter().any(|&m| !m.is_empty()) { if global.constants.is_none() { @@ -896,7 +898,12 @@ impl Engine { None }; - scope.push_dynamic_value(var_name.clone(), entry_type, value); + if let Some(index) = index { + value.set_access_mode(access); + *scope.get_mut_by_index(scope.len() - index.get()) = value; + } else { + scope.push_entry(var_name.clone(), access, value); + } #[cfg(not(feature = "no_module"))] if let Some(alias) = _alias { diff --git a/src/optimizer.rs b/src/optimizer.rs index daad6b3d..412c0f39 100644 --- a/src/optimizer.rs +++ b/src/optimizer.rs @@ -793,7 +793,7 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b } // for id in expr { block } Stmt::For(x, ..) => { - optimize_expr(&mut x.1, state, false); + optimize_expr(&mut x.2, state, false); *x.3 = optimize_stmt_block(mem::take(&mut *x.3), state, false, true, false); } // let id = expr; diff --git a/src/parser.rs b/src/parser.rs index bcee4392..9365a817 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -53,7 +53,7 @@ pub struct ParseState<'e> { /// Encapsulates a local stack with variable names to simulate an actual runtime scope. pub stack: Scope<'e>, /// Size of the local variables stack upon entry of the current block scope. - pub entry_stack_len: usize, + pub block_stack_len: usize, /// Tracks a list of external variables (variables that are not explicitly declared in the scope). #[cfg(not(feature = "no_closure"))] pub external_vars: Vec, @@ -94,7 +94,7 @@ impl<'e> ParseState<'e> { allow_capture: true, interned_strings: StringsInterner::new(), stack: Scope::new(), - entry_stack_len: 0, + block_stack_len: 0, #[cfg(not(feature = "no_module"))] imports: StaticVec::new_const(), } @@ -2587,7 +2587,7 @@ fn parse_for( state.stack.rewind(prev_stack_len); Ok(Stmt::For( - Box::new((loop_var, expr, counter_var, body.into())), + Box::new((loop_var, counter_var, expr, body.into())), settings.pos, )) } @@ -2597,7 +2597,7 @@ fn parse_let( input: &mut TokenStream, state: &mut ParseState, lib: &mut FnLib, - var_type: AccessMode, + access: AccessMode, is_export: bool, settings: ParseSettings, ) -> ParseResult { @@ -2620,7 +2620,7 @@ fn parse_let( if let Some(ref filter) = state.engine.def_var_filter { let will_shadow = state.stack.iter().any(|(v, ..)| v == name.as_ref()); let level = settings.level; - let is_const = var_type == AccessMode::ReadOnly; + let is_const = access == AccessMode::ReadOnly; let info = VarDefInfo { name: &name, is_const, @@ -2648,10 +2648,6 @@ fn parse_let( } let name = state.get_identifier("", name); - let var_def = Ident { - name: name.clone(), - pos, - }; // let name = ... let expr = if match_token(input, Token::Equals).0 { @@ -2667,22 +2663,34 @@ fn parse_let( AST_OPTION_NONE }; - match var_type { + let existing = state.stack.get_index(&name).and_then(|(n, a)| { + if n < state.block_stack_len { + // Defined in parent block + None + } else if a == AccessMode::ReadOnly && access != AccessMode::ReadOnly { + // Overwrite constant + None + } else { + Some(n) + } + }); + + let idx = if let Some(n) = existing { + state.stack.get_mut_by_index(n).set_access_mode(access); + Some(NonZeroUsize::new(state.stack.len() - n).unwrap()) + } else { + state.stack.push_entry(name.as_str(), access, Dynamic::UNIT); + None + }; + + let var_def = (Ident { name, pos }, expr, idx).into(); + + Ok(match access { // let name = expr - AccessMode::ReadWrite => { - state.stack.push(name, ()); - Ok(Stmt::Var((var_def, expr).into(), export, settings.pos)) - } + AccessMode::ReadWrite => Stmt::Var(var_def, export, settings.pos), // const name = { expr:constant } - AccessMode::ReadOnly => { - state.stack.push_constant(name, ()); - Ok(Stmt::Var( - (var_def, expr).into(), - AST_OPTION_CONSTANT + export, - settings.pos, - )) - } - } + AccessMode::ReadOnly => Stmt::Var(var_def, AST_OPTION_CONSTANT + export, settings.pos), + }) } /// Parse an import statement. @@ -2798,8 +2806,8 @@ fn parse_block( let mut statements = Vec::with_capacity(8); - let prev_entry_stack_len = state.entry_stack_len; - state.entry_stack_len = state.stack.len(); + let prev_entry_stack_len = state.block_stack_len; + state.block_stack_len = state.stack.len(); #[cfg(not(feature = "no_module"))] let orig_imports_len = state.imports.len(); @@ -2859,8 +2867,8 @@ fn parse_block( } }; - state.stack.rewind(state.entry_stack_len); - state.entry_stack_len = prev_entry_stack_len; + state.stack.rewind(state.block_stack_len); + state.block_stack_len = prev_entry_stack_len; #[cfg(not(feature = "no_module"))] state.imports.truncate(orig_imports_len); diff --git a/src/types/scope.rs b/src/types/scope.rs index 85fd70d4..728470f9 100644 --- a/src/types/scope.rs +++ b/src/types/scope.rs @@ -37,7 +37,7 @@ const SCOPE_ENTRIES_INLINED: usize = 8; /// /// my_scope.push("z", 40_i64); /// -/// engine.eval_with_scope::<()>(&mut my_scope, "let x = z + 1; z = 0;")?; +/// engine.run(&mut my_scope, "let x = z + 1; z = 0;")?; /// /// assert_eq!(engine.eval_with_scope::(&mut my_scope, "x + 1")?, 42); /// @@ -185,7 +185,7 @@ impl Scope<'_> { /// ``` #[inline(always)] pub fn push(&mut self, name: impl Into, value: impl Variant + Clone) -> &mut Self { - self.push_dynamic_value(name, AccessMode::ReadWrite, Dynamic::from(value)) + self.push_entry(name, AccessMode::ReadWrite, Dynamic::from(value)) } /// Add (push) a new [`Dynamic`] entry to the [`Scope`]. /// @@ -201,7 +201,7 @@ impl Scope<'_> { /// ``` #[inline(always)] pub fn push_dynamic(&mut self, name: impl Into, value: Dynamic) -> &mut Self { - self.push_dynamic_value(name, value.access_mode(), value) + self.push_entry(name, value.access_mode(), value) } /// Add (push) a new constant to the [`Scope`]. /// @@ -224,7 +224,7 @@ impl Scope<'_> { name: impl Into, value: impl Variant + Clone, ) -> &mut Self { - self.push_dynamic_value(name, AccessMode::ReadOnly, Dynamic::from(value)) + self.push_entry(name, AccessMode::ReadOnly, Dynamic::from(value)) } /// Add (push) a new constant with a [`Dynamic`] value to the Scope. /// @@ -247,11 +247,11 @@ impl Scope<'_> { name: impl Into, value: Dynamic, ) -> &mut Self { - self.push_dynamic_value(name, AccessMode::ReadOnly, value) + self.push_entry(name, AccessMode::ReadOnly, value) } /// Add (push) a new entry with a [`Dynamic`] value to the [`Scope`]. #[inline] - pub(crate) fn push_dynamic_value( + pub(crate) fn push_entry( &mut self, name: impl Into, access: AccessMode, @@ -622,7 +622,7 @@ impl> Extend<(K, Dynamic)> for Scope<'_> { #[inline] fn extend>(&mut self, iter: T) { for (name, value) in iter { - self.push_dynamic_value(name, AccessMode::ReadWrite, value); + self.push_entry(name, AccessMode::ReadWrite, value); } } } @@ -640,7 +640,7 @@ impl> Extend<(K, bool, Dynamic)> for Scope<'_> { #[inline] fn extend>(&mut self, iter: T) { for (name, is_constant, value) in iter { - self.push_dynamic_value( + self.push_entry( name, if is_constant { AccessMode::ReadOnly diff --git a/tests/optimizer.rs b/tests/optimizer.rs index cac15cf2..886b4fff 100644 --- a/tests/optimizer.rs +++ b/tests/optimizer.rs @@ -84,7 +84,7 @@ fn test_optimizer_parse() -> Result<(), Box> { assert_eq!( format!("{:?}", ast), - r#"AST { body: [Var(("DECISION" @ 1:7, false @ 1:18), (Constant), 1:1), Expr(123 @ 1:51)] }"# + r#"AST { body: [Var(("DECISION" @ 1:7, false @ 1:18, None), (Constant), 1:1), Expr(123 @ 1:51)] }"# ); let ast = engine.compile("if 1 == 2 { 42 }")?; diff --git a/tests/options.rs b/tests/options.rs index d7e45780..803a5802 100644 --- a/tests/options.rs +++ b/tests/options.rs @@ -25,11 +25,15 @@ fn test_options_allow() -> Result<(), Box> { assert!(engine.compile("let x = || 42;").is_err()); } - engine.compile("while x > y { foo(z); }")?; + let ast = engine.compile("let x = 0; while x < 10 { x += 1; }")?; engine.set_allow_looping(false); - assert!(engine.compile("while x > y { foo(z); }").is_err()); + engine.run_ast(&ast)?; + + assert!(engine + .compile("let x = 0; while x < 10 { x += 1; }") + .is_err()); engine.compile("let x = 42; let x = 123;")?; diff --git a/tests/var_scope.rs b/tests/var_scope.rs index 84678747..813dfe5a 100644 --- a/tests/var_scope.rs +++ b/tests/var_scope.rs @@ -5,17 +5,67 @@ fn test_var_scope() -> Result<(), Box> { let engine = Engine::new(); let mut scope = Scope::new(); - engine.eval_with_scope::<()>(&mut scope, "let x = 4 + 5")?; + engine.run_with_scope(&mut scope, "let x = 4 + 5")?; assert_eq!(engine.eval_with_scope::(&mut scope, "x")?, 9); - engine.eval_with_scope::<()>(&mut scope, "x += 1; x += 2;")?; + engine.run_with_scope(&mut scope, "x += 1; x += 2;")?; assert_eq!(engine.eval_with_scope::(&mut scope, "x")?, 12); scope.set_value("x", 42 as INT); assert_eq!(engine.eval_with_scope::(&mut scope, "x")?, 42); - engine.eval_with_scope::<()>(&mut scope, "{let x = 3}")?; + engine.run_with_scope(&mut scope, "{ let x = 3 }")?; assert_eq!(engine.eval_with_scope::(&mut scope, "x")?, 42); + #[cfg(not(feature = "no_optimize"))] + if engine.optimization_level() != rhai::OptimizationLevel::None { + scope.clear(); + engine.run_with_scope(&mut scope, "let x = 3; let x = 42; let x = 123;")?; + assert_eq!(scope.len(), 1); + assert_eq!(scope.get_value::("x").unwrap(), 123); + + scope.clear(); + engine.run_with_scope( + &mut scope, + "let x = 3; let y = 0; let x = 42; let y = 999; let x = 123;", + )?; + assert_eq!(scope.len(), 2); + assert_eq!(scope.get_value::("x").unwrap(), 123); + assert_eq!(scope.get_value::("y").unwrap(), 999); + + scope.clear(); + engine.run_with_scope( + &mut scope, + "const x = 3; let y = 0; let x = 42; let y = 999; const x = 123;", + )?; + assert_eq!(scope.len(), 3); + assert_eq!(scope.get_value::("x").unwrap(), 123); + assert_eq!(scope.get_value::("y").unwrap(), 999); + + scope.clear(); + engine.run_with_scope( + &mut scope, + "let x = 3; let y = 0; { let x = 42; let y = 999; } let x = 123;", + )?; + + assert_eq!( + engine.eval::( + " + let sum = 0; + for x in 0..10 { + let x = 42; + sum += x; + } + sum + ", + )?, + 420 + ); + } + + assert_eq!(scope.len(), 2); + assert_eq!(scope.get_value::("x").unwrap(), 123); + assert_eq!(scope.get_value::("y").unwrap(), 0); + Ok(()) } @@ -60,7 +110,7 @@ fn test_scope_eval() -> Result<(), Box> { // First invocation engine - .eval_with_scope::<()>(&mut scope, " let x = 4 + 5 - y + z; y = 1;") + .run_with_scope(&mut scope, " let x = 4 + 5 - y + z; y = 1;") .expect("variables y and z should exist"); // Second invocation using the same state