From c14fbdb14da7e16bef240496335d4c0cc1425ca5 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 29 Oct 2022 12:09:18 +0800 Subject: [PATCH] Add loop expressions. --- CHANGELOG.md | 7 +++++++ src/api/options.rs | 28 ++++++++++++++++++------- src/ast/stmt.rs | 6 +++--- src/eval/stmt.rs | 29 ++++++++++++++++---------- src/optimizer.rs | 49 ++++---------------------------------------- src/parser.rs | 38 +++++++++++++++++++++++++++++++--- src/types/error.rs | 2 +- tests/expressions.rs | 9 +++++--- tests/for.rs | 16 +++++++++++++++ tests/while_loop.rs | 35 +++++++++++++++++++++++++++++++ 10 files changed, 146 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49aa5966..b6f6d5c7 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/api/options.rs b/src/api/options.rs index d5596cfd..42b57456 100644 --- a/src/api/options.rs +++ b/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)] diff --git a/src/ast/stmt.rs b/src/ast/stmt.rs index 284d0133..ed6c51a3 100644 --- a/src/ast/stmt.rs +++ b/src/ast/stmt.rs @@ -581,14 +581,14 @@ pub enum Stmt { TryCatch(Box, Position), /// [expression][Expr] Expr(Box), - /// `continue`/`break` + /// `continue`/`break` expr /// /// ### Flags /// /// * [`NONE`][ASTFlags::NONE] = `continue` /// * [`BREAK`][ASTFlags::BREAK] = `break` - BreakLoop(ASTFlags, Position), - /// `return`/`throw` + BreakLoop(Option>, ASTFlags, Position), + /// `return`/`throw` expr /// /// ### Flags /// diff --git a/src/eval/stmt.rs b/src/eval/stmt.rs index 770d4174..ae933564 100644 --- a/src/eval/stmt.rs +++ b/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) => { - Err(ERR::LoopBreak(options.contains(ASTFlags::BREAK), *pos).into()) + Stmt::BreakLoop(expr, options, pos) => { + 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 diff --git a/src/optimizer.rs b/src/optimizer.rs index bd51a7c4..08abb8f2 100644 --- a/src/optimizer.rs +++ b/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), diff --git a/src/parser.rs b/src/parser.rs index e2bcb181..9ca3a8a0 100644 --- a/src/parser.rs +++ b/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 + (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); diff --git a/src/types/error.rs b/src/types/error.rs index 7991c44f..906f32c9 100644 --- a/src/types/error.rs +++ b/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), diff --git a/tests/expressions.rs b/tests/expressions.rs index 4d0b4888..ed5e3a16 100644 --- a/tests/expressions.rs +++ b/tests/expressions.rs @@ -55,10 +55,13 @@ fn test_expressions() -> Result<(), Box> { ) .is_err()); - assert!(engine.eval_expression::<()>("40 + 2;").is_err()); - assert!(engine.eval_expression::<()>("40 + { 2 }").is_err()); - assert!(engine.eval_expression::<()>("x = 42").is_err()); + assert!(engine.compile_expression("40 + 2;").is_err()); + assert!(engine.compile_expression("40 + { 2 }").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 }")?; diff --git a/tests/for.rs b/tests/for.rs index 92e68af1..af2eed86 100644 --- a/tests/for.rs +++ b/tests/for.rs @@ -231,6 +231,22 @@ fn test_for_loop() -> Result<(), Box> { ); } + assert_eq!( + engine.eval::( + 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(()) } diff --git a/tests/while_loop.rs b/tests/while_loop.rs index 4e18a371..ebcdb8a0 100644 --- a/tests/while_loop.rs +++ b/tests/while_loop.rs @@ -22,6 +22,22 @@ fn test_while() -> Result<(), Box> { 6 ); + assert_eq!( + engine.eval::( + " + 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> { )?, 6 ); + assert_eq!( + engine.eval::( + " + 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::("do { break 42; } while false")?, 42); Ok(()) }