From 0e6f022e751b92bddea52b0952d7b6433658c972 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 12 Dec 2025 05:14:54 +0000 Subject: [PATCH] Improve Eldritch error messages Improved error messages for type mismatches in arithmetic, comparison, and bitwise operations, as well as builtin functions `len`, `type`, and `range`. Updated logic to propagate error kinds (TypeError, ValueError, etc.) from builtins via string prefixes. Updated tests to match new error messages. --- .../src/interpreter/builtins/len.rs | 4 +- .../src/interpreter/builtins/range.rs | 4 +- .../src/interpreter/builtins/type_.rs | 2 +- .../eldritch-core/src/interpreter/eval.rs | 94 +++++++++++++++---- .../src/interpreter/operations.rs | 21 ++++- .../eldritchv2/eldritch-core/tests/basics.rs | 4 +- .../eldritch-core/tests/builtins.rs | 6 +- .../eldritch-core/tests/coverage_boost.rs | 22 ++--- .../tests/coverage_boost_extra.rs | 4 +- .../eldritch-core/tests/functions.rs | 2 +- 10 files changed, 116 insertions(+), 47 deletions(-) diff --git a/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/len.rs b/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/len.rs index 7a4ff075c..52e599886 100644 --- a/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/len.rs +++ b/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/len.rs @@ -11,7 +11,7 @@ use spin::RwLock; pub fn builtin_len(_env: &Arc>, args: &[Value]) -> Result { if args.len() != 1 { return Err(format!( - "len() takes exactly one argument ({} given)", + "TypeError: len() takes exactly one argument ({} given)", args.len() )); } @@ -22,6 +22,6 @@ pub fn builtin_len(_env: &Arc>, args: &[Value]) -> Result Ok(Value::Int(d.read().len() as i64)), Value::Tuple(t) => Ok(Value::Int(t.len() as i64)), Value::Set(s) => Ok(Value::Int(s.read().len() as i64)), - _ => Err(format!("'len()' is not defined for type: {:?}", args[0])), + _ => Err(format!("TypeError: object of type '{}' has no len()", crate::interpreter::introspection::get_type_name(&args[0]))), } } diff --git a/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/range.rs b/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/range.rs index fed850421..3f07beb33 100644 --- a/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/range.rs +++ b/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/range.rs @@ -15,10 +15,10 @@ pub fn builtin_range(_env: &Arc>, args: &[Value]) -> Result< [Value::Int(end)] => (0, *end, 1), [Value::Int(start), Value::Int(end)] => (*start, *end, 1), [Value::Int(start), Value::Int(end), Value::Int(step)] => (*start, *end, *step), - _ => return Err("Range expects 1-3 integer arguments.".to_string()), + _ => return Err("TypeError: range expects 1-3 integer arguments".to_string()), }; if step == 0 { - return Err("range() arg 3 must not be zero".to_string()); + return Err("ValueError: range() arg 3 must not be zero".to_string()); } let mut list = Vec::new(); diff --git a/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/type_.rs b/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/type_.rs index 4ca701f7a..51e03c774 100644 --- a/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/type_.rs +++ b/implants/lib/eldritchv2/eldritch-core/src/interpreter/builtins/type_.rs @@ -9,7 +9,7 @@ use spin::RwLock; /// Returns a string representation of the type of the object. pub fn builtin_type(_env: &Arc>, args: &[Value]) -> Result { if args.len() != 1 { - return Err(String::from("type() expects exactly one argument")); + return Err(String::from("TypeError: type() takes exactly 1 argument")); } Ok(Value::String(get_type_name(&args[0]))) } diff --git a/implants/lib/eldritchv2/eldritch-core/src/interpreter/eval.rs b/implants/lib/eldritchv2/eldritch-core/src/interpreter/eval.rs index c3e2f828f..b1461857d 100644 --- a/implants/lib/eldritchv2/eldritch-core/src/interpreter/eval.rs +++ b/implants/lib/eldritchv2/eldritch-core/src/interpreter/eval.rs @@ -220,7 +220,7 @@ fn evaluate_index( Value::List(l) => { let idx_int = match idx_val { Value::Int(i) => i, - _ => return interp.error(EldritchErrorKind::TypeError, "List indices must be integers", index.span), + _ => return interp.error(EldritchErrorKind::TypeError, "list indices must be integers", index.span), }; let list = l.read(); let true_idx = if idx_int < 0 { @@ -236,7 +236,7 @@ fn evaluate_index( Value::Tuple(t) => { let idx_int = match idx_val { Value::Int(i) => i, - _ => return interp.error(EldritchErrorKind::TypeError, "Tuple indices must be integers", index.span), + _ => return interp.error(EldritchErrorKind::TypeError, "tuple indices must be integers", index.span), }; let true_idx = if idx_int < 0 { t.len() as i64 + idx_int @@ -255,7 +255,7 @@ fn evaluate_index( None => interp.error(EldritchErrorKind::KeyError, &format!("KeyError: '{idx_val}'"), span), } } - _ => interp.error(EldritchErrorKind::TypeError, &format!("Type not subscriptable: {obj_val:?}"), obj.span), + _ => interp.error(EldritchErrorKind::TypeError, &format!("'{}' object is not subscriptable", get_type_name(&obj_val)), obj.span), } } @@ -272,7 +272,7 @@ fn evaluate_slice( let step_val = if let Some(s) = step { match evaluate(interp, s)? { Value::Int(i) => i, - _ => return interp.error(EldritchErrorKind::TypeError, "Slice step must be integer", s.span), + _ => return interp.error(EldritchErrorKind::TypeError, "slice step must be an integer", s.span), } } else { 1 @@ -285,7 +285,7 @@ fn evaluate_slice( let start_val_opt = if let Some(s) = start { match evaluate(interp, s)? { Value::Int(i) => Some(i), - _ => return interp.error(EldritchErrorKind::TypeError, "Slice start must be integer", s.span), + _ => return interp.error(EldritchErrorKind::TypeError, "slice start must be an integer", s.span), } } else { None @@ -294,7 +294,7 @@ fn evaluate_slice( let stop_val_opt = if let Some(s) = stop { match evaluate(interp, s)? { Value::Int(i) => Some(i), - _ => return interp.error(EldritchErrorKind::TypeError, "Slice stop must be integer", s.span), + _ => return interp.error(EldritchErrorKind::TypeError, "slice stop must be an integer", s.span), } } else { None @@ -371,7 +371,7 @@ fn evaluate_slice( } Ok(Value::String(result_chars.into_iter().collect())) } - _ => interp.error(EldritchErrorKind::TypeError, &format!("Type not sliceable: {obj_val:?}"), obj.span), + _ => interp.error(EldritchErrorKind::TypeError, &format!("'{}' object is not subscriptable", get_type_name(&obj_val)), obj.span), } } @@ -511,7 +511,10 @@ fn call_function( interp.push_frame("", span); } - let res = f(&interp.env, args_slice).map_err(|e| EldritchError::new(EldritchErrorKind::RuntimeError, &e, span).with_stack(interp.call_stack.clone())); + let res = f(&interp.env, args_slice).map_err(|e| { + let (kind, msg) = parse_error_kind(&e); + EldritchError::new(kind, msg, span).with_stack(interp.call_stack.clone()) + }); interp.pop_frame(); res } @@ -522,7 +525,10 @@ fn call_function( } else { interp.push_frame("", span); } - let res = f(&interp.env, args_slice, &kw_args_val).map_err(|e| EldritchError::new(EldritchErrorKind::RuntimeError, &e, span).with_stack(interp.call_stack.clone())); + let res = f(&interp.env, args_slice, &kw_args_val).map_err(|e| { + let (kind, msg) = parse_error_kind(&e); + EldritchError::new(kind, msg, span).with_stack(interp.call_stack.clone()) + }); interp.pop_frame(); res } @@ -624,7 +630,7 @@ fn call_function( keys.sort(); return interp.error( EldritchErrorKind::TypeError, - &format!("Function '{name}' got unexpected keyword arguments: {keys:?}"), + &format!("{}() got an unexpected keyword argument '{}'", name, keys[0]), span, ); } @@ -657,12 +663,18 @@ fn call_function( if let Value::Foreign(foreign) = receiver.as_ref() { foreign .call_method(interp, &method_name, args_slice, &kw_args_val) - .map_err(|e| EldritchError::new(EldritchErrorKind::RuntimeError, &e, span).with_stack(interp.call_stack.clone())) + .map_err(|e| { + let (kind, msg) = parse_error_kind(&e); + EldritchError::new(kind, msg, span).with_stack(interp.call_stack.clone()) + }) } else if !kw_args_val.is_empty() { Err(EldritchError::new(EldritchErrorKind::TypeError, "BoundMethod does not accept keyword arguments", span).with_stack(interp.call_stack.clone())) } else { call_bound_method(&receiver, &method_name, args_slice) - .map_err(|e| EldritchError::new(EldritchErrorKind::RuntimeError, &e, span).with_stack(interp.call_stack.clone())) + .map_err(|e| { + let (kind, msg) = parse_error_kind(&e); + EldritchError::new(kind, msg, span).with_stack(interp.call_stack.clone()) + }) } }; interp.pop_frame(); @@ -875,7 +887,10 @@ fn call_value( // Push stack frame // Native function name? interp.push_frame("", span); - let res = f(&interp.env, args).map_err(|e| EldritchError::new(EldritchErrorKind::RuntimeError, &e, span).with_stack(interp.call_stack.clone())); + let res = f(&interp.env, args).map_err(|e| { + let (kind, msg) = parse_error_kind(&e); + EldritchError::new(kind, msg, span).with_stack(interp.call_stack.clone()) + }); interp.pop_frame(); res } @@ -913,11 +928,14 @@ fn call_value( Value::BoundMethod(receiver, method_name) => { interp.push_frame(method_name, span); let res = call_bound_method(receiver, method_name, args) - .map_err(|e| EldritchError::new(EldritchErrorKind::RuntimeError, &e, span).with_stack(interp.call_stack.clone())); + .map_err(|e| { + let (kind, msg) = parse_error_kind(&e); + EldritchError::new(kind, msg, span).with_stack(interp.call_stack.clone()) + }); interp.pop_frame(); res }, - _ => interp.error(EldritchErrorKind::TypeError, "not callable", span), + _ => interp.error(EldritchErrorKind::TypeError, &format!("'{}' object is not callable", get_type_name(func)), span), } } @@ -1278,7 +1296,25 @@ fn apply_binary_op( } } - _ => interp.error(EldritchErrorKind::TypeError, "Unsupported binary op", span), + (val_a, op_kind, val_b) => interp.error(EldritchErrorKind::TypeError, &format!( + "unsupported operand type(s) for {}: '{}' and '{}'", + match op_kind { + TokenKind::Plus => "+", + TokenKind::Minus => "-", + TokenKind::Star => "*", + TokenKind::Slash => "/", + TokenKind::SlashSlash => "//", + TokenKind::Percent => "%", + TokenKind::BitAnd => "&", + TokenKind::BitOr => "|", + TokenKind::BitXor => "^", + TokenKind::LShift => "<<", + TokenKind::RShift => ">>", + _ => "binary op", + }, + get_type_name(&val_a), + get_type_name(&val_b) + ), span), } } @@ -1359,13 +1395,13 @@ fn string_modulo_format( _ => { return interp.error( EldritchErrorKind::ValueError, - &format!("Unsupported format specifier: %{next}"), + &format!("unsupported format character '{}' (0x{:x})", next, next as u32), span, ); } } } else { - return interp.error(EldritchErrorKind::ValueError, "incomplete format", span); + return interp.error(EldritchErrorKind::ValueError, "incomplete format key", span); } } else { result.push(c); @@ -1387,7 +1423,7 @@ fn to_int_or_error(interp: &Interpreter, v: &Value, spec: char, span: Span) -> R _ => interp.error( EldritchErrorKind::TypeError, &format!( - "%{} format: a number is required, not {:?}", + "%{} format: a number is required, not {}", spec, get_type_name(v) ), @@ -1404,7 +1440,7 @@ fn to_float_or_error(interp: &Interpreter, v: &Value, spec: char, span: Span) -> _ => interp.error( EldritchErrorKind::TypeError, &format!( - "%{} format: a number is required, not {:?}", + "%{} format: a number is required, not {}", spec, get_type_name(v) ), @@ -1488,3 +1524,21 @@ pub(crate) fn apply_binary_op_pub( ) -> Result { apply_binary_op(interp, left, op, right, span) } + +fn parse_error_kind(msg: &str) -> (EldritchErrorKind, &str) { + if let Some(rest) = msg.strip_prefix("TypeError: ") { + (EldritchErrorKind::TypeError, rest) + } else if let Some(rest) = msg.strip_prefix("ValueError: ") { + (EldritchErrorKind::ValueError, rest) + } else if let Some(rest) = msg.strip_prefix("IndexError: ") { + (EldritchErrorKind::IndexError, rest) + } else if let Some(rest) = msg.strip_prefix("KeyError: ") { + (EldritchErrorKind::KeyError, rest) + } else if let Some(rest) = msg.strip_prefix("AttributeError: ") { + (EldritchErrorKind::AttributeError, rest) + } else if let Some(rest) = msg.strip_prefix("NameError: ") { + (EldritchErrorKind::NameError, rest) + } else { + (EldritchErrorKind::RuntimeError, msg) + } +} diff --git a/implants/lib/eldritchv2/eldritch-core/src/interpreter/operations.rs b/implants/lib/eldritchv2/eldritch-core/src/interpreter/operations.rs index f8512ab62..4b18b8342 100644 --- a/implants/lib/eldritchv2/eldritch-core/src/interpreter/operations.rs +++ b/implants/lib/eldritchv2/eldritch-core/src/interpreter/operations.rs @@ -290,7 +290,7 @@ pub(crate) fn apply_arithmetic_op( Ok(Value::Int(res)) } - _ => interp.error(EldritchErrorKind::TypeError, &format!("Unsupported operand types for {}: {} and {}", + _ => interp.error(EldritchErrorKind::TypeError, &format!("unsupported operand type(s) for {}: '{}' and '{}'", match op { TokenKind::Plus => "+", TokenKind::Minus => "-", @@ -359,7 +359,14 @@ pub(crate) fn apply_comparison_op( // Mismatched types if core::mem::discriminant(a) != core::mem::discriminant(b) { return interp.error(EldritchErrorKind::TypeError, &format!( - "Type mismatch or unsortable types: {} <-> {}", + "'{}' not supported between instances of '{}' and '{}'", + match op { + TokenKind::Lt => "<", + TokenKind::Gt => ">", + TokenKind::LtEq => "<=", + TokenKind::GtEq => ">=", + _ => unreachable!(), + }, get_type_name(a), get_type_name(b) ), span); @@ -425,7 +432,15 @@ pub(crate) fn apply_bitwise_op( Ok(Value::Dictionary(Arc::new(RwLock::new(new_dict)))) } _ => interp.error(EldritchErrorKind::TypeError, &format!( - "Unsupported operand types for bitwise op: {} and {}", + "unsupported operand type(s) for {}: '{}' and '{}'", + match op { + TokenKind::BitAnd => "&", + TokenKind::BitOr => "|", + TokenKind::BitXor => "^", + TokenKind::LShift => "<<", + TokenKind::RShift => ">>", + _ => "?", + }, get_type_name(a), get_type_name(b) ), span), } diff --git a/implants/lib/eldritchv2/eldritch-core/tests/basics.rs b/implants/lib/eldritchv2/eldritch-core/tests/basics.rs index df29e15b1..4565d8298 100644 --- a/implants/lib/eldritchv2/eldritch-core/tests/basics.rs +++ b/implants/lib/eldritchv2/eldritch-core/tests/basics.rs @@ -118,7 +118,7 @@ fn test_introspection() { fn test_basic_errors() { assert::fail("1 / 0", "divide by zero"); assert::fail("undefined_var", "Undefined variable"); - assert::fail("1 + 'string'", "Unsupported binary op"); + assert::fail("1 + 'string'", "unsupported operand type(s) for +: 'int' and 'string'"); // Verify type mismatch in comparison - assert::fail("1 < 'a'", "Type mismatch or unsortable types"); + assert::fail("1 < 'a'", "'<' not supported between instances of 'int' and 'string'"); } diff --git a/implants/lib/eldritchv2/eldritch-core/tests/builtins.rs b/implants/lib/eldritchv2/eldritch-core/tests/builtins.rs index 554e6c289..3fdd1946b 100644 --- a/implants/lib/eldritchv2/eldritch-core/tests/builtins.rs +++ b/implants/lib/eldritchv2/eldritch-core/tests/builtins.rs @@ -73,7 +73,7 @@ fn test_core_builtins() { "#, ); - assert::fail("len(1)", "not defined for type"); + assert::fail("len(1)", "object of type 'int' has no len()"); } #[test] @@ -165,8 +165,8 @@ fn test_type_regression() { assert::pass(r#" assert(type(1) == "int") "#); - assert::fail("type()", "expects exactly one argument"); - assert::fail("type(1, 2)", "expects exactly one argument"); + assert::fail("type()", "takes exactly 1 argument"); + assert::fail("type(1, 2)", "takes exactly 1 argument"); } #[test] diff --git a/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost.rs b/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost.rs index b659127ea..3e941f30a 100644 --- a/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost.rs +++ b/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost.rs @@ -57,7 +57,7 @@ fn test_coverage_utils() { #[test] fn test_coverage_builtins() { assert::fail("int([])", "argument must be a string, bytes or number"); - assert::fail("range()", "Range expects 1-3 integer arguments"); + assert::fail("range()", "range expects 1-3 integer arguments"); // This now works because range supports 1, 2, or 3 args // assert::fail("range(1, 2, 3)", "Range expects one or two integer arguments"); assert::pass("range(1, 2, 3)"); @@ -71,7 +71,7 @@ fn test_coverage_builtins() { assert(len((1, 2, 3)) == 3) "#, ); - assert::fail("len(1)", "is not defined for type"); + assert::fail("len(1)", "object of type 'int' has no len()"); assert::pass( r#" @@ -165,14 +165,14 @@ fn test_coverage_interpreter_edge_cases() { // Slice step 0 assert::fail("[][::0]", "slice step cannot be zero"); - assert::fail("[][::'a']", "Slice step must be integer"); - assert::fail("[][:'a']", "Slice stop must be integer"); - assert::fail("[][ 'a':]", "Slice start must be integer"); - assert::fail("1[::]", "Type not sliceable"); + assert::fail("[][::'a']", "slice step must be an integer"); + assert::fail("[][:'a']", "slice stop must be an integer"); + assert::fail("[][ 'a':]", "slice start must be an integer"); + assert::fail("1[::]", "'int' object is not subscriptable"); // Indexing - assert::fail("1[0]", "Type not subscriptable"); - assert::fail("[][ 'a' ]", "List indices must be integers"); + assert::fail("1[0]", "'int' object is not subscriptable"); + assert::fail("[][ 'a' ]", "list indices must be integers"); // assert::fail("{}[ 1 ]", "Dictionary keys must be strings"); // Dot access tests @@ -188,9 +188,9 @@ fn test_coverage_interpreter_edge_cases() { assert::fail("{}.a()", "has no method 'a'"); // Augmented assignment errors - assert::fail("a = 1; a += 's'", "Unsupported binary op"); - assert::fail("l=[1]; l[0] += 's'", "Unsupported binary op"); - assert::fail("d={'a':1}; d['a'] += 's'", "Unsupported binary op"); + assert::fail("a = 1; a += 's'", "unsupported operand type(s) for +: 'int' and 'string'"); + assert::fail("l=[1]; l[0] += 's'", "unsupported operand type(s) for +: 'int' and 'string'"); + assert::fail("d={'a':1}; d['a'] += 's'", "unsupported operand type(s) for +: 'int' and 'string'"); } #[test] diff --git a/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost_extra.rs b/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost_extra.rs index 273b8f821..64c7535a8 100644 --- a/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost_extra.rs +++ b/implants/lib/eldritchv2/eldritch-core/tests/coverage_boost_extra.rs @@ -26,7 +26,7 @@ fn test_eval_index_out_of_bounds() { // Eldritch doesn't seem to support string indexing in eval.rs yet? // Wait, evaluate_index only handles List, Tuple, Dictionary. // Let's verify this failure. - assert::fail("'abc'[0]", "Type not subscriptable"); + assert::fail("'abc'[0]", "'string' object is not subscriptable"); } #[test] @@ -34,7 +34,7 @@ fn test_string_format_errors() { assert::fail("'%d' % 's'", "%d format: a number is required"); assert::fail("'%s' % (1, 2)", "not all arguments converted"); assert::fail("'%' % 1", "incomplete format"); - assert::fail("'%q' % 1", "Unsupported format specifier"); + assert::fail("'%q' % 1", "unsupported format character 'q'"); } #[test] diff --git a/implants/lib/eldritchv2/eldritch-core/tests/functions.rs b/implants/lib/eldritchv2/eldritch-core/tests/functions.rs index 0f4dd8aa4..00d16ab84 100644 --- a/implants/lib/eldritchv2/eldritch-core/tests/functions.rs +++ b/implants/lib/eldritchv2/eldritch-core/tests/functions.rs @@ -116,5 +116,5 @@ fn test_function_errors() { assert::fail("1()", "Cannot call value"); assert::fail("def f(x): pass; f()", "Missing required argument"); assert::fail("def f(): pass; f(1)", "too many positional arguments"); - assert::fail("def f(): pass; f(a=1)", "unexpected keyword arguments"); + assert::fail("def f(): pass; f(a=1)", "got an unexpected keyword argument 'a'"); }