From e6a695036bf7081a219f3dc74fc29b1702c0fff2 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Tue, 24 Sep 2024 16:11:46 -0700 Subject: [PATCH] Add context to the console Logger trait (#4005) This is not backward compatible but should have minimal impact. This is also more future proof by passing the ConsoleState which is more opaque and allows us to update the way the console object is constructed and passed. --- core/runtime/src/console/mod.rs | 183 ++++++++++++++++++------------ core/runtime/src/console/tests.rs | 18 +-- core/runtime/src/lib.rs | 2 +- 3 files changed, 120 insertions(+), 83 deletions(-) diff --git a/core/runtime/src/console/mod.rs b/core/runtime/src/console/mod.rs index 2eda34cb58..bc2efa8bc8 100644 --- a/core/runtime/src/console/mod.rs +++ b/core/runtime/src/console/mod.rs @@ -32,25 +32,25 @@ pub trait Logger: Trace + Sized { /// /// # Errors /// Returning an error will throw an exception in JavaScript. - fn log(&self, msg: String, state: &Console) -> JsResult<()>; + fn log(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()>; /// Log an info message (`console.info`). /// /// # Errors /// Returning an error will throw an exception in JavaScript. - fn info(&self, msg: String, state: &Console) -> JsResult<()>; + fn info(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()>; /// Log a warning message (`console.warn`). /// /// # Errors /// Returning an error will throw an exception in JavaScript. - fn warn(&self, msg: String, state: &Console) -> JsResult<()>; + fn warn(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()>; /// Log an error message (`console.error`). /// /// # Errors /// Returning an error will throw an exception in JavaScript. - fn error(&self, msg: String, state: &Console) -> JsResult<()>; + fn error(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()>; } /// The default implementation for logging from the console. @@ -63,24 +63,24 @@ struct DefaultLogger; impl Logger for DefaultLogger { #[inline] - fn log(&self, msg: String, state: &Console) -> JsResult<()> { - let indent = 2 * state.groups.len(); + fn log(&self, msg: String, state: &ConsoleState, _context: &mut Context) -> JsResult<()> { + let indent = state.indent(); writeln!(std::io::stdout(), "{msg:>indent$}").map_err(JsError::from_rust) } #[inline] - fn info(&self, msg: String, state: &Console) -> JsResult<()> { - self.log(msg, state) + fn info(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()> { + self.log(msg, state, context) } #[inline] - fn warn(&self, msg: String, state: &Console) -> JsResult<()> { - self.log(msg, state) + fn warn(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()> { + self.log(msg, state, context) } #[inline] - fn error(&self, msg: String, state: &Console) -> JsResult<()> { - let indent = 2 * state.groups.len(); + fn error(&self, msg: String, state: &ConsoleState, _context: &mut Context) -> JsResult<()> { + let indent = state.indent(); writeln!(std::io::stderr(), "{msg:>indent$}").map_err(JsError::from_rust) } } @@ -181,19 +181,53 @@ fn formatter(data: &[JsValue], context: &mut Context) -> JsResult { } } -/// This is the internal console object state. -#[derive(Debug, Default, Trace, Finalize, JsData)] -pub struct Console { +/// The current state of the console, passed to the logger backend. +/// This should not be copied or cloned. References are only valid +/// for the current logging call. +#[derive(Debug, Default, Trace, Finalize)] +pub struct ConsoleState { /// The map of console counters, used in `console.count()`. - pub count_map: FxHashMap, + count_map: FxHashMap, /// The map of console timers, used in `console.time`, `console.timeLog` /// and `console.timeEnd`. - pub timer_map: FxHashMap, + timer_map: FxHashMap, /// The current list of groups. Groups should be indented, but some logging /// libraries may want to use them in a different way. - pub groups: Vec, + groups: Vec, +} + +impl ConsoleState { + /// Returns the indentation level that should be applied to logging. + #[must_use] + pub fn indent(&self) -> usize { + 2 * self.groups.len() + } + + /// Returns the current list of groups. + #[must_use] + pub fn groups(&self) -> &Vec { + &self.groups + } + + /// Returns the count map. + #[must_use] + pub fn count_map(&self) -> &FxHashMap { + &self.count_map + } + + /// Returns the timer map. + #[must_use] + pub fn timer_map(&self) -> &FxHashMap { + &self.timer_map + } +} + +/// This is the internal console object state. +#[derive(Debug, Default, Trace, Finalize, JsData)] +pub struct Console { + state: ConsoleState, } impl Console { @@ -394,7 +428,7 @@ impl Console { args[0] = JsValue::new(concat); } - logger.error(formatter(&args, context)?, console)?; + logger.error(formatter(&args, context)?, &console.state, context)?; } Ok(JsValue::undefined()) @@ -418,7 +452,7 @@ impl Console { _: &impl Logger, _: &mut Context, ) -> JsResult { - console.groups.clear(); + console.state.groups.clear(); Ok(JsValue::undefined()) } @@ -439,7 +473,7 @@ impl Console { logger: &impl Logger, context: &mut Context, ) -> JsResult { - logger.log(formatter(args, context)?, console)?; + logger.log(formatter(args, context)?, &console.state, context)?; Ok(JsValue::undefined()) } @@ -460,7 +494,7 @@ impl Console { logger: &impl Logger, context: &mut Context, ) -> JsResult { - logger.error(formatter(args, context)?, console)?; + logger.error(formatter(args, context)?, &console.state, context)?; Ok(JsValue::undefined()) } @@ -481,7 +515,7 @@ impl Console { logger: &impl Logger, context: &mut Context, ) -> JsResult { - logger.info(formatter(args, context)?, console)?; + logger.info(formatter(args, context)?, &console.state, context)?; Ok(JsValue::undefined()) } @@ -502,7 +536,7 @@ impl Console { logger: &impl Logger, context: &mut Context, ) -> JsResult { - logger.log(formatter(args, context)?, console)?; + logger.log(formatter(args, context)?, &console.state, context)?; Ok(JsValue::undefined()) } @@ -524,7 +558,7 @@ impl Console { context: &mut Context, ) -> JsResult { if !args.is_empty() { - logger.log(formatter(args, context)?, console)?; + logger.log(formatter(args, context)?, &console.state, context)?; } let stack_trace_dump = context @@ -535,7 +569,7 @@ impl Console { .map(JsString::to_std_string_escaped) .collect::>() .join("\n"); - logger.log(stack_trace_dump, console)?; + logger.log(stack_trace_dump, &console.state, context)?; Ok(JsValue::undefined()) } @@ -557,7 +591,7 @@ impl Console { logger: &impl Logger, context: &mut Context, ) -> JsResult { - logger.warn(formatter(args, context)?, console)?; + logger.warn(formatter(args, context)?, &console.state, context)?; Ok(JsValue::undefined()) } @@ -584,10 +618,10 @@ impl Console { }; let msg = format!("count {}:", label.to_std_string_escaped()); - let c = console.count_map.entry(label).or_insert(0); + let c = console.state.count_map.entry(label).or_insert(0); *c += 1; - logger.info(format!("{msg} {c}"), console)?; + logger.info(format!("{msg} {c}"), &console.state, context)?; Ok(JsValue::undefined()) } @@ -613,11 +647,12 @@ impl Console { None => "default".into(), }; - console.count_map.remove(&label); + console.state.count_map.remove(&label); logger.warn( format!("countReset {}", label.to_std_string_escaped()), - console, + &console.state, + context, )?; Ok(JsValue::undefined()) @@ -653,13 +688,14 @@ impl Console { None => "default".into(), }; - if let Entry::Vacant(e) = console.timer_map.entry(label.clone()) { + if let Entry::Vacant(e) = console.state.timer_map.entry(label.clone()) { let time = Self::system_time_in_ms(); e.insert(time); } else { logger.warn( format!("Timer '{}' already exist", label.to_std_string_escaped()), - console, + &console.state, + context, )?; } @@ -688,22 +724,20 @@ impl Console { None => "default".into(), }; - console.timer_map.get(&label).map_or_else( - || { - logger.warn( - format!("Timer '{}' doesn't exist", label.to_std_string_escaped()), - console, - ) - }, - |t| { - let time = Self::system_time_in_ms(); - let mut concat = format!("{}: {} ms", label.to_std_string_escaped(), time - t); - for msg in args.iter().skip(1) { - concat = concat + " " + &msg.display().to_string(); - } - logger.log(concat, console) - }, - )?; + if let Some(t) = console.state.timer_map.get(&label) { + let time = Self::system_time_in_ms(); + let mut concat = format!("{}: {} ms", label.to_std_string_escaped(), time - t); + for msg in args.iter().skip(1) { + concat = concat + " " + &msg.display().to_string(); + } + logger.log(concat, &console.state, context)?; + } else { + logger.warn( + format!("Timer '{}' doesn't exist", label.to_std_string_escaped()), + &console.state, + context, + )?; + } Ok(JsValue::undefined()) } @@ -730,25 +764,24 @@ impl Console { None => "default".into(), }; - console.timer_map.remove(&label).map_or_else( - || { - logger.warn( - format!("Timer '{}' doesn't exist", label.to_std_string_escaped()), - console, - ) - }, - |t| { - let time = Self::system_time_in_ms(); - logger.info( - format!( - "{}: {} ms - timer removed", - label.to_std_string_escaped(), - time - t - ), - console, - ) - }, - )?; + if let Some(t) = console.state.timer_map.remove(&label) { + let time = Self::system_time_in_ms(); + logger.info( + format!( + "{}: {} ms - timer removed", + label.to_std_string_escaped(), + time - t + ), + &console.state, + context, + )?; + } else { + logger.warn( + format!("Timer '{}' doesn't exist", label.to_std_string_escaped()), + &console.state, + context, + )?; + }; Ok(JsValue::undefined()) } @@ -772,8 +805,8 @@ impl Console { ) -> JsResult { let group_label = formatter(args, context)?; - logger.info(format!("group: {group_label}"), console)?; - console.groups.push(group_label); + logger.info(format!("group: {group_label}"), &console.state, context)?; + console.state.groups.push(group_label); Ok(JsValue::undefined()) } @@ -816,7 +849,7 @@ impl Console { _: &impl Logger, _: &mut Context, ) -> JsResult { - console.groups.pop(); + console.state.groups.pop(); Ok(JsValue::undefined()) } @@ -837,9 +870,13 @@ impl Console { args: &[JsValue], console: &Self, logger: &impl Logger, - _: &mut Context, + context: &mut Context, ) -> JsResult { - logger.info(args.get_or_undefined(0).display_obj(true), console)?; + logger.info( + args.get_or_undefined(0).display_obj(true), + &console.state, + context, + )?; Ok(JsValue::undefined()) } } diff --git a/core/runtime/src/console/tests.rs b/core/runtime/src/console/tests.rs index 29d7a72b63..d521575e34 100644 --- a/core/runtime/src/console/tests.rs +++ b/core/runtime/src/console/tests.rs @@ -1,4 +1,4 @@ -use super::{formatter, Console}; +use super::{formatter, Console, ConsoleState}; use crate::test::{run_test_actions, run_test_actions_with, TestAction}; use crate::Logger; use boa_engine::{js_string, property::Attribute, Context, JsError, JsResult, JsValue}; @@ -120,22 +120,22 @@ struct RecordingLogger { } impl Logger for RecordingLogger { - fn log(&self, msg: String, state: &Console) -> JsResult<()> { + fn log(&self, msg: String, state: &ConsoleState, _: &mut Context) -> JsResult<()> { use std::fmt::Write; - let indent = 2 * state.groups.len(); + let indent = state.indent(); writeln!(self.log.borrow_mut(), "{msg:>indent$}").map_err(JsError::from_rust) } - fn info(&self, msg: String, state: &Console) -> JsResult<()> { - self.log(msg, state) + fn info(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()> { + self.log(msg, state, context) } - fn warn(&self, msg: String, state: &Console) -> JsResult<()> { - self.log(msg, state) + fn warn(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()> { + self.log(msg, state, context) } - fn error(&self, msg: String, state: &Console) -> JsResult<()> { - self.log(msg, state) + fn error(&self, msg: String, state: &ConsoleState, context: &mut Context) -> JsResult<()> { + self.log(msg, state, context) } } diff --git a/core/runtime/src/lib.rs b/core/runtime/src/lib.rs index 58fbe5116c..38dea056c2 100644 --- a/core/runtime/src/lib.rs +++ b/core/runtime/src/lib.rs @@ -54,7 +54,7 @@ mod console; #[doc(inline)] -pub use console::{Console, Logger}; +pub use console::{Console, ConsoleState, Logger}; mod text;