refactor: Move source map lookups to core (denoland#14274)

The following transformations gradually faced by "JsError" have all been moved up front to "JsError::from_v8_exception()": - finding the first non-"deno:" source line; - moving "JsError::script_resource_name" etc. into the first error stack in case of syntax errors; - source mapping "JsError::script_resource_name" etc. when wrapping the error even though the frame locations are source mapped earlier; - removing "JsError::{script_resource_name,line_number,start_column,end_column}" entirely in favour of "js_error.frames.get(0)". We also no longer pass a js-side callback to "core/02_error.js" from cli. I avoided doing this on previous occasions because the source map lookups were in an awkward place.
Gaubee · Apr 15, 2022 · 8b31fc2 · 8b31fc2
1 parent b4af648
commit 8b31fc2
Show file tree

Hide file tree

Showing 26 changed files with 289 additions and 477 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -90,7 +90,6 @@ secure_tempfile = { version = "=3.2.0", package = "tempfile" } # different name
 semver-parser = "=0.10.2"
 serde = { version = "=1.0.136", features = ["derive"] }
 shell-escape = "=0.1.5"
-sourcemap = "=6.0.1"
 text-size = "=1.1.0"
 text_lines = "=0.4.1"
 tokio = { version = "=1.17", features = ["full"] }

diff --git a/cli/fmt_errors.rs b/cli/fmt_errors.rs
@@ -134,17 +134,17 @@ fn format_stack(
   message_line: &str,
   cause: Option<&str>,
   source_line: Option<&str>,
-  start_column: Option<i64>,
-  end_column: Option<i64>,
+  source_line_frame_index: Option<usize>,
   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,
-    start_column,
-    end_column,
+    column_number,
     is_error,
     level,
   ));
@@ -171,12 +171,11 @@ fn format_stack(
 /// a pretty printed version of that line.
 fn format_maybe_source_line(
   source_line: Option<&str>,
-  start_column: Option<i64>,
-  end_column: Option<i64>,
+  column_number: Option<i64>,
   is_error: bool,
   level: usize,
 ) -> String {
-  if source_line.is_none() || start_column.is_none() || end_column.is_none() {
+  if source_line.is_none() || column_number.is_none() {
     return "".to_string();
   }
 
@@ -191,37 +190,24 @@ fn format_maybe_source_line(
     return format!("\n{}", source_line);
   }
 
-  assert!(start_column.is_some());
-  assert!(end_column.is_some());
   let mut s = String::new();
-  let start_column = start_column.unwrap();
-  let end_column = end_column.unwrap();
+  let column_number = column_number.unwrap();
 
-  if start_column as usize >= source_line.len() {
+  if column_number as usize > source_line.len() {
     return format!(
       "\n{} Couldn't format source line: Column {} is out of bounds (source may have changed at runtime)",
-      crate::colors::yellow("Warning"), start_column + 1,
+      crate::colors::yellow("Warning"), column_number,
     );
   }
 
-  // TypeScript uses `~` always, but V8 would utilise `^` always, even when
-  // doing ranges, so here, if we only have one marker (very common with V8
-  // errors) we will use `^` instead.
-  let underline_char = if (end_column - start_column) <= 1 {
-    '^'
-  } else {
-    '~'
-  };
-  for _i in 0..start_column {
+  for _i in 0..(column_number - 1) {
     if source_line.chars().nth(_i as usize).unwrap() == '\t' {
       s.push('\t');
     } else {
       s.push(' ');
     }
   }
-  for _i in 0..(end_column - start_column) {
-    s.push(underline_char);
-  }
+  s.push('^');
   let color_underline = if is_error {
     red(&s).to_string()
   } else {
@@ -254,24 +240,6 @@ impl Deref for PrettyJsError {
 
 impl fmt::Display for PrettyJsError {
   fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-    let mut frames = self.0.frames.clone();
-
-    // 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()
-      && self.0.script_resource_name.is_some()
-      && self.0.line_number.is_some()
-      && self.0.start_column.is_some()
-    {
-      frames = vec![JsStackFrame::from_location(
-        self.0.script_resource_name.clone(),
-        self.0.line_number,
-        self.0.start_column.map(|n| n + 1),
-      )];
-    }
-
     let cause = self
       .0
       .cause
@@ -286,9 +254,8 @@ impl fmt::Display for PrettyJsError {
         &self.0.exception_message,
         cause.as_deref(),
         self.0.source_line.as_deref(),
-        self.0.start_column,
-        self.0.end_column,
-        &frames,
+        self.0.source_line_frame_index,
+        &self.0.frames,
         0
       )
     )?;
@@ -305,22 +272,17 @@ mod tests {
 
   #[test]
   fn test_format_none_source_line() {
-    let actual = format_maybe_source_line(None, None, None, false, 0);
+    let actual = format_maybe_source_line(None, None, false, 0);
     assert_eq!(actual, "");
   }
 
   #[test]
   fn test_format_some_source_line() {
-    let actual = format_maybe_source_line(
-      Some("console.log('foo');"),
-      Some(8),
-      Some(11),
-      true,
-      0,
-    );
+    let actual =
+      format_maybe_source_line(Some("console.log('foo');"), Some(9), true, 0);
     assert_eq!(
       strip_ansi_codes(&actual),
-      "\nconsole.log(\'foo\');\n        ~~~"
+      "\nconsole.log(\'foo\');\n        ^"
     );
   }
 }
diff --git a/cli/main.rs b/cli/main.rs
@@ -29,7 +29,6 @@ mod module_loader;
 mod ops;
 mod proc_state;
 mod resolver;
-mod source_maps;
 mod standalone;
 mod text_encoding;
 mod tools;
@@ -71,7 +70,6 @@ use crate::module_loader::CliModuleLoader;
 use crate::proc_state::ProcState;
 use crate::resolver::ImportMapResolver;
 use crate::resolver::JsxResolver;
-use crate::source_maps::apply_source_map;
 use deno_ast::MediaType;
 use deno_core::error::generic_error;
 use deno_core::error::AnyError;
@@ -127,13 +125,6 @@ fn create_web_worker_preload_module_callback(
 
 fn create_web_worker_callback(ps: ProcState) -> Arc<CreateWebWorkerCb> {
   Arc::new(move |args| {
-    let global_state_ = ps.clone();
-    let js_error_create_fn = Rc::new(move |core_js_error| {
-      let source_mapped_error =
-        apply_source_map(&core_js_error, global_state_.clone());
-      PrettyJsError::create(source_mapped_error)
-    });
-
     let maybe_inspector_server = ps.maybe_inspector_server.clone();
 
     let module_loader = CliModuleLoader::new_for_worker(
@@ -149,7 +140,6 @@ fn create_web_worker_callback(ps: ProcState) -> Arc<CreateWebWorkerCb> {
     let options = WebWorkerOptions {
       bootstrap: BootstrapOptions {
         args: ps.flags.argv.clone(),
-        apply_source_maps: true,
         cpu_count: std::thread::available_parallelism()
           .map(|p| p.get())
           .unwrap_or(1),
@@ -176,7 +166,8 @@ fn create_web_worker_callback(ps: ProcState) -> Arc<CreateWebWorkerCb> {
       module_loader,
       create_web_worker_cb,
       preload_module_cb,
-      js_error_create_fn: Some(js_error_create_fn),
+      source_map_getter: Some(Box::new(ps.clone())),
+      js_error_create_fn: Some(Rc::new(PrettyJsError::create)),
       use_deno_namespace: args.use_deno_namespace,
       worker_type: args.worker_type,
       maybe_inspector_server,
@@ -206,14 +197,6 @@ pub fn create_main_worker(
 ) -> MainWorker {
   let module_loader = CliModuleLoader::new(ps.clone());
 
-  let global_state_ = ps.clone();
-
-  let js_error_create_fn = Rc::new(move |core_js_error| {
-    let source_mapped_error =
-      apply_source_map(&core_js_error, global_state_.clone());
-    PrettyJsError::create(source_mapped_error)
-  });
-
   let maybe_inspector_server = ps.maybe_inspector_server.clone();
   let should_break_on_first_statement = ps.flags.inspect_brk.is_some();
 
@@ -252,7 +235,6 @@ pub fn create_main_worker(
 
   let options = WorkerOptions {
     bootstrap: BootstrapOptions {
-      apply_source_maps: true,
       args: ps.flags.argv.clone(),
       cpu_count: std::thread::available_parallelism()
         .map(|p| p.get())
@@ -274,7 +256,8 @@ pub fn create_main_worker(
     root_cert_store: ps.root_cert_store.clone(),
     user_agent: version::get_user_agent(),
     seed: ps.flags.seed,
-    js_error_create_fn: Some(js_error_create_fn),
+    source_map_getter: Some(Box::new(ps.clone())),
+    js_error_create_fn: Some(Rc::new(PrettyJsError::create)),
     create_web_worker_cb,
     web_worker_preload_module_cb,
     maybe_inspector_server,
@@ -1168,7 +1151,7 @@ async fn run_command(
   run_flags: RunFlags,
 ) -> Result<i32, AnyError> {
   if !flags.has_check_flag && flags.type_check_mode == TypeCheckMode::All {
-    info!("{} In future releases `deno run` will not automatically type check without the --check flag. 
+    info!("{} In future releases `deno run` will not automatically type check without the --check flag.
 To opt into this new behavior now, specify DENO_FUTURE_CHECK=1.", colors::yellow("Warning"));
   }
 

diff --git a/cli/ops/errors.rs b/cli/ops/errors.rs
@@ -2,70 +2,22 @@
 
 use crate::diagnostics::Diagnostics;
 use crate::fmt_errors::format_file_name;
-use crate::proc_state::ProcState;
-use crate::source_maps::get_orig_position;
-use crate::source_maps::CachedMaps;
 use deno_core::error::AnyError;
 use deno_core::op;
 use deno_core::serde_json;
 use deno_core::serde_json::json;
 use deno_core::serde_json::Value;
 use deno_core::Extension;
-use deno_core::OpState;
-use serde::Deserialize;
-use serde::Serialize;
-use std::collections::HashMap;
 
 pub fn init() -> Extension {
   Extension::builder()
     .ops(vec![
-      op_apply_source_map::decl(),
       op_format_diagnostic::decl(),
       op_format_file_name::decl(),
     ])
     .build()
 }
 
-#[derive(Deserialize)]
-#[serde(rename_all = "camelCase")]
-struct ApplySourceMap {
-  file_name: String,
-  line_number: i32,
-  column_number: i32,
-}
-
-#[derive(Serialize)]
-#[serde(rename_all = "camelCase")]
-struct AppliedSourceMap {
-  file_name: String,
-  line_number: u32,
-  column_number: u32,
-}
-
-#[op]
-fn op_apply_source_map(
-  state: &mut OpState,
-  args: ApplySourceMap,
-) -> Result<AppliedSourceMap, AnyError> {
-  let mut mappings_map: CachedMaps = HashMap::new();
-  let ps = state.borrow::<ProcState>().clone();
-
-  let (orig_file_name, orig_line_number, orig_column_number, _) =
-    get_orig_position(
-      args.file_name,
-      args.line_number.into(),
-      args.column_number.into(),
-      &mut mappings_map,
-      ps,
-    );
-
-  Ok(AppliedSourceMap {
-    file_name: orig_file_name,
-    line_number: orig_line_number as u32,
-    column_number: orig_column_number as u32,
-  })
-}
-
 #[op]
 fn op_format_diagnostic(args: Value) -> Result<Value, AnyError> {
   let diagnostic: Diagnostics = serde_json::from_value(args)?;

diff --git a/cli/proc_state.rs b/cli/proc_state.rs
@@ -20,7 +20,6 @@ use crate::lockfile::as_maybe_locker;
 use crate::lockfile::Lockfile;
 use crate::resolver::ImportMapResolver;
 use crate::resolver::JsxResolver;
-use crate::source_maps::SourceMapGetter;
 use crate::version;
 
 use deno_ast::MediaType;
@@ -38,6 +37,7 @@ use deno_core::ModuleSource;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
 use deno_core::SharedArrayBufferStore;
+use deno_core::SourceMapGetter;
 use deno_graph::create_graph;
 use deno_graph::source::CacheInfo;
 use deno_graph::source::LoadFuture;