Disallow duplicated function definitions.

RELEASES.md

@@ -8,6 +8,8 @@ Breaking changes
 ----------------
 
 * The error variant `EvalAltResult::ErrorInFunctionCall` has a new parameter holding the _source_ of the function.
+* `ParseErrorType::WrongFnDefinition` is renamed `FnWrongDefinition`.
+* Redefining an existing function within the same script now throws a new `ParseErrorType::FnDuplicatedDefinition`. This is to prevent accidental overwriting an earlier function definition.
 
 Enhancements
 ------------

src/fn_call.rs

@@ -734,7 +734,7 @@ impl Engine {
 
         // If new functions are defined within the eval string, it is an error
         if ast.lib().count().0 != 0 {
-            return Err(ParseErrorType::WrongFnDefinition.into());
+            return Err(ParseErrorType::FnWrongDefinition.into());
         }
 
         // Evaluate the AST

src/parse_error.rs

@@ -131,7 +131,12 @@ pub enum ParseErrorType {
     /// Defining a function `fn` in an appropriate place (e.g. inside another function).
     ///
     /// Never appears under the `no_function` feature.
-    WrongFnDefinition,
+    FnWrongDefinition,
+    /// Defining a function with a name that conflicts with an existing function.
+    /// Wrapped values are the function name and number of parameters.
+    ///
+    /// Never appears under the `no_object` feature.
+    FnDuplicatedDefinition(String, usize),
     /// Missing a function name after the `fn` keyword.
     ///
     /// Never appears under the `no_function` feature.
@@ -180,7 +185,7 @@ impl ParseErrorType {
     pub(crate) fn desc(&self) -> &str {
         match self {
             Self::UnexpectedEOF => "Script is incomplete",
-            Self::BadInput(p) => p.desc(),
+            Self::BadInput(err) => err.desc(),
             Self::UnknownOperator(_) => "Unknown operator",
             Self::MissingToken(_, _) => "Expecting a certain token that is missing",
             Self::MalformedCallExpr(_) => "Invalid expression in function call arguments",
@@ -193,12 +198,13 @@ impl ParseErrorType {
             Self::VariableExpected => "Expecting name of a variable",
             Self::Reserved(_) => "Invalid use of reserved keyword",
             Self::ExprExpected(_) => "Expecting an expression",
+            Self::FnWrongDefinition => "Function definitions must be at global level and cannot be inside a block or another function",
+            Self::FnDuplicatedDefinition(_, _) => "Function already exists",
             Self::FnMissingName => "Expecting function name in function declaration",
             Self::FnMissingParams(_) => "Expecting parameters in function declaration",
             Self::FnDuplicatedParam(_,_) => "Duplicated parameters in function declaration",
             Self::FnMissingBody(_) => "Expecting body statement block for function declaration",
             Self::WrongDocComment => "Doc-comment must be followed immediately by a function definition",
-            Self::WrongFnDefinition => "Function definitions must be at global level and cannot be inside a block or another function",
             Self::WrongExport => "Export statement can only appear at global level",
             Self::AssignmentToConstant(_) => "Cannot assign to a constant value",
             Self::AssignmentToInvalidLHS(_) => "Expression cannot be assigned to",
@@ -221,6 +227,14 @@ impl fmt::Display for ParseErrorType {
                 f.write_str(if s.is_empty() { self.desc() } else { s })
             }
 
+            Self::FnDuplicatedDefinition(s, n) => {
+                write!(f, "Function '{}' with ", s)?;
+                match n {
+                    0 => f.write_str("no parameters already exists"),
+                    1 => f.write_str("1 parameter already exists"),
+                    _ => write!(f, "{} parameters already exists", n),
+                }
+            }
             Self::DuplicatedProperty(s) => {
                 write!(f, "Duplicated property '{}' for object map literal", s)
             }

src/parser.rs

@@ -2585,7 +2585,7 @@ fn parse_stmt(
 
         // fn ...
         #[cfg(not(feature = "no_function"))]
-        Token::Fn if !settings.is_global => Err(PERR::WrongFnDefinition.into_err(settings.pos)),
+        Token::Fn if !settings.is_global => Err(PERR::FnWrongDefinition.into_err(settings.pos)),
 
         #[cfg(not(feature = "no_function"))]
         Token::Fn | Token::Private => {
@@ -2621,13 +2621,20 @@ fn parse_stmt(
 
                     let func = parse_fn(input, &mut new_state, lib, access, settings, _comments)?;
 
-                    lib.insert(
                     // Qualifiers (none) + function name + number of arguments.
-                        calc_script_fn_hash(empty(), &func.name, func.params.len()).unwrap(),
+                    let hash = calc_script_fn_hash(empty(), &func.name, func.params.len()).unwrap();
-                        func,
-                    );
 
-                    Ok(Stmt::Noop(settings.pos))
+                    if lib.contains_key(&hash) {
+                        return Err(PERR::FnDuplicatedDefinition(
+                            func.name.into_owned(),
+                            func.params.len(),
+                        )
+                        .into_err(pos));
+                    }
+
+                    lib.insert(hash, func);
+
+                    Ok(Stmt::Noop(pos))
                 }
 
                 (_, pos) => Err(PERR::MissingToken(

tests/internal_fn.rs

@@ -1,6 +1,6 @@
 #![cfg(not(feature = "no_function"))]
 
-use rhai::{Engine, EvalAltResult, INT};
+use rhai::{Engine, EvalAltResult, ParseErrorType, INT};
 
 #[test]
 fn test_internal_fn() -> Result<(), Box<EvalAltResult>> {
@@ -46,18 +46,30 @@ fn test_internal_fn_overloading() -> Result<(), Box<EvalAltResult>> {
 
     assert_eq!(
         engine.eval::<INT>(
-            r#"
+            r"
                 fn abc(x,y,z) { 2*x + 3*y + 4*z + 888 }
-                fn abc(x) { x + 42 }
                 fn abc(x,y) { x + 2*y + 88 }
                 fn abc() { 42 }
-                fn abc(x) { x - 42 }    // should override previous definition
+                fn abc(x) { x - 42 }
 
                 abc() + abc(1) + abc(1,2) + abc(1,2,3)
-    "#
+            "
         )?,
         1002
     );
 
+    assert_eq!(
+        *engine
+            .compile(
+                r"
+                fn abc(x) { x + 42 }
+                fn abc(x) { x - 42 }
+            "
+            )
+            .expect_err("should error")
+            .0,
+        ParseErrorType::FnDuplicatedDefinition("abc".to_string(), 1)
+    );
+
     Ok(())
 }