Optimize variable shadowing.

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
 =============

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 {

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<Ident>, StmtBlock)>, Position),
+    For(Box<(Ident, Option<Ident>, 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<NonZeroUsize>)>,
+        OptionFlags,
+        Position,
+    ),
     /// expr op`=` expr
     Assignment(Box<(Option<OpAssignment<'static>>, BinaryExpr)>, Position),
     /// func `(` expr `,` ... `)`
@@ -380,12 +385,12 @@ pub enum Stmt {
     /// * [`AST_OPTION_NONE`] = `return`
     /// * [`AST_OPTION_BREAK`] = `throw`
     Return(Option<Box<Expr>>, OptionFlags, Position),
-    /// `import` expr `as` var
+    /// `import` expr `as` alias
     ///
     /// Not available under `no_module`.
     #[cfg(not(feature = "no_module"))]
     Import(Box<(Expr, Option<Ident>)>, 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() {

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 (Ident { name: var_name, .. }, expr, index) = x.as_ref();
-                let expr = &x.1;
 
-                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 {

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;

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<crate::ast::Ident>,
@@ -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<Stmt> {
@@ -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 => {
+        AccessMode::ReadWrite => Stmt::Var(var_def, export, settings.pos),
-            state.stack.push(name, ());
-            Ok(Stmt::Var((var_def, expr).into(), export, settings.pos))
-        }
         // const name = { expr:constant }
-        AccessMode::ReadOnly => {
+        AccessMode::ReadOnly => Stmt::Var(var_def, AST_OPTION_CONSTANT + export, settings.pos),
-            state.stack.push_constant(name, ());
+    })
-            Ok(Stmt::Var(
-                (var_def, expr).into(),
-                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;
+    let prev_entry_stack_len = state.block_stack_len;
-    state.entry_stack_len = state.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.stack.rewind(state.block_stack_len);
-    state.entry_stack_len = prev_entry_stack_len;
+    state.block_stack_len = prev_entry_stack_len;
 
     #[cfg(not(feature = "no_module"))]
     state.imports.truncate(orig_imports_len);

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::<i64>(&mut my_scope, "x + 1")?, 42);
 ///
@@ -185,7 +185,7 @@ impl Scope<'_> {
     /// ```
     #[inline(always)]
     pub fn push(&mut self, name: impl Into<Identifier>, 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<Identifier>, 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<Identifier>,
         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<Identifier>,
         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<Identifier>,
         access: AccessMode,
@@ -622,7 +622,7 @@ impl<K: Into<Identifier>> Extend<(K, Dynamic)> for Scope<'_> {
     #[inline]
     fn extend<T: IntoIterator<Item = (K, Dynamic)>>(&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<K: Into<Identifier>> Extend<(K, bool, Dynamic)> for Scope<'_> {
     #[inline]
     fn extend<T: IntoIterator<Item = (K, bool, Dynamic)>>(&mut self, iter: T) {
         for (name, is_constant, value) in iter {
-            self.push_dynamic_value(
+            self.push_entry(
                 name,
                 if is_constant {
                     AccessMode::ReadOnly

tests/optimizer.rs

@@ -84,7 +84,7 @@ fn test_optimizer_parse() -> Result<(), Box<EvalAltResult>> {
 
     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 }")?;

tests/options.rs

@@ -25,11 +25,15 @@ fn test_options_allow() -> Result<(), Box<EvalAltResult>> {
         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;")?;
 

tests/var_scope.rs

@@ -5,17 +5,67 @@ fn test_var_scope() -> Result<(), Box<EvalAltResult>> {
     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::<INT>(&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::<INT>(&mut scope, "x")?, 12);
 
     scope.set_value("x", 42 as INT);
     assert_eq!(engine.eval_with_scope::<INT>(&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::<INT>(&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::<INT>("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::<INT>("x").unwrap(), 123);
+        assert_eq!(scope.get_value::<INT>("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::<INT>("x").unwrap(), 123);
+        assert_eq!(scope.get_value::<INT>("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::<INT>(
+                "
+                    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::<INT>("x").unwrap(), 123);
+    assert_eq!(scope.get_value::<INT>("y").unwrap(), 0);
+
     Ok(())
 }
 
@@ -60,7 +110,7 @@ fn test_scope_eval() -> Result<(), Box<EvalAltResult>> {
 
     // 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