From a87be28a46b67e53354f8ce69386057ddbb0f46c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Sat, 16 Apr 2022 16:12:26 +0200 Subject: [PATCH] feat: Better formatting for AggregateError (#14285) This commit adds "aggregated" field to "deno_core::JsError" that stores instances of "JsError" recursively to properly handle "AggregateError" formatting. Appropriate logics was added to "PrettyJsError" and "console" API to format AggregateErrors. Co-authored-by: Nayeem Rahman --- cli/fmt_errors.rs | 97 ++++++++----------- cli/tests/integration/run_tests.rs | 12 +++ cli/tests/testdata/aggregate_error.out | 18 ++++ cli/tests/testdata/aggregate_error.ts | 9 ++ cli/tests/testdata/complex_error.ts | 18 ++++ cli/tests/testdata/complex_error.ts.out | 44 +++++++++ cli/tests/testdata/error_cause.ts.out | 8 +- .../testdata/error_cause_recursive.ts.out | 10 +- core/error.rs | 22 +++++ ext/console/02_console.js | 50 ++++++++-- 10 files changed, 210 insertions(+), 78 deletions(-) create mode 100644 cli/tests/testdata/aggregate_error.out create mode 100644 cli/tests/testdata/aggregate_error.ts create mode 100644 cli/tests/testdata/complex_error.ts create mode 100644 cli/tests/testdata/complex_error.ts.out diff --git a/cli/fmt_errors.rs b/cli/fmt_errors.rs index fb7ccdb865..953dfec8b5 100644 --- a/cli/fmt_errors.rs +++ b/cli/fmt_errors.rs @@ -128,45 +128,6 @@ fn format_frame(frame: &JsStackFrame) -> String { result } -#[allow(clippy::too_many_arguments)] -fn format_stack( - is_error: bool, - message_line: &str, - cause: Option<&str>, - source_line: Option<&str>, - source_line_frame_index: Option, - frames: &[JsStackFrame], - level: usize, -) -> String { - let mut s = String::new(); - s.push_str(&format!("{:indent$}{}", "", message_line, indent = level)); - let column_number = - source_line_frame_index.and_then(|i| frames.get(i).unwrap().column_number); - s.push_str(&format_maybe_source_line( - source_line, - column_number, - is_error, - level, - )); - for frame in frames { - s.push_str(&format!( - "\n{:indent$} at {}", - "", - format_frame(frame), - indent = level - )); - } - if let Some(cause) = cause { - s.push_str(&format!( - "\n{:indent$}Caused by: {}", - "", - cause, - indent = level - )); - } - s -} - /// Take an optional source line and associated information to format it into /// a pretty printed version of that line. fn format_maybe_source_line( @@ -219,6 +180,43 @@ fn format_maybe_source_line( format!("\n{}{}\n{}{}", indent, source_line, indent, color_underline) } +fn format_js_error(js_error: &JsError, is_child: bool) -> String { + let mut s = String::new(); + s.push_str(&js_error.exception_message); + if let Some(aggregated) = &js_error.aggregated { + for aggregated_error in aggregated { + let error_string = format_js_error(aggregated_error, true); + for line in error_string.trim_start_matches("Uncaught ").lines() { + s.push_str(&format!("\n {}", line)); + } + } + } + let column_number = js_error + .source_line_frame_index + .and_then(|i| js_error.frames.get(i).unwrap().column_number); + s.push_str(&format_maybe_source_line( + if is_child { + None + } else { + js_error.source_line.as_deref() + }, + column_number, + true, + 0, + )); + for frame in &js_error.frames { + s.push_str(&format!("\n at {}", format_frame(frame))); + } + if let Some(cause) = &js_error.cause { + let error_string = format_js_error(cause, true); + s.push_str(&format!( + "\nCaused by: {}", + error_string.trim_start_matches("Uncaught ") + )); + } + s +} + /// Wrapper around deno_core::JsError which provides colorful /// string representation. #[derive(Debug)] @@ -240,26 +238,7 @@ impl Deref for PrettyJsError { impl fmt::Display for PrettyJsError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let cause = self - .0 - .cause - .clone() - .map(|cause| format!("{}", PrettyJsError(*cause))); - - write!( - f, - "{}", - &format_stack( - true, - &self.0.exception_message, - cause.as_deref(), - self.0.source_line.as_deref(), - self.0.source_line_frame_index, - &self.0.frames, - 0 - ) - )?; - Ok(()) + write!(f, "{}", &format_js_error(&self.0, false)) } } diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 969a57a9fe..b918e403df 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -2714,3 +2714,15 @@ itest!(set_timeout_error_handled { args: "run --quiet set_timeout_error_handled.ts", output: "set_timeout_error_handled.ts.out", }); + +itest!(aggregate_error { + args: "run --quiet aggregate_error.ts", + output: "aggregate_error.out", + exit_code: 1, +}); + +itest!(complex_error { + args: "run --quiet complex_error.ts", + output: "complex_error.ts.out", + exit_code: 1, +}); diff --git a/cli/tests/testdata/aggregate_error.out b/cli/tests/testdata/aggregate_error.out new file mode 100644 index 0000000000..7d0c09c705 --- /dev/null +++ b/cli/tests/testdata/aggregate_error.out @@ -0,0 +1,18 @@ +AggregateError: Multiple errors. + at [WILDCARD]/aggregate_error.ts:1:24 + +AggregateError: Multiple errors. + Error: Error message 1. + at [WILDCARD]/aggregate_error.ts:2:3 + Error: Error message 2. + at [WILDCARD]/aggregate_error.ts:3:3 + at [WILDCARD]/aggregate_error.ts:1:24 + +error: Uncaught AggregateError: Multiple errors. + Error: Error message 1. + at [WILDCARD]/aggregate_error.ts:2:3 + Error: Error message 2. + at [WILDCARD]/aggregate_error.ts:3:3 +const aggregateError = new AggregateError([ + ^ + at [WILDCARD]/aggregate_error.ts:1:24 diff --git a/cli/tests/testdata/aggregate_error.ts b/cli/tests/testdata/aggregate_error.ts new file mode 100644 index 0000000000..ce4b543762 --- /dev/null +++ b/cli/tests/testdata/aggregate_error.ts @@ -0,0 +1,9 @@ +const aggregateError = new AggregateError([ + new Error("Error message 1."), + new Error("Error message 2."), +], "Multiple errors."); +console.log(aggregateError.stack); +console.log(); +console.log(aggregateError); +console.log(); +throw aggregateError; diff --git a/cli/tests/testdata/complex_error.ts b/cli/tests/testdata/complex_error.ts new file mode 100644 index 0000000000..b462992ac7 --- /dev/null +++ b/cli/tests/testdata/complex_error.ts @@ -0,0 +1,18 @@ +const error = new AggregateError( + [ + new AggregateError([new Error("qux1"), new Error("quux1")]), + new Error("bar1", { cause: new Error("baz1") }), + ], + "foo1", + { + cause: new AggregateError([ + new AggregateError([new Error("qux2"), new Error("quux2")]), + new Error("bar2", { cause: new Error("baz2") }), + ], "foo2"), + }, +); +console.log(error.stack); +console.log(); +console.log(error); +console.log(); +throw error; diff --git a/cli/tests/testdata/complex_error.ts.out b/cli/tests/testdata/complex_error.ts.out new file mode 100644 index 0000000000..eef1b7699c --- /dev/null +++ b/cli/tests/testdata/complex_error.ts.out @@ -0,0 +1,44 @@ +AggregateError: foo1 + at [WILDCARD]/complex_error.ts:1:15 + +AggregateError: foo1 + AggregateError + Error: qux1 + at [WILDCARD]/complex_error.ts:3:25 + Error: quux1 + at [WILDCARD]/complex_error.ts:3:44 + at [WILDCARD]/complex_error.ts:3:5 + Error: bar1 + at [WILDCARD]/complex_error.ts:4:5 + Caused by Error: baz1 + at [WILDCARD]/complex_error.ts:4:32 + at [WILDCARD]/complex_error.ts:1:15 +Caused by AggregateError: foo2 + at [WILDCARD]/complex_error.ts:8:12 + +error: Uncaught AggregateError: foo1 + AggregateError + Error: qux1 + at [WILDCARD]/complex_error.ts:3:25 + Error: quux1 + at [WILDCARD]/complex_error.ts:3:44 + at [WILDCARD]/complex_error.ts:3:5 + Error: bar1 + at [WILDCARD]/complex_error.ts:4:5 + Caused by: Error: baz1 + at [WILDCARD]/complex_error.ts:4:32 +const error = new AggregateError( + ^ + at [WILDCARD]/complex_error.ts:1:15 +Caused by: AggregateError: foo2 + AggregateError + Error: qux2 + at [WILDCARD]/complex_error.ts:9:27 + Error: quux2 + at [WILDCARD]/complex_error.ts:9:46 + at [WILDCARD]/complex_error.ts:9:7 + Error: bar2 + at [WILDCARD]/complex_error.ts:10:7 + Caused by: Error: baz2 + at [WILDCARD]/complex_error.ts:10:34 + at [WILDCARD]/complex_error.ts:8:12 diff --git a/cli/tests/testdata/error_cause.ts.out b/cli/tests/testdata/error_cause.ts.out index 0f32b1bddb..512ab43266 100644 --- a/cli/tests/testdata/error_cause.ts.out +++ b/cli/tests/testdata/error_cause.ts.out @@ -6,12 +6,10 @@ error: Uncaught Error: foo at b (file:///[WILDCARD]/error_cause.ts:7:3) at c (file:///[WILDCARD]/error_cause.ts:11:3) at file:///[WILDCARD]/error_cause.ts:14:1 -Caused by: Uncaught Error: bar - throw new Error("foo", { cause: new Error("bar", { cause: "deno" as any }) }); - ^ +Caused by: Error: bar at a (file:///[WILDCARD]/error_cause.ts:3:35) at b (file:///[WILDCARD]/error_cause.ts:7:3) at c (file:///[WILDCARD]/error_cause.ts:11:3) at file:///[WILDCARD]/error_cause.ts:14:1 -Caused by: Uncaught deno -[WILDCARD] \ No newline at end of file +Caused by: deno +[WILDCARD] diff --git a/cli/tests/testdata/error_cause_recursive.ts.out b/cli/tests/testdata/error_cause_recursive.ts.out index 8bfda02fb3..ac729574d3 100644 --- a/cli/tests/testdata/error_cause_recursive.ts.out +++ b/cli/tests/testdata/error_cause_recursive.ts.out @@ -3,12 +3,8 @@ error: Uncaught Error: bar const y = new Error("bar", { cause: x }); ^ at file:///[WILDCARD]/error_cause_recursive.ts:2:11 -Caused by: Uncaught Error: foo -const x = new Error("foo"); - ^ +Caused by: Error: foo at file:///[WILDCARD]/error_cause_recursive.ts:1:11 -Caused by: Uncaught Error: bar -const y = new Error("bar", { cause: x }); - ^ +Caused by: Error: bar at file:///[WILDCARD]/error_cause_recursive.ts:2:11 -[WILDCARD] \ No newline at end of file +[WILDCARD] diff --git a/core/error.rs b/core/error.rs index 59fe170c81..8014dceab6 100644 --- a/core/error.rs +++ b/core/error.rs @@ -104,6 +104,7 @@ pub struct JsError { pub frames: Vec, pub source_line: Option, pub source_line_frame_index: Option, + pub aggregated: Option>, } #[derive(Debug, PartialEq, Clone, serde::Deserialize, serde::Serialize)] @@ -305,6 +306,25 @@ impl JsError { } } + // Read an array of stored errors, this is only defined for `AggregateError` + let aggregated_errors = get_property(scope, exception, "errors"); + let aggregated_errors: Option> = + aggregated_errors.and_then(|a| a.try_into().ok()); + + let mut aggregated: Option> = None; + + if let Some(errors) = aggregated_errors { + if errors.length() > 0 { + let mut agg = vec![]; + for i in 0..errors.length() { + let error = errors.get_index(scope, i).unwrap(); + let js_error = Self::from_v8_exception(scope, error); + agg.push(js_error); + } + aggregated = Some(agg); + } + } + Self { name: e.name, message: e.message, @@ -314,6 +334,7 @@ impl JsError { source_line_frame_index, frames, stack, + aggregated, } } else { // The exception is not a JS Error object. @@ -328,6 +349,7 @@ impl JsError { source_line_frame_index: None, frames: vec![], stack: None, + aggregated: None, } } } diff --git a/ext/console/02_console.js b/ext/console/02_console.js index d29bee8018..0f51bded83 100644 --- a/ext/console/02_console.js +++ b/ext/console/02_console.js @@ -9,6 +9,8 @@ const colors = window.__bootstrap.colors; const { ArrayBufferIsView, + AggregateErrorPrototype, + ArrayPrototypeUnshift, isNaN, DataViewPrototype, DatePrototype, @@ -947,16 +949,50 @@ } ArrayPrototypeShift(causes); - return (MapPrototypeGet(refMap, value) ?? "") + value.stack + - ArrayPrototypeJoin( + let finalMessage = (MapPrototypeGet(refMap, value) ?? ""); + + if (ObjectPrototypeIsPrototypeOf(AggregateErrorPrototype, value)) { + const stackLines = StringPrototypeSplit(value.stack, "\n"); + while (true) { + const line = ArrayPrototypeShift(stackLines); + if (RegExpPrototypeTest(/\s+at/, line)) { + ArrayPrototypeUnshift(stackLines, line); + break; + } + + finalMessage += line; + finalMessage += "\n"; + } + const aggregateMessage = ArrayPrototypeJoin( ArrayPrototypeMap( - causes, - (cause) => - "\nCaused by " + (MapPrototypeGet(refMap, cause) ?? "") + - (cause?.stack ?? cause), + value.errors, + (error) => + StringPrototypeReplace( + inspectArgs([error]), + /^(?!\s*$)/gm, + StringPrototypeRepeat(" ", 4), + ), ), - "", + "\n", ); + finalMessage += aggregateMessage; + finalMessage += "\n"; + finalMessage += ArrayPrototypeJoin(stackLines, "\n"); + } else { + finalMessage += value.stack; + } + + finalMessage += ArrayPrototypeJoin( + ArrayPrototypeMap( + causes, + (cause) => + "\nCaused by " + (MapPrototypeGet(refMap, cause) ?? "") + + (cause?.stack ?? cause), + ), + "", + ); + + return finalMessage; } function inspectStringObject(value, inspectOptions) {