From b8c4054c20d2b2d635d70f64d3ea0ededd41b990 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 4 Dec 2021 17:57:28 +0800 Subject: [PATCH] Add strict variables mode. --- CHANGELOG.md | 8 ++- src/api/options.rs | 13 ++++ src/parser.rs | 152 +++++++++++++++++++++++++++------------ src/types/parse_error.rs | 27 +++++-- tests/options.rs | 58 ++++++++++++++- 5 files changed, 203 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aec63d31..2e8c0fb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,13 @@ Bug fixes New features ------------ -* New options for `Engine` which allows disabling `if`-expressions, `switch`-expressions, statement expressions, anonymous functions and/or looping (i.e. `while`, `loop`, `do` and `for` statements). +* New options for `Engine` which allows disabling `if`-expressions, `switch`-expressions, statement expressions, anonymous functions and/or looping (i.e. `while`, `loop`, `do` and `for` statements): + * `Engine::set_allow_if_expression` + * `Engine::set_allow_switch_expression` + * `Engine::set_allow_statement_expression` + * `Engine::set_allow_anonymous_fn` + * `Engine::set_allow_looping` +* New _strict variables_ mode for `Engine` (enabled via `Engine::set_strict_variables`) to throw parse errors on undefined variable usage. Two new parse error variants, `ParseErrorType::VariableNotFound` and `ParseErrorType::ModuleNotFound`, are added. Enhancements ------------ diff --git a/src/api/options.rs b/src/api/options.rs index 0a8f725a..a6e1f59b 100644 --- a/src/api/options.rs +++ b/src/api/options.rs @@ -18,6 +18,8 @@ pub struct LanguageOptions { pub allow_anonymous_fn: bool, /// Is looping allowed? pub allow_loop: bool, + /// Strict variables mode? + pub strict_var: bool, } impl LanguageOptions { @@ -31,6 +33,7 @@ impl LanguageOptions { #[cfg(not(feature = "no_function"))] allow_anonymous_fn: true, allow_loop: true, + strict_var: false, } } } @@ -98,4 +101,14 @@ impl Engine { pub fn set_allow_looping(&mut self, enable: bool) { self.options.allow_loop = enable; } + /// Is strict variables mode enabled? + #[inline(always)] + pub fn strict_variables(&self) -> bool { + self.options.strict_var + } + /// Set whether strict variables mode is enabled. + #[inline(always)] + pub fn set_strict_variables(&mut self, enable: bool) { + self.options.strict_var = enable; + } } diff --git a/src/parser.rs b/src/parser.rs index 3388ded2..ac80dce9 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -147,6 +147,7 @@ impl<'e> ParseState<'e> { /// /// Return `None` when the variable name is not found in the `stack`. #[inline] + #[must_use] pub fn access_var(&mut self, name: impl AsRef, pos: Position) -> Option { let name = name.as_ref(); let mut barrier = false; @@ -226,10 +227,16 @@ struct ParseSettings { /// Is the construct being parsed located at function definition level? #[cfg(not(feature = "no_function"))] is_function_scope: bool, + /// Is the construct being parsed located inside a closure? + #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_closure"))] + is_closure: bool, /// Is the current position inside a loop? is_breakable: bool, /// Default language options. default_options: LanguageOptions, + /// Is strict variables mode enabled? + strict_var: bool, /// Is anonymous function allowed? #[cfg(not(feature = "no_function"))] allow_anonymous_fn: bool, @@ -239,8 +246,6 @@ struct ParseSettings { allow_switch_expr: bool, /// Is statement-expression allowed? allow_stmt_expr: bool, - /// Is looping allowed? - allow_loop: bool, /// Current expression nesting level. level: usize, } @@ -491,15 +496,23 @@ fn parse_fn_call( Token::RightParen => { eat_token(input, Token::RightParen); - let hash = namespace.as_mut().map_or_else( - || calc_fn_hash(&id, 0), - |modules| { - #[cfg(not(feature = "no_module"))] - modules.set_index(state.find_module(&modules[0].name)); + let hash = if let Some(modules) = namespace.as_mut() { + #[cfg(not(feature = "no_module"))] + { + let index = state.find_module(&modules[0].name); - calc_qualified_fn_hash(modules.iter().map(|m| m.name.as_str()), &id, 0) - }, - ); + if !settings.is_function_scope && settings.strict_var && index.is_none() { + return Err(ParseErrorType::ModuleUndefined(modules[0].name.to_string()) + .into_err(modules[0].pos)); + } + + modules.set_index(index); + } + + calc_qualified_fn_hash(modules.iter().map(|m| m.name.as_str()), &id, 0) + } else { + calc_fn_hash(&id, 0) + }; let hashes = if is_valid_function_name(&id) { hash.into() @@ -537,19 +550,25 @@ fn parse_fn_call( (Token::RightParen, _) => { eat_token(input, Token::RightParen); - let hash = namespace.as_mut().map_or_else( - || calc_fn_hash(&id, args.len()), - |modules| { - #[cfg(not(feature = "no_module"))] - modules.set_index(state.find_module(&modules[0].name)); + let hash = if let Some(modules) = namespace.as_mut() { + #[cfg(not(feature = "no_module"))] + { + let index = state.find_module(&modules[0].name); - calc_qualified_fn_hash( - modules.iter().map(|m| m.name.as_str()), - &id, - args.len(), - ) - }, - ); + if !settings.is_function_scope && settings.strict_var && index.is_none() { + return Err(ParseErrorType::ModuleUndefined( + modules[0].name.to_string(), + ) + .into_err(modules[0].pos)); + } + + modules.set_index(index); + } + + calc_qualified_fn_hash(modules.iter().map(|m| m.name.as_str()), &id, args.len()) + } else { + calc_fn_hash(&id, args.len()) + }; let hashes = if is_valid_function_name(&id) { hash.into() @@ -1171,25 +1190,42 @@ fn parse_primary( new_state.max_expr_depth = new_state.max_function_expr_depth; } - let settings = ParseSettings { + let new_settings = ParseSettings { allow_if_expr: settings.default_options.allow_if_expr, allow_switch_expr: settings.default_options.allow_switch_expr, allow_stmt_expr: settings.default_options.allow_stmt_expr, allow_anonymous_fn: settings.default_options.allow_anonymous_fn, - allow_loop: settings.default_options.allow_loop, + strict_var: if cfg!(feature = "no_closure") { + settings.strict_var + } else { + // A capturing closure can access variables not defined locally + false + }, is_global: false, is_function_scope: true, + #[cfg(not(feature = "no_closure"))] + is_closure: true, is_breakable: false, level: 0, ..settings }; - let (expr, func) = parse_anon_fn(input, &mut new_state, lib, settings)?; + let (expr, func) = parse_anon_fn(input, &mut new_state, lib, new_settings)?; #[cfg(not(feature = "no_closure"))] - new_state.external_vars.iter().for_each(|(closure, &pos)| { - state.access_var(closure, pos); - }); + new_state.external_vars.iter().try_for_each( + |(captured_var, &pos)| -> Result<_, ParseError> { + let index = state.access_var(captured_var, pos); + + if !settings.is_closure && settings.strict_var && index.is_none() { + // If the parent scope is not inside another capturing closure + Err(ParseErrorType::VariableUndefined(captured_var.to_string()) + .into_err(pos)) + } else { + Ok(()) + } + }, + )?; let hash_script = calc_fn_hash(&func.name, func.params.len()); lib.insert(hash_script, func.into()); @@ -1292,6 +1328,13 @@ fn parse_primary( // Normal variable access _ => { let index = state.access_var(&s, settings.pos); + + if settings.strict_var && index.is_none() { + return Err( + ParseErrorType::VariableUndefined(s.to_string()).into_err(settings.pos) + ); + } + let short_index = index.and_then(|x| { if x.get() <= u8::MAX as usize { NonZeroU8::new(x.get() as u8) @@ -1465,15 +1508,21 @@ fn parse_primary( _ => None, }; - if let Some(x) = namespaced_variable { - match x { - (_, Some((namespace, hash)), name) => { - *hash = calc_qualified_var_hash(namespace.iter().map(|v| v.name.as_str()), name); + if let Some((_, Some((namespace, hash)), name)) = namespaced_variable { + *hash = calc_qualified_var_hash(namespace.iter().map(|v| v.name.as_str()), name); - #[cfg(not(feature = "no_module"))] - namespace.set_index(state.find_module(&namespace[0].name)); + #[cfg(not(feature = "no_module"))] + { + let index = state.find_module(&namespace[0].name); + + if !settings.is_function_scope && settings.strict_var && index.is_none() { + return Err( + ParseErrorType::ModuleUndefined(namespace[0].name.to_string()) + .into_err(namespace[0].pos), + ); } - _ => unreachable!("expecting namespace-qualified variable access"), + + namespace.set_index(index); } } @@ -2800,14 +2849,15 @@ fn parse_stmt( new_state.max_expr_depth = new_state.max_function_expr_depth; } - let settings = ParseSettings { + let new_settings = ParseSettings { allow_if_expr: settings.default_options.allow_if_expr, allow_switch_expr: settings.default_options.allow_switch_expr, allow_stmt_expr: settings.default_options.allow_stmt_expr, allow_anonymous_fn: settings.default_options.allow_anonymous_fn, - allow_loop: settings.default_options.allow_loop, + strict_var: settings.strict_var, is_global: false, is_function_scope: true, + is_closure: false, is_breakable: false, level: 0, pos, @@ -2819,7 +2869,7 @@ fn parse_stmt( &mut new_state, lib, access, - settings, + new_settings, #[cfg(not(feature = "no_function"))] #[cfg(feature = "metadata")] comments, @@ -2850,21 +2900,25 @@ fn parse_stmt( Token::If => parse_if(input, state, lib, settings.level_up()), Token::Switch => parse_switch(input, state, lib, settings.level_up()), - Token::While | Token::Loop if settings.allow_loop => { + Token::While | Token::Loop if settings.default_options.allow_loop => { parse_while_loop(input, state, lib, settings.level_up()) } - Token::Do if settings.allow_loop => parse_do(input, state, lib, settings.level_up()), - Token::For if settings.allow_loop => parse_for(input, state, lib, settings.level_up()), + Token::Do if settings.default_options.allow_loop => { + parse_do(input, state, lib, settings.level_up()) + } + Token::For if settings.default_options.allow_loop => { + parse_for(input, state, lib, settings.level_up()) + } - Token::Continue if settings.allow_loop && settings.is_breakable => { + Token::Continue if settings.default_options.allow_loop && settings.is_breakable => { let pos = eat_token(input, Token::Continue); Ok(Stmt::BreakLoop(AST_OPTION_NONE, pos)) } - Token::Break if settings.allow_loop && settings.is_breakable => { + Token::Break if settings.default_options.allow_loop && settings.is_breakable => { let pos = eat_token(input, Token::Break); Ok(Stmt::BreakLoop(AST_OPTION_BREAK_OUT, pos)) } - Token::Continue | Token::Break if settings.allow_loop => { + Token::Continue | Token::Break if settings.default_options.allow_loop => { Err(PERR::LoopBreak.into_err(token_pos)) } @@ -3250,10 +3304,13 @@ impl Engine { allow_stmt_expr: false, #[cfg(not(feature = "no_function"))] allow_anonymous_fn: false, - allow_loop: false, + strict_var: self.options.strict_var, is_global: true, #[cfg(not(feature = "no_function"))] is_function_scope: false, + #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_closure"))] + is_closure: false, is_breakable: false, level: 0, pos: Position::NONE, @@ -3308,10 +3365,13 @@ impl Engine { allow_stmt_expr: self.options.allow_stmt_expr, #[cfg(not(feature = "no_function"))] allow_anonymous_fn: self.options.allow_anonymous_fn, - allow_loop: self.options.allow_loop, + strict_var: self.options.strict_var, is_global: true, #[cfg(not(feature = "no_function"))] is_function_scope: false, + #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_closure"))] + is_closure: false, is_breakable: false, level: 0, pos: Position::NONE, diff --git a/src/types/parse_error.rs b/src/types/parse_error.rs index da537cdb..1e9a591c 100644 --- a/src/types/parse_error.rs +++ b/src/types/parse_error.rs @@ -164,6 +164,16 @@ pub enum ParseErrorType { /// Assignment to an inappropriate LHS (left-hand-side) expression. /// Wrapped value is the error message (if any). AssignmentToInvalidLHS(String), + /// A variable is not found. + /// + /// Only appears when strict variables mode is enabled. + VariableUndefined(String), + /// An imported module is not found. + /// + /// Only appears when strict variables mode is enabled. + /// + /// Never appears under the `no_module` feature. + ModuleUndefined(String), /// Expression exceeding the maximum levels of complexity. /// /// Never appears under the `unchecked` feature. @@ -210,7 +220,7 @@ impl fmt::Display for ParseErrorType { }, Self::FnDuplicatedDefinition(s, n) => { - write!(f, "Function '{}' with ", s)?; + write!(f, "Function {} with ", s)?; match n { 0 => f.write_str("no parameters already exists"), 1 => f.write_str("1 parameter already exists"), @@ -219,14 +229,17 @@ impl fmt::Display for ParseErrorType { } Self::FnMissingBody(s) => match s.as_str() { "" => f.write_str("Expecting body statement block for anonymous function"), - s => write!(f, "Expecting body statement block for function '{}'", s) + s => write!(f, "Expecting body statement block for function {}", s) }, - Self::FnMissingParams(s) => write!(f, "Expecting parameters for function '{}'", s), - Self::FnDuplicatedParam(s, arg) => write!(f, "Duplicated parameter '{}' for function '{}'", arg, s), + Self::FnMissingParams(s) => write!(f, "Expecting parameters for function {}", s), + Self::FnDuplicatedParam(s, arg) => write!(f, "Duplicated parameter {} for function {}", arg, s), - Self::DuplicatedProperty(s) => write!(f, "Duplicated property '{}' for object map literal", s), + Self::DuplicatedProperty(s) => write!(f, "Duplicated property for object map literal: {}", s), Self::DuplicatedSwitchCase => f.write_str("Duplicated switch case"), - Self::DuplicatedVariable(s) => write!(f, "Duplicated variable name '{}'", s), + Self::DuplicatedVariable(s) => write!(f, "Duplicated variable name: {}", s), + + Self::VariableUndefined(s) => write!(f, "Undefined variable: {}", s), + Self::ModuleUndefined(s) => write!(f, "Undefined module: {}", s), Self::MismatchedType(r, a) => write!(f, "Expecting {}, not {}", r, a), Self::ExprExpected(s) => write!(f, "Expecting {} expression", s), @@ -237,7 +250,7 @@ impl fmt::Display for ParseErrorType { Self::AssignmentToConstant(s) => match s.as_str() { "" => f.write_str("Cannot assign to a constant value"), - s => write!(f, "Cannot assign to constant '{}'", s) + s => write!(f, "Cannot assign to constant {}", s) }, Self::AssignmentToInvalidLHS(s) => match s.as_str() { "" => f.write_str("Expression cannot be assigned to"), diff --git a/tests/options.rs b/tests/options.rs index 01574e74..336a2856 100644 --- a/tests/options.rs +++ b/tests/options.rs @@ -1,7 +1,7 @@ use rhai::{Engine, EvalAltResult}; #[test] -fn test_options() -> Result<(), Box> { +fn test_options_allow() -> Result<(), Box> { let mut engine = Engine::new(); engine.compile("let x = if y { z } else { w };")?; @@ -33,3 +33,59 @@ fn test_options() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_options_strict_var() -> Result<(), Box> { + let mut engine = Engine::new(); + + engine.compile("let x = if y { z } else { w };")?; + + #[cfg(not(feature = "no_function"))] + engine.compile("fn foo(x) { x + y }")?; + + #[cfg(not(feature = "no_module"))] + engine.compile("print(h::y::z);")?; + + #[cfg(not(feature = "no_module"))] + engine.compile("let x = h::y::foo();")?; + + #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_module"))] + engine.compile("fn foo() { h::y::foo() }")?; + + #[cfg(not(feature = "no_function"))] + engine.compile("let f = |y| x * y;")?; + + engine.set_strict_variables(true); + + assert!(engine.compile("let x = if y { z } else { w };").is_err()); + engine.compile("let y = 42; let x = y;")?; + + #[cfg(not(feature = "no_function"))] + assert!(engine.compile("fn foo(x) { x + y }").is_err()); + + #[cfg(not(feature = "no_module"))] + { + assert!(engine.compile("print(h::y::z);").is_err()); + engine.compile(r#"import "hello" as h; print(h::y::z);"#)?; + assert!(engine.compile("let x = h::y::foo();").is_err()); + engine.compile(r#"import "hello" as h; let x = h::y::foo();"#)?; + } + + #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_module"))] + engine.compile("fn foo() { h::y::foo() }")?; + + #[cfg(not(feature = "no_function"))] + { + assert!(engine.compile("let f = |y| x * y;").is_err()); + #[cfg(not(feature = "no_closure"))] + { + engine.compile("let x = 42; let f = |y| x * y;")?; + engine.compile("let x = 42; let f = |y| { || x + y };")?; + assert!(engine.compile("fn foo() { |y| { || x + y } }").is_err()); + } + } + + Ok(()) +}