Add loop expressions.

CHANGELOG.md

@@ -10,6 +10,7 @@ Bug fixes
 * `Engine::parse_json` now returns an error on unquoted keys to be consistent with JSON specifications.
 * `import` statements inside `eval` no longer cause errors in subsequent code.
 * Functions marked `global` in `import`ed modules with no alias names now work properly.
+* Incorrect loop optimizations that are too aggressive (e.g. unrolling a `do { ... } until true` with a `break` statement inside) and cause crashes are removed.
 
 Speed Improvements
 ------------------
@@ -19,6 +20,12 @@ Speed Improvements
 New features
 ------------
 
+### Loop expressions
+
+* Loops (such as `loop`, `do`, `while` and `for`) can now act as _expressions_, with the `break` statement returning an optional value.
+* Normal loops return `()` as the value.
+* Loop expressions can be enabled/disabled via `Engine::set_allow_loop_expressions`
+
 ### Stable hashing
 
 * It is now possible to specify a fixed _seed_ for use with the `ahash` hasher, via an environment variable, in order to force stable (i.e. deterministic) hashes for function signatures.

src/api/options.rs

@@ -12,23 +12,25 @@ bitflags! {
         const IF_EXPR = 0b_0000_0000_0001;
         /// Is `switch` expression allowed?
         const SWITCH_EXPR = 0b_0000_0000_0010;
+        /// Are loop expressions allowed?
+        const LOOP_EXPR = 0b_0000_0000_0100;
         /// Is statement-expression allowed?
-        const STMT_EXPR = 0b_0000_0000_0100;
+        const STMT_EXPR = 0b_0000_0000_1000;
         /// Is anonymous function allowed?
         #[cfg(not(feature = "no_function"))]
-        const ANON_FN = 0b_0000_0000_1000;
+        const ANON_FN = 0b_0000_0001_0000;
         /// Is looping allowed?
-        const LOOPING = 0b_0000_0001_0000;
+        const LOOPING = 0b_0000_0010_0000;
         /// Is variables shadowing allowed?
-        const SHADOW = 0b_0000_0010_0000;
+        const SHADOW = 0b_0000_0100_0000;
         /// Strict variables mode?
-        const STRICT_VAR = 0b_0000_0100_0000;
+        const STRICT_VAR = 0b_0000_1000_0000;
         /// Raise error if an object map property does not exist?
         /// Returns `()` if `false`.
         #[cfg(not(feature = "no_object"))]
-        const FAIL_ON_INVALID_MAP_PROPERTY = 0b_0000_1000_0000;
+        const FAIL_ON_INVALID_MAP_PROPERTY = 0b_0001_0000_0000;
         /// Fast operators mode?
-        const FAST_OPS = 0b_0001_0000_0000;
+        const FAST_OPS = 0b_0010_0000_0000;
     }
 }
 
@@ -81,6 +83,18 @@ impl Engine {
     pub fn set_allow_switch_expression(&mut self, enable: bool) {
         self.options.set(LangOptions::SWITCH_EXPR, enable);
     }
+    /// Are loop expressions allowed?
+    /// Default is `true`.
+    #[inline(always)]
+    #[must_use]
+    pub const fn allow_loop_expressions(&self) -> bool {
+        self.options.contains(LangOptions::LOOP_EXPR)
+    }
+    /// Set whether loop expressions are allowed.
+    #[inline(always)]
+    pub fn set_allow_loop_expressions(&mut self, enable: bool) {
+        self.options.set(LangOptions::LOOP_EXPR, enable);
+    }
     /// Is statement-expression allowed?
     /// Default is `true`.
     #[inline(always)]

src/ast/stmt.rs

@@ -581,14 +581,14 @@ pub enum Stmt {
     TryCatch(Box<TryCatchBlock>, Position),
     /// [expression][Expr]
     Expr(Box<Expr>),
-    /// `continue`/`break`
+    /// `continue`/`break` expr
     ///
     /// ### Flags
     ///
     /// * [`NONE`][ASTFlags::NONE] = `continue`
     /// * [`BREAK`][ASTFlags::BREAK] = `break`
-    BreakLoop(ASTFlags, Position),
+    BreakLoop(Option<Box<Expr>>, ASTFlags, Position),
-    /// `return`/`throw`
+    /// `return`/`throw` expr
     ///
     /// ### Flags
     ///

src/eval/stmt.rs

@@ -493,7 +493,7 @@ impl Engine {
                             Ok(_) => (),
                             Err(err) => match *err {
                                 ERR::LoopBreak(false, ..) => (),
-                                ERR::LoopBreak(true, ..) => break Ok(Dynamic::UNIT),
+                                ERR::LoopBreak(true, value, ..) => break Ok(value),
                                 _ => break Err(err),
                             },
                         }
@@ -524,7 +524,7 @@ impl Engine {
                                 Ok(_) => (),
                                 Err(err) => match *err {
                                     ERR::LoopBreak(false, ..) => (),
-                                    ERR::LoopBreak(true, ..) => break Ok(Dynamic::UNIT),
+                                    ERR::LoopBreak(true, value, ..) => break Ok(value),
                                     _ => break Err(err),
                                 },
                             }
@@ -547,7 +547,7 @@ impl Engine {
                             Ok(_) => (),
                             Err(err) => match *err {
                                 ERR::LoopBreak(false, ..) => continue,
-                                ERR::LoopBreak(true, ..) => break Ok(Dynamic::UNIT),
+                                ERR::LoopBreak(true, value, ..) => break Ok(value),
                                 _ => break Err(err),
                             },
                         }
@@ -614,7 +614,7 @@ impl Engine {
 
                         let loop_result = func(iter_obj)
                             .enumerate()
-                            .try_for_each(|(x, iter_value)| {
+                            .try_fold(Dynamic::UNIT, |_, (x, iter_value)| {
                                 // Increment counter
                                 if counter_index < usize::MAX {
                                     // As the variable increments from 0, this should always work
@@ -644,26 +644,26 @@ impl Engine {
                                 self.track_operation(global, statements.position())?;
 
                                 if statements.is_empty() {
-                                    return Ok(());
+                                    return Ok(Dynamic::UNIT);
                                 }
 
                                 self.eval_stmt_block(
                                     scope, global, caches, lib, this_ptr, statements, true, level,
                                 )
-                                .map(|_| ())
+                                .map(|_| Dynamic::UNIT)
                                 .or_else(|err| match *err {
-                                    ERR::LoopBreak(false, ..) => Ok(()),
+                                    ERR::LoopBreak(false, ..) => Ok(Dynamic::UNIT),
                                     _ => Err(err),
                                 })
                             })
                             .or_else(|err| match *err {
-                                ERR::LoopBreak(true, ..) => Ok(()),
+                                ERR::LoopBreak(true, value, ..) => Ok(value),
                                 _ => Err(err),
                             });
 
                         scope.rewind(orig_scope_len);
 
-                        loop_result.map(|_| Dynamic::UNIT)
+                        loop_result
                     } else {
                         Err(ERR::ErrorFor(expr.start_position()).into())
                     }
@@ -673,8 +673,15 @@ impl Engine {
             }
 
             // Continue/Break statement
-            Stmt::BreakLoop(options, pos) => {
+            Stmt::BreakLoop(expr, options, pos) => {
-                Err(ERR::LoopBreak(options.contains(ASTFlags::BREAK), *pos).into())
+                let is_break = options.contains(ASTFlags::BREAK);
+
+                if let Some(ref expr) = expr {
+                    self.eval_expr(scope, global, caches, lib, this_ptr, expr, level)
+                        .and_then(|v| ERR::LoopBreak(is_break, v, *pos).into())
+                } else {
+                    Err(ERR::LoopBreak(is_break, Dynamic::UNIT, *pos).into())
+                }
             }
 
             // Try/Catch statement

src/optimizer.rs

@@ -8,7 +8,7 @@ use crate::engine::{KEYWORD_DEBUG, KEYWORD_EVAL, KEYWORD_FN_PTR, KEYWORD_PRINT,
 use crate::eval::{Caches, GlobalRuntimeState};
 use crate::func::builtin::get_builtin_binary_op_fn;
 use crate::func::hashing::get_hasher;
-use crate::tokenizer::{Span, Token};
+use crate::tokenizer::Token;
 use crate::types::dynamic::AccessMode;
 use crate::{
     calc_fn_hash, calc_fn_params_hash, combine_hashes, Dynamic, Engine, FnPtr, Identifier,
@@ -785,50 +785,6 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
                 *condition = Expr::Unit(*pos);
             }
             **body = optimize_stmt_block(mem::take(&mut **body), state, false, true, false);
-
-            if body.len() == 1 {
-                match body[0] {
-                    // while expr { break; } -> { expr; }
-                    Stmt::BreakLoop(options, pos) if options.contains(ASTFlags::BREAK) => {
-                        // Only a single break statement - turn into running the guard expression once
-                        state.set_dirty();
-                        if condition.is_unit() {
-                            *stmt = Stmt::Noop(pos);
-                        } else {
-                            let mut statements = vec![Stmt::Expr(mem::take(condition).into())];
-                            if preserve_result {
-                                statements.push(Stmt::Noop(pos));
-                            }
-                            *stmt = (statements, Span::new(pos, Position::NONE)).into();
-                        };
-                    }
-                    _ => (),
-                }
-            }
-        }
-        // do { block } until true -> { block }
-        Stmt::Do(x, options, ..)
-            if matches!(x.0, Expr::BoolConstant(true, ..))
-                && options.contains(ASTFlags::NEGATED) =>
-        {
-            state.set_dirty();
-            *stmt = (
-                optimize_stmt_block(mem::take(&mut *x.1), state, false, true, false),
-                x.1.span(),
-            )
-                .into();
-        }
-        // do { block } while false -> { block }
-        Stmt::Do(x, options, ..)
-            if matches!(x.0, Expr::BoolConstant(false, ..))
-                && !options.contains(ASTFlags::NEGATED) =>
-        {
-            state.set_dirty();
-            *stmt = (
-                optimize_stmt_block(mem::take(&mut *x.1), state, false, true, false),
-                x.1.span(),
-            )
-                .into();
         }
         // do { block } while|until expr
         Stmt::Do(x, ..) => {
@@ -916,6 +872,9 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
             }
         }
 
+        // break expr;
+        Stmt::BreakLoop(Some(ref mut expr), ..) => optimize_expr(expr, state, false),
+
         // return expr;
         Stmt::Return(Some(ref mut expr), ..) => optimize_expr(expr, state, false),
 

src/parser.rs

@@ -1380,6 +1380,23 @@ impl Engine {
                 self.parse_if(input, state, lib, settings.level_up())?
                     .into(),
             )),
+            // Loops are allowed to act as expressions
+            Token::While | Token::Loop if settings.options.contains(LangOptions::LOOP_EXPR) => {
+                Expr::Stmt(Box::new(
+                    self.parse_while_loop(input, state, lib, settings.level_up())?
+                        .into(),
+                ))
+            }
+            Token::Do if settings.options.contains(LangOptions::LOOP_EXPR) => Expr::Stmt(Box::new(
+                self.parse_do(input, state, lib, settings.level_up())?
+                    .into(),
+            )),
+            Token::For if settings.options.contains(LangOptions::LOOP_EXPR) => {
+                Expr::Stmt(Box::new(
+                    self.parse_for(input, state, lib, settings.level_up())?
+                        .into(),
+                ))
+            }
             // Switch statement is allowed to act as expressions
             Token::Switch if settings.options.contains(LangOptions::SWITCH_EXPR) => {
                 Expr::Stmt(Box::new(
@@ -3411,11 +3428,26 @@ impl Engine {
 
             Token::Continue if self.allow_looping() && settings.is_breakable => {
                 let pos = eat_token(input, Token::Continue);
-                Ok(Stmt::BreakLoop(ASTFlags::NONE, pos))
+                Ok(Stmt::BreakLoop(None, ASTFlags::NONE, pos))
             }
             Token::Break if self.allow_looping() && settings.is_breakable => {
                 let pos = eat_token(input, Token::Break);
-                Ok(Stmt::BreakLoop(ASTFlags::BREAK, pos))
+
+                let expr = match input.peek().expect(NEVER_ENDS) {
+                    // `break` at <EOF>
+                    (Token::EOF, ..) => None,
+                    // `break` at end of block
+                    (Token::RightBrace, ..) => None,
+                    // `break;`
+                    (Token::SemiColon, ..) => None,
+                    // `break`  with expression
+                    _ => Some(
+                        self.parse_expr(input, state, lib, settings.level_up())?
+                            .into(),
+                    ),
+                };
+
+                Ok(Stmt::BreakLoop(expr, ASTFlags::BREAK, pos))
             }
             Token::Continue | Token::Break if self.allow_looping() => {
                 Err(PERR::LoopBreak.into_err(token_pos))
@@ -3840,7 +3872,7 @@ impl Engine {
         let mut functions = StraightHashMap::default();
 
         let mut options = self.options;
-        options.remove(LangOptions::STMT_EXPR);
+        options.remove(LangOptions::STMT_EXPR | LangOptions::LOOP_EXPR);
         #[cfg(not(feature = "no_function"))]
         options.remove(LangOptions::ANON_FN);
 

src/types/error.rs

@@ -117,7 +117,7 @@ pub enum EvalAltResult {
     /// Breaking out of loops - not an error if within a loop.
     /// The wrapped value, if true, means breaking clean out of the loop (i.e. a `break` statement).
     /// The wrapped value, if false, means breaking the current context (i.e. a `continue` statement).
-    LoopBreak(bool, Position),
+    LoopBreak(bool, Dynamic, Position),
     /// Not an error: Value returned from a script via the `return` keyword.
     /// Wrapped value is the result value.
     Return(Dynamic, Position),

tests/expressions.rs

@@ -55,10 +55,13 @@ fn test_expressions() -> Result<(), Box<EvalAltResult>> {
         )
         .is_err());
 
-    assert!(engine.eval_expression::<()>("40 + 2;").is_err());
+    assert!(engine.compile_expression("40 + 2;").is_err());
-    assert!(engine.eval_expression::<()>("40 + { 2 }").is_err());
+    assert!(engine.compile_expression("40 + { 2 }").is_err());
-    assert!(engine.eval_expression::<()>("x = 42").is_err());
+    assert!(engine.compile_expression("x = 42").is_err());
     assert!(engine.compile_expression("let x = 42").is_err());
+    assert!(engine
+        .compile_expression("do { break 42; } while true")
+        .is_err());
 
     engine.compile("40 + { let x = 2; x }")?;
 

tests/for.rs

@@ -231,6 +231,22 @@ fn test_for_loop() -> Result<(), Box<EvalAltResult>> {
         );
     }
 
+    assert_eq!(
+        engine.eval::<INT>(
+            r#"
+                let a = [123, 999, 42, 0, true, "hello", "world!", 987.6543];
+
+                for (item, count) in a {
+                    switch item.type_of() {
+                        "i64" if item.is_even => break count,
+                        "f64" if item.to_int().is_even => break count,
+                    }
+                }
+            "#
+        )?,
+        2
+    );
+
     Ok(())
 }
 

tests/while_loop.rs

@@ -22,6 +22,22 @@ fn test_while() -> Result<(), Box<EvalAltResult>> {
         6
     );
 
+    assert_eq!(
+        engine.eval::<INT>(
+            "
+                let x = 0;
+
+                while x < 10 {
+                    x += 1;
+                    if x > 5 { break x * 2; }
+                    if x > 3 { continue; }
+                    x += 3;
+                }
+            ",
+        )?,
+        12
+    );
+
     Ok(())
 }
 
@@ -46,6 +62,25 @@ fn test_do() -> Result<(), Box<EvalAltResult>> {
         )?,
         6
     );
+    assert_eq!(
+        engine.eval::<INT>(
+            "
+                let x = 0;
+
+                do {
+                    x += 1;
+                    if x > 5 { break x * 2; }
+                    if x > 3 { continue; }
+                    x += 3;
+                } while x < 10;
+            ",
+        )?,
+        12
+    );
+
+    engine.run("do {} while false")?;
+
+    assert_eq!(engine.eval::<INT>("do { break 42; } while false")?, 42);
 
     Ok(())
 }