From e2a47b2a65263433c4cb3855a1e55adabf9bc154 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sun, 3 Jan 2021 20:54:08 +0800 Subject: [PATCH] Disallow duplicated function definitions. --- RELEASES.md | 2 ++ src/fn_call.rs | 2 +- src/parse_error.rs | 20 +++++++++++++++++--- src/parser.rs | 21 ++++++++++++++------- tests/internal_fn.rs | 22 +++++++++++++++++----- 5 files changed, 51 insertions(+), 16 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index d377aa7c..e43a841c 100644 --- a/RELEASES.md +++ b/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 ------------ diff --git a/src/fn_call.rs b/src/fn_call.rs index 78879f50..21bc570c 100644 --- a/src/fn_call.rs +++ b/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 diff --git a/src/parse_error.rs b/src/parse_error.rs index 0a03726c..e42c851a 100644 --- a/src/parse_error.rs +++ b/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) } diff --git a/src/parser.rs b/src/parser.rs index 7f43e9c9..10c7945b 100644 --- a/src/parser.rs +++ b/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(), - func, - ); + // Qualifiers (none) + function name + number of arguments. + let hash = calc_script_fn_hash(empty(), &func.name, func.params.len()).unwrap(); - 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( diff --git a/tests/internal_fn.rs b/tests/internal_fn.rs index 67417344..80d904e7 100644 --- a/tests/internal_fn.rs +++ b/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> { @@ -46,18 +46,30 @@ fn test_internal_fn_overloading() -> Result<(), Box> { assert_eq!( engine.eval::( - 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(()) }