From b183737fc973955a528853a9e06b201a9bb9b72b Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 12 Apr 2023 12:45:36 +0100 Subject: [PATCH] fix(core): preserve syntax error locations in dynamic imports (#18664) Fixes #6259. Adds the location for v8 syntax errors to the message (`message += " at {location}"`) when rethrowing them for dynamic imports. Discussing with @bartlomieju on discord I proposed just preserving v8's error and not reconstructing it, allowing the standard stack trace to just point to the syntax error instead of the dynamic import. But on further thought this way has parity with SWC's syntax errors + has the advantage of showing both the syntax error and dynamic import location. ```ts // temp.js await import("./temp2.js"); // temp2.js function foo() { await Promise.resolve(); } // Before: // error: Uncaught (in promise) SyntaxError: Unexpected reserved word // await import("./temp2.js"); // ^ // at async file:///.../temp.js:1:1 // After: // error: Uncaught (in promise) SyntaxError: Unexpected reserved word at file:///.../temp2.js:2:3 // await import("./temp2.js"); // ^ // at async file:///.../temp.js:1:1 ``` --- cli/tests/integration/run_tests.rs | 6 + .../run/dynamic_import_syntax_error.js | 1 + .../run/dynamic_import_syntax_error.js.out | 4 + .../run/dynamic_import_syntax_error_import.js | 5 + core/bindings.rs | 31 ++-- core/error.rs | 140 +++++++----------- 6 files changed, 88 insertions(+), 99 deletions(-) create mode 100644 cli/tests/testdata/run/dynamic_import_syntax_error.js create mode 100644 cli/tests/testdata/run/dynamic_import_syntax_error.js.out create mode 100644 cli/tests/testdata/run/dynamic_import_syntax_error_import.js diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 1d70a9cb74..b5bb04fdd9 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -4352,6 +4352,12 @@ itest!(node_prefix_missing { exit_code: 1, }); +itest!(dynamic_import_syntax_error { + args: "run -A run/dynamic_import_syntax_error.js", + output: "run/dynamic_import_syntax_error.js.out", + exit_code: 1, +}); + itest!(extension_import { args: "run run/extension_import.ts", output: "run/extension_import.ts.out", diff --git a/cli/tests/testdata/run/dynamic_import_syntax_error.js b/cli/tests/testdata/run/dynamic_import_syntax_error.js new file mode 100644 index 0000000000..be8ec132f9 --- /dev/null +++ b/cli/tests/testdata/run/dynamic_import_syntax_error.js @@ -0,0 +1 @@ +await import("./dynamic_import_syntax_error_import.js"); diff --git a/cli/tests/testdata/run/dynamic_import_syntax_error.js.out b/cli/tests/testdata/run/dynamic_import_syntax_error.js.out new file mode 100644 index 0000000000..91e69eac70 --- /dev/null +++ b/cli/tests/testdata/run/dynamic_import_syntax_error.js.out @@ -0,0 +1,4 @@ +error: Uncaught (in promise) SyntaxError: Unexpected reserved word at [WILDCARD]/dynamic_import_syntax_error_import.js:3:3 +await import("./dynamic_import_syntax_error_import.js"); +^ + at async [WILDCARD]/dynamic_import_syntax_error.js:1:1 diff --git a/cli/tests/testdata/run/dynamic_import_syntax_error_import.js b/cli/tests/testdata/run/dynamic_import_syntax_error_import.js new file mode 100644 index 0000000000..bcf0756385 --- /dev/null +++ b/cli/tests/testdata/run/dynamic_import_syntax_error_import.js @@ -0,0 +1,5 @@ +// deno-lint-ignore-file +function foo() { + await Promise.resolve(); +} +foo(); diff --git a/core/bindings.rs b/core/bindings.rs index 8e701c9034..5650b78f36 100644 --- a/core/bindings.rs +++ b/core/bindings.rs @@ -7,6 +7,7 @@ use log::debug; use v8::MapFnTo; use crate::error::is_instance_of_error; +use crate::error::JsStackFrame; use crate::modules::get_asserted_module_type_from_assertions; use crate::modules::parse_import_assertions; use crate::modules::resolve_helper; @@ -436,26 +437,26 @@ fn catch_dynamic_import_promise_error( if is_instance_of_error(scope, arg) { let e: crate::error::NativeJsError = serde_v8::from_v8(scope, arg).unwrap(); let name = e.name.unwrap_or_else(|| "Error".to_string()); - let message = v8::Exception::create_message(scope, arg); - if message.get_stack_trace(scope).unwrap().get_frame_count() == 0 { + let msg = v8::Exception::create_message(scope, arg); + if msg.get_stack_trace(scope).unwrap().get_frame_count() == 0 { let arg: v8::Local = arg.try_into().unwrap(); let message_key = v8::String::new_external_onebyte_static(scope, b"message").unwrap(); let message = arg.get(scope, message_key.into()).unwrap(); + let mut message: v8::Local = message.try_into().unwrap(); + if let Some(stack_frame) = JsStackFrame::from_v8_message(scope, msg) { + if let Some(location) = stack_frame.maybe_format_location() { + let str = + format!("{} at {location}", message.to_rust_string_lossy(scope)); + message = v8::String::new(scope, &str).unwrap(); + } + } let exception = match name.as_str() { - "RangeError" => { - v8::Exception::range_error(scope, message.try_into().unwrap()) - } - "TypeError" => { - v8::Exception::type_error(scope, message.try_into().unwrap()) - } - "SyntaxError" => { - v8::Exception::syntax_error(scope, message.try_into().unwrap()) - } - "ReferenceError" => { - v8::Exception::reference_error(scope, message.try_into().unwrap()) - } - _ => v8::Exception::error(scope, message.try_into().unwrap()), + "RangeError" => v8::Exception::range_error(scope, message), + "TypeError" => v8::Exception::type_error(scope, message), + "SyntaxError" => v8::Exception::syntax_error(scope, message), + "ReferenceError" => v8::Exception::reference_error(scope, message), + _ => v8::Exception::error(scope, message), }; let code_key = v8::String::new_external_onebyte_static(scope, b"code").unwrap(); diff --git a/core/error.rs b/core/error.rs index b35544bb1e..64898678ae 100644 --- a/core/error.rs +++ b/core/error.rs @@ -195,6 +195,44 @@ impl JsStackFrame { promise_index: None, } } + + /// Gets the source mapped stack frame corresponding to the + /// (script_resource_name, line_number, column_number) from a v8 message. + /// For non-syntax errors, it should also correspond to the first stack frame. + pub fn from_v8_message<'a>( + scope: &'a mut v8::HandleScope, + message: v8::Local<'a, v8::Message>, + ) -> Option { + let f = message.get_script_resource_name(scope)?; + let f: v8::Local = f.try_into().ok()?; + let f = f.to_rust_string_lossy(scope); + let l = message.get_line_number(scope)? as i64; + // V8's column numbers are 0-based, we want 1-based. + let c = message.get_start_column() as i64 + 1; + let state_rc = JsRuntime::state(scope); + let state = &mut *state_rc.borrow_mut(); + if let Some(source_map_getter) = &state.source_map_getter { + let (f, l, c) = apply_source_map( + f, + l, + c, + &mut state.source_map_cache, + source_map_getter.as_ref(), + ); + Some(JsStackFrame::from_location(Some(f), Some(l), Some(c))) + } else { + Some(JsStackFrame::from_location(Some(f), Some(l), Some(c))) + } + } + + pub fn maybe_format_location(&self) -> Option { + Some(format!( + "{}:{}:{}", + self.file_name.as_ref()?, + self.line_number?, + self.column_number? + )) + } } fn get_property<'a>( @@ -231,42 +269,18 @@ impl JsError { let scope = &mut v8::HandleScope::new(scope); let exception_message = msg.get(scope).to_rust_string_lossy(scope); - let state_rc = JsRuntime::state(scope); // Convert them into Vec let mut frames: Vec = vec![]; - let mut source_line = None; let mut source_line_frame_index = None; + + if let Some(stack_frame) = JsStackFrame::from_v8_message(scope, msg) { + frames = vec![stack_frame]; + } { + let state_rc = JsRuntime::state(scope); let state = &mut *state_rc.borrow_mut(); - - let script_resource_name = msg - .get_script_resource_name(scope) - .and_then(|v| v8::Local::::try_from(v).ok()) - .map(|v| v.to_rust_string_lossy(scope)); - let line_number: Option = - msg.get_line_number(scope).and_then(|v| v.try_into().ok()); - let column_number: Option = msg.get_start_column().try_into().ok(); - if let (Some(f), Some(l), Some(c)) = - (script_resource_name, line_number, column_number) - { - // V8's column numbers are 0-based, we want 1-based. - let c = c + 1; - if let Some(source_map_getter) = &state.source_map_getter { - let (f, l, c) = apply_source_map( - f, - l, - c, - &mut state.source_map_cache, - source_map_getter.as_ref(), - ); - frames = vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; - } else { - frames = vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; - } - } - if let Some(source_map_getter) = &state.source_map_getter { for (i, frame) in frames.iter().enumerate() { if let (Some(file_name), Some(line_number)) = @@ -377,48 +391,21 @@ impl JsError { Some(frames_v8) => serde_v8::from_v8(scope, frames_v8.into()).unwrap(), None => vec![], }; - let mut source_line = None; let mut source_line_frame_index = None; + + // When the stack frame array is empty, but the source location given by + // (script_resource_name, line_number, start_column + 1) exists, this is + // likely a syntax error. For the sake of formatting we treat it like it + // was given as a single stack frame. + if frames.is_empty() { + if let Some(stack_frame) = JsStackFrame::from_v8_message(scope, msg) { + frames = vec![stack_frame]; + } + } { let state_rc = JsRuntime::state(scope); let state = &mut *state_rc.borrow_mut(); - - // When the stack frame array is empty, but the source location given by - // (script_resource_name, line_number, start_column + 1) exists, this is - // likely a syntax error. For the sake of formatting we treat it like it - // was given as a single stack frame. - if frames.is_empty() { - let script_resource_name = msg - .get_script_resource_name(scope) - .and_then(|v| v8::Local::::try_from(v).ok()) - .map(|v| v.to_rust_string_lossy(scope)); - let line_number: Option = - msg.get_line_number(scope).and_then(|v| v.try_into().ok()); - let column_number: Option = - msg.get_start_column().try_into().ok(); - if let (Some(f), Some(l), Some(c)) = - (script_resource_name, line_number, column_number) - { - // V8's column numbers are 0-based, we want 1-based. - let c = c + 1; - if let Some(source_map_getter) = &state.source_map_getter { - let (f, l, c) = apply_source_map( - f, - l, - c, - &mut state.source_map_cache, - source_map_getter.as_ref(), - ); - frames = - vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; - } else { - frames = - vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; - } - } - } - if let Some(source_map_getter) = &state.source_map_getter { for (i, frame) in frames.iter().enumerate() { if let (Some(file_name), Some(line_number)) = @@ -502,16 +489,6 @@ impl JsError { impl std::error::Error for JsError {} -fn format_source_loc( - file_name: &str, - line_number: i64, - column_number: i64, -) -> String { - let line_number = line_number; - let column_number = column_number; - format!("{file_name}:{line_number}:{column_number}") -} - impl Display for JsError { fn fmt(&self, f: &mut Formatter) -> fmt::Result { if let Some(stack) = &self.stack { @@ -521,14 +498,9 @@ impl Display for JsError { } } write!(f, "{}", self.exception_message)?; - let frame = self.frames.first(); - if let Some(frame) = frame { - if let (Some(f_), Some(l), Some(c)) = - (&frame.file_name, frame.line_number, frame.column_number) - { - let source_loc = format_source_loc(f_, l, c); - write!(f, "\n at {source_loc}")?; - } + let location = self.frames.first().and_then(|f| f.maybe_format_location()); + if let Some(location) = location { + write!(f, "\n at {location}")?; } Ok(()) }