Fix optimizer constants propagation bug.

CHANGELOG.md

@@ -13,6 +13,7 @@ Bug fixes
 * Re-optimizing an AST via `optimize_ast` with constants now works correctly for closures. Previously the hidden `Share` nodes are not removed and causes variable-not-found errors during runtime if the constants are not available in the scope.
 * Expressions such as `(v[0].func()).prop` now parse correctly.
 * Shadowed variable exports are now handled correctly.
+* Shadowed constant definitions are now optimized correctly when propagated (e.g. `const X = 1; const X = 1 + 1 + 1; X` now evaluates to 3 instead of 0).
 
 New features
 ------------

src/optimizer.rs

@@ -14,10 +14,12 @@ use crate::func::builtin::get_builtin_binary_op_fn;
 use crate::func::hashing::get_hasher;
 use crate::module::ModuleFlags;
 use crate::tokenizer::Token;
+use crate::types::scope::SCOPE_ENTRIES_INLINED;
 use crate::{
-    calc_fn_hash, calc_fn_hash_full, Dynamic, Engine, FnPtr, ImmutableString, Position, Scope,
+    calc_fn_hash, calc_fn_hash_full, Dynamic, Engine, FnArgsVec, FnPtr, ImmutableString, Position,
-    StaticVec, AST,
+    Scope, AST,
 };
+use smallvec::SmallVec;
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
 use std::{
@@ -52,12 +54,12 @@ impl Default for OptimizationLevel {
 #[derive(Debug, Clone)]
 struct OptimizerState<'a> {
     /// Has the [`AST`] been changed during this pass?
-    changed: bool,
+    is_dirty: bool,
-    /// Collection of variables/constants to use for eager function evaluations.
+    /// Stack of variables/constants for constants propagation.
-    variables: StaticVec<(ImmutableString, Option<Dynamic>)>,
+    variables: SmallVec<[(ImmutableString, Option<Dynamic>); SCOPE_ENTRIES_INLINED]>,
     /// Activate constants propagation?
     propagate_constants: bool,
-    /// An [`Engine`] instance for eager function evaluation.
+    /// [`Engine`] instance for eager function evaluation.
     engine: &'a Engine,
     /// The global runtime state.
     global: GlobalRuntimeState,
@@ -84,8 +86,8 @@ impl<'a> OptimizerState<'a> {
         }
 
         Self {
-            changed: false,
+            is_dirty: false,
-            variables: StaticVec::new_const(),
+            variables: SmallVec::new_const(),
             propagate_constants: true,
             engine,
             global: _global,
@@ -96,48 +98,42 @@ impl<'a> OptimizerState<'a> {
     /// Set the [`AST`] state to be dirty (i.e. changed).
     #[inline(always)]
     pub fn set_dirty(&mut self) {
-        self.changed = true;
+        self.is_dirty = true;
     }
     /// Set the [`AST`] state to be not dirty (i.e. unchanged).
     #[inline(always)]
     pub fn clear_dirty(&mut self) {
-        self.changed = false;
+        self.is_dirty = false;
     }
     /// Is the [`AST`] dirty (i.e. changed)?
     #[inline(always)]
     pub const fn is_dirty(&self) -> bool {
-        self.changed
+        self.is_dirty
     }
-    /// Prune the list of constants back to a specified size.
+    /// Rewind the variables stack back to a specified size.
     #[inline(always)]
-    pub fn restore_var(&mut self, len: usize) {
+    pub fn rewind_var(&mut self, len: usize) {
         self.variables.truncate(len);
     }
-    /// Add a new variable to the list.
+    /// Add a new variable to the stack.
     ///
-    /// `Some(value)` if constant, `None` or otherwise.
+    /// `Some(value)` if literal constant (which can be used for constants propagation), `None` otherwise.
     #[inline(always)]
     pub fn push_var(&mut self, name: ImmutableString, value: Option<Dynamic>) {
         self.variables.push((name, value));
     }
-    /// Look up a constant from the list.
+    /// Look up a literal constant from the variables stack.
     #[inline]
-    pub fn find_constant(&self, name: &str) -> Option<&Dynamic> {
+    pub fn find_literal_constant(&self, name: &str) -> Option<&Dynamic> {
-        if !self.propagate_constants {
+        self.variables
-            return None;
+            .iter()
-        }
+            .rev()
-
+            .find(|(n, _)| n.as_str() == name)
-        for (n, value) in self.variables.iter().rev() {
+            .and_then(|(_, value)| value.as_ref())
-            if n.as_str() == name {
-                return value.as_ref();
-            }
-        }
-
-        None
     }
     /// Call a registered function
     #[inline]
-    pub fn call_fn_with_constant_arguments(
+    pub fn call_fn_with_const_args(
         &mut self,
         fn_name: &str,
         op_token: Option<&Token>,
@@ -150,7 +146,7 @@ impl<'a> OptimizerState<'a> {
                 fn_name,
                 op_token,
                 calc_fn_hash(None, fn_name, arg_values.len()),
-                &mut arg_values.iter_mut().collect::<StaticVec<_>>(),
+                &mut arg_values.iter_mut().collect::<FnArgsVec<_>>(),
                 false,
                 Position::NONE,
             )
@@ -225,19 +221,16 @@ fn optimize_stmt_block(
         statements.iter_mut().for_each(|stmt| {
             match stmt {
                 Stmt::Var(x, options, ..) => {
-                    if options.contains(ASTFlags::CONSTANT) {
-                        // Add constant literals into the state
                     optimize_expr(&mut x.1, state, false);
 
-                        if x.1.is_constant() {
+                    let value = if options.contains(ASTFlags::CONSTANT) && x.1.is_constant() {
-                            state
+                        // constant literal
-                                .push_var(x.0.name.clone(), Some(x.1.get_literal_value().unwrap()));
+                        Some(x.1.get_literal_value().unwrap())
-                        }
                     } else {
-                        // Add variables into the state
+                        // variable
-                        optimize_expr(&mut x.1, state, false);
+                        None
-                        state.push_var(x.0.name.clone(), None);
+                    };
-                    }
+                    state.push_var(x.0.name.clone(), value);
                 }
                 // Optimize the statement
                 _ => optimize_stmt(stmt, state, preserve_result),
@@ -371,7 +364,7 @@ fn optimize_stmt_block(
         }
 
         // Pop the stack and remove all the local constants
-        state.restore_var(orig_constants_len);
+        state.rewind_var(orig_constants_len);
         state.propagate_constants = orig_propagate_constants;
 
         if !state.is_dirty() {
@@ -845,12 +838,16 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
         // Share constants
         #[cfg(not(feature = "no_closure"))]
         Stmt::Share(x) => {
-            let len = x.len();
+            let orig_len = x.len();
-            x.retain(|(v, _)| !state.find_constant(v).is_some());
+
-            if x.len() != len {
+            if state.propagate_constants {
+                x.retain(|(v, _)| state.find_literal_constant(v).is_none());
+
+                if x.len() != orig_len {
                     state.set_dirty();
                 }
             }
+        }
 
         // All other statements - skip
         _ => (),
@@ -1141,8 +1138,8 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, _chaining: bool) {
                 && state.optimization_level == OptimizationLevel::Simple // simple optimizations
                 && x.constant_args() // all arguments are constants
         => {
-            let arg_values = &mut x.args.iter().map(|arg_expr| arg_expr.get_literal_value().unwrap()).collect::<StaticVec<_>>();
+            let arg_values = &mut x.args.iter().map(|arg_expr| arg_expr.get_literal_value().unwrap()).collect::<FnArgsVec<_>>();
-            let arg_types: StaticVec<_> = arg_values.iter().map(Dynamic::type_id).collect();
+            let arg_types = arg_values.iter().map(Dynamic::type_id).collect::<FnArgsVec<_>>();
 
             match x.name.as_str() {
                 KEYWORD_TYPE_OF if arg_values.len() == 1 => {
@@ -1204,13 +1201,13 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, _chaining: bool) {
             let has_script_fn = false;
 
             if !has_script_fn {
-                let arg_values = &mut x.args.iter().map(Expr::get_literal_value).collect::<Option<StaticVec<_>>>().unwrap();
+                let arg_values = &mut x.args.iter().map(Expr::get_literal_value).collect::<Option<FnArgsVec<_>>>().unwrap();
 
                 let result = match x.name.as_str() {
                     KEYWORD_TYPE_OF if arg_values.len() == 1 => Some(state.engine.map_type_name(arg_values[0].type_name()).into()),
                     #[cfg(not(feature = "no_closure"))]
                     crate::engine::KEYWORD_IS_SHARED if arg_values.len() == 1 => Some(Dynamic::FALSE),
-                    _ => state.call_fn_with_constant_arguments(&x.name, x.op_token.as_ref(), arg_values)
+                    _ => state.call_fn_with_const_args(&x.name, x.op_token.as_ref(), arg_values)
                 };
 
                 if let Some(r) = result {
@@ -1246,9 +1243,9 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, _chaining: bool) {
         // constant-name
         #[cfg(not(feature = "no_module"))]
         Expr::Variable(x, ..) if !x.1.is_empty() => (),
-        Expr::Variable(x, .., pos) if state.find_constant(&x.3).is_some() => {
+        Expr::Variable(x, .., pos) if state.propagate_constants && state.find_literal_constant(&x.3).is_some() => {
             // Replace constant with value
-            *expr = Expr::from_dynamic(state.find_constant(&x.3).unwrap().clone(), *pos);
+            *expr = Expr::from_dynamic(state.find_literal_constant(&x.3).unwrap().clone(), *pos);
             state.set_dirty();
         }
 
@@ -1341,7 +1338,7 @@ impl Engine {
         &self,
         scope: Option<&Scope>,
         statements: StmtBlockContainer,
-        #[cfg(not(feature = "no_function"))] functions: StaticVec<
+        #[cfg(not(feature = "no_function"))] functions: crate::StaticVec<
             crate::Shared<crate::ast::ScriptFnDef>,
         >,
         optimization_level: OptimizationLevel,

src/types/scope.rs

@@ -12,7 +12,7 @@ use std::{
 };
 
 /// Keep a number of entries inline (since [`Dynamic`] is usually small enough).
-const SCOPE_ENTRIES_INLINED: usize = 8;
+pub const SCOPE_ENTRIES_INLINED: usize = 8;
 
 /// Type containing information about the current scope. Useful for keeping state between
 /// [`Engine`][crate::Engine] evaluation runs.

tests/optimizer.rs

@@ -5,15 +5,14 @@ use rhai::{Engine, EvalAltResult, Module, OptimizationLevel, Scope, INT};
 #[test]
 fn test_optimizer() -> Result<(), Box<EvalAltResult>> {
     let mut engine = Engine::new();
-    engine.set_optimization_level(OptimizationLevel::Full);
+    engine.set_optimization_level(OptimizationLevel::Simple);
 
-    #[cfg(not(feature = "no_function"))]
     assert_eq!(
         engine.eval::<INT>(
             "
-                fn foo(x) { print(x); return; }
+                const X = 0;
-                fn foo2(x) { if x > 0 {} return; }
+                const X = 40 + 2 - 1 + 1;
-                42
+                X
             "
         )?,
         42
@@ -202,6 +201,18 @@ fn test_optimizer_full() -> Result<(), Box<EvalAltResult>> {
 
     engine.set_optimization_level(OptimizationLevel::Full);
 
+    #[cfg(not(feature = "no_function"))]
+    assert_eq!(
+        engine.eval::<INT>(
+            "
+                fn foo(x) { print(x); return; }
+                fn foo2(x) { if x > 0 {} return; }
+                42
+            "
+        )?,
+        42
+    );
+
     engine
         .register_type_with_name::<TestStruct>("TestStruct")
         .register_fn("ts", |n: INT| TestStruct(n))