Merge pull request #699 from schungx/master

Bug fixes.
2023-01-30 14:12:04 +08:00 · 2023-01-30 14:12:04 +08:00 · 6980096a09
commit 6980096a09
parent 663125a66d 6400b33b1b
9 changed files with 135 additions and 58 deletions
--- 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
--- 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`
--- a/src/eval/debugger.rs
+++ b/src/eval/debugger.rs
@ -505,14 +505,21 @@ impl Engine {
        event: DebuggerEvent,
    ) -> Result<Option<DebuggerStatus>, Box<crate::EvalAltResult>> {
        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)
--- 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());
--- 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());
                    }
                }
--- 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)
--- a/src/parser.rs
+++ b/src/parser.rs
@ -65,11 +65,13 @@ 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<Box<crate::FnArgsVec<Ident>>>,
-    /// 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.
+    /// 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,
    /// Encapsulates a local stack with imported [module][crate::Module] names.
    #[cfg(not(feature = "no_module"))]
@ -118,7 +120,6 @@ 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,
            interned_strings,
            external_constants,
@ -296,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;
@ -312,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 {
@ -456,8 +464,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)),
@ -1308,7 +1316,7 @@ impl Engine {
        state: &mut ParseState,
        lib: &mut FnLib,
        settings: ParseSettings,
-        is_property: bool,
+        options: ChainingFlags,
    ) -> ParseResult<Expr> {
        let (token, token_pos) = input.peek().expect(NEVER_ENDS);

@ -1444,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,
@ -1593,14 +1599,12 @@ 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 => {
-                        #[cfg(not(feature = "no_closure"))]
-                        {
+                    (Token::LeftParen | Token::Bang | Token::Unit, _) => {
                        // 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,
@ -1609,12 +1613,18 @@ impl Engine {
                    }
                    // Namespace qualification
                    #[cfg(not(feature = "no_module"))]
-                    Token::DoubleColon => {
-                        #[cfg(not(feature = "no_closure"))]
-                        {
+                    (token @ Token::DoubleColon, pos) => {
+                        if options.contains(ChainingFlags::DISALLOW_NAMESPACES) {
+                            return Err(LexError::ImproperSymbol(
+                                token.literal_syntax().into(),
+                                String::new(),
+                            )
+                            .into_err(*pos));
+                        }
+
                        // 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)
                    }
@ -1622,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)
@ -1694,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.
@ -1705,6 +1722,7 @@ impl Engine {
        lib: &mut FnLib,
        settings: ParseSettings,
        mut lhs: Expr,
+        options: ChainingFlags,
    ) -> ParseResult<Expr> {
        let mut settings = settings;

@ -1751,6 +1769,7 @@ impl Engine {

                    let (.., ns, _, name) = *x;
                    settings.pos = pos;
+
                    self.parse_fn_call(input, state, lib, settings, name, no_args, true, ns)?
                }
                // Function call
@ -1758,8 +1777,20 @@ impl Engine {
                    let (.., ns, _, name) = *x;
                    let no_args = t == Token::Unit;
                    settings.pos = pos;
+
                    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 options.contains(ChainingFlags::DISALLOW_NAMESPACES) =>
+                {
+                    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,11 +1800,9 @@ 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"))]
@ -1792,12 +1821,9 @@ 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.<var>
                            state.allow_capture = false;
                        }
-                        }
                        (Token::Reserved(s), ..) if is_keyword_function(s).1 => (),
                        (Token::Reserved(s), pos) => {
                            return Err(PERR::Reserved(s.to_string()).into_err(*pos))
@ -1805,12 +1831,15 @@ impl Engine {
                        (.., pos) => return Err(PERR::PropertyExpected.into_err(*pos)),
                    }

-                    let rhs = 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
@ -1982,7 +2011,7 @@ impl Engine {
            // <EOF>
            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()),
        }
    }

@ -2081,11 +2110,13 @@ impl Engine {
                }
            }
            // ??? && ??? = rhs, ??? || ??? = rhs, xxx ?? xxx = rhs
-            Expr::And(..) | Expr::Or(..) | Expr::Coalesce(..) => Err(LexError::ImproperSymbol(
-                "=".into(),
+            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)),
+                .into_err(op_pos))
+            }
            // expr = rhs
            _ => Err(PERR::AssignmentToInvalidLHS(String::new()).into_err(lhs.position())),
        }
--- a/tests/functions.rs
+++ b/tests/functions.rs
@ -143,6 +143,21 @@ fn test_functions_global_module() -> Result<(), Box<EvalAltResult>> {
        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::<INT>(
+            "
+                fn foo() { global::ANSWER }
+                foo()
+            "
+        )?,
+        123
+    );
+
    Ok(())
 }

--- a/tests/modules.rs
+++ b/tests/modules.rs
@ -14,6 +14,13 @@ fn test_module() {
    assert_eq!(module.get_var_value::<INT>("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<EvalAltResult>> {
    let mut module = Module::new();