fix: use inline source maps when present in js (denoland#8995)

Author: lucacasonato

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

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::<Arc<ProgramState>>().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,
     );

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<Vec<u8>> {
+  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: Send + Sync>(_: S) {}

cli/source_maps.rs

@@ -33,14 +33,15 @@ pub fn apply_source_map<G: SourceMapGetter>(
   // 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<G: SourceMapGetter>(
     }
     _ => 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,28 +74,43 @@ fn get_maybe_orig_position<G: SourceMapGetter>(
   file_name: Option<String>,
   line_number: Option<i64>,
   column_number: Option<i64>,
+  source_line: Option<String>,
   mappings_map: &mut CachedMaps,
   getter: Arc<G>,
-) -> (Option<String>, Option<i64>, Option<i64>) {
+) -> (Option<String>, Option<i64>, Option<i64>, Option<String>) {
   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),
   }
 }
 
 pub fn get_orig_position<G: SourceMapGetter>(
   file_name: String,
   line_number: i64,
   column_number: i64,
+  source_line: Option<String>,
   mappings_map: &mut CachedMaps,
   getter: Arc<G>,
-) -> (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<String>) {
+  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<G: SourceMapGetter>(
         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,
+            )
+          }
         },
       }
     }

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]

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])

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])

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

cli/tests/inline_js_source_map.ts

@@ -0,0 +1,6 @@
+1 + 1;
+interface Test {
+  hello: string;
+}
+
+// throw new Error("Hello world!" as string);

cli/tests/inline_js_source_map_2.js

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

cli/tests/inline_js_source_map_2.js.out

@@ -0,0 +1,6 @@
+1 + 1;
+interface Test {
+  hello: string;
+}
+
+throw new Error("Hello world!" as unknown as string);

cli/tests/inline_js_source_map_2.ts

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

cli/tests/inline_js_source_map_2_with_inline_contents.js

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

cli/tests/inline_js_source_map_2_with_inline_contents.js.out

@@ -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;