From 58fad030eeeed5cec7bf6d2cc2f4f7663c10d32d Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Wed, 8 Mar 2023 21:47:57 +0800 Subject: [PATCH] Do not default compares of different types. --- CHANGELOG.md | 1 + src/eval/data_check.rs | 171 ++++++++++++++++++------------------ src/eval/global_state.rs | 14 --- src/eval/mod.rs | 4 + src/func/builtin.rs | 59 +------------ src/packages/array_basic.rs | 5 +- tests/mismatched_op.rs | 5 +- tests/ops.rs | 27 +++++- 8 files changed, 125 insertions(+), 161 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07b73d9d..643e1074 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Bug fixes * `x += y` where `x` and `y` are `char` now works correctly. * Expressions such as `!inside` now parses correctly instead of as `!in` followed by `side`. * Custom syntax starting with symbols now works correctly and no longer raises a parse error. +* Comparing different custom types now works correctly when the appropriate comparison operators are registered. Potentially breaking changes ---------------------------- diff --git a/src/eval/data_check.rs b/src/eval/data_check.rs index f5016ea3..563881ec 100644 --- a/src/eval/data_check.rs +++ b/src/eval/data_check.rs @@ -8,90 +8,91 @@ use std::borrow::Borrow; #[cfg(feature = "no_std")] use std::prelude::v1::*; +/// Recursively calculate the sizes of an array. +/// +/// Sizes returned are `(` [`Array`][crate::Array], [`Map`][crate::Map] and [`String`] `)`. +/// +/// # Panics +/// +/// Panics if any interior data is shared (should never happen). +#[cfg(not(feature = "no_index"))] +#[inline] +pub fn calc_array_sizes(array: &crate::Array) -> (usize, usize, usize) { + let (mut ax, mut mx, mut sx) = (0, 0, 0); + + for value in array { + ax += 1; + + match value.0 { + Union::Array(ref a, ..) => { + let (a, m, s) = calc_array_sizes(a); + ax += a; + mx += m; + sx += s; + } + Union::Blob(ref a, ..) => ax += 1 + a.len(), + #[cfg(not(feature = "no_object"))] + Union::Map(ref m, ..) => { + let (a, m, s) = calc_map_sizes(m); + ax += a; + mx += m; + sx += s; + } + Union::Str(ref s, ..) => sx += s.len(), + #[cfg(not(feature = "no_closure"))] + Union::Shared(..) => { + unreachable!("shared values discovered within data") + } + _ => (), + } + } + + (ax, mx, sx) +} +/// Recursively calculate the sizes of a map. +/// +/// Sizes returned are `(` [`Array`][crate::Array], [`Map`][crate::Map] and [`String`] `)`. +/// +/// # Panics +/// +/// Panics if any interior data is shared (should never happen). +#[cfg(not(feature = "no_object"))] +#[inline] +pub fn calc_map_sizes(map: &crate::Map) -> (usize, usize, usize) { + let (mut ax, mut mx, mut sx) = (0, 0, 0); + + for value in map.values() { + mx += 1; + + match value.0 { + #[cfg(not(feature = "no_index"))] + Union::Array(ref a, ..) => { + let (a, m, s) = calc_array_sizes(a); + ax += a; + mx += m; + sx += s; + } + #[cfg(not(feature = "no_index"))] + Union::Blob(ref a, ..) => ax += 1 + a.len(), + Union::Map(ref m, ..) => { + let (a, m, s) = calc_map_sizes(m); + ax += a; + mx += m; + sx += s; + } + Union::Str(ref s, ..) => sx += s.len(), + #[cfg(not(feature = "no_closure"))] + Union::Shared(..) => { + unreachable!("shared values discovered within data") + } + _ => (), + } + } + + (ax, mx, sx) +} + impl Dynamic { - /// Recursively calculate the sizes of an array. - /// - /// Sizes returned are `(` [`Array`][crate::Array], [`Map`][crate::Map] and [`String`] `)`. - /// - /// # Panics - /// - /// Panics if any interior data is shared (should never happen). - #[cfg(not(feature = "no_index"))] - #[inline] - pub(crate) fn calc_array_sizes(array: &crate::Array) -> (usize, usize, usize) { - let (mut ax, mut mx, mut sx) = (0, 0, 0); - - for value in array { - ax += 1; - - match value.0 { - Union::Array(ref a, ..) => { - let (a, m, s) = Self::calc_array_sizes(a); - ax += a; - mx += m; - sx += s; - } - Union::Blob(ref a, ..) => ax += 1 + a.len(), - #[cfg(not(feature = "no_object"))] - Union::Map(ref m, ..) => { - let (a, m, s) = Self::calc_map_sizes(m); - ax += a; - mx += m; - sx += s; - } - Union::Str(ref s, ..) => sx += s.len(), - #[cfg(not(feature = "no_closure"))] - Union::Shared(..) => { - unreachable!("shared values discovered within data") - } - _ => (), - } - } - - (ax, mx, sx) - } - /// Recursively calculate the sizes of a map. - /// - /// Sizes returned are `(` [`Array`][crate::Array], [`Map`][crate::Map] and [`String`] `)`. - /// - /// # Panics - /// - /// Panics if any interior data is shared (should never happen). - #[cfg(not(feature = "no_object"))] - #[inline] - pub(crate) fn calc_map_sizes(map: &crate::Map) -> (usize, usize, usize) { - let (mut ax, mut mx, mut sx) = (0, 0, 0); - - for value in map.values() { - mx += 1; - - match value.0 { - #[cfg(not(feature = "no_index"))] - Union::Array(ref a, ..) => { - let (a, m, s) = Self::calc_array_sizes(a); - ax += a; - mx += m; - sx += s; - } - #[cfg(not(feature = "no_index"))] - Union::Blob(ref a, ..) => ax += 1 + a.len(), - Union::Map(ref m, ..) => { - let (a, m, s) = Self::calc_map_sizes(m); - ax += a; - mx += m; - sx += s; - } - Union::Str(ref s, ..) => sx += s.len(), - #[cfg(not(feature = "no_closure"))] - Union::Shared(..) => { - unreachable!("shared values discovered within data") - } - _ => (), - } - } - - (ax, mx, sx) - } /// Recursively calculate the sizes of a value. /// /// Sizes returned are `(` [`Array`][crate::Array], [`Map`][crate::Map] and [`String`] `)`. @@ -103,11 +104,11 @@ impl Dynamic { pub(crate) fn calc_data_sizes(&self, _top: bool) -> (usize, usize, usize) { match self.0 { #[cfg(not(feature = "no_index"))] - Union::Array(ref arr, ..) => Self::calc_array_sizes(arr), + Union::Array(ref arr, ..) => calc_array_sizes(arr), #[cfg(not(feature = "no_index"))] Union::Blob(ref blob, ..) => (blob.len(), 0, 0), #[cfg(not(feature = "no_object"))] - Union::Map(ref map, ..) => Self::calc_map_sizes(map), + Union::Map(ref map, ..) => calc_map_sizes(map), Union::Str(ref s, ..) => (0, 0, s.len()), #[cfg(not(feature = "no_closure"))] Union::Shared(..) if _top => self.read_lock::().unwrap().calc_data_sizes(true), diff --git a/src/eval/global_state.rs b/src/eval/global_state.rs index 69e08c5e..caaf210f 100644 --- a/src/eval/global_state.rs +++ b/src/eval/global_state.rs @@ -127,20 +127,6 @@ impl GlobalRuntimeState { pub fn get_shared_import(&self, index: usize) -> Option { self.modules.as_ref().and_then(|m| m.get(index).cloned()) } - /// Get a mutable reference to the globally-imported [module][crate::Module] at a - /// particular index. - /// - /// Not available under `no_module`. - #[cfg(not(feature = "no_module"))] - #[allow(dead_code)] - #[inline] - #[must_use] - pub(crate) fn get_shared_import_mut( - &mut self, - index: usize, - ) -> Option<&mut crate::SharedModule> { - self.modules.as_deref_mut().and_then(|m| m.get_mut(index)) - } /// Get the index of a globally-imported [module][crate::Module] by name. /// /// Not available under `no_module`. diff --git a/src/eval/mod.rs b/src/eval/mod.rs index 733eb5ff..443000e9 100644 --- a/src/eval/mod.rs +++ b/src/eval/mod.rs @@ -11,6 +11,10 @@ mod target; pub use cache::{Caches, FnResolutionCache, FnResolutionCacheEntry}; #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))] pub use chaining::ChainType; +#[cfg(not(feature = "no_index"))] +pub use data_check::calc_array_sizes; +#[cfg(not(feature = "no_object"))] +pub use data_check::calc_map_sizes; #[cfg(feature = "debugging")] pub use debugger::{ BreakPoint, CallStackFrame, Debugger, DebuggerCommand, DebuggerEvent, DebuggerStatus, diff --git a/src/func/builtin.rs b/src/func/builtin.rs index 1b69ba43..d6b2a9e5 100644 --- a/src/func/builtin.rs +++ b/src/func/builtin.rs @@ -27,48 +27,6 @@ use rust_decimal::Decimal; /// The `unchecked` feature is not active. const CHECKED_BUILD: bool = cfg!(not(feature = "unchecked")); -/// Is the type a numeric type? -#[inline] -#[must_use] -fn is_numeric(type_id: TypeId) -> bool { - if type_id == TypeId::of::() { - return true; - } - - #[cfg(not(feature = "only_i64"))] - #[cfg(not(feature = "only_i32"))] - if type_id == TypeId::of::() - || type_id == TypeId::of::() - || type_id == TypeId::of::() - || type_id == TypeId::of::() - || type_id == TypeId::of::() - || type_id == TypeId::of::() - || type_id == TypeId::of::() - || type_id == TypeId::of::() - { - return true; - } - - #[cfg(not(feature = "only_i64"))] - #[cfg(not(feature = "only_i32"))] - #[cfg(not(target_family = "wasm"))] - if type_id == TypeId::of::() || type_id == TypeId::of::() { - return true; - } - - #[cfg(not(feature = "no_float"))] - if type_id == TypeId::of::() || type_id == TypeId::of::() { - return true; - } - - #[cfg(feature = "decimal")] - if type_id == TypeId::of::() { - return true; - } - - false -} - /// A function that returns `true`. #[inline(always)] #[allow(clippy::unnecessary_wraps)] @@ -584,22 +542,7 @@ pub fn get_builtin_binary_op_fn(op: &Token, x: &Dynamic, y: &Dynamic) -> Option< // One of the operands is a custom type, so it is never built-in if x.is_variant() || y.is_variant() { - return if is_numeric(type1) && is_numeric(type2) { - // Disallow comparisons between different numeric types - None - } else if type1 != type2 { - // If the types are not the same, default to not compare - match op { - NotEqualsTo => Some((const_true_fn, false)), - EqualsTo | GreaterThan | GreaterThanEqualsTo | LessThan | LessThanEqualsTo => { - Some((const_false_fn, false)) - } - _ => None, - } - } else { - // Disallow comparisons between the same type - None - }; + return None; } // Default comparison operators for different types diff --git a/src/packages/array_basic.rs b/src/packages/array_basic.rs index 0e968c4c..c8cb1a86 100644 --- a/src/packages/array_basic.rs +++ b/src/packages/array_basic.rs @@ -2,9 +2,10 @@ use crate::api::deprecated::deprecated_array_functions; use crate::engine::OP_EQUALS; -use crate::eval::{calc_index, calc_offset_len}; +use crate::eval::{calc_index, calc_offset_len, calc_array_sizes}; use crate::module::ModuleFlags; use crate::plugin::*; + use crate::{ def_package, Array, Dynamic, ExclusiveRange, FnPtr, InclusiveRange, NativeCallContext, Position, RhaiResultOf, StaticVec, ERR, INT, MAX_USIZE_INT, @@ -237,7 +238,7 @@ pub mod array_functions { #[cfg(not(feature = "unchecked"))] if _ctx.engine().max_array_size() > 0 { let pad = len - array.len(); - let (a, m, s) = Dynamic::calc_array_sizes(array); + let (a, m, s) = calc_array_sizes(array); let (ax, mx, sx) = item.calc_data_sizes(true); _ctx.engine() diff --git a/tests/mismatched_op.rs b/tests/mismatched_op.rs index 7147330a..85d31857 100644 --- a/tests/mismatched_op.rs +++ b/tests/mismatched_op.rs @@ -39,7 +39,10 @@ fn test_mismatched_op_custom_type() -> Result<(), Box> { ").expect_err("should error"), EvalAltResult::ErrorFunctionNotFound(f, ..) if f == "== (TestStruct, TestStruct)")); - assert!(!engine.eval::("new_ts() == 42")?); + assert!( + matches!(*engine.eval::("new_ts() == 42").expect_err("should error"), + EvalAltResult::ErrorFunctionNotFound(f, ..) if f.starts_with("== (TestStruct, ")) + ); assert!(matches!( *engine.eval::("60 + new_ts()").expect_err("should error"), diff --git a/tests/ops.rs b/tests/ops.rs index 1077284c..ce6c5611 100644 --- a/tests/ops.rs +++ b/tests/ops.rs @@ -35,7 +35,11 @@ fn test_ops_other_number_types() -> Result<(), Box> { EvalAltResult::ErrorFunctionNotFound(f, ..) if f.starts_with("== (u16,") )); - assert!(!engine.eval_with_scope::(&mut scope, r#"x == "hello""#)?); + assert!( + matches!(*engine.eval_with_scope::(&mut scope, r#"x == "hello""#).expect_err("should error"), + EvalAltResult::ErrorFunctionNotFound(f, ..) if f.starts_with("== (u16,") + ) + ); Ok(()) } @@ -63,3 +67,24 @@ fn test_ops_precedence() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_ops_custom_types() -> Result<(), Box> { + #[derive(Debug, Clone, PartialEq, Eq)] + struct Test1; + #[derive(Debug, Clone, PartialEq, Eq)] + struct Test2; + + let mut engine = Engine::new(); + + engine + .register_type_with_name::("Test1") + .register_type_with_name::("Test2") + .register_fn("new_ts1", || Test1) + .register_fn("new_ts2", || Test2) + .register_fn("==", |x: Test1, y: Test2| true); + + assert!(engine.eval::("let x = new_ts1(); let y = new_ts2(); x == y")?); + + Ok(()) +}