From 8b31fc23cd80de9baa62535e95367da7a21c9cfd Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Fri, 15 Apr 2022 15:08:09 +0100 Subject: [PATCH] refactor: Move source map lookups to core (#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. --- Cargo.lock | 2 +- cli/Cargo.toml | 1 - cli/fmt_errors.rs | 72 ++--- cli/main.rs | 27 +- cli/ops/errors.rs | 48 ---- cli/proc_state.rs | 2 +- cli/source_maps.rs | 264 ------------------ cli/standalone.rs | 2 +- cli/tests/integration/test_tests.rs | 2 +- cli/tests/testdata/future_check1.out | 2 +- ...s_source_map_2_with_inline_contents.js.out | 2 - cli/tests/testdata/test/aggregate_error.out | 1 - cli/tools/coverage/mod.rs | 4 +- core/02_error.js | 28 +- core/Cargo.toml | 1 + core/bindings.rs | 41 +++ core/error.rs | 114 ++++++-- core/lib.rs | 3 + core/runtime.rs | 15 +- core/source_map.rs | 99 +++++++ runtime/examples/hello_runtime.rs | 2 +- runtime/js/40_error_stack.js | 7 +- runtime/js/99_main.js | 4 - runtime/web_worker.rs | 18 +- runtime/worker.rs | 5 +- runtime/worker_bootstrap.rs | 2 - 26 files changed, 290 insertions(+), 478 deletions(-) delete mode 100644 cli/source_maps.rs create mode 100644 core/source_map.rs diff --git a/Cargo.lock b/Cargo.lock index 56189dd041..83e19afab0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -786,7 +786,6 @@ dependencies = [ "semver-parser 0.10.2", "serde", "shell-escape", - "sourcemap", "tempfile", "test_util", "text-size", @@ -866,6 +865,7 @@ dependencies = [ "serde", "serde_json", "serde_v8", + "sourcemap", "tokio", "url", "v8", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index c044d6c725..ed0885c0e3 100644 --- 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 index 106ecaaf21..fb7ccdb865 100644 --- 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, - end_column: Option, + 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, - 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, - end_column: Option, + column_number: Option, 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 index 689d5c6342..320e4e8c39 100644 --- 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 { 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 { 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 { 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 { 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 index a219b462de..f0f5fce04d 100644 --- 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 { - let mut mappings_map: CachedMaps = HashMap::new(); - let ps = state.borrow::().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 { let diagnostic: Diagnostics = serde_json::from_value(args)?; diff --git a/cli/proc_state.rs b/cli/proc_state.rs index fc132dadcd..d4173ee2e1 100644 --- 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; diff --git a/cli/source_maps.rs b/cli/source_maps.rs deleted file mode 100644 index d92caf7f4a..0000000000 --- a/cli/source_maps.rs +++ /dev/null @@ -1,264 +0,0 @@ -// Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. - -//! This mod provides functions to remap a `JsError` based on a source map. - -use deno_core::error::JsError; -use sourcemap::SourceMap; -use std::collections::HashMap; -use std::str; - -pub trait SourceMapGetter: Sync + Send + Clone { - /// Returns the raw source map file. - fn get_source_map(&self, file_name: &str) -> Option>; - fn get_source_line( - &self, - file_name: &str, - line_number: usize, - ) -> Option; -} - -/// Cached filename lookups. The key can be None if a previous lookup failed to -/// find a SourceMap. -pub type CachedMaps = HashMap>; - -/// Apply a source map to a `deno_core::JsError`, returning a `JsError` where -/// file names and line/column numbers point to the location in the original -/// source, rather than the transpiled source code. -pub fn apply_source_map( - js_error: &JsError, - getter: G, -) -> JsError { - // Note that js_error.frames has already been source mapped in - // prepareStackTrace(). - let mut mappings_map: CachedMaps = HashMap::new(); - - let (script_resource_name, line_number, start_column, source_line) = - get_maybe_orig_position( - js_error.script_resource_name.clone(), - js_error.line_number, - // start_column is 0-based, we need 1-based here. - js_error.start_column.map(|n| n + 1), - &mut mappings_map, - getter.clone(), - ); - let start_column = start_column.map(|n| n - 1); - // It is better to just move end_column to be the same distance away from - // start column because sometimes the code point is not available in the - // source file map. - let end_column = match js_error.end_column { - Some(ec) => { - start_column.map(|sc| ec - (js_error.start_column.unwrap() - sc)) - } - _ => None, - }; - - let mut script_resource_name = script_resource_name; - let mut line_number = line_number; - let mut start_column = start_column; - let mut end_column = end_column; - let mut source_line = source_line; - if script_resource_name - .as_ref() - .map_or(true, |s| s.starts_with("deno:")) - { - for frame in &js_error.frames { - if let (Some(file_name), Some(line_number_)) = - (&frame.file_name, frame.line_number) - { - if !file_name.starts_with("deno:") { - script_resource_name = Some(file_name.clone()); - line_number = Some(line_number_); - start_column = frame.column_number.map(|n| n - 1); - end_column = frame.column_number; - // Lookup expects 0-based line and column numbers, but ours are 1-based. - source_line = - getter.get_source_line(file_name, (line_number_ - 1) as usize); - break; - } - } - } - } - - let cause = js_error - .cause - .clone() - .map(|cause| Box::new(apply_source_map(&*cause, getter))); - - JsError { - name: js_error.name.clone(), - message: js_error.message.clone(), - exception_message: js_error.exception_message.clone(), - cause, - source_line, - script_resource_name, - line_number, - start_column, - end_column, - frames: js_error.frames.clone(), - stack: None, - } -} - -fn get_maybe_orig_position( - file_name: Option, - line_number: Option, - column_number: Option, - mappings_map: &mut CachedMaps, - getter: G, -) -> (Option, Option, Option, Option) { - match (file_name, line_number, column_number) { - (Some(file_name_v), Some(line_v), Some(column_v)) => { - let (file_name, line_number, column_number, source_line) = - get_orig_position(file_name_v, line_v, column_v, mappings_map, getter); - ( - Some(file_name), - Some(line_number), - Some(column_number), - source_line, - ) - } - _ => (None, None, None, None), - } -} - -pub fn get_orig_position( - file_name: String, - line_number: i64, - column_number: i64, - mappings_map: &mut CachedMaps, - getter: G, -) -> (String, i64, i64, Option) { - // Lookup expects 0-based line and column numbers, but ours are 1-based. - let line_number = line_number - 1; - let column_number = column_number - 1; - - let default_pos = (file_name.clone(), line_number, column_number, None); - let maybe_source_map = get_mappings(&file_name, mappings_map, getter.clone()); - let (file_name, line_number, column_number, source_line) = - match maybe_source_map { - None => default_pos, - Some(source_map) => { - match source_map.lookup_token(line_number as u32, column_number as u32) - { - None => default_pos, - Some(token) => match token.get_source() { - None => default_pos, - Some(source_file_name) => { - // The `source_file_name` written by tsc in the source map is - // sometimes only the basename of the URL, or has unwanted `<`/`>` - // around it. Use the `file_name` we get from V8 if - // `source_file_name` does not parse as a URL. - let file_name = match deno_core::resolve_url(source_file_name) { - Ok(m) if m.scheme() == "blob" => file_name, - Ok(m) => m.to_string(), - Err(_) => file_name, - }; - let source_line = - if let Some(source_view) = token.get_source_view() { - source_view - .get_line(token.get_src_line()) - .map(|s| s.to_string()) - } else { - None - }; - ( - file_name, - i64::from(token.get_src_line()), - i64::from(token.get_src_col()), - source_line, - ) - } - }, - } - } - }; - let source_line = source_line - .or_else(|| getter.get_source_line(&file_name, line_number as usize)); - (file_name, line_number + 1, column_number + 1, source_line) -} - -fn get_mappings<'a, G: SourceMapGetter>( - file_name: &str, - mappings_map: &'a mut CachedMaps, - getter: G, -) -> &'a Option { - mappings_map - .entry(file_name.to_string()) - .or_insert_with(|| parse_map_string(file_name, getter)) -} - -// TODO(kitsonk) parsed source maps should probably be cached in state in -// the module meta data. -fn parse_map_string( - file_name: &str, - getter: G, -) -> Option { - getter - .get_source_map(file_name) - .and_then(|raw_source_map| SourceMap::from_slice(&raw_source_map).ok()) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[derive(Clone)] - struct MockSourceMapGetter {} - - impl SourceMapGetter for MockSourceMapGetter { - fn get_source_map(&self, file_name: &str) -> Option> { - let s = match file_name { - "foo_bar.ts" => { - r#"{"sources": ["foo_bar.ts"], "mappings":";;;IAIA,OAAO,CAAC,GAAG,CAAC,qBAAqB,EAAE,EAAE,CAAC,OAAO,CAAC,CAAC;IAC/C,OAAO,CAAC,GAAG,CAAC,eAAe,EAAE,IAAI,CAAC,QAAQ,CAAC,IAAI,CAAC,CAAC;IACjD,OAAO,CAAC,GAAG,CAAC,WAAW,EAAE,IAAI,CAAC,QAAQ,CAAC,EAAE,CAAC,CAAC;IAE3C,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC"}"# - } - "bar_baz.ts" => { - r#"{"sources": ["bar_baz.ts"], "mappings":";;;IAEA,CAAC,KAAK,IAAI,EAAE;QACV,MAAM,GAAG,GAAG,sDAAa,OAAO,2BAAC,CAAC;QAClC,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC;IACnB,CAAC,CAAC,EAAE,CAAC;IAEQ,QAAA,GAAG,GAAG,KAAK,CAAC;IAEzB,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC"}"# - } - _ => return None, - }; - Some(s.as_bytes().to_owned()) - } - - fn get_source_line( - &self, - file_name: &str, - line_number: usize, - ) -> Option { - let s = match file_name { - "foo_bar.ts" => vec![ - "console.log('foo');", - "console.log('foo');", - "console.log('foo');", - "console.log('foo');", - "console.log('foo');", - ], - _ => return None, - }; - if s.len() > line_number { - Some(s[line_number].to_string()) - } else { - None - } - } - } - - #[test] - fn apply_source_map_line() { - let e = JsError { - name: Some("TypeError".to_string()), - message: Some("baz".to_string()), - exception_message: "TypeError: baz".to_string(), - cause: None, - source_line: Some("foo".to_string()), - script_resource_name: Some("foo_bar.ts".to_string()), - line_number: Some(4), - start_column: Some(16), - end_column: None, - frames: vec![], - stack: None, - }; - let getter = MockSourceMapGetter {}; - let actual = apply_source_map(&e, getter); - assert_eq!(actual.source_line, Some("console.log('foo');".to_string())); - } -} diff --git a/cli/standalone.rs b/cli/standalone.rs index 5005fd142f..9f2aba9bd1 100644 --- a/cli/standalone.rs +++ b/cli/standalone.rs @@ -273,7 +273,6 @@ pub async fn run( let options = WorkerOptions { bootstrap: BootstrapOptions { - apply_source_maps: false, args: metadata.argv, cpu_count: std::thread::available_parallelism() .map(|p| p.get()) @@ -293,6 +292,7 @@ pub async fn run( .unsafely_ignore_certificate_errors, root_cert_store: Some(root_cert_store), seed: metadata.seed, + source_map_getter: None, js_error_create_fn: None, create_web_worker_cb, web_worker_preload_module_cb, diff --git a/cli/tests/integration/test_tests.rs b/cli/tests/integration/test_tests.rs index bf0ceb038e..6a0d5c1ab4 100644 --- a/cli/tests/integration/test_tests.rs +++ b/cli/tests/integration/test_tests.rs @@ -246,7 +246,7 @@ itest!(shuffle_with_seed { }); itest!(aggregate_error { - args: "test test/aggregate_error.ts", + args: "test --quiet test/aggregate_error.ts", exit_code: 1, output: "test/aggregate_error.out", }); diff --git a/cli/tests/testdata/future_check1.out b/cli/tests/testdata/future_check1.out index 9c7592fc55..7505efcd4a 100644 --- a/cli/tests/testdata/future_check1.out +++ b/cli/tests/testdata/future_check1.out @@ -1,3 +1,3 @@ -Warning In future releases `deno run` will not automatically type check without the --check flag. +Warning 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. Check [WILDCARD]/future_check.ts diff --git a/cli/tests/testdata/inline_js_source_map_2_with_inline_contents.js.out b/cli/tests/testdata/inline_js_source_map_2_with_inline_contents.js.out index 9280361f71..4f9127da66 100644 --- a/cli/tests/testdata/inline_js_source_map_2_with_inline_contents.js.out +++ b/cli/tests/testdata/inline_js_source_map_2_with_inline_contents.js.out @@ -1,4 +1,2 @@ error: Uncaught Error: Hello world! -throw new Error("Hello world!" as unknown as string); - ^ at http://localhost:4545/inline_js_source_map_2.ts:6:7 diff --git a/cli/tests/testdata/test/aggregate_error.out b/cli/tests/testdata/test/aggregate_error.out index 1fbdcb3502..35349e67ca 100644 --- a/cli/tests/testdata/test/aggregate_error.out +++ b/cli/tests/testdata/test/aggregate_error.out @@ -1,4 +1,3 @@ -Check [WILDCARD]/testdata/test/aggregate_error.ts running 1 test from test/aggregate_error.ts aggregate ... FAILED ([WILDCARD]) diff --git a/cli/tools/coverage/mod.rs b/cli/tools/coverage/mod.rs index 092f6c8256..3387795836 100644 --- a/cli/tools/coverage/mod.rs +++ b/cli/tools/coverage/mod.rs @@ -5,7 +5,6 @@ use crate::flags::CoverageFlags; use crate::flags::Flags; use crate::fs_util::collect_files; use crate::proc_state::ProcState; -use crate::source_maps::SourceMapGetter; use crate::tools::fmt::format_json; use deno_ast::MediaType; @@ -14,10 +13,11 @@ use deno_core::anyhow::anyhow; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::serde_json; +use deno_core::sourcemap::SourceMap; use deno_core::url::Url; use deno_core::LocalInspectorSession; +use deno_core::SourceMapGetter; use regex::Regex; -use sourcemap::SourceMap; use std::fs; use std::fs::File; use std::io::BufWriter; diff --git a/core/02_error.js b/core/02_error.js index 756098738d..c6b8081698 100644 --- a/core/02_error.js +++ b/core/02_error.js @@ -2,6 +2,7 @@ "use strict"; ((window) => { + const core = Deno.core; const { Error, ObjectFreeze, @@ -188,24 +189,8 @@ }; } - /** - * Returns a function that can be used as `Error.prepareStackTrace`. - * - * This function accepts an optional argument, a function that performs - * source mapping. It is not required to pass this argument, but - * in such case only JavaScript sources will have proper position in - * stack frames. - * @param {( - * fileName: string, - * lineNumber: number, - * columnNumber: number - * ) => { - * fileName: string, - * lineNumber: number, - * columnNumber: number - * }} sourceMappingFn - */ - function createPrepareStackTrace(sourceMappingFn, formatFileNameFn) { + /** Returns a function that can be used as `Error.prepareStackTrace`. */ + function createPrepareStackTrace(formatFileNameFn) { return function prepareStackTrace( error, callSites, @@ -214,13 +199,10 @@ const fileName = callSite.getFileName(); const lineNumber = callSite.getLineNumber(); const columnNumber = callSite.getColumnNumber(); - if ( - sourceMappingFn && fileName && lineNumber != null && - columnNumber != null - ) { + if (fileName && lineNumber != null && columnNumber != null) { return patchCallSite( callSite, - sourceMappingFn({ + core.applySourceMap({ fileName, lineNumber, columnNumber, diff --git a/core/Cargo.toml b/core/Cargo.toml index a466080351..c3503ff0d1 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -25,6 +25,7 @@ pin-project = "1.0.7" serde = { version = "1.0.129", features = ["derive"] } serde_json = { version = "1.0.66", features = ["preserve_order"] } serde_v8 = { version = "0.40.0", path = "../serde_v8" } +sourcemap = "=6.0.1" url = { version = "2.2.2", features = ["serde"] } v8 = "0.41.0" diff --git a/core/bindings.rs b/core/bindings.rs index 46dcefe389..889229bb5a 100644 --- a/core/bindings.rs +++ b/core/bindings.rs @@ -10,6 +10,7 @@ use crate::modules::ModuleMap; use crate::ops::OpCtx; use crate::ops_builtin::WasmStreamingResource; use crate::resolve_url_or_path; +use crate::source_map::apply_source_map as apply_source_map_; use crate::JsRuntime; use crate::PromiseId; use crate::ResourceId; @@ -21,6 +22,7 @@ use serde::Deserialize; use serde::Serialize; use serde_v8::to_v8; use std::cell::RefCell; +use std::collections::HashMap; use std::option::Option; use std::os::raw::c_void; use url::Url; @@ -109,6 +111,9 @@ pub static EXTERNAL_REFERENCES: Lazy = v8::ExternalReference { function: terminate.map_fn_to(), }, + v8::ExternalReference { + function: apply_source_map.map_fn_to(), + }, ]) }); @@ -237,6 +242,7 @@ pub fn initialize_context<'s>( set_func(scope, core_val, "abortWasmStreaming", abort_wasm_streaming); set_func(scope, core_val, "destructureError", destructure_error); set_func(scope, core_val, "terminate", terminate); + set_func(scope, core_val, "applySourceMap", apply_source_map); // Direct bindings on `window`. set_func(scope, global, "queueMicrotask", queue_microtask); @@ -1324,6 +1330,41 @@ fn terminate( scope.terminate_execution(); } +fn apply_source_map( + scope: &mut v8::HandleScope, + args: v8::FunctionCallbackArguments, + mut rv: v8::ReturnValue, +) { + #[derive(Deserialize, Serialize)] + #[serde(rename_all = "camelCase")] + struct Location { + file_name: String, + line_number: u32, + column_number: u32, + } + let state_rc = JsRuntime::state(scope); + let state = state_rc.borrow(); + if let Some(source_map_getter) = &state.source_map_getter { + let mut location = match serde_v8::from_v8::(scope, args.get(0)) { + Ok(location) => location, + Err(error) => return throw_type_error(scope, error.to_string()), + }; + let (f, l, c, _) = apply_source_map_( + location.file_name, + location.line_number.into(), + location.column_number.into(), + &mut HashMap::new(), + source_map_getter.as_ref(), + ); + location.file_name = f; + location.line_number = l as u32; + location.column_number = c as u32; + rv.set(serde_v8::to_v8(scope, location).unwrap()); + } else { + rv.set(args.get(0)); + } +} + fn create_host_object( scope: &mut v8::HandleScope, _args: v8::FunctionCallbackArguments, diff --git a/core/error.rs b/core/error.rs index 362bf2fa5b..59fe170c81 100644 --- a/core/error.rs +++ b/core/error.rs @@ -1,7 +1,10 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. +use crate::runtime::JsRuntime; +use crate::source_map::apply_source_map; use anyhow::Error; use std::borrow::Cow; +use std::collections::HashMap; use std::collections::HashSet; use std::fmt; use std::fmt::Debug; @@ -95,15 +98,12 @@ pub fn get_custom_error_class(error: &Error) -> Option<&'static str> { pub struct JsError { pub name: Option, pub message: Option, - pub exception_message: String, - pub cause: Option>, - pub source_line: Option, - pub script_resource_name: Option, - pub line_number: Option, - pub start_column: Option, // 0-based - pub end_column: Option, // 0-based - pub frames: Vec, pub stack: Option, + pub cause: Option>, + pub exception_message: String, + pub frames: Vec, + pub source_line: Option, + pub source_line_frame_index: Option, } #[derive(Debug, PartialEq, Clone, serde::Deserialize, serde::Serialize)] @@ -236,25 +236,82 @@ impl JsError { frames_v8.and_then(|a| a.try_into().ok()); // Convert them into Vec - let frames: Vec = match frames_v8 { + let mut frames: Vec = match frames_v8 { Some(frames_v8) => serde_v8::from_v8(scope, frames_v8.into()).unwrap(), None => vec![], }; + + let state_rc = JsRuntime::state(scope); + let state = state_rc.borrow(); + + // 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 HashMap::new(), + 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))]; + } + } + } + + let mut source_line = None; + let mut source_line_frame_index = None; + 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)) = + (&frame.file_name, frame.line_number) + { + if !file_name.trim_start_matches('[').starts_with("deno:") { + // Source lookup expects a 0-based line number, ours are 1-based. + source_line = source_map_getter + .get_source_line(file_name, (line_number - 1) as usize); + source_line_frame_index = Some(i); + break; + } + } + } + } else if let Some(frame) = frames.first() { + if let Some(file_name) = &frame.file_name { + if !file_name.trim_start_matches('[').starts_with("deno:") { + source_line = msg + .get_source_line(scope) + .map(|v| v.to_rust_string_lossy(scope)); + source_line_frame_index = Some(0); + } + } + } + Self { name: e.name, message: e.message, exception_message, cause, - 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)), - source_line: msg - .get_source_line(scope) - .map(|v| v.to_rust_string_lossy(scope)), - line_number: msg.get_line_number(scope).and_then(|v| v.try_into().ok()), - start_column: msg.get_start_column().try_into().ok(), - end_column: msg.get_end_column().try_into().ok(), + source_line, + source_line_frame_index, frames, stack, } @@ -267,11 +324,8 @@ impl JsError { message: None, exception_message: msg.get(scope).to_rust_string_lossy(scope), cause: None, - script_resource_name: None, source_line: None, - line_number: None, - start_column: None, - end_column: None, + source_line_frame_index: None, frames: vec![], stack: None, } @@ -299,15 +353,13 @@ impl Display for JsError { return write!(f, "{}", stack); } } - write!(f, "{}", self.exception_message)?; - if let Some(script_resource_name) = &self.script_resource_name { - if self.line_number.is_some() && self.start_column.is_some() { - let source_loc = format_source_loc( - script_resource_name, - self.line_number.unwrap(), - self.start_column.unwrap(), - ); + 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)?; } } diff --git a/core/lib.rs b/core/lib.rs index 652ad2cd6a..9a8cc8ef99 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -16,6 +16,7 @@ mod ops_builtin; mod ops_metrics; mod resources; mod runtime; +mod source_map; // Re-exports pub use anyhow; @@ -28,6 +29,7 @@ pub use serde_v8::Buffer as ZeroCopyBuf; pub use serde_v8::ByteString; pub use serde_v8::StringOrBuffer; pub use serde_v8::U16String; +pub use sourcemap; pub use url; pub use v8; @@ -96,6 +98,7 @@ pub use crate::runtime::JsRuntime; pub use crate::runtime::RuntimeOptions; pub use crate::runtime::SharedArrayBufferStore; pub use crate::runtime::Snapshot; +pub use crate::source_map::SourceMapGetter; pub use deno_ops::op; pub fn v8_version() -> &'static str { diff --git a/core/runtime.rs b/core/runtime.rs index a6442ce976..714f1aba84 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -17,6 +17,7 @@ use crate::modules::NoopModuleLoader; use crate::op_void_async; use crate::op_void_sync; use crate::ops::*; +use crate::source_map::SourceMapGetter; use crate::Extension; use crate::OpMiddlewareFn; use crate::OpResult; @@ -159,6 +160,7 @@ pub(crate) struct JsRuntimeState { /// A counter used to delay our dynamic import deadlock detection by one spin /// of the event loop. dyn_module_evaluate_idle_counter: u32, + pub(crate) source_map_getter: Option>, pub(crate) js_error_create_fn: Rc, pub(crate) pending_ops: FuturesUnordered, pub(crate) unrefed_ops: HashSet, @@ -225,6 +227,9 @@ fn v8_init(v8_platform: Option>) { #[derive(Default)] pub struct RuntimeOptions { + /// Source map reference for errors. + pub source_map_getter: Option>, + /// Allows a callback to be set whenever a V8 exception is made. This allows /// the caller to wrap the JsError into an error. By default this callback /// is set to `JsError::create()`. @@ -380,6 +385,7 @@ impl JsRuntime { js_uncaught_exception_cb: None, has_tick_scheduled: false, js_wasm_streaming_cb: None, + source_map_getter: options.source_map_getter, js_error_create_fn, pending_ops: FuturesUnordered::new(), unrefed_ops: HashSet::new(), @@ -1023,7 +1029,6 @@ pub(crate) fn exception_to_err_result<'s, T>( in_promise: bool, ) -> Result { let state_rc = JsRuntime::state(scope); - let mut state = state_rc.borrow_mut(); let is_terminating_exception = scope.is_execution_terminating(); let mut exception = exception; @@ -1036,6 +1041,7 @@ pub(crate) fn exception_to_err_result<'s, T>( // If the termination is the result of a `Deno.core.terminate` call, we want // to use the exception that was passed to it rather than the exception that // was passed to this function. + let mut state = state_rc.borrow_mut(); exception = state .explicit_terminate_exception .take() @@ -1058,7 +1064,7 @@ pub(crate) fn exception_to_err_result<'s, T>( js_error.exception_message.trim_start_matches("Uncaught ") ); } - let js_error = (state.js_error_create_fn)(js_error); + let js_error = (state_rc.borrow().js_error_create_fn)(js_error); if is_terminating_exception { // Re-enable exception termination. @@ -2158,7 +2164,8 @@ pub mod tests { let r = runtime.execute_script("i.js", src); let e = r.unwrap_err(); let js_error = e.downcast::().unwrap(); - assert_eq!(js_error.end_column, Some(11)); + let frame = js_error.frames.first().unwrap(); + assert_eq!(frame.column_number, Some(12)); } #[test] @@ -2500,7 +2507,7 @@ main(); "#, ); let expected_error = r#"Uncaught SyntaxError: Invalid or unexpected token - at error_without_stack.js:3:14"#; + at error_without_stack.js:3:15"#; assert_eq!(result.unwrap_err().to_string(), expected_error); } diff --git a/core/source_map.rs b/core/source_map.rs new file mode 100644 index 0000000000..d33b66586f --- /dev/null +++ b/core/source_map.rs @@ -0,0 +1,99 @@ +// Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. + +//! This mod provides functions to remap a `JsError` based on a source map. + +use crate::resolve_url; +pub use sourcemap::SourceMap; +use std::collections::HashMap; +use std::str; + +pub trait SourceMapGetter: Sync + Send { + /// Returns the raw source map file. + fn get_source_map(&self, file_name: &str) -> Option>; + fn get_source_line( + &self, + file_name: &str, + line_number: usize, + ) -> Option; +} + +/// Cached filename lookups. The key can be None if a previous lookup failed to +/// find a SourceMap. +pub type CachedMaps = HashMap>; + +pub fn apply_source_map( + file_name: String, + line_number: i64, + column_number: i64, + mappings_map: &mut CachedMaps, + getter: &G, +) -> (String, i64, i64, Option) { + // Lookup expects 0-based line and column numbers, but ours are 1-based. + let line_number = line_number - 1; + let column_number = column_number - 1; + + let default_pos = (file_name.clone(), line_number, column_number, None); + let maybe_source_map = get_mappings(&file_name, mappings_map, getter); + let (file_name, line_number, column_number, source_line) = + match maybe_source_map { + None => default_pos, + Some(source_map) => { + match source_map.lookup_token(line_number as u32, column_number as u32) + { + None => default_pos, + Some(token) => match token.get_source() { + None => default_pos, + Some(source_file_name) => { + // The `source_file_name` written by tsc in the source map is + // sometimes only the basename of the URL, or has unwanted `<`/`>` + // around it. Use the `file_name` we get from V8 if + // `source_file_name` does not parse as a URL. + let file_name = match resolve_url(source_file_name) { + Ok(m) if m.scheme() == "blob" => file_name, + Ok(m) => m.to_string(), + Err(_) => file_name, + }; + let source_line = + if let Some(source_view) = token.get_source_view() { + source_view + .get_line(token.get_src_line()) + .map(|s| s.to_string()) + } else { + None + }; + ( + file_name, + i64::from(token.get_src_line()), + i64::from(token.get_src_col()), + source_line, + ) + } + }, + } + } + }; + let source_line = source_line + .or_else(|| getter.get_source_line(&file_name, line_number as usize)); + (file_name, line_number + 1, column_number + 1, source_line) +} + +fn get_mappings<'a, G: SourceMapGetter + ?Sized>( + file_name: &str, + mappings_map: &'a mut CachedMaps, + getter: &G, +) -> &'a Option { + mappings_map + .entry(file_name.to_string()) + .or_insert_with(|| parse_map_string(file_name, getter)) +} + +// TODO(kitsonk) parsed source maps should probably be cached in state in +// the module meta data. +fn parse_map_string( + file_name: &str, + getter: &G, +) -> Option { + getter + .get_source_map(file_name) + .and_then(|raw_source_map| SourceMap::from_slice(&raw_source_map).ok()) +} diff --git a/runtime/examples/hello_runtime.rs b/runtime/examples/hello_runtime.rs index 0a0d6fe236..b4716076e8 100644 --- a/runtime/examples/hello_runtime.rs +++ b/runtime/examples/hello_runtime.rs @@ -28,7 +28,6 @@ async fn main() -> Result<(), AnyError> { let options = WorkerOptions { bootstrap: BootstrapOptions { - apply_source_maps: false, args: vec![], cpu_count: 1, debug_flag: false, @@ -45,6 +44,7 @@ async fn main() -> Result<(), AnyError> { root_cert_store: None, user_agent: "hello_runtime".to_string(), seed: None, + source_map_getter: None, js_error_create_fn: None, web_worker_preload_module_cb, create_web_worker_cb, diff --git a/runtime/js/40_error_stack.js b/runtime/js/40_error_stack.js index 7ed4632731..1ecf0f6bb1 100644 --- a/runtime/js/40_error_stack.js +++ b/runtime/js/40_error_stack.js @@ -13,12 +13,7 @@ } function opApplySourceMap(location) { - const res = core.opSync("op_apply_source_map", location); - return { - fileName: res.fileName, - lineNumber: res.lineNumber, - columnNumber: res.columnNumber, - }; + return core.applySourceMap(location); } window.__bootstrap.errorStack = { diff --git a/runtime/js/99_main.js b/runtime/js/99_main.js index 44d457e5cb..0b83846caf 100644 --- a/runtime/js/99_main.js +++ b/runtime/js/99_main.js @@ -222,10 +222,6 @@ delete Object.prototype.__proto__; build.setBuildInfo(runtimeOptions.target); util.setLogDebug(runtimeOptions.debugFlag, source); const prepareStackTrace = core.createPrepareStackTrace( - // TODO(bartlomieju): a very crude way to disable - // source mapping of errors. This condition is true - // only for compiled standalone binaries. - runtimeOptions.applySourceMaps ? errorStack.opApplySourceMap : undefined, errorStack.opFormatFileName, ); // deno-lint-ignore prefer-primordials diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs index 5d416b9353..40b222417c 100644 --- a/runtime/web_worker.rs +++ b/runtime/web_worker.rs @@ -29,6 +29,7 @@ use deno_core::ModuleLoader; use deno_core::ModuleSpecifier; use deno_core::RuntimeOptions; use deno_core::SharedArrayBufferStore; +use deno_core::SourceMapGetter; use deno_tls::rustls::RootCertStore; use deno_web::create_entangled_message_port; use deno_web::BlobStore; @@ -91,12 +92,15 @@ impl Serialize for WorkerControlEvent { WorkerControlEvent::TerminalError(error) | WorkerControlEvent::Error(error) => { let value = match error.downcast_ref::() { - Some(js_error) => json!({ - "message": js_error.exception_message, - "fileName": js_error.script_resource_name, - "lineNumber": js_error.line_number, - "columnNumber": js_error.start_column, - }), + Some(js_error) => { + let frame = js_error.frames.first(); + json!({ + "message": js_error.exception_message, + "fileName": frame.map(|f| f.file_name.as_ref()), + "lineNumber": frame.map(|f| f.line_number.as_ref()), + "columnNumber": frame.map(|f| f.column_number.as_ref()), + }) + } None => json!({ "message": error.to_string(), }), @@ -320,6 +324,7 @@ pub struct WebWorkerOptions { pub module_loader: Rc, pub create_web_worker_cb: Arc, pub preload_module_cb: Arc, + pub source_map_getter: Option>, pub js_error_create_fn: Option>, pub use_deno_namespace: bool, pub worker_type: WebWorkerType, @@ -436,6 +441,7 @@ impl WebWorker { let mut js_runtime = JsRuntime::new(RuntimeOptions { module_loader: Some(options.module_loader.clone()), startup_snapshot: Some(js::deno_isolate_init()), + source_map_getter: options.source_map_getter, js_error_create_fn: options.js_error_create_fn.clone(), get_error_class_fn: options.get_error_class_fn, shared_array_buffer_store: options.shared_array_buffer_store.clone(), diff --git a/runtime/worker.rs b/runtime/worker.rs index 983f174aa7..fa147a7e66 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -20,6 +20,7 @@ use deno_core::ModuleLoader; use deno_core::ModuleSpecifier; use deno_core::RuntimeOptions; use deno_core::SharedArrayBufferStore; +use deno_core::SourceMapGetter; use deno_tls::rustls::RootCertStore; use deno_web::BlobStore; use log::debug; @@ -54,6 +55,7 @@ pub struct WorkerOptions { // Callbacks invoked when creating new instance of WebWorker pub create_web_worker_cb: Arc, pub web_worker_preload_module_cb: Arc, + pub source_map_getter: Option>, pub js_error_create_fn: Option>, pub maybe_inspector_server: Option>, pub should_break_on_first_statement: bool, @@ -155,6 +157,7 @@ impl MainWorker { let mut js_runtime = JsRuntime::new(RuntimeOptions { module_loader: Some(options.module_loader.clone()), startup_snapshot: Some(js::deno_isolate_init()), + source_map_getter: options.source_map_getter, js_error_create_fn: options.js_error_create_fn.clone(), get_error_class_fn: options.get_error_class_fn, shared_array_buffer_store: options.shared_array_buffer_store.clone(), @@ -357,7 +360,6 @@ mod tests { let options = WorkerOptions { bootstrap: BootstrapOptions { - apply_source_maps: false, args: vec![], cpu_count: 1, debug_flag: false, @@ -374,6 +376,7 @@ mod tests { unsafely_ignore_certificate_errors: None, root_cert_store: None, seed: None, + source_map_getter: None, js_error_create_fn: None, web_worker_preload_module_cb: Arc::new(|_| unreachable!()), create_web_worker_cb: Arc::new(|_| unreachable!()), diff --git a/runtime/worker_bootstrap.rs b/runtime/worker_bootstrap.rs index 05bde731f9..f1ffd1b3db 100644 --- a/runtime/worker_bootstrap.rs +++ b/runtime/worker_bootstrap.rs @@ -8,7 +8,6 @@ use deno_core::ModuleSpecifier; pub struct BootstrapOptions { /// Sets `Deno.args` in JS runtime. pub args: Vec, - pub apply_source_maps: bool, pub cpu_count: usize, pub debug_flag: bool, pub enable_testing_features: bool, @@ -28,7 +27,6 @@ impl BootstrapOptions { let payload = json!({ // Shared bootstrap args "args": self.args, - "applySourceMaps": self.apply_source_maps, "cpuCount": self.cpu_count, "debugFlag": self.debug_flag, "denoVersion": self.runtime_version,