From 796690f50627355fff29da1c8b6bcd68da7df252 Mon Sep 17 00:00:00 2001
From: Stephen Chung <Stephen.Chung@intexact.com>
Date: Fri, 27 Mar 2020 16:46:08 +0800
Subject: [PATCH] Detect duplicated parameters in function definitions.

---
 src/engine.rs    |  6 +-----
 src/error.rs     | 10 ++++++++++
 src/parser.rs    | 22 +++++++++++++++++++---
 tests/call_fn.rs | 19 ++++++++++++++++++-
 4 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/src/engine.rs b/src/engine.rs
index aea9f29e..9ce74b9d 100644
--- a/src/engine.rs
+++ b/src/engine.rs
@@ -32,7 +32,7 @@ pub type FnAny = dyn Fn(&mut FnCallArgs, Position) -> Result<Dynamic, EvalAltRes
 
 type IteratorFn = dyn Fn(&Dynamic) -> Box<dyn Iterator<Item = Dynamic>>;
 
-pub(crate) const MAX_CALL_STACK_DEPTH: usize = 50;
+pub(crate) const MAX_CALL_STACK_DEPTH: usize = 64;
 pub(crate) const KEYWORD_PRINT: &str = "print";
 pub(crate) const KEYWORD_DEBUG: &str = "debug";
 pub(crate) const KEYWORD_DUMP_AST: &str = "dump_ast";
@@ -242,10 +242,6 @@ impl Engine<'_> {
         pos: Position,
         level: usize,
     ) -> Result<Dynamic, EvalAltResult> {
-        if level >= self.max_call_stack_depth {
-            return Err(EvalAltResult::ErrorStackOverflow(pos));
-        }
-
         // First search in script-defined functions (can override built-in)
         if let Some(fn_def) = self.fn_lib.get_function(fn_name, args.len()) {
             let mut scope = Scope::new();
diff --git a/src/error.rs b/src/error.rs
index e1b41510..4ec60ec9 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -82,6 +82,9 @@ pub enum ParseErrorType {
     /// A function definition is missing the parameters list. Wrapped value is the function name.
     #[cfg(not(feature = "no_function"))]
     FnMissingParams(String),
+    /// A function definition has duplicated parameters. Wrapped values are the function name and parameter name.
+    #[cfg(not(feature = "no_function"))]
+    FnDuplicateParam(String, String),
     /// A function definition is missing the body. Wrapped value is the function name.
     #[cfg(not(feature = "no_function"))]
     FnMissingBody(String),
@@ -146,6 +149,8 @@ impl ParseError {
             #[cfg(not(feature = "no_function"))]
             ParseErrorType::FnMissingParams(_) => "Expecting parameters in function declaration",
             #[cfg(not(feature = "no_function"))]
+            ParseErrorType::FnDuplicateParam(_,_) => "Duplicated parameters in function declaration",
+            #[cfg(not(feature = "no_function"))]
             ParseErrorType::FnMissingBody(_) => "Expecting body statement block for function declaration",
             #[cfg(not(feature = "no_function"))]
             ParseErrorType::WrongFnDefinition => "Function definitions must be at global level and cannot be inside a block or another function",
@@ -187,6 +192,11 @@ impl fmt::Display for ParseError {
                 write!(f, "Expecting body statement block for function '{}'", s)?
             }
 
+            #[cfg(not(feature = "no_function"))]
+            ParseErrorType::FnDuplicateParam(ref s, ref arg) => {
+                write!(f, "Duplicated parameter '{}' for function '{}'", arg, s)?
+            }
+
             ParseErrorType::MissingRightParen(ref s) | ParseErrorType::MissingRightBrace(ref s) => {
                 write!(f, "{} for {}", self.desc(), s)?
             }
diff --git a/src/parser.rs b/src/parser.rs
index d87910b8..3881e7ec 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -2287,8 +2287,8 @@ fn parse_fn<'a>(
                 .next()
                 .ok_or_else(|| PERR::MissingRightParen(end_err.to_string()).into_err_eof())?
             {
-                (Token::Identifier(s), _) => {
-                    params.push(s);
+                (Token::Identifier(s), pos) => {
+                    params.push((s, pos));
                 }
                 (_, pos) => return Err(PERR::MissingRightParen(end_err).into_err(pos)),
             }
@@ -2305,6 +2305,22 @@ fn parse_fn<'a>(
         }
     }
 
+    // Check for duplicating parameters
+    params
+        .iter()
+        .enumerate()
+        .try_for_each(|(i, (p1, _))| {
+            params
+                .iter()
+                .skip(i + 1)
+                .find(|(p2, _)| p2 == p1)
+                .map_or_else(|| Ok(()), |(p2, pos)| Err((p2, *pos)))
+        })
+        .map_err(|(p, pos)| {
+            PERR::FnDuplicateParam(name.to_string(), p.to_string()).into_err(pos)
+        })?;
+
+    // Parse function body
     let body = match input.peek() {
         Some((Token::LeftBrace, _)) => parse_block(input, false, allow_stmt_expr)?,
         Some((_, pos)) => return Err(PERR::FnMissingBody(name).into_err(*pos)),
@@ -2313,7 +2329,7 @@ fn parse_fn<'a>(
 
     Ok(FnDef {
         name,
-        params,
+        params: params.into_iter().map(|(p, _)| p).collect(),
         body,
         pos,
     })
diff --git a/tests/call_fn.rs b/tests/call_fn.rs
index ca6303d6..3a1c37ec 100644
--- a/tests/call_fn.rs
+++ b/tests/call_fn.rs
@@ -1,5 +1,22 @@
 #![cfg(not(feature = "no_function"))]
-use rhai::{Engine, EvalAltResult, INT};
+use rhai::{Engine, EvalAltResult, ParseError, ParseErrorType, INT};
+
+#[test]
+fn test_fn() -> Result<(), EvalAltResult> {
+    let mut engine = Engine::new();
+
+    // Expect duplicated parameters error
+    match engine
+        .compile("fn hello(x, x) { x }")
+        .expect_err("should be error")
+        .error_type()
+    {
+        ParseErrorType::FnDuplicateParam(f, p) if f == "hello" && p == "x" => (),
+        _ => assert!(false, "wrong error"),
+    }
+
+    Ok(())
+}
 
 #[test]
 fn test_call_fn() -> Result<(), EvalAltResult> {