From d090cd2420f62686ba7af6fc2f1299c9eec54575 Mon Sep 17 00:00:00 2001 From: Filip Tibell Date: Thu, 17 Oct 2024 11:23:20 +0200 Subject: [PATCH] Remove redundant stack trace information in error formatter --- crates/lune-utils/src/fmt/error/components.rs | 41 +++++++++- .../lune-utils/src/fmt/error/stack_trace.rs | 40 ++++++++++ crates/lune-utils/src/fmt/error/tests.rs | 77 +++++++++++++++++-- 3 files changed, 151 insertions(+), 7 deletions(-) diff --git a/crates/lune-utils/src/fmt/error/components.rs b/crates/lune-utils/src/fmt/error/components.rs index 941b8d0..b1f826a 100644 --- a/crates/lune-utils/src/fmt/error/components.rs +++ b/crates/lune-utils/src/fmt/error/components.rs @@ -124,7 +124,7 @@ impl From for ErrorComponents { } // We will then try to extract any stack trace - let trace = if let LuaError::CallbackError { + let mut trace = if let LuaError::CallbackError { ref traceback, ref cause, } = *error @@ -147,6 +147,45 @@ impl From for ErrorComponents { None }; + // Sometimes, we can get duplicate stack trace lines that only + // mention "[C]", without a function name or path, and these can + // be safely ignored / removed if the following line has more info + if let Some(trace) = &mut trace { + let lines = trace.lines_mut(); + loop { + let first_is_c_and_empty = lines + .first() + .is_some_and(|line| line.source().is_c() && line.is_empty()); + let second_is_c_and_nonempty = lines + .get(1) + .is_some_and(|line| line.source().is_c() && !line.is_empty()); + if first_is_c_and_empty && second_is_c_and_nonempty { + lines.remove(0); + } else { + break; + } + } + } + + // Finally, we do some light postprocessing to remove duplicate + // information, such as the location prefix in the error message + if let Some(message) = messages.last_mut() { + if let Some(line) = trace + .iter() + .flat_map(StackTrace::lines) + .find(|line| line.source().is_lua()) + { + let location_prefix = format!( + "[string \"{}\"]:{}:", + line.path().unwrap(), + line.line_number().unwrap() + ); + if message.starts_with(&location_prefix) { + *message = message[location_prefix.len()..].trim().to_string(); + } + } + } + ErrorComponents { messages, trace } } } diff --git a/crates/lune-utils/src/fmt/error/stack_trace.rs b/crates/lune-utils/src/fmt/error/stack_trace.rs index a33ec9a..6e18054 100644 --- a/crates/lune-utils/src/fmt/error/stack_trace.rs +++ b/crates/lune-utils/src/fmt/error/stack_trace.rs @@ -39,6 +39,24 @@ pub enum StackTraceSource { Lua, } +impl StackTraceSource { + /** + Returns `true` if the error originated from a C / Rust function, `false` otherwise. + */ + #[must_use] + pub const fn is_c(self) -> bool { + matches!(self, Self::C) + } + + /** + Returns `true` if the error originated from a Lua (user) function, `false` otherwise. + */ + #[must_use] + pub const fn is_lua(self) -> bool { + matches!(self, Self::Lua) + } +} + /** Stack trace line parsed from a [`LuaError`]. */ @@ -82,6 +100,20 @@ impl StackTraceLine { pub fn function_name(&self) -> Option<&str> { self.function_name.as_deref() } + + /** + Returns `true` if the stack trace line contains no "useful" information, `false` otherwise. + + Useful information is determined as one of: + + - A path + - A line number + - A function name + */ + #[must_use] + pub const fn is_empty(&self) -> bool { + self.path.is_none() && self.line_number.is_none() && self.function_name.is_none() + } } impl FromStr for StackTraceLine { @@ -145,6 +177,14 @@ impl StackTrace { pub fn lines(&self) -> &[StackTraceLine] { &self.lines } + + /** + Returns the individual stack trace lines, mutably. + */ + #[must_use] + pub fn lines_mut(&mut self) -> &mut Vec { + &mut self.lines + } } impl FromStr for StackTrace { diff --git a/crates/lune-utils/src/fmt/error/tests.rs b/crates/lune-utils/src/fmt/error/tests.rs index ff5a5f3..963a0af 100644 --- a/crates/lune-utils/src/fmt/error/tests.rs +++ b/crates/lune-utils/src/fmt/error/tests.rs @@ -2,7 +2,7 @@ use mlua::prelude::*; use crate::fmt::ErrorComponents; -fn new_lua_result() -> LuaResult<()> { +fn new_lua_runtime_error() -> LuaResult<()> { let lua = Lua::new(); lua.globals() @@ -17,13 +17,34 @@ fn new_lua_result() -> LuaResult<()> { lua.load("f()").set_name("chunk_name").eval() } +fn new_lua_script_error() -> LuaResult<()> { + let lua = Lua::new(); + + lua.load( + "local function inner()\ + \n error(\"oh no, a script error\")\ + \nend\ + \n\ + \nlocal function outer()\ + \n inner()\ + \nend\ + \n\ + \nouter()\ + ", + ) + .set_name("chunk_name") + .eval() +} + // Tests for error context stack mod context { use super::*; #[test] fn preserves_original() { - let lua_error = new_lua_result().context("additional context").unwrap_err(); + let lua_error = new_lua_runtime_error() + .context("additional context") + .unwrap_err(); let components = ErrorComponents::from(lua_error); assert_eq!(components.messages()[0], "additional context"); @@ -34,7 +55,7 @@ mod context { fn preserves_levels() { // NOTE: The behavior in mlua is to preserve a single level of context // and not all levels (context gets replaced on each call to `context`) - let lua_error = new_lua_result() + let lua_error = new_lua_runtime_error() .context("level 1") .context("level 2") .context("level 3") @@ -54,7 +75,7 @@ mod error_components { #[test] fn message() { - let lua_error = new_lua_result().unwrap_err(); + let lua_error = new_lua_runtime_error().unwrap_err(); let components = ErrorComponents::from(lua_error); assert_eq!(components.messages()[0], "oh no, a runtime error"); @@ -62,7 +83,7 @@ mod error_components { #[test] fn stack_begin_end() { - let lua_error = new_lua_result().unwrap_err(); + let lua_error = new_lua_runtime_error().unwrap_err(); let formatted = format!("{}", ErrorComponents::from(lua_error)); assert!(formatted.contains("Stack Begin")); @@ -71,7 +92,7 @@ mod error_components { #[test] fn stack_lines() { - let lua_error = new_lua_result().unwrap_err(); + let lua_error = new_lua_runtime_error().unwrap_err(); let components = ErrorComponents::from(lua_error); let mut lines = components.trace().unwrap().lines().iter(); @@ -83,3 +104,47 @@ mod error_components { assert_eq!(line_2, "Script 'chunk_name', Line 1"); } } + +// Tests for general formatting +mod general { + use super::*; + + #[test] + fn message_does_not_contain_location() { + let lua_error = new_lua_script_error().unwrap_err(); + + let components = ErrorComponents::from(lua_error); + let trace = components.trace().unwrap(); + + let first_message = components.messages().first().unwrap(); + let first_lua_stack_line = trace + .lines() + .iter() + .find(|line| line.source().is_lua()) + .unwrap(); + + let location_prefix = format!( + "[string \"{}\"]:{}:", + first_lua_stack_line.path().unwrap(), + first_lua_stack_line.line_number().unwrap() + ); + + assert!(!first_message.starts_with(&location_prefix)); + } + + #[test] + fn no_redundant_c_mentions() { + let lua_error = new_lua_script_error().unwrap_err(); + + let components = ErrorComponents::from(lua_error); + let trace = components.trace().unwrap(); + + let c_stack_lines = trace + .lines() + .iter() + .filter(|line| line.source().is_c()) + .collect::>(); + + assert_eq!(c_stack_lines.len(), 1); // Just the "error" call + } +}