Merge pull request #434 from schungx/master

Bug fixes.

CHANGELOG.md

@@ -8,6 +8,7 @@ Bug fixes
 ---------
 
 * Fixed bug in using indexing/dotting inside index bracket.
+* `while` and `loop` statements are no longer considered _pure_ (since a loop can go on forever and this is a side effect).
 
 
 Version 1.0.0

codegen/Cargo.toml

@@ -16,7 +16,7 @@ default = []
 metadata = []
 
 [dev-dependencies]
-rhai = { path = "..", version = "1.0" }
+rhai = { path = "..", version = "1" }
 trybuild = "1"
 
 [dependencies]

src/ast.rs

@@ -16,7 +16,7 @@ use std::{
     hash::Hash,
     mem,
     num::{NonZeroU8, NonZeroUsize},
-    ops::{Add, AddAssign, Deref, DerefMut},
+    ops::{Add, AddAssign, Deref, DerefMut, Not, Sub, SubAssign},
 };
 
 #[cfg(not(feature = "no_float"))]
@@ -950,16 +950,109 @@ impl From<StmtBlock> for Stmt {
     }
 }
 
-/// _(internals)_ Type of variable declaration.
+/// A type that holds a configuration option with bit-flags.
 /// Exported under the `internals` feature only.
-///
+#[derive(PartialEq, Eq, Copy, Clone, Hash, Default)]
-/// # Volatile Data Structure
+pub struct OptionFlags(u8);
-///
+
-/// This type is volatile and may change.
+impl OptionFlags {
-#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)]
+    /// Does this [`BitOptions`] contain a particular option flag?
-pub enum VarDeclaration {
+    #[inline(always)]
-    Let,
+    #[must_use]
-    Const,
+    pub const fn contains(self, flag: Self) -> bool {
+        self.0 & flag.0 != 0
+    }
+}
+
+impl Not for OptionFlags {
+    type Output = Self;
+
+    #[inline(always)]
+    fn not(self) -> Self::Output {
+        Self(!self.0)
+    }
+}
+
+impl Add for OptionFlags {
+    type Output = Self;
+
+    #[inline(always)]
+    fn add(self, rhs: Self) -> Self::Output {
+        Self(self.0 | rhs.0)
+    }
+}
+
+impl AddAssign for OptionFlags {
+    #[inline(always)]
+    fn add_assign(&mut self, rhs: Self) {
+        self.0 |= rhs.0
+    }
+}
+
+impl Sub for OptionFlags {
+    type Output = Self;
+
+    #[inline(always)]
+    fn sub(self, rhs: Self) -> Self::Output {
+        Self(self.0 & !rhs.0)
+    }
+}
+
+impl SubAssign for OptionFlags {
+    #[inline(always)]
+    fn sub_assign(&mut self, rhs: Self) {
+        self.0 &= !rhs.0
+    }
+}
+
+/// Option bit-flags for [`AST`] nodes.
+#[allow(non_snake_case)]
+pub mod AST_OPTION_FLAGS {
+    use super::OptionFlags;
+
+    /// _(internals)_ No options for the [`AST`][crate::AST] node.
+    /// Exported under the `internals` feature only.
+    pub const AST_OPTION_NONE: OptionFlags = OptionFlags(0b0000_0000);
+    /// _(internals)_ The [`AST`][crate::AST] node is constant.
+    /// Exported under the `internals` feature only.
+    pub const AST_OPTION_CONSTANT: OptionFlags = OptionFlags(0b0000_0001);
+    /// _(internals)_ The [`AST`][crate::AST] node is exported.
+    /// Exported under the `internals` feature only.
+    pub const AST_OPTION_EXPORTED: OptionFlags = OptionFlags(0b0000_0010);
+    /// _(internals)_ The [`AST`][crate::AST] node is in negated mode.
+    /// Exported under the `internals` feature only.
+    pub const AST_OPTION_NEGATED: OptionFlags = OptionFlags(0b0000_0100);
+
+    impl std::fmt::Debug for OptionFlags {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            fn write_option(
+                options: &OptionFlags,
+                f: &mut std::fmt::Formatter<'_>,
+                num_flags: &mut usize,
+                flag: OptionFlags,
+                name: &str,
+            ) -> std::fmt::Result {
+                if options.contains(flag) {
+                    if *num_flags > 0 {
+                        f.write_str("+")?;
+                    }
+                    f.write_str(name)?;
+                    *num_flags += 1;
+                }
+                Ok(())
+            }
+
+            let num_flags = &mut 0;
+
+            f.write_str("(")?;
+            write_option(self, f, num_flags, AST_OPTION_CONSTANT, "Constant")?;
+            write_option(self, f, num_flags, AST_OPTION_EXPORTED, "Exported")?;
+            write_option(self, f, num_flags, AST_OPTION_NEGATED, "Negated")?;
+            f.write_str(")")?;
+
+            Ok(())
+        }
+    }
 }
 
 /// _(internals)_ A statement.
@@ -980,14 +1073,26 @@ pub enum Stmt {
         Box<(BTreeMap<u64, Box<(Option<Expr>, StmtBlock)>>, StmtBlock)>,
         Position,
     ),
-    /// `while` expr `{` stmt `}`
+    /// `while` expr `{` stmt `}` | `loop` `{` stmt `}`
+    ///
+    /// If the guard expression is [`UNIT`][Expr::Unit], then it is a `loop` statement.
     While(Expr, Box<StmtBlock>, Position),
     /// `do` `{` stmt `}` `while`|`until` expr
-    Do(Box<StmtBlock>, Expr, bool, Position),
+    ///
+    /// ### Option Flags
+    ///
+    /// * [`AST_FLAG_NONE`][AST_FLAGS::AST_FLAG_NONE] = `while`
+    /// * [`AST_FLAG_NEGATED`][AST_FLAGS::AST_FLAG_NEGATED] = `until`
+    Do(Box<StmtBlock>, Expr, OptionFlags, Position),
     /// `for` `(` id `,` counter `)` `in` expr `{` stmt `}`
     For(Expr, Box<(Ident, Option<Ident>, StmtBlock)>, Position),
-    /// \[`export`\] `let`/`const` id `=` expr
+    /// \[`export`\] `let`|`const` id `=` expr
-    Var(Expr, Box<Ident>, VarDeclaration, bool, Position),
+    ///
+    /// ### Option Flags
+    ///
+    /// * [`AST_FLAG_EXPORTED`][AST_FLAGS::AST_FLAG_EXPORTED] = `export`
+    /// * [`AST_FLAG_CONSTANT`][AST_FLAGS::AST_FLAG_CONSTANT] = `const`
+    Var(Expr, Box<Ident>, OptionFlags, Position),
     /// expr op`=` expr
     Assignment(Box<(Expr, Option<OpAssignment<'static>>, Expr)>, Position),
     /// func `(` expr `,` ... `)`
@@ -1068,7 +1173,7 @@ impl Stmt {
             | Self::Do(_, _, _, pos)
             | Self::For(_, _, pos)
             | Self::Return(_, _, pos)
-            | Self::Var(_, _, _, _, pos)
+            | Self::Var(_, _, _, pos)
             | Self::TryCatch(_, pos) => *pos,
 
             Self::Expr(x) => x.position(),
@@ -1097,7 +1202,7 @@ impl Stmt {
             | Self::Do(_, _, _, pos)
             | Self::For(_, _, pos)
             | Self::Return(_, _, pos)
-            | Self::Var(_, _, _, _, pos)
+            | Self::Var(_, _, _, pos)
             | Self::TryCatch(_, pos) => *pos = new_pos,
 
             Self::Expr(x) => {
@@ -1131,7 +1236,7 @@ impl Stmt {
             | Self::For(_, _, _)
             | Self::TryCatch(_, _) => false,
 
-            Self::Var(_, _, _, _, _)
+            Self::Var(_, _, _, _)
             | Self::Assignment(_, _)
             | Self::Continue(_)
             | Self::Break(_)
@@ -1158,7 +1263,7 @@ impl Stmt {
             // A No-op requires a semicolon in order to know it is an empty statement!
             Self::Noop(_) => false,
 
-            Self::Var(_, _, _, _, _)
+            Self::Var(_, _, _, _)
             | Self::Assignment(_, _)
             | Self::FnCall(_, _)
             | Self::Expr(_)
@@ -1195,11 +1300,23 @@ impl Stmt {
                     })
                     && (x.1).0.iter().all(Stmt::is_pure)
             }
-            Self::While(condition, block, _) | Self::Do(block, condition, _, _) => {
+
-                condition.is_pure() && block.0.iter().all(Stmt::is_pure)
+            // Loops that exit can be pure because it can never be infinite.
+            Self::While(Expr::BoolConstant(false, _), _, _) => true,
+            Self::Do(body, Expr::BoolConstant(x, _), options, _)
+                if *x == options.contains(AST_OPTION_FLAGS::AST_OPTION_NEGATED) =>
+            {
+                body.iter().all(Stmt::is_pure)
             }
+
+            // Loops are never pure since they can be infinite - and that's a side effect.
+            Self::While(_, _, _) | Self::Do(_, _, _, _) => false,
+
+            // For loops can be pure because if the iterable is pure, it is finite,
+            // so infinite loops can never occur.
             Self::For(iterable, x, _) => iterable.is_pure() && (x.2).0.iter().all(Stmt::is_pure),
-            Self::Var(_, _, _, _, _) | Self::Assignment(_, _) | Self::FnCall(_, _) => false,
+
+            Self::Var(_, _, _, _) | Self::Assignment(_, _) | Self::FnCall(_, _) => false,
             Self::Block(block, _) => block.iter().all(|stmt| stmt.is_pure()),
             Self::Continue(_) | Self::Break(_) | Self::Return(_, _, _) => false,
             Self::TryCatch(x, _) => {
@@ -1225,7 +1342,7 @@ impl Stmt {
     #[must_use]
     pub fn is_internally_pure(&self) -> bool {
         match self {
-            Self::Var(expr, _, _, _, _) => expr.is_pure(),
+            Self::Var(expr, _, _, _) => expr.is_pure(),
 
             #[cfg(not(feature = "no_module"))]
             Self::Import(expr, _, _) => expr.is_pure(),
@@ -1263,7 +1380,7 @@ impl Stmt {
         }
 
         match self {
-            Self::Var(e, _, _, _, _) => {
+            Self::Var(e, _, _, _) => {
                 if !e.walk(path, on_node) {
                     return false;
                 }

src/engine.rs

@@ -1,6 +1,6 @@
 //! Main module defining the script evaluation [`Engine`].
 
-use crate::ast::{Expr, FnCallExpr, Ident, OpAssignment, ReturnType, Stmt, VarDeclaration};
+use crate::ast::{Expr, FnCallExpr, Ident, OpAssignment, ReturnType, Stmt, AST_OPTION_FLAGS::*};
 use crate::custom_syntax::CustomSyntax;
 use crate::dynamic::{map_std_type_name, AccessMode, Union, Variant};
 use crate::fn_hash::get_hasher;
@@ -2542,15 +2542,30 @@ impl Engine {
                 })
             }
 
+            // Loop
+            Stmt::While(Expr::Unit(_), body, _) => loop {
+                if !body.is_empty() {
+                    match self.eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, level)
+                    {
+                        Ok(_) => (),
+                        Err(err) => match *err {
+                            EvalAltResult::LoopBreak(false, _) => (),
+                            EvalAltResult::LoopBreak(true, _) => return Ok(Dynamic::UNIT),
+                            _ => return Err(err),
+                        },
+                    }
+                } else {
+                    #[cfg(not(feature = "unchecked"))]
+                    self.inc_operations(state, body.position())?;
+                }
+            },
+
             // While loop
             Stmt::While(expr, body, _) => loop {
-                let condition = if !expr.is_unit() {
+                let condition = self
-                    self.eval_expr(scope, mods, state, lib, this_ptr, expr, level)?
+                    .eval_expr(scope, mods, state, lib, this_ptr, expr, level)?
-                        .as_bool()
+                    .as_bool()
-                        .map_err(|typ| self.make_type_mismatch_err::<bool>(typ, expr.position()))?
+                    .map_err(|typ| self.make_type_mismatch_err::<bool>(typ, expr.position()))?;
-                } else {
-                    true
-                };
 
                 if !condition {
                     return Ok(Dynamic::UNIT);
@@ -2569,7 +2584,9 @@ impl Engine {
             },
 
             // Do loop
-            Stmt::Do(body, expr, is_while, _) => loop {
+            Stmt::Do(body, expr, options, _) => loop {
+                let is_while = !options.contains(AST_OPTION_NEGATED);
+
                 if !body.is_empty() {
                     match self.eval_stmt_block(scope, mods, state, lib, this_ptr, body, true, level)
                     {
@@ -2587,7 +2604,7 @@ impl Engine {
                     .as_bool()
                     .map_err(|typ| self.make_type_mismatch_err::<bool>(typ, expr.position()))?;
 
-                if condition ^ *is_while {
+                if condition ^ is_while {
                     return Ok(Dynamic::UNIT);
                 }
             },
@@ -2851,12 +2868,14 @@ impl Engine {
             }
 
             // Let/const statement
-            Stmt::Var(expr, x, var_type, export, _) => {
+            Stmt::Var(expr, x, options, _) => {
                 let name = &x.name;
-                let entry_type = match var_type {
+                let entry_type = if options.contains(AST_OPTION_CONSTANT) {
-                    VarDeclaration::Let => AccessMode::ReadWrite,
+                    AccessMode::ReadOnly
-                    VarDeclaration::Const => AccessMode::ReadOnly,
+                } else {
+                    AccessMode::ReadWrite
                 };
+                let export = options.contains(AST_OPTION_EXPORTED);
 
                 let value = self
                     .eval_expr(scope, mods, state, lib, this_ptr, expr, level)?
@@ -2893,9 +2912,9 @@ impl Engine {
 
                     (
                         name.to_string().into(),
-                        if *export { Some(name.clone()) } else { None },
+                        if export { Some(name.clone()) } else { None },
                     )
-                } else if *export {
+                } else if export {
                     unreachable!("exported variable not on global level");
                 } else {
                     (unsafe_cast_var_name_to_lifetime(name).into(), None)

src/lib.rs

@@ -228,7 +228,7 @@ pub use token::{InputStream, Token, TokenizeState, TokenizerControl, TokenizerCo
 #[deprecated = "this type is volatile and may change"]
 pub use ast::{
     ASTNode, BinaryExpr, CustomExpr, Expr, FloatWrapper, FnCallExpr, FnCallHashes, Ident,
-    OpAssignment, ReturnType, ScriptFnDef, Stmt, StmtBlock, VarDeclaration,
+    OpAssignment, OptionFlags, ReturnType, ScriptFnDef, Stmt, StmtBlock, AST_OPTION_FLAGS::*,
 };
 
 #[cfg(feature = "internals")]

src/optimize.rs

@@ -1,6 +1,6 @@
 //! Module implementing the [`AST`] optimizer.
 
-use crate::ast::{Expr, OpAssignment, Stmt, VarDeclaration};
+use crate::ast::{Expr, OpAssignment, Stmt, AST_OPTION_FLAGS::*};
 use crate::dynamic::AccessMode;
 use crate::engine::{KEYWORD_DEBUG, KEYWORD_EVAL, KEYWORD_FN_PTR, KEYWORD_PRINT, KEYWORD_TYPE_OF};
 use crate::fn_builtin::get_builtin_binary_op_fn;
@@ -201,23 +201,24 @@ fn optimize_stmt_block(
         // Optimize each statement in the block
         statements.iter_mut().for_each(|stmt| {
             match stmt {
-                // Add constant literals into the state
+                Stmt::Var(value_expr, x, options, _) => {
-                Stmt::Var(value_expr, x, VarDeclaration::Const, _, _) => {
+                    if options.contains(AST_OPTION_CONSTANT) {
-                    optimize_expr(value_expr, state, false);
+                        // Add constant literals into the state
+                        optimize_expr(value_expr, state, false);
 
-                    if value_expr.is_constant() {
+                        if value_expr.is_constant() {
-                        state.push_var(
+                            state.push_var(
-                            &x.name,
+                                &x.name,
-                            AccessMode::ReadOnly,
+                                AccessMode::ReadOnly,
-                            value_expr.get_literal_value(),
+                                value_expr.get_literal_value(),
-                        );
+                            );
+                        }
+                    } else {
+                        // Add variables into the state
+                        optimize_expr(value_expr, state, false);
+                        state.push_var(&x.name, AccessMode::ReadWrite, None);
                     }
                 }
-                // Add variables into the state
-                Stmt::Var(value_expr, x, VarDeclaration::Let, _, _) => {
-                    optimize_expr(value_expr, state, false);
-                    state.push_var(&x.name, AccessMode::ReadWrite, None);
-                }
                 // Optimize the statement
                 _ => optimize_stmt(stmt, state, preserve_result),
             }
@@ -232,7 +233,7 @@ fn optimize_stmt_block(
             .find_map(|(i, stmt)| match stmt {
                 stmt if !is_pure(stmt) => Some(i),
 
-                Stmt::Var(e, _, _, _, _) | Stmt::Expr(e) if !e.is_constant() => Some(i),
+                Stmt::Var(e, _, _, _) | Stmt::Expr(e) if !e.is_constant() => Some(i),
 
                 #[cfg(not(feature = "no_module"))]
                 Stmt::Import(e, _, _) if !e.is_constant() => Some(i),
@@ -558,6 +559,9 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
         // while expr { block }
         Stmt::While(condition, body, _) => {
             optimize_expr(condition, state, false);
+            if let Expr::BoolConstant(true, pos) = condition {
+                *condition = Expr::Unit(*pos);
+            }
             let block = mem::take(body.statements_mut()).into_vec();
             *body.statements_mut() = optimize_stmt_block(block, state, false, true, false).into();
 
@@ -582,8 +586,9 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
             }
         }
         // do { block } while false | do { block } until true -> { block }
-        Stmt::Do(body, Expr::BoolConstant(true, _), false, _)
+        Stmt::Do(body, Expr::BoolConstant(x, _), options, _)
-        | Stmt::Do(body, Expr::BoolConstant(false, _), true, _) => {
+            if *x == options.contains(AST_OPTION_NEGATED) =>
+        {
             state.set_dirty();
             let block_pos = body.position();
             let block = mem::take(body.statements_mut()).into_vec();
@@ -605,7 +610,9 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
             *x.2.statements_mut() = optimize_stmt_block(body, state, false, true, false).into();
         }
         // let id = expr;
-        Stmt::Var(expr, _, VarDeclaration::Let, _, _) => optimize_expr(expr, state, false),
+        Stmt::Var(expr, _, options, _) if !options.contains(AST_OPTION_CONSTANT) => {
+            optimize_expr(expr, state, false)
+        }
         // import expr as var;
         #[cfg(not(feature = "no_module"))]
         Stmt::Import(expr, _, _) => optimize_expr(expr, state, false),

src/parse.rs

@@ -2,7 +2,7 @@
 
 use crate::ast::{
     BinaryExpr, CustomExpr, Expr, FnCallExpr, FnCallHashes, Ident, OpAssignment, ReturnType,
-    ScriptFnDef, Stmt, StmtBlock, VarDeclaration,
+    ScriptFnDef, Stmt, StmtBlock, AST_OPTION_FLAGS::*,
 };
 use crate::custom_syntax::{
     CustomSyntax, CUSTOM_SYNTAX_MARKER_BLOCK, CUSTOM_SYNTAX_MARKER_BOOL, CUSTOM_SYNTAX_MARKER_EXPR,
@@ -2224,9 +2224,9 @@ fn parse_do(
     settings.is_breakable = true;
     let body = parse_block(input, state, lib, settings.level_up())?;
 
-    let is_while = match input.next().expect(NEVER_ENDS) {
+    let negated = match input.next().expect(NEVER_ENDS) {
-        (Token::While, _) => true,
+        (Token::While, _) => AST_OPTION_NONE,
-        (Token::Until, _) => false,
+        (Token::Until, _) => AST_OPTION_NEGATED,
         (_, pos) => {
             return Err(
                 PERR::MissingToken(Token::While.into(), "for the do statement".into())
@@ -2244,7 +2244,7 @@ fn parse_do(
     Ok(Stmt::Do(
         Box::new(body.into()),
         guard,
-        is_while,
+        negated,
         settings.pos,
     ))
 }
@@ -2381,21 +2381,20 @@ fn parse_let(
 
     state.stack.push((name, var_type));
 
+    let export = if export {
+        AST_OPTION_EXPORTED
+    } else {
+        AST_OPTION_NONE
+    };
+
     match var_type {
         // let name = expr
-        AccessMode::ReadWrite => Ok(Stmt::Var(
+        AccessMode::ReadWrite => Ok(Stmt::Var(expr, var_def.into(), export, settings.pos)),
-            expr,
-            var_def.into(),
-            VarDeclaration::Let,
-            export,
-            settings.pos,
-        )),
         // const name = { expr:constant }
         AccessMode::ReadOnly => Ok(Stmt::Var(
             expr,
             var_def.into(),
-            VarDeclaration::Const,
+            AST_OPTION_CONSTANT + export,
-            export,
             settings.pos,
         )),
     }

tests/optimizer.rs

@@ -86,7 +86,7 @@ fn test_optimizer_parse() -> Result<(), Box<EvalAltResult>> {
 
     assert_eq!(
         format!("{:?}", ast),
-        r#"AST { source: None, body: Block[Var(false @ 1:18, "DECISION" @ 1:7, Const, false, 1:1), Expr(123 @ 1:51)], functions: Module, resolver: None }"#
+        r#"AST { source: None, body: Block[Var(false @ 1:18, "DECISION" @ 1:7, (Constant), 1:1), Expr(123 @ 1:51)], functions: Module, resolver: None }"#
     );
 
     let ast = engine.compile("if 1 == 2 { 42 }")?;

tests/while_loop.rs

@@ -49,3 +49,17 @@ fn test_do() -> Result<(), Box<EvalAltResult>> {
 
     Ok(())
 }
+
+#[cfg(not(feature = "unchecked"))]
+#[test]
+fn test_infinite_loops() -> Result<(), Box<EvalAltResult>> {
+    let mut engine = Engine::new();
+
+    engine.set_max_operations(1024);
+
+    assert!(engine.consume("loop {}").is_err());
+    assert!(engine.consume("while true {}").is_err());
+    assert!(engine.consume("do {} while true").is_err());
+
+    Ok(())
+}