Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat: Add native SourceMap support #819

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3d8c9c8
feat: Add native SourceMap support
bartlomieju Jul 11, 2024
2905e0d
update comment
bartlomieju Jul 11, 2024
23cdef9
prototype source mapping url being non-base64 url
bartlomieju Jul 12, 2024
ec8e4f1
start moving source map loading to ModuleLoader
bartlomieju Jul 12, 2024
333c579
Merge branch 'main' into source_map_getter
bartlomieju Jul 12, 2024
72ffd9d
Merge branch 'main' into source_map_getter
bartlomieju Jul 15, 2024
588dc8a
Merge branch 'main' into source_map_getter
bartlomieju Jul 15, 2024
a78caa2
Merge branch 'main' into source_map_getter
bartlomieju Jul 16, 2024
a46cd55
fixes after merge
bartlomieju Jul 16, 2024
0a2821c
rename
bartlomieju Jul 16, 2024
6fc5d1d
lint
bartlomieju Jul 16, 2024
0166243
use ModuleCode static for ext_source_maps
bartlomieju Jul 16, 2024
6bfb77b
more cleanup
bartlomieju Jul 17, 2024
224bcf2
remove a TODO comment
bartlomieju Jul 17, 2024
01bf21c
Merge branch 'source_map_perf' into source_map_getter
bartlomieju Jul 17, 2024
7ff9928
remove setting up `ModuleMap` for `SourceMapper`
bartlomieju Jul 17, 2024
40aff1a
simplify SourceMapper::get_source_line
bartlomieju Jul 17, 2024
84d6c3f
lint
bartlomieju Jul 17, 2024
7a380af
Merge branch 'main' into source_map_getter
bartlomieju Jul 17, 2024
85a9634
fix after merge
bartlomieju Jul 17, 2024
b0bfb5c
simplify error
bartlomieju Jul 17, 2024
a6c6f76
self-review
bartlomieju Jul 17, 2024
126eb6b
add a TODO
bartlomieju Jul 17, 2024
4e4036d
remove base64
bartlomieju Jul 17, 2024
b253371
wip - dcore, can't print source_line in JsError
bartlomieju Jul 17, 2024
a503d80
add transpiler api
bartlomieju Jul 18, 2024
cc573b4
wip upgrade
bartlomieju Jul 18, 2024
f01fbe8
Merge branch 'main' into source_map_getter
bartlomieju Jul 18, 2024
2d4fdb9
Merge remote-tracking branch 'upstream/main' into source_map_getter
bartlomieju Jul 26, 2024
1ccf6c6
Merge branch 'main' into source_map_getter
bartlomieju Jul 29, 2024
0795341
Merge remote-tracking branch 'upstream/main' into source_map_getter
bartlomieju Aug 2, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions bar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
"use strict";

throw new Error("Hello world!");
//# sourceMappingURL=bar.js.map

asd;
11 changes: 11 additions & 0 deletions bar.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ snapshot_flags_eager_parse = []

[dependencies]
anyhow.workspace = true
base64 = "0.22.1"
bincode.workspace = true
bit-set.workspace = true
bit-vec.workspace = true
Expand Down
21 changes: 7 additions & 14 deletions core/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ impl JsStackFrame {
let c = message.get_start_column() as u32 + 1;
let state = JsRuntime::state_from(scope);
let mut source_mapper = state.source_mapper.borrow_mut();
match source_mapper.apply_source_map(&f, l, c) {
match source_mapper.apply_source_map(scope, &f, l, c) {
SourceMapApplication::Unchanged => Some(JsStackFrame::from_location(
Some(f),
Some(l.into()),
Expand Down Expand Up @@ -354,15 +354,12 @@ impl JsError {
{
let state = JsRuntime::state_from(scope);
let mut source_mapper = state.source_mapper.borrow_mut();
for (i, frame) in frames.iter().enumerate() {
if let Some(frame) = frames.first() {
if let (Some(file_name), Some(line_number)) =
(&frame.file_name, frame.line_number)
{
if !file_name.trim_start_matches('[').starts_with("ext:") {
source_line = source_mapper.get_source_line(file_name, line_number);
source_line_frame_index = Some(i);
break;
}
Comment on lines -543 to -547
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify this in a separate PR. We support source maps for ext: modules so most likely this is not needed

source_line = source_mapper.get_source_line(file_name, line_number);
source_line_frame_index = Some(0);
}
}
}
Expand Down Expand Up @@ -475,16 +472,12 @@ impl JsError {
{
let state = JsRuntime::state_from(scope);
let mut source_mapper = state.source_mapper.borrow_mut();
for (i, frame) in frames.iter().enumerate() {
if let Some(frame) = frames.first() {
if let (Some(file_name), Some(line_number)) =
(&frame.file_name, frame.line_number)
{
if !file_name.trim_start_matches('[').starts_with("ext:") {
source_line =
source_mapper.get_source_line(file_name, line_number);
source_line_frame_index = Some(i);
break;
}
source_line = source_mapper.get_source_line(file_name, line_number);
source_line_frame_index = Some(0);
}
}
}
Expand Down
13 changes: 9 additions & 4 deletions core/ops_builtin_v8.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1047,9 +1047,10 @@ fn write_line_and_col_to_ret_buf(
// 2: mapped line, column, and file name. new line, column, and file name are in
// ret_buf. retrieve file name by calling `op_apply_source_map_filename`
// immediately after this op returns.
#[op2(fast)]
#[op2]
#[smi]
pub fn op_apply_source_map(
scope: &mut v8::HandleScope,
state: &JsRuntimeState,
#[string] file_name: &str,
#[smi] line_number: u32,
Expand All @@ -1060,8 +1061,12 @@ pub fn op_apply_source_map(
return Err(type_error("retBuf must be 8 bytes"));
}
let mut source_mapper = state.source_mapper.borrow_mut();
let application =
source_mapper.apply_source_map(file_name, line_number, column_number);
let application = source_mapper.apply_source_map(
scope,
file_name,
line_number,
column_number,
);
match application {
SourceMapApplication::Unchanged => Ok(0),
SourceMapApplication::LineAndColumn {
Expand Down Expand Up @@ -1137,7 +1142,7 @@ pub fn op_current_user_call_site(
let application = js_runtime_state
.source_mapper
.borrow_mut()
.apply_source_map(&file_name, line_number, column_number);
.apply_source_map(scope, &file_name, line_number, column_number);

match application {
SourceMapApplication::Unchanged => {
Expand Down
126 changes: 122 additions & 4 deletions core/source_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@
// TODO(bartlomieju): remove once `SourceMapGetter` is removed.
#![allow(deprecated)]

use crate::module_specifier::ModuleResolutionError;
use crate::resolve_import;
use crate::resolve_url;
use crate::runtime::JsRealm;
use crate::ModuleLoader;
use crate::ModuleName;
use crate::RequestedModuleType;
use base64::prelude::BASE64_STANDARD;
use base64::Engine;
pub use sourcemap::SourceMap;
use std::borrow::Cow;
use std::collections::HashMap;
Expand Down Expand Up @@ -89,13 +95,93 @@ impl SourceMapper {
std::mem::take(&mut self.ext_source_maps)
}

pub fn apply_source_map_from_module_map(
&mut self,
scope: &mut v8::HandleScope,
file_name: &str,
line_number: u32,
column_number: u32,
) -> Option<SourceMapApplication> {
let module_map_rc = JsRealm::module_map_from(scope);
let id = module_map_rc.get_id(file_name, RequestedModuleType::None)?;

let module_handle = module_map_rc.get_handle(id).unwrap();
let module = v8::Local::new(scope, module_handle);
let unbound_module_script = module.get_unbound_module_script(scope);
let maybe_source_mapping_url =
unbound_module_script.get_source_mapping_url(scope);

// TODO(bartlomieju): This should be the last fallback and it's only useful
// for eval - probably for `Deno.core.evalContext()`.
// let maybe_source_url = unbound_module_script
// .get_source_url(scope)
// .to_rust_string_lossy(scope);
// eprintln!("maybe source url {}", maybe_source_url);

if !maybe_source_mapping_url.is_string() {
return None;
}

let source_map_string =
maybe_source_mapping_url.to_rust_string_lossy(scope);
// eprintln!("maybe source mapping url {}", source_map_string);

// TODO(bartlomieju): this is a fast path - if it fails, we should try to parse
// the URL (or resolve it from the current file being mapped) and fallback to
// acquiring a source map from that URL. In Deno we might want to apply permissions
// checks for fetching the map.
let source_map = if let Some(b64) =
source_map_string.strip_prefix("data:application/json;base64,")
{
let decoded_b64 = BASE64_STANDARD.decode(b64).ok()?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is duplicated in CLI, maybe we can have a common helper shared between the two?

// eprintln!(
// "source map {:?}",
// String::from_utf8(decoded_b64.clone()).unwrap()
// );
SourceMap::from_slice(&decoded_b64).ok()?
} else {
let url = match resolve_import(&source_map_string, file_name) {
Ok(url) => Some(url),
Err(err) => match err {
ModuleResolutionError::ImportPrefixMissing(_, _) => {
resolve_import(&format!("./{}", source_map_string), file_name).ok()
}
_ => None,
},
};
// eprintln!(
// "source map url sourceMappingURL={} file_name={} url={}",
// source_map_string,
// file_name,
// url.as_ref().map(|s| s.as_str()).unwrap_or_default()
// );
let url = url?;
if url.scheme() != "file" {
return None;
}
let contents = module_map_rc
.loader
.borrow()
.get_source_map(url.to_file_path().ok()?.to_str()?)?;
SourceMap::from_slice(&contents).ok()?
};

Some(Self::compute_application(
&source_map,
file_name,
line_number,
column_number,
))
}

/// Apply a source map to the passed location. If there is no source map for
/// this location, or if the location remains unchanged after mapping, the
/// changed values are returned.
///
/// Line and column numbers are 1-based.
pub fn apply_source_map(
&mut self,
scope: &mut v8::HandleScope,
file_name: &str,
line_number: u32,
column_number: u32,
Expand All @@ -104,6 +190,17 @@ impl SourceMapper {
let line_number = line_number - 1;
let column_number = column_number - 1;

// TODO(bartlomieju): requires scope and should only be called in a fallback op,
// that will access scope if the fast op doesn't return anything.
if let Some(app) = self.apply_source_map_from_module_map(
scope,
file_name,
line_number,
column_number,
) {
return app;
}

let getter = self.getter.as_ref();
let maybe_source_map =
self.maps.entry(file_name.to_owned()).or_insert_with(|| {
Expand All @@ -123,6 +220,15 @@ impl SourceMapper {
return SourceMapApplication::Unchanged;
};

Self::compute_application(source_map, file_name, line_number, column_number)
}

fn compute_application(
source_map: &SourceMap,
file_name: &str,
line_number: u32,
column_number: u32,
) -> SourceMapApplication {
let Some(token) = source_map.lookup_token(line_number, column_number)
else {
return SourceMapApplication::Unchanged;
Expand Down Expand Up @@ -212,11 +318,13 @@ mod tests {

use super::*;
use crate::ascii_str;
use crate::JsRuntime;
use crate::ModuleCodeString;
use crate::ModuleLoadResponse;
use crate::ModuleSpecifier;
use crate::RequestedModuleType;
use crate::ResolutionKind;
use crate::RuntimeOptions;

struct SourceMapLoaderContent {
source_map: Option<ModuleCodeString>,
Expand Down Expand Up @@ -279,19 +387,29 @@ mod tests {
},
);

let mut source_mapper = SourceMapper::new(Rc::new(loader), None);
let loader = Rc::new(loader);

let mut js_runtime = JsRuntime::new(RuntimeOptions {
module_loader: Some(loader.clone()),
..Default::default()
});
let state = JsRuntime::state_from(js_runtime.v8_isolate());
let scope = &mut js_runtime.handle_scope();
let mut source_mapper = state.source_mapper.borrow_mut();

// Non-existent file
let application =
source_mapper.apply_source_map("file:///doesnt_exist.js", 1, 1);
source_mapper.apply_source_map(scope, "file:///doesnt_exist.js", 1, 1);
assert_eq!(application, SourceMapApplication::Unchanged);

// File with no source map
let application = source_mapper.apply_source_map("file:///b.js", 1, 1);
let application =
source_mapper.apply_source_map(scope, "file:///b.js", 1, 1);
assert_eq!(application, SourceMapApplication::Unchanged);

// File with a source map
let application = source_mapper.apply_source_map("file:///a.ts", 1, 21);
let application =
source_mapper.apply_source_map(scope, "file:///a.ts", 1, 21);
assert_eq!(
application,
SourceMapApplication::LineAndColumn {
Expand Down
6 changes: 6 additions & 0 deletions foo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
"use strict";

throw new Error("Hello world!");
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L3J1bi9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sInNvdXJjZXNDb250ZW50IjpbIjErMTtcbmludGVyZmFjZSBUZXN0IHtcbiAgaGVsbG86IHN0cmluZztcbn1cblxudGhyb3cgbmV3IEVycm9yKFwiSGVsbG8gd29ybGQhXCIgYXMgdW5rbm93biBhcyBzdHJpbmcpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ==

asd;
Loading