From 39bbbbce70c13575857b216b79ec21c37a923760 Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Wed, 6 Jan 2021 00:10:36 +0100 Subject: [PATCH] fix: use inline source maps when present in js (#8995) --- .dprintrc.json | 1 + cli/ops/errors.rs | 3 +- cli/program_state.rs | 40 +++++---- cli/source_maps.rs | 82 ++++++++++++------- cli/tests/059_fs_relative_path_perm.ts.out | 2 + cli/tests/073_worker_error.ts.out | 2 + cli/tests/074_worker_nested_error.ts.out | 2 + .../fmt/expected_fmt_check_tests_dir.out | 2 +- cli/tests/inline_js_source_map.ts | 6 ++ cli/tests/inline_js_source_map_2.js | 4 + cli/tests/inline_js_source_map_2.js.out | 4 + cli/tests/inline_js_source_map_2.ts | 6 ++ ...ne_js_source_map_2_with_inline_contents.js | 4 + ...s_source_map_2_with_inline_contents.js.out | 4 + ..._js_source_map_with_contents_from_graph.js | 4 + ...source_map_with_contents_from_graph.js.out | 4 + cli/tests/integration_tests.rs | 35 ++++++++ 17 files changed, 157 insertions(+), 48 deletions(-) create mode 100644 cli/tests/inline_js_source_map.ts create mode 100644 cli/tests/inline_js_source_map_2.js create mode 100644 cli/tests/inline_js_source_map_2.js.out create mode 100644 cli/tests/inline_js_source_map_2.ts create mode 100644 cli/tests/inline_js_source_map_2_with_inline_contents.js create mode 100644 cli/tests/inline_js_source_map_2_with_inline_contents.js.out create mode 100644 cli/tests/inline_js_source_map_with_contents_from_graph.js create mode 100644 cli/tests/inline_js_source_map_with_contents_from_graph.js.out diff --git a/.dprintrc.json b/.dprintrc.json index 9217366411..5658f9bc78 100644 --- a/.dprintrc.json +++ b/.dprintrc.json @@ -21,6 +21,7 @@ "cli/dts/lib.webworker*.d.ts", "cli/dts/typescript.d.ts", "cli/tests/encoding", + "cli/tests/inline_js_source_map*", "cli/tsc/*typescript.js", "test_util/wpt", "gh-pages", diff --git a/cli/ops/errors.rs b/cli/ops/errors.rs index d9893b0ef8..067e901ea8 100644 --- a/cli/ops/errors.rs +++ b/cli/ops/errors.rs @@ -37,11 +37,12 @@ fn op_apply_source_map( let mut mappings_map: CachedMaps = HashMap::new(); let program_state = state.borrow::>().clone(); - let (orig_file_name, orig_line_number, orig_column_number) = + let (orig_file_name, orig_line_number, orig_column_number, _) = get_orig_position( args.file_name, args.line_number.into(), args.column_number.into(), + None, &mut mappings_map, program_state, ); diff --git a/cli/program_state.rs b/cli/program_state.rs index 5eda6b3fa1..dfe8aa6c3b 100644 --- a/cli/program_state.rs +++ b/cli/program_state.rs @@ -300,24 +300,10 @@ impl SourceMapGetter for ProgramState { maybe_map } else { let code = String::from_utf8(code).unwrap(); - let lines: Vec<&str> = code.split('\n').collect(); - if let Some(last_line) = lines.last() { - if last_line - .starts_with("//# sourceMappingURL=data:application/json;base64,") - { - let input = last_line.trim_start_matches( - "//# sourceMappingURL=data:application/json;base64,", - ); - let decoded_map = base64::decode(input) - .expect("Unable to decode source map from emitted file."); - Some(decoded_map) - } else { - None - } - } else { - None - } + source_map_from_code(code) } + } else if let Ok(source) = self.load(specifier, None) { + source_map_from_code(source.code) } else { None } @@ -345,6 +331,26 @@ impl SourceMapGetter for ProgramState { } } +fn source_map_from_code(code: String) -> Option> { + let lines: Vec<&str> = code.split('\n').collect(); + if let Some(last_line) = lines.last() { + if last_line + .starts_with("//# sourceMappingURL=data:application/json;base64,") + { + let input = last_line.trim_start_matches( + "//# sourceMappingURL=data:application/json;base64,", + ); + let decoded_map = base64::decode(input) + .expect("Unable to decode source map from emitted file."); + Some(decoded_map) + } else { + None + } + } else { + None + } +} + #[test] fn thread_safe() { fn f(_: S) {} diff --git a/cli/source_maps.rs b/cli/source_maps.rs index 318665e9de..26ba4ca294 100644 --- a/cli/source_maps.rs +++ b/cli/source_maps.rs @@ -33,14 +33,15 @@ pub fn apply_source_map( // prepareStackTrace(). let mut mappings_map: CachedMaps = HashMap::new(); - let (script_resource_name, line_number, start_column) = + 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), + js_error.source_line.clone(), &mut mappings_map, - getter.clone(), + getter, ); let start_column = start_column.map(|n| n - 1); // It is better to just move end_column to be the same distance away from @@ -56,20 +57,6 @@ pub fn apply_source_map( } _ => None, }; - // if there is a source line that we might be different in the source file, we - // will go fetch it from the getter - let source_line = match line_number { - Some(ln) - if js_error.source_line.is_some() && script_resource_name.is_some() => - { - getter.get_source_line( - &js_error.script_resource_name.clone().unwrap(), - // Getter expects 0-based line numbers, but ours are 1-based. - ln as usize - 1, - ) - } - _ => js_error.source_line.clone(), - }; JsError { message: js_error.message.clone(), @@ -87,16 +74,29 @@ fn get_maybe_orig_position( file_name: Option, line_number: Option, column_number: Option, + source_line: Option, mappings_map: &mut CachedMaps, getter: Arc, -) -> (Option, Option, Option) { +) -> (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) = - get_orig_position(file_name_v, line_v, column_v, mappings_map, getter); - (Some(file_name), Some(line_number), Some(column_number)) + let (file_name, line_number, column_number, source_line) = + get_orig_position( + file_name_v, + line_v, + column_v, + source_line, + mappings_map, + getter, + ); + ( + Some(file_name), + Some(line_number), + Some(column_number), + source_line, + ) } - _ => (None, None, None), + _ => (None, None, None, source_line), } } @@ -104,11 +104,13 @@ pub fn get_orig_position( file_name: String, line_number: i64, column_number: i64, + source_line: Option, mappings_map: &mut CachedMaps, getter: Arc, -) -> (String, i64, i64) { - let maybe_source_map = get_mappings(&file_name, mappings_map, getter); - let default_pos = (file_name, line_number, column_number); +) -> (String, i64, i64, Option) { + let maybe_source_map = get_mappings(&file_name, mappings_map, getter.clone()); + let default_pos = + (file_name, line_number, column_number, source_line.clone()); // Lookup expects 0-based line and column numbers, but ours are 1-based. let line_number = line_number - 1; @@ -121,11 +123,33 @@ pub fn get_orig_position( None => default_pos, Some(token) => match token.get_source() { None => default_pos, - Some(original) => ( - original.to_string(), - i64::from(token.get_src_line()) + 1, - i64::from(token.get_src_col()) + 1, - ), + Some(original) => { + let maybe_source_line = + if let Some(source_view) = token.get_source_view() { + source_view.get_line(token.get_src_line()) + } else { + None + }; + + let source_line = if let Some(source_line) = maybe_source_line { + Some(source_line.to_string()) + } else if let Some(source_line) = getter.get_source_line( + original, + // Getter expects 0-based line numbers, but ours are 1-based. + token.get_src_line() as usize, + ) { + Some(source_line) + } else { + source_line + }; + + ( + original.to_string(), + i64::from(token.get_src_line()) + 1, + i64::from(token.get_src_col()) + 1, + source_line, + ) + } }, } } diff --git a/cli/tests/059_fs_relative_path_perm.ts.out b/cli/tests/059_fs_relative_path_perm.ts.out index 83df419039..92d6b5966c 100644 --- a/cli/tests/059_fs_relative_path_perm.ts.out +++ b/cli/tests/059_fs_relative_path_perm.ts.out @@ -1,2 +1,4 @@ [WILDCARD]error: Uncaught PermissionDenied: read access to "non-existent", run again with the --allow-read flag + throw new ErrorClass(res.err.message); + ^ at [WILDCARD] diff --git a/cli/tests/073_worker_error.ts.out b/cli/tests/073_worker_error.ts.out index 244e564175..e464789792 100644 --- a/cli/tests/073_worker_error.ts.out +++ b/cli/tests/073_worker_error.ts.out @@ -2,4 +2,6 @@ at foo ([WILDCARD]) at [WILDCARD] error: Uncaught (in promise) Error: Unhandled error event reached main worker. + throw new Error("Unhandled error event reached main worker."); + ^ at Worker.#poll ([WILDCARD]) diff --git a/cli/tests/074_worker_nested_error.ts.out b/cli/tests/074_worker_nested_error.ts.out index 244e564175..e464789792 100644 --- a/cli/tests/074_worker_nested_error.ts.out +++ b/cli/tests/074_worker_nested_error.ts.out @@ -2,4 +2,6 @@ at foo ([WILDCARD]) at [WILDCARD] error: Uncaught (in promise) Error: Unhandled error event reached main worker. + throw new Error("Unhandled error event reached main worker."); + ^ at Worker.#poll ([WILDCARD]) diff --git a/cli/tests/fmt/expected_fmt_check_tests_dir.out b/cli/tests/fmt/expected_fmt_check_tests_dir.out index e78b42db32..00d7cb3fa4 100644 --- a/cli/tests/fmt/expected_fmt_check_tests_dir.out +++ b/cli/tests/fmt/expected_fmt_check_tests_dir.out @@ -1,2 +1,2 @@ [WILDCARD] -error: Found 2 not formatted files in [WILDCARD] files +error: Found 5 not formatted files in [WILDCARD] files diff --git a/cli/tests/inline_js_source_map.ts b/cli/tests/inline_js_source_map.ts new file mode 100644 index 0000000000..5ae7c226a8 --- /dev/null +++ b/cli/tests/inline_js_source_map.ts @@ -0,0 +1,6 @@ +1 + 1; +interface Test { + hello: string; +} + +// throw new Error("Hello world!" as string); diff --git a/cli/tests/inline_js_source_map_2.js b/cli/tests/inline_js_source_map_2.js new file mode 100644 index 0000000000..199ff7bb0b --- /dev/null +++ b/cli/tests/inline_js_source_map_2.js @@ -0,0 +1,4 @@ +"use strict"; +1 + 1; +throw new Error("Hello world!"); +//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L2NsaS90ZXN0cy9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ== \ No newline at end of file diff --git a/cli/tests/inline_js_source_map_2.js.out b/cli/tests/inline_js_source_map_2.js.out new file mode 100644 index 0000000000..008eb06573 --- /dev/null +++ b/cli/tests/inline_js_source_map_2.js.out @@ -0,0 +1,4 @@ +error: Uncaught Error: Hello world! +throw new Error("Hello world!"); + ^ + at http://localhost:4545/cli/tests/inline_js_source_map_2.ts:6:7 diff --git a/cli/tests/inline_js_source_map_2.ts b/cli/tests/inline_js_source_map_2.ts new file mode 100644 index 0000000000..fa50586e62 --- /dev/null +++ b/cli/tests/inline_js_source_map_2.ts @@ -0,0 +1,6 @@ +1 + 1; +interface Test { + hello: string; +} + +throw new Error("Hello world!" as unknown as string); diff --git a/cli/tests/inline_js_source_map_2_with_inline_contents.js b/cli/tests/inline_js_source_map_2_with_inline_contents.js new file mode 100644 index 0000000000..a225b3975e --- /dev/null +++ b/cli/tests/inline_js_source_map_2_with_inline_contents.js @@ -0,0 +1,4 @@ +"use strict"; + +throw new Error("Hello world!"); +//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L2NsaS90ZXN0cy9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sInNvdXJjZXNDb250ZW50IjpbIjErMTtcbmludGVyZmFjZSBUZXN0IHtcbiAgaGVsbG86IHN0cmluZztcbn1cblxudGhyb3cgbmV3IEVycm9yKFwiSGVsbG8gd29ybGQhXCIgYXMgdW5rbm93biBhcyBzdHJpbmcpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ== \ No newline at end of file diff --git a/cli/tests/inline_js_source_map_2_with_inline_contents.js.out b/cli/tests/inline_js_source_map_2_with_inline_contents.js.out new file mode 100644 index 0000000000..c123de41e1 --- /dev/null +++ b/cli/tests/inline_js_source_map_2_with_inline_contents.js.out @@ -0,0 +1,4 @@ +error: Uncaught Error: Hello world! +throw new Error("Hello world!" as unknown as string); + ^ + at http://localhost:4545/cli/tests/inline_js_source_map_2.ts:6:7 diff --git a/cli/tests/inline_js_source_map_with_contents_from_graph.js b/cli/tests/inline_js_source_map_with_contents_from_graph.js new file mode 100644 index 0000000000..5324e0ed24 --- /dev/null +++ b/cli/tests/inline_js_source_map_with_contents_from_graph.js @@ -0,0 +1,4 @@ +"use strict"; +import "http://localhost:4545/cli/tests/inline_js_source_map.ts"; +throw new Error("Hello world!"); +//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L2NsaS90ZXN0cy9pbmxpbmVfanNfc291cmNlX21hcC50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiO0FBQUEsQ0FBQyxHQUFDLENBQUMsQ0FBQztBQUtKLE1BQU0sSUFBSSxLQUFLLENBQUMsY0FBK0IsQ0FBQyxDQUFDIn0= \ No newline at end of file diff --git a/cli/tests/inline_js_source_map_with_contents_from_graph.js.out b/cli/tests/inline_js_source_map_with_contents_from_graph.js.out new file mode 100644 index 0000000000..a0ba9fe982 --- /dev/null +++ b/cli/tests/inline_js_source_map_with_contents_from_graph.js.out @@ -0,0 +1,4 @@ +error: Uncaught Error: Hello world! +// throw new Error("Hello world!" as string); + ^ + at http://localhost:4545/cli/tests/inline_js_source_map.ts:6:7 diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index ef50153018..f4081a7ea6 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -3446,6 +3446,41 @@ itest!(local_sources_not_cached_in_memory { output: "no_mem_cache.js.out", }); +// This test checks that inline source map data is used. It uses a hand crafted +// source map that maps to a file that exists, but is not loaded into the module +// graph (inline_js_source_map_2.ts) (because there are no direct dependencies). +// Source line is not remapped because no inline source contents are included in +// the sourcemap and the file is not present in the dependency graph. +itest!(inline_js_source_map_2 { + args: "run --quiet inline_js_source_map_2.js", + output: "inline_js_source_map_2.js.out", + exit_code: 1, +}); + +// This test checks that inline source map data is used. It uses a hand crafted +// source map that maps to a file that exists, but is not loaded into the module +// graph (inline_js_source_map_2.ts) (because there are no direct dependencies). +// Source line remapped using th inline source contents that are included in the +// inline source map. +itest!(inline_js_source_map_2_with_inline_contents { + args: "run --quiet inline_js_source_map_2_with_inline_contents.js", + output: "inline_js_source_map_2_with_inline_contents.js.out", + exit_code: 1, +}); + +// This test checks that inline source map data is used. It uses a hand crafted +// source map that maps to a file that exists, and is loaded into the module +// graph because of a direct import statement (inline_js_source_map.ts). The +// source map was generated from an earlier version of this file, where the throw +// was not commented out. The source line is remapped using source contents that +// from the module graph. +itest!(inline_js_source_map_with_contents_from_graph { + args: "run --quiet inline_js_source_map_with_contents_from_graph.js", + output: "inline_js_source_map_with_contents_from_graph.js.out", + exit_code: 1, + http_server: true, +}); + #[test] fn cafile_env_fetch() { use deno_core::url::Url;