From 2f3777b475909e731b67f6e5ad36d8d83d39b87a Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 29 Mar 2024 14:39:56 +0100 Subject: [PATCH 1/8] Fail sort when predicate fails --- src/builtin/functions/sequences.rs | 9 ++++++++- tests/tests.rs | 13 +++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/builtin/functions/sequences.rs b/src/builtin/functions/sequences.rs index db3784f..44e1846 100644 --- a/src/builtin/functions/sequences.rs +++ b/src/builtin/functions/sequences.rs @@ -67,16 +67,23 @@ pub(crate) fn add(ctx: &mut TulispContext) { ) -> Result { let pred = eval(ctx, &pred)?; let mut vec: Vec<_> = seq.base_iter().collect(); + let mut err = None; vec.sort_by(|v1, v2| { if funcall::(ctx, &pred, &list!(v1.clone(), v2.clone()).unwrap()) .map(|v| v.null()) - .unwrap_or(false) + .unwrap_or_else(|x| { + err = Some(x); + false + }) { Ordering::Equal } else { Ordering::Less } }); + if let Some(err) = err { + return Err(err); + } let ret = vec .iter() .fold(list!(), |v1, v2| list!(,@v1 ,(*v2).clone()).unwrap()); diff --git a/tests/tests.rs b/tests/tests.rs index 83de63c..f309997 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -933,11 +933,24 @@ fn test_sort() -> Result<(), Error> { program: "(sort '(20 10 30 15 45) '>)", result: "'(45 30 20 15 10)", } + tulisp_assert! { + program: r#"(sort '("hello" "world") '>)"#, + error: r#"ERR TypeMismatch: Expected number, found: "world" +:1.18-1.24: at "world" +:1.1-1.29: at (sort '("hello" "world") '>) +"#, + } tulisp_assert! { program: "(sort '(20 10 30 15 45) '<<)", error: r#"ERR TypeMismatch: Variable definition is void: << :1.26-1.28: at << :1.1-1.29: at (sort '(20 10 30 15 45) '<<) +"# + } + tulisp_assert! { + program: "(sort '(20 10 30 15 45))", + error: r#"ERR Undefined: function is void: nil +:1.1-1.25: at (sort '(20 10 30 15 45)) "# } tulisp_assert! { From 3d9458ee6b1d2006a5323be3416a2ee72f056e10 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 29 Mar 2024 14:48:52 +0100 Subject: [PATCH 2/8] Rebuild sorted list faster using `cons` from reverse sorted results --- src/builtin/functions/sequences.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/builtin/functions/sequences.rs b/src/builtin/functions/sequences.rs index 44e1846..8815961 100644 --- a/src/builtin/functions/sequences.rs +++ b/src/builtin/functions/sequences.rs @@ -76,9 +76,9 @@ pub(crate) fn add(ctx: &mut TulispContext) { false }) { - Ordering::Equal - } else { Ordering::Less + } else { + Ordering::Greater } }); if let Some(err) = err { @@ -86,7 +86,7 @@ pub(crate) fn add(ctx: &mut TulispContext) { } let ret = vec .iter() - .fold(list!(), |v1, v2| list!(,@v1 ,(*v2).clone()).unwrap()); + .fold(TulispObject::nil(), |v1, v2| TulispObject::cons(v2.clone(), v1)); Ok(ret) } } From ccf417d3703882ca8c535e10cc9bd364ef0771c1 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 29 Mar 2024 15:32:53 +0100 Subject: [PATCH 3/8] Add a Copy-on-Write `eval_cow` method This should eventually replace the `eval` method and all data should be passed around as `Cow<'_, TulispObject>`. --- src/eval.rs | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 69c0173..665480e 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use crate::{ context::TulispContext, error::{Error, ErrorKind}, @@ -216,13 +218,7 @@ fn eval_back_quote(ctx: &mut TulispContext, mut vv: TulispObject) -> Result Result { - let mut result = None; - eval_basic(ctx, expr, &mut result)?; - if let Some(result) = result { - Ok(result) - } else { - Ok(expr.clone()) - } + eval_cow(ctx, expr).map(|x| x.into_owned()) } pub(crate) fn eval_check_null(ctx: &mut TulispContext, expr: &TulispObject) -> Result { @@ -249,6 +245,20 @@ pub(crate) fn eval_and_then( } } +// Eventually this should replace eval +pub(crate) fn eval_cow<'a>( + ctx: &mut TulispContext, + expr: &'a TulispObject, +) -> Result, Error> { + let mut result = None; + eval_basic(ctx, expr, &mut result)?; + if let Some(result) = result { + Ok(Cow::Owned(result)) + } else { + Ok(Cow::Borrowed(expr)) + } +} + pub(crate) fn eval_basic<'a>( ctx: &mut TulispContext, expr: &'a TulispObject, From 78dabfe2d35c39deb63b6a06960083ae27c79ddb Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 29 Mar 2024 15:52:03 +0100 Subject: [PATCH 4/8] Replace all uses of `eval_and_then` with `eval_cow` --- src/builtin/functions/conditionals.rs | 6 +-- src/builtin/functions/functions.rs | 7 ++-- .../functions/numbers/rounding_operations.rs | 40 +++++++++---------- src/context.rs | 12 +----- src/eval.rs | 14 ------- 5 files changed, 26 insertions(+), 53 deletions(-) diff --git a/src/builtin/functions/conditionals.rs b/src/builtin/functions/conditionals.rs index fcd365f..a77d27c 100644 --- a/src/builtin/functions/conditionals.rs +++ b/src/builtin/functions/conditionals.rs @@ -1,6 +1,6 @@ use crate::{ builtin::functions::common::eval_2_arg_special_form, - eval::{eval_and_then, eval_basic}, + eval::{eval_basic, eval_cow}, list, lists::{last, length}, Error, ErrorKind, TulispContext, TulispObject, TulispValue, @@ -11,7 +11,7 @@ use tulisp_proc_macros::{crate_fn, crate_fn_no_eval}; pub(crate) fn add(ctx: &mut TulispContext) { fn impl_if(ctx: &mut TulispContext, args: &TulispObject) -> Result { eval_2_arg_special_form(ctx, "if", args, true, |ctx, cond, then, else_body| { - if eval_and_then(ctx, cond, |x| Ok(x.is_truthy()))? { + if eval_cow(ctx, cond)?.is_truthy() { ctx.eval(then) } else { ctx.eval_progn(else_body) @@ -39,7 +39,7 @@ pub(crate) fn add(ctx: &mut TulispContext) { fn cond(ctx: &mut TulispContext, args: &TulispObject) -> Result { for item in args.base_iter() { - if item.car_and_then(|x| eval_and_then(ctx, x, |x| Ok(x.is_truthy())))? { + if item.car_and_then(|x| Ok(eval_cow(ctx, x)?.is_truthy()))? { return item.cdr_and_then(|x| ctx.eval_progn(x)); } } diff --git a/src/builtin/functions/functions.rs b/src/builtin/functions/functions.rs index c37c4bc..0762995 100644 --- a/src/builtin/functions/functions.rs +++ b/src/builtin/functions/functions.rs @@ -3,8 +3,7 @@ use crate::context::Scope; use crate::context::TulispContext; use crate::error::Error; use crate::error::ErrorKind; -use crate::eval::eval; -use crate::eval::eval_and_then; +use crate::eval::{eval, eval_cow}; use crate::eval::eval_check_null; use crate::eval::DummyEval; use crate::eval::Eval; @@ -278,7 +277,7 @@ pub(crate) fn add(ctx: &mut TulispContext) { args.car_and_then(|arg| ctx.eval(arg)) }) })?; - args.car_and_then(|name_sym| ctx.eval_and_then(name_sym, |name| name.set(value.clone())))?; + args.car_and_then(|name_sym| eval_cow(ctx, name_sym)?.set(value.clone()))?; Ok(value) } intern_set_func!(ctx, set); @@ -671,7 +670,7 @@ pub(crate) fn add(ctx: &mut TulispContext) { } Ok(true) => {} } - args.car_and_then(|arg| eval_and_then(ctx, &arg, |x| Ok(x.$name().into()))) + args.car_and_then(|arg| Ok(eval_cow(ctx, &arg)?.$name().into())) } intern_set_func!(ctx, $name); }; diff --git a/src/builtin/functions/numbers/rounding_operations.rs b/src/builtin/functions/numbers/rounding_operations.rs index 0a53dcd..602dfaa 100644 --- a/src/builtin/functions/numbers/rounding_operations.rs +++ b/src/builtin/functions/numbers/rounding_operations.rs @@ -1,39 +1,37 @@ use std::rc::Rc; use crate::{ - builtin::functions::common::eval_1_arg_special_form, eval::eval_and_then, Error, ErrorKind, + builtin::functions::common::eval_1_arg_special_form, eval::eval_cow, Error, ErrorKind, TulispContext, TulispObject, TulispValue, }; pub(crate) fn add(ctx: &mut TulispContext) { fn fround(ctx: &mut TulispContext, args: &TulispObject) -> Result { eval_1_arg_special_form(ctx, "fround", args, false, |ctx, arg1, _| { - eval_and_then(ctx, arg1, |x| { - if x.floatp() { - Ok(f64::round(x.as_float().unwrap()).into()) - } else { - Err(Error::new( - ErrorKind::TypeMismatch, - format!("Expected float for fround. Got: {}", x), - )) - } - }) + let val = eval_cow(ctx, arg1)?; + if val.floatp() { + Ok(f64::round(val.as_float().unwrap()).into()) + } else { + Err(Error::new( + ErrorKind::TypeMismatch, + format!("Expected float for fround. Got: {}", val), + )) + } }) } intern_set_func!(ctx, fround); fn ftruncate(ctx: &mut TulispContext, args: &TulispObject) -> Result { eval_1_arg_special_form(ctx, "ftruncate", args, false, |ctx, arg1, _| { - eval_and_then(ctx, arg1, |x| { - if x.floatp() { - Ok(f64::trunc(x.as_float().unwrap()).into()) - } else { - Err(Error::new( - ErrorKind::TypeMismatch, - format!("Expected float for ftruncate. Got: {}", x), - )) - } - }) + let val = eval_cow(ctx, arg1)?; + if val.floatp() { + Ok(f64::trunc(val.as_float().unwrap()).into()) + } else { + Err(Error::new( + ErrorKind::TypeMismatch, + format!("Expected float for ftruncate. Got: {}", val), + )) + } }) } intern_set_func!(ctx, ftruncate); diff --git a/src/context.rs b/src/context.rs index 1526a79..170cda3 100644 --- a/src/context.rs +++ b/src/context.rs @@ -3,7 +3,7 @@ use std::{collections::HashMap, fs, rc::Rc}; use crate::{ builtin, error::Error, - eval::{eval, eval_and_then, eval_basic, funcall, DummyEval}, + eval::{eval, eval_basic, funcall, DummyEval}, list, parse::parse, TulispObject, TulispValue, @@ -107,16 +107,6 @@ impl TulispContext { eval(self, value) } - /// Evaluates the given value, run the given function on the result of the - /// evaluation, and returns the result of the function. - pub fn eval_and_then( - &mut self, - expr: &TulispObject, - f: impl FnOnce(&TulispObject) -> Result, - ) -> Result { - eval_and_then(self, expr, f) - } - /// Calls the given function with the given arguments, and returns the /// result. pub fn funcall( diff --git a/src/eval.rs b/src/eval.rs index 665480e..0c153aa 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -231,20 +231,6 @@ pub(crate) fn eval_check_null(ctx: &mut TulispContext, expr: &TulispObject) -> R } } -pub(crate) fn eval_and_then( - ctx: &mut TulispContext, - expr: &TulispObject, - func: impl FnOnce(&TulispObject) -> Result, -) -> Result { - let mut result = None; - eval_basic(ctx, expr, &mut result)?; - if let Some(result) = result { - func(&result) - } else { - func(expr) - } -} - // Eventually this should replace eval pub(crate) fn eval_cow<'a>( ctx: &mut TulispContext, From 19e2dc4b4830bd4cfa708c759c8e5d6524858477 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 29 Mar 2024 15:54:20 +0100 Subject: [PATCH 5/8] Replace all uses of `eval_check_null` with `eval_cow` --- src/builtin/functions/functions.rs | 3 +-- src/eval.rs | 10 ---------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/builtin/functions/functions.rs b/src/builtin/functions/functions.rs index 0762995..0fd79e7 100644 --- a/src/builtin/functions/functions.rs +++ b/src/builtin/functions/functions.rs @@ -4,7 +4,6 @@ use crate::context::TulispContext; use crate::error::Error; use crate::error::ErrorKind; use crate::eval::{eval, eval_cow}; -use crate::eval::eval_check_null; use crate::eval::DummyEval; use crate::eval::Eval; use crate::lists; @@ -230,7 +229,7 @@ pub(crate) fn add(ctx: &mut TulispContext) { rest: TulispObject, ) -> Result { let mut result = TulispObject::nil(); - while !eval_check_null(ctx, &condition)? { + while !eval_cow(ctx, &condition)?.null() { result = ctx.eval_progn(&rest)?; } Ok(result) diff --git a/src/eval.rs b/src/eval.rs index 0c153aa..11b0214 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -221,16 +221,6 @@ pub(crate) fn eval(ctx: &mut TulispContext, expr: &TulispObject) -> Result Result { - let mut result = None; - eval_basic(ctx, expr, &mut result)?; - if let Some(result) = result { - Ok(result.null()) - } else { - Ok(expr.null()) - } -} - // Eventually this should replace eval pub(crate) fn eval_cow<'a>( ctx: &mut TulispContext, From adf7e280bc89f15e1af0ec262588c4834226b42f Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 29 Mar 2024 16:20:41 +0100 Subject: [PATCH 6/8] Replace all uses of `eval_basic` outside `eval.rs` with `eval_cow` --- src/builtin/functions/conditionals.rs | 29 ++++++++------------------- src/cons.rs | 18 +++++------------ src/context.rs | 6 ++---- src/eval.rs | 2 +- 4 files changed, 16 insertions(+), 39 deletions(-) diff --git a/src/builtin/functions/conditionals.rs b/src/builtin/functions/conditionals.rs index a77d27c..396d9e7 100644 --- a/src/builtin/functions/conditionals.rs +++ b/src/builtin/functions/conditionals.rs @@ -1,6 +1,6 @@ use crate::{ builtin::functions::common::eval_2_arg_special_form, - eval::{eval_basic, eval_cow}, + eval::eval_cow, list, lists::{last, length}, Error, ErrorKind, TulispContext, TulispObject, TulispValue, @@ -57,19 +57,11 @@ pub(crate) fn add(ctx: &mut TulispContext) { fn and(ctx: &mut TulispContext, rest: TulispObject) -> Result { let mut ret = TulispObject::nil(); for item in rest.base_iter() { - let mut result = None; - eval_basic(ctx, &item, &mut result)?; - if let Some(result) = result { - if result.null() { - return Ok(result); - } - ret = result; - } else { - if item.null() { - return Ok(item); - } - ret = item; + let result = eval_cow(ctx, &item)?; + if result.null() { + return Ok(result.into_owned()); } + ret = result.into_owned(); } Ok(ret) } @@ -77,14 +69,9 @@ pub(crate) fn add(ctx: &mut TulispContext) { #[crate_fn_no_eval(add_func = "ctx")] fn or(ctx: &mut TulispContext, rest: TulispObject) -> Result { for item in rest.base_iter() { - let mut result = None; - eval_basic(ctx, &item, &mut result)?; - if let Some(result) = result { - if !result.null() { - return Ok(result); - } - } else if !item.null() { - return Ok(item); + let result = eval_cow(ctx, &item)?; + if !result.null() { + return Ok(result.into_owned()); } } Ok(TulispObject::nil()) diff --git a/src/cons.rs b/src/cons.rs index 01aeadf..cea44f9 100644 --- a/src/cons.rs +++ b/src/cons.rs @@ -2,7 +2,7 @@ use std::marker::PhantomData; use crate::error::Error; use crate::error::ErrorKind; -use crate::eval::eval_basic; +use crate::eval::eval_cow; use crate::object::Span; use crate::TulispContext; use crate::TulispObject; @@ -143,20 +143,12 @@ impl BaseIter { ) -> Option> { if let Some(ref next) = self.next { let Cons { car, cdr } = next; - let new_car = { - let mut result = None; - if let Err(e) = eval_basic(ctx, car, &mut result) { - return Some(Err(e)); - }; - if let Some(result) = result { - Ok(result) - } else { - Ok(next.car.clone()) - } + let new_car = match eval_cow(ctx, car) { + Ok(vv) => vv.into_owned(), + Err(e) => return Some(Err(e)), }; - self.next = cdr.as_list_cons(); - Some(new_car) + Some(Ok(new_car)) } else { None } diff --git a/src/context.rs b/src/context.rs index 170cda3..8c0bf1c 100644 --- a/src/context.rs +++ b/src/context.rs @@ -3,7 +3,7 @@ use std::{collections::HashMap, fs, rc::Rc}; use crate::{ builtin, error::Error, - eval::{eval, eval_basic, funcall, DummyEval}, + eval::{eval, eval_cow, funcall, DummyEval}, list, parse::parse, TulispObject, TulispValue, @@ -171,10 +171,8 @@ impl TulispContext { /// last one. pub fn eval_progn(&mut self, seq: &TulispObject) -> Result { let mut ret = None; - let mut result = None; for val in seq.base_iter() { - eval_basic(self, &val, &mut result)?; - ret = Some(result.take().unwrap_or(val)) + ret = Some(eval_cow(self, &val)?.into_owned()) } Ok(ret.unwrap_or_else(TulispObject::nil)) } diff --git a/src/eval.rs b/src/eval.rs index 11b0214..c20466c 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -235,7 +235,7 @@ pub(crate) fn eval_cow<'a>( } } -pub(crate) fn eval_basic<'a>( +fn eval_basic<'a>( ctx: &mut TulispContext, expr: &'a TulispObject, result: &'a mut Option, From a72034148f54feb31a06bc03db8c61813cfd1ac2 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Sun, 31 Mar 2024 01:02:18 +0100 Subject: [PATCH 7/8] Replace `eval_basic` in `zip_function_args` with `eval` --- src/eval.rs | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index c20466c..a3c4ef8 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -12,8 +12,7 @@ pub(crate) trait Evaluator { fn eval( ctx: &mut TulispContext, value: &TulispObject, - result: &mut Option, - ) -> Result<(), Error>; + ) -> Result; } pub(crate) struct Eval; @@ -21,9 +20,8 @@ impl Evaluator for Eval { fn eval( ctx: &mut TulispContext, value: &TulispObject, - result: &mut Option, - ) -> Result<(), Error> { - eval_basic(ctx, value, result) + ) -> Result { + eval(ctx, value) } } @@ -32,9 +30,8 @@ impl Evaluator for DummyEval { fn eval( _ctx: &mut TulispContext, _value: &TulispObject, - _result: &mut Option, - ) -> Result<(), Error> { - Ok(()) + ) -> Result { + Ok(_value.to_owned()) } } @@ -44,26 +41,20 @@ fn zip_function_args( args: &TulispObject, ) -> Result<(), Error> { let mut args_iter = args.base_iter(); - let mut eval_result = None; for param in params.iter() { let val = if param.is_optional { match args_iter.next() { - Some(vv) => { - E::eval(ctx, &vv, &mut eval_result)?; - eval_result.take().unwrap_or(vv) - } + Some(vv) => E::eval(ctx, &vv)?, None => TulispObject::nil(), } } else if param.is_rest { let ret = TulispObject::nil(); for arg in args_iter.by_ref() { - E::eval(ctx, &arg, &mut eval_result)?; - ret.push(eval_result.take().unwrap_or(arg))?; + ret.push(E::eval(ctx, &arg)?)?; } ret } else if let Some(vv) = args_iter.next() { - E::eval(ctx, &vv, &mut eval_result)?; - eval_result.take().unwrap_or(vv) + E::eval(ctx, &vv)? } else { return Err(Error::new( ErrorKind::TypeMismatch, From 90ec17e639de5bf3ca82ae6b063a05a0e7cc992b Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Sun, 31 Mar 2024 01:16:46 +0100 Subject: [PATCH 8/8] Replace `eval_basic` implementation with `eval_cow` --- src/eval.rs | 46 ++++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index a3c4ef8..54055d2 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -217,32 +217,21 @@ pub(crate) fn eval_cow<'a>( ctx: &mut TulispContext, expr: &'a TulispObject, ) -> Result, Error> { - let mut result = None; - eval_basic(ctx, expr, &mut result)?; - if let Some(result) = result { - Ok(Cow::Owned(result)) - } else { - Ok(Cow::Borrowed(expr)) - } -} - -fn eval_basic<'a>( - ctx: &mut TulispContext, - expr: &'a TulispObject, - result: &'a mut Option, -) -> Result<(), Error> { match &*expr.inner_ref() { TulispValue::List { .. } => { - *result = Some(eval_form::(ctx, expr).map_err(|e| e.with_trace(expr.clone()))?); + return Ok(Cow::Owned( + eval_form::(ctx, expr) + .map_err(|e| e.with_trace(expr.clone()))? + )); } TulispValue::Symbol { value, .. } => { if value.is_constant() { - return Ok(()); + return Ok(Cow::Borrowed(expr)); } - *result = Some(value.get().map_err(|e| e.with_trace(expr.clone()))?); + return Ok(Cow::Owned(value.get().map_err(|e| e.with_trace(expr.clone()))?)); } TulispValue::LexicalBinding { value, .. } => { - *result = Some(value.get().map_err(|e| e.with_trace(expr.clone()))?); + return Ok(Cow::Owned(value.get().map_err(|e| e.with_trace(expr.clone()))?)); } TulispValue::Int { .. } | TulispValue::Float { .. } @@ -254,13 +243,18 @@ fn eval_basic<'a>( | TulispValue::Any(_) | TulispValue::Bounce | TulispValue::Nil - | TulispValue::T => {} - TulispValue::Quote { value, .. } => { - *result = Some(value.clone()); + | TulispValue::T => { + return Ok(Cow::Borrowed(expr)); + } + TulispValue::Sharpquote { value } + | TulispValue::Quote { value } => { + return Ok(Cow::Owned(value.clone())); } TulispValue::Backquote { value } => { - *result = - Some(eval_back_quote(ctx, value.clone()).map_err(|e| e.with_trace(expr.clone()))?); + return Ok(Cow::Owned( + eval_back_quote(ctx, value.clone()) + .map_err(|e| e.with_trace(expr.clone()))? + )); } TulispValue::Unquote { .. } => { return Err(Error::new( @@ -274,11 +268,7 @@ fn eval_basic<'a>( "Splice without backquote".to_string(), )) } - TulispValue::Sharpquote { value, .. } => { - *result = Some(value.clone()); - } - }; - Ok(()) + } } pub fn macroexpand(ctx: &mut TulispContext, inp: TulispObject) -> Result {