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

@@ -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",

cli/tests/testdata/run/dynamic_import_syntax_error.js

@@ -0,0 +1 @@
+await import("./dynamic_import_syntax_error_import.js");

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

cli/tests/testdata/run/dynamic_import_syntax_error_import.js

@@ -0,0 +1,5 @@
+// deno-lint-ignore-file
+function foo() {
+  await Promise.resolve();
+}
+foo();

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<v8::Object> = 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<v8::String> = 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();

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<Self> {
+    let f = message.get_script_resource_name(scope)?;
+    let f: v8::Local<v8::String> = 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<String> {
+    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<JsStackFrame>
     let mut frames: Vec<JsStackFrame> = 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::<v8::String>::try_from(v).ok())
-        .map(|v| v.to_rust_string_lossy(scope));
-      let line_number: Option<i64> =
-        msg.get_line_number(scope).and_then(|v| v.try_into().ok());
-      let column_number: Option<i64> = 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::<v8::String>::try_from(v).ok())
-            .map(|v| v.to_rust_string_lossy(scope));
-          let line_number: Option<i64> =
-            msg.get_line_number(scope).and_then(|v| v.try_into().ok());
-          let column_number: Option<i64> =
-            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(())
   }