From f4949a2beb805cc5531836de3a6768eea4e00c4f Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Fri, 27 Jan 2023 22:31:14 +0800 Subject: [PATCH 1/5] Always search scope after scope is modified. --- src/eval/debugger.rs | 11 +++++++++-- src/eval/expr.rs | 11 ++++++++++- src/eval/stmt.rs | 9 ++++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/eval/debugger.rs b/src/eval/debugger.rs index 1bc1a5a5..652134a3 100644 --- a/src/eval/debugger.rs +++ b/src/eval/debugger.rs @@ -505,14 +505,21 @@ impl Engine { event: DebuggerEvent, ) -> Result, Box> { if let Some(ref x) = self.debugger_interface { + let orig_scope_len = scope.len(); + let src = global.source_raw().cloned(); let src = src.as_ref().map(|s| s.as_str()); let context = EvalContext::new(self, global, caches, scope, this_ptr); let (.., ref on_debugger) = **x; - let command = on_debugger(context, event, node, src, node.position())?; + let command = on_debugger(context, event, node, src, node.position()); - match command { + if orig_scope_len != scope.len() { + // The scope is changed, always search from now on + global.always_search_scope = true; + } + + match command? { DebuggerCommand::Continue => { global.debugger_mut().status = DebuggerStatus::CONTINUE; Ok(None) diff --git a/src/eval/expr.rs b/src/eval/expr.rs index 75d9c0a5..925a8132 100644 --- a/src/eval/expr.rs +++ b/src/eval/expr.rs @@ -174,9 +174,18 @@ impl Engine { // Check the variable resolver, if any if let Some(ref resolve_var) = self.resolve_var { + let orig_scope_len = scope.len(); + let context = EvalContext::new(self, global, caches, scope, this_ptr); let var_name = expr.get_variable_name(true).expect("`Expr::Variable`"); - match resolve_var(var_name, index, context) { + let resolved_var = resolve_var(var_name, index, context); + + if orig_scope_len != scope.len() { + // The scope is changed, always search from now on + global.always_search_scope = true; + } + + match resolved_var { Ok(Some(mut result)) => { result.set_access_mode(AccessMode::ReadOnly); return Ok(result.into()); diff --git a/src/eval/stmt.rs b/src/eval/stmt.rs index db2523ff..f9111ce6 100644 --- a/src/eval/stmt.rs +++ b/src/eval/stmt.rs @@ -728,10 +728,17 @@ impl Engine { nesting_level: global.scope_level, will_shadow, }; + let orig_scope_len = scope.len(); let context = EvalContext::new(self, global, caches, scope, this_ptr.as_deref_mut()); + let filter_result = filter(true, info, context); - if !filter(true, info, context)? { + if orig_scope_len != scope.len() { + // The scope is changed, always search from now on + global.always_search_scope = true; + } + + if !filter_result? { return Err(ERR::ErrorForbiddenVariable(var_name.to_string(), *pos).into()); } } From 2c631daa07b01fb057c0b2350aefef14b95ec2c3 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 28 Jan 2023 12:40:42 +0800 Subject: [PATCH 2/5] Add test for auto global constants for global namespace. --- tests/functions.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/functions.rs b/tests/functions.rs index 8e4a7f36..af9b9ba0 100644 --- a/tests/functions.rs +++ b/tests/functions.rs @@ -143,6 +143,21 @@ fn test_functions_global_module() -> Result<(), Box> { 123 ); + // Other globals + let mut module = Module::new(); + module.set_var("ANSWER", 123 as INT); + engine.register_global_module(module.into()); + + assert_eq!( + engine.eval::( + " + fn foo() { global::ANSWER } + foo() + " + )?, + 123 + ); + Ok(()) } From 62696853b426553592d2ee703312f65d93b9cf60 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sun, 29 Jan 2023 15:23:33 +0800 Subject: [PATCH 3/5] Fix bug with parsing improper module separator. --- CHANGELOG.md | 1 + src/parser.rs | 82 +++++++++++++++++++++++++++++++++++------------- tests/modules.rs | 7 +++++ 3 files changed, 69 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df828457..ce0981c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Bug fixes * Complex indexing/dotting chains now parse correctly, for example: `a[b][c[d]].e` * `map` and `filter` for arrays are marked `pure`. Warnings are added to the documentation of pure array methods that take `this` closures. +* Syntax such as `foo.bar::baz` no longer panics, but returns a proper parse error. Version 1.12.0 diff --git a/src/parser.rs b/src/parser.rs index 1230dbfc..3088825e 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -65,12 +65,15 @@ pub struct ParseState<'e, 's> { /// Tracks a list of external variables (variables that are not explicitly declared in the scope). #[cfg(not(feature = "no_closure"))] pub external_vars: Option>>, - /// An indicator that disables variable capturing into externals one single time - /// up until the nearest consumed Identifier token. - /// If set to false the next call to [`access_var`][ParseState::access_var] will not capture the variable. - /// All consequent calls to [`access_var`][ParseState::access_var] will not be affected. + /// An indicator that, when set to `false`, disables variable capturing into externals one + /// single time up until the nearest consumed Identifier token. + /// If set to false the next call to [`access_var`][ParseState::access_var] will not capture the + /// variable. All consequent calls to [`access_var`][ParseState::access_var] will not be affected. #[cfg(not(feature = "no_closure"))] pub allow_capture: bool, + /// If set to `false`, no namespace-qualified identifiers are parsed. + #[cfg(not(feature = "no_module"))] + pub allow_namespace: bool, /// Encapsulates a local stack with imported [module][crate::Module] names. #[cfg(not(feature = "no_module"))] pub imports: Option>>, @@ -98,7 +101,8 @@ impl fmt::Debug for ParseState<'_, '_> { #[cfg(not(feature = "no_module"))] f.field("imports", &self.imports) - .field("global_imports", &self.global_imports); + .field("global_imports", &self.global_imports) + .field("allow_namespace", &self.allow_namespace); f.finish() } @@ -120,6 +124,8 @@ impl<'e, 's> ParseState<'e, 's> { external_vars: None, #[cfg(not(feature = "no_closure"))] allow_capture: true, + #[cfg(not(feature = "no_module"))] + allow_namespace: true, interned_strings, external_constants, global: None, @@ -456,8 +462,8 @@ fn ensure_not_statement_expr( /// Make sure that the next expression is not a mis-typed assignment (i.e. `a = b` instead of `a == b`). fn ensure_not_assignment(input: &mut TokenStream) -> ParseResult<()> { match input.peek().expect(NEVER_ENDS) { - (Token::Equals, pos) => Err(LexError::ImproperSymbol( - "=".into(), + (token @ Token::Equals, pos) => Err(LexError::ImproperSymbol( + token.literal_syntax().into(), "Possibly a typo of '=='?".into(), ) .into_err(*pos)), @@ -1593,9 +1599,9 @@ impl Engine { token => unreachable!("Token::Identifier expected but gets {:?}", token), }; - match input.peek().expect(NEVER_ENDS).0 { + match input.peek().expect(NEVER_ENDS) { // Function call - Token::LeftParen | Token::Bang | Token::Unit => { + (Token::LeftParen | Token::Bang | Token::Unit, _) => { #[cfg(not(feature = "no_closure"))] { // Once the identifier consumed we must enable next variables capturing @@ -1609,7 +1615,15 @@ impl Engine { } // Namespace qualification #[cfg(not(feature = "no_module"))] - Token::DoubleColon => { + (token @ Token::DoubleColon, pos) => { + if !state.allow_namespace { + return Err(LexError::ImproperSymbol( + token.literal_syntax().into(), + String::new(), + ) + .into_err(*pos)); + } + #[cfg(not(feature = "no_closure"))] { // Once the identifier consumed we must enable next variables capturing @@ -1751,6 +1765,10 @@ impl Engine { let (.., ns, _, name) = *x; settings.pos = pos; + + #[cfg(not(feature = "no_module"))] + auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = true } + self.parse_fn_call(input, state, lib, settings, name, no_args, true, ns)? } // Function call @@ -1758,8 +1776,21 @@ impl Engine { let (.., ns, _, name) = *x; let no_args = t == Token::Unit; settings.pos = pos; + + #[cfg(not(feature = "no_module"))] + auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = true } + self.parse_fn_call(input, state, lib, settings, name, no_args, false, ns)? } + // Disallowed module separator + #[cfg(not(feature = "no_module"))] + (_, token @ Token::DoubleColon) if !state.allow_namespace => { + return Err(LexError::ImproperSymbol( + token.literal_syntax().into(), + String::new(), + ) + .into_err(tail_pos)) + } // module access #[cfg(not(feature = "no_module"))] (Expr::Variable(x, .., pos), Token::DoubleColon) => { @@ -1769,15 +1800,16 @@ impl Engine { namespace.push(var_name_def); - Expr::Variable( - (None, namespace, 0, state.get_interned_string(id2)).into(), - None, - pos2, - ) + let var_name = state.get_interned_string(id2); + + Expr::Variable((None, namespace, 0, var_name).into(), None, pos2) } // Indexing #[cfg(not(feature = "no_index"))] (expr, token @ (Token::LeftBracket | Token::QuestionBracket)) => { + #[cfg(not(feature = "no_module"))] + auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = true } + let opt = match token { Token::LeftBracket => ASTFlags::NONE, Token::QuestionBracket => ASTFlags::NEGATED, @@ -1805,7 +1837,13 @@ impl Engine { (.., pos) => return Err(PERR::PropertyExpected.into_err(*pos)), } - let rhs = self.parse_primary(input, state, lib, settings.level_up()?, true)?; + let rhs = { + #[cfg(not(feature = "no_module"))] + auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = false } + + self.parse_primary(input, state, lib, settings.level_up()?, true)? + }; + let op_flags = match op { Token::Period => ASTFlags::NONE, Token::Elvis => ASTFlags::NEGATED, @@ -2081,11 +2119,13 @@ impl Engine { } } // ??? && ??? = rhs, ??? || ??? = rhs, xxx ?? xxx = rhs - Expr::And(..) | Expr::Or(..) | Expr::Coalesce(..) => Err(LexError::ImproperSymbol( - "=".into(), - "Possibly a typo of '=='?".into(), - ) - .into_err(op_pos)), + Expr::And(..) | Expr::Or(..) | Expr::Coalesce(..) if !op_info.is_op_assignment() => { + Err(LexError::ImproperSymbol( + Token::Equals.literal_syntax().into(), + "Possibly a typo of '=='?".into(), + ) + .into_err(op_pos)) + } // expr = rhs _ => Err(PERR::AssignmentToInvalidLHS(String::new()).into_err(lhs.position())), } diff --git a/tests/modules.rs b/tests/modules.rs index babc862a..2c74e935 100644 --- a/tests/modules.rs +++ b/tests/modules.rs @@ -14,6 +14,13 @@ fn test_module() { assert_eq!(module.get_var_value::("answer").unwrap(), 42); } +#[test] +fn test_module_syntax() { + let engine = Engine::new(); + assert!(engine.compile("abc.def::xyz").is_err()); + assert!(engine.compile("abc.def::xyz()").is_err()); +} + #[test] fn test_module_sub_module() -> Result<(), Box> { let mut module = Module::new(); From 9f966f64734162817b1d046857f87d334a13028a Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sun, 29 Jan 2023 16:01:34 +0800 Subject: [PATCH 4/5] Fix test output. --- codegen/ui_tests/rhai_fn_duplicate_attr.stderr | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/codegen/ui_tests/rhai_fn_duplicate_attr.stderr b/codegen/ui_tests/rhai_fn_duplicate_attr.stderr index c8101565..57ce356d 100644 --- a/codegen/ui_tests/rhai_fn_duplicate_attr.stderr +++ b/codegen/ui_tests/rhai_fn_duplicate_attr.stderr @@ -4,14 +4,14 @@ error: duplicated attribute 'rhai_fn' 6 | #[rhai_fn(pure)] | ^ -error[E0433]: failed to resolve: use of undeclared crate or module `test_module` - --> ui_tests/rhai_fn_duplicate_attr.rs:13:8 - | -13 | if test_module::test_fn(n) { - | ^^^^^^^^^^^ use of undeclared crate or module `test_module` - error[E0425]: cannot find value `n` in this scope --> ui_tests/rhai_fn_duplicate_attr.rs:13:29 | 13 | if test_module::test_fn(n) { | ^ not found in this scope + +error[E0433]: failed to resolve: use of undeclared crate or module `test_module` + --> ui_tests/rhai_fn_duplicate_attr.rs:13:8 + | +13 | if test_module::test_fn(n) { + | ^^^^^^^^^^^ use of undeclared crate or module `test_module` From 6400b33b1b60a4d0ac60e72b6f5adaff91d42561 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Mon, 30 Jan 2023 12:31:33 +0800 Subject: [PATCH 5/5] Use bitflag options for chains parsing. --- src/func/call.rs | 4 +- src/parser.rs | 101 +++++++++++++++++++++-------------------------- 2 files changed, 48 insertions(+), 57 deletions(-) diff --git a/src/func/call.rs b/src/func/call.rs index 2532face..837d6fb7 100644 --- a/src/func/call.rs +++ b/src/func/call.rs @@ -11,7 +11,7 @@ use crate::eval::{Caches, FnResolutionCacheEntry, GlobalRuntimeState}; use crate::tokenizer::{is_valid_function_name, Token}; use crate::{ calc_fn_hash, calc_fn_hash_full, Dynamic, Engine, FnArgsVec, FnPtr, ImmutableString, - OptimizationLevel, Position, RhaiError, RhaiResult, RhaiResultOf, Scope, Shared, ERR, + OptimizationLevel, Position, RhaiResult, RhaiResultOf, Scope, Shared, ERR, }; #[cfg(feature = "no_std")] use hashbrown::hash_map::Entry; @@ -1053,7 +1053,7 @@ impl Engine { let mut arg_values = curry .into_iter() .map(Ok) - .chain(a_expr.iter().map(|expr| -> Result<_, RhaiError> { + .chain(a_expr.iter().map(|expr| -> Result<_, crate::RhaiError> { let this_ptr = this_ptr.as_deref_mut(); self.get_arg_value(global, caches, scope, this_ptr, expr) .map(|(v, ..)| v) diff --git a/src/parser.rs b/src/parser.rs index 3088825e..590d97f3 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -67,13 +67,12 @@ pub struct ParseState<'e, 's> { pub external_vars: Option>>, /// An indicator that, when set to `false`, disables variable capturing into externals one /// single time up until the nearest consumed Identifier token. - /// If set to false the next call to [`access_var`][ParseState::access_var] will not capture the - /// variable. All consequent calls to [`access_var`][ParseState::access_var] will not be affected. - #[cfg(not(feature = "no_closure"))] + /// + /// If set to `false` the next call to [`access_var`][ParseState::access_var] will not capture + /// the variable. + /// + /// All consequent calls to [`access_var`][ParseState::access_var] will not be affected. pub allow_capture: bool, - /// If set to `false`, no namespace-qualified identifiers are parsed. - #[cfg(not(feature = "no_module"))] - pub allow_namespace: bool, /// Encapsulates a local stack with imported [module][crate::Module] names. #[cfg(not(feature = "no_module"))] pub imports: Option>>, @@ -101,8 +100,7 @@ impl fmt::Debug for ParseState<'_, '_> { #[cfg(not(feature = "no_module"))] f.field("imports", &self.imports) - .field("global_imports", &self.global_imports) - .field("allow_namespace", &self.allow_namespace); + .field("global_imports", &self.global_imports); f.finish() } @@ -122,10 +120,7 @@ impl<'e, 's> ParseState<'e, 's> { expr_filter: |_| true, #[cfg(not(feature = "no_closure"))] external_vars: None, - #[cfg(not(feature = "no_closure"))] allow_capture: true, - #[cfg(not(feature = "no_module"))] - allow_namespace: true, interned_strings, external_constants, global: None, @@ -302,11 +297,8 @@ bitflags! { /// Is the construct being parsed located at global level? const GLOBAL_LEVEL = 0b0000_0001; /// Is the construct being parsed located inside a function definition? - #[cfg(not(feature = "no_function"))] const FN_SCOPE = 0b0000_0010; /// Is the construct being parsed located inside a closure definition? - #[cfg(not(feature = "no_function"))] - #[cfg(not(feature = "no_closure"))] const CLOSURE_SCOPE = 0b0000_0100; /// Is the construct being parsed located inside a breakable loop? const BREAKABLE = 0b0000_1000; @@ -318,6 +310,16 @@ bitflags! { } } +bitflags! { + /// Bit-flags containing all status for parsing property/indexing/namespace chains. + struct ChainingFlags: u8 { + /// Is the construct being parsed a property? + const PROPERTY = 0b0000_0001; + /// Disallow namespaces? + const DISALLOW_NAMESPACES = 0b0000_0010; + } +} + /// A type that encapsulates all the settings for a particular parsing function. #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] pub struct ParseSettings { @@ -1314,7 +1316,7 @@ impl Engine { state: &mut ParseState, lib: &mut FnLib, settings: ParseSettings, - is_property: bool, + options: ChainingFlags, ) -> ParseResult { let (token, token_pos) = input.peek().expect(NEVER_ENDS); @@ -1450,15 +1452,13 @@ impl Engine { #[cfg(feature = "no_closure")] let options = self.options | (settings.options & LangOptions::STRICT_VAR); - // Brand new flags, turn on function scope + // Brand new flags, turn on function scope and closure scope let flags = ParseSettingFlags::FN_SCOPE + | ParseSettingFlags::CLOSURE_SCOPE | (settings.flags & (ParseSettingFlags::DISALLOW_UNQUOTED_MAP_PROPERTIES | ParseSettingFlags::DISALLOW_STATEMENTS_IN_BLOCKS)); - #[cfg(not(feature = "no_closure"))] - let flags = flags | ParseSettingFlags::CLOSURE_SCOPE; // turn on closure scope - let new_settings = ParseSettings { flags, options, @@ -1602,11 +1602,9 @@ impl Engine { match input.peek().expect(NEVER_ENDS) { // Function call (Token::LeftParen | Token::Bang | Token::Unit, _) => { - #[cfg(not(feature = "no_closure"))] - { - // Once the identifier consumed we must enable next variables capturing - state.allow_capture = true; - } + // Once the identifier consumed we must enable next variables capturing + state.allow_capture = true; + Expr::Variable( (None, ns, 0, state.get_interned_string(*s)).into(), None, @@ -1616,7 +1614,7 @@ impl Engine { // Namespace qualification #[cfg(not(feature = "no_module"))] (token @ Token::DoubleColon, pos) => { - if !state.allow_namespace { + if options.contains(ChainingFlags::DISALLOW_NAMESPACES) { return Err(LexError::ImproperSymbol( token.literal_syntax().into(), String::new(), @@ -1624,11 +1622,9 @@ impl Engine { .into_err(*pos)); } - #[cfg(not(feature = "no_closure"))] - { - // Once the identifier consumed we must enable next variables capturing - state.allow_capture = true; - } + // Once the identifier consumed we must enable next variables capturing + state.allow_capture = true; + let name = state.get_interned_string(*s); Expr::Variable((None, ns, 0, name).into(), None, settings.pos) } @@ -1636,7 +1632,7 @@ impl Engine { _ => { let (index, is_func) = state.access_var(&s, lib, settings.pos); - if !is_property + if !options.contains(ChainingFlags::PROPERTY) && !is_func && index.is_none() && settings.has_option(LangOptions::STRICT_VAR) @@ -1708,7 +1704,14 @@ impl Engine { return Ok(root_expr); } - self.parse_postfix(input, state, lib, settings, root_expr) + self.parse_postfix( + input, + state, + lib, + settings, + root_expr, + ChainingFlags::empty(), + ) } /// Tail processing of all possible postfix operators of a primary expression. @@ -1719,6 +1722,7 @@ impl Engine { lib: &mut FnLib, settings: ParseSettings, mut lhs: Expr, + options: ChainingFlags, ) -> ParseResult { let mut settings = settings; @@ -1766,9 +1770,6 @@ impl Engine { let (.., ns, _, name) = *x; settings.pos = pos; - #[cfg(not(feature = "no_module"))] - auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = true } - self.parse_fn_call(input, state, lib, settings, name, no_args, true, ns)? } // Function call @@ -1777,14 +1778,13 @@ impl Engine { let no_args = t == Token::Unit; settings.pos = pos; - #[cfg(not(feature = "no_module"))] - auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = true } - self.parse_fn_call(input, state, lib, settings, name, no_args, false, ns)? } // Disallowed module separator #[cfg(not(feature = "no_module"))] - (_, token @ Token::DoubleColon) if !state.allow_namespace => { + (_, token @ Token::DoubleColon) + if options.contains(ChainingFlags::DISALLOW_NAMESPACES) => + { return Err(LexError::ImproperSymbol( token.literal_syntax().into(), String::new(), @@ -1807,9 +1807,6 @@ impl Engine { // Indexing #[cfg(not(feature = "no_index"))] (expr, token @ (Token::LeftBracket | Token::QuestionBracket)) => { - #[cfg(not(feature = "no_module"))] - auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = true } - let opt = match token { Token::LeftBracket => ASTFlags::NONE, Token::QuestionBracket => ASTFlags::NEGATED, @@ -1824,11 +1821,8 @@ impl Engine { // Expression after dot must start with an identifier match input.peek().expect(NEVER_ENDS) { (Token::Identifier(..), ..) => { - #[cfg(not(feature = "no_closure"))] - { - // Prevents capturing of the object properties as vars: xxx. - state.allow_capture = false; - } + // Prevents capturing of the object properties as vars: xxx. + state.allow_capture = false; } (Token::Reserved(s), ..) if is_keyword_function(s).1 => (), (Token::Reserved(s), pos) => { @@ -1837,18 +1831,15 @@ impl Engine { (.., pos) => return Err(PERR::PropertyExpected.into_err(*pos)), } - let rhs = { - #[cfg(not(feature = "no_module"))] - auto_restore! { let orig_allow_namespace = state.allow_namespace; state.allow_namespace = false } - - self.parse_primary(input, state, lib, settings.level_up()?, true)? - }; - let op_flags = match op { Token::Period => ASTFlags::NONE, Token::Elvis => ASTFlags::NEGATED, _ => unreachable!("`.` or `?.`"), }; + let options = ChainingFlags::PROPERTY | ChainingFlags::DISALLOW_NAMESPACES; + let rhs = + self.parse_primary(input, state, lib, settings.level_up()?, options)?; + Self::make_dot_expr(state, expr, rhs, ASTFlags::NONE, op_flags, tail_pos)? } // Unknown postfix operator @@ -2020,7 +2011,7 @@ impl Engine { // Token::EOF => Err(PERR::UnexpectedEOF.into_err(settings.pos)), // All other tokens - _ => self.parse_primary(input, state, lib, settings, false), + _ => self.parse_primary(input, state, lib, settings, ChainingFlags::empty()), } }