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

Add Function::{ScriptId, GetScriptOrigin}, ScriptOrigin::Get{ScriptId, ResourceName, SourceMapUrl} bindings #1250

Merged
merged 6 commits into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
24 changes: 24 additions & 0 deletions src/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1952,6 +1952,16 @@ int v8__Function__GetScriptLineNumber(const v8::Function& self) {
return ptr_to_local(&self)->GetScriptLineNumber();
}

int v8__Function__ScriptId(const v8::Function& self) {
return ptr_to_local(&self)->ScriptId();
}

const v8::ScriptOrigin* v8__Function__GetScriptOrigin(const v8::Function& self) {
std::unique_ptr<v8::ScriptOrigin> u =
std::make_unique<v8::ScriptOrigin>(ptr_to_local(&self)->GetScriptOrigin());
return u.release();
}

const v8::Signature* v8__Signature__New(v8::Isolate* isolate,
const v8::FunctionTemplate* templ) {
return local_to_ptr(v8::Signature::New(isolate, ptr_to_local(templ)));
Expand Down Expand Up @@ -2280,6 +2290,20 @@ void v8__ScriptOrigin__CONSTRUCT(
ptr_to_local(&source_map_url), resource_is_opaque, is_wasm, is_module);
}

int v8__ScriptOrigin__GetScriptId(const v8::ScriptOrigin& self) {
return ptr_to_local(&self)->ScriptId();
}

const v8::Value* v8__ScriptOrigin__GetResourceName(
const v8::ScriptOrigin& self) {
return local_to_ptr(ptr_to_local(&self)->ResourceName());
}

const v8::Value* v8__ScriptOrigin__GetSourceMapUrl(
const v8::ScriptOrigin& self) {
return local_to_ptr(ptr_to_local(&self)->SourceMapUrl());
}

const v8::Value* v8__ScriptOrModule__GetResourceName(
const v8::ScriptOrModule& self) {
return local_to_ptr(ptr_to_local(&self)->GetResourceName());
Expand Down
24 changes: 23 additions & 1 deletion src/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::support::MapFnTo;
use crate::support::ToCFn;
use crate::support::UnitType;
use crate::support::{int, Opaque};
use crate::undefined;
use crate::Context;
use crate::Function;
use crate::HandleScope;
Expand All @@ -23,6 +22,7 @@ use crate::Signature;
use crate::String;
use crate::UniqueRef;
use crate::Value;
use crate::{undefined, ScriptOrigin};

extern "C" {
fn v8__Function__New(
Expand Down Expand Up @@ -50,6 +50,10 @@ extern "C" {
fn v8__Function__SetName(this: *const Function, name: *const String);
fn v8__Function__GetScriptColumnNumber(this: *const Function) -> int;
fn v8__Function__GetScriptLineNumber(this: *const Function) -> int;
fn v8__Function__ScriptId(this: *const Function) -> int;
fn v8__Function__GetScriptOrigin(
this: *const Function,
) -> *const ScriptOrigin<'static>;

fn v8__Function__CreateCodeCache(
script: *const Function,
Expand Down Expand Up @@ -903,6 +907,24 @@ impl Function {
(ret >= 0).then_some(ret as u32)
}

#[inline(always)]
pub fn get_script_origin(&self) -> &ScriptOrigin {
unsafe {
let ptr = v8__Function__GetScriptOrigin(self);
&*ptr
}
}

/// Returns scriptId.
#[inline(always)]
pub fn get_script_id(&self) -> Option<u32> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be i32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was unsure about this. V8 docs are sparse on when a script ID is < 0, but the only cases I could find in the source were 1) uninitialized ScriptOrigin classes (value is -1) and 2) kTemporaryScriptId = -2. Due to uncertainty on when these cases actually occur, I just treated these as None values.

I can change this to i32 instead if you think that would make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmastrac Would you suggest i32 instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should use i32 here. Also please remove get_ prefix - in v8/include/v8-function.h this function is named ScriptId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsafe {
let ret = v8__Function__ScriptId(self);
// kTemporaryScriptId == -2 and kNoScriptId == 0
(ret > 0).then_some(ret as u32)
}
}

/// Creates and returns code cache for the specified unbound_script.
/// This will return nullptr if the script cannot be serialized. The
/// CachedData returned by this function should be owned by the caller.
Expand Down
31 changes: 31 additions & 0 deletions src/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ extern "C" {
is_wasm: bool,
is_module: bool,
);
fn v8__ScriptOrigin__GetScriptId(origin: *const ScriptOrigin) -> i32;
fn v8__ScriptOrigin__GetResourceName(
origin: *const ScriptOrigin,
) -> *const Value;
fn v8__ScriptOrigin__GetSourceMapUrl(
origin: *const ScriptOrigin,
) -> *const Value;
}

impl Script {
Expand Down Expand Up @@ -125,4 +132,28 @@ impl<'s> ScriptOrigin<'s> {
buf.assume_init()
}
}

#[inline(always)]
pub fn get_script_id(&self) -> Option<u32> {
rakeeb-hossain marked this conversation as resolved.
Show resolved Hide resolved
unsafe {
let ret = v8__ScriptOrigin__GetScriptId(self as *const _);
(ret > 0).then_some(ret as u32)
}
}

#[inline(always)]
pub fn get_resource_name(&self) -> Option<Local<'s, Value>> {
unsafe {
let ptr = v8__ScriptOrigin__GetResourceName(self);
Local::from_raw(ptr)
}
}

#[inline(always)]
pub fn get_source_map_url(&self) -> Option<Local<'s, Value>> {
rakeeb-hossain marked this conversation as resolved.
Show resolved Hide resolved
unsafe {
let ptr = v8__ScriptOrigin__GetSourceMapUrl(self);
Local::from_raw(ptr)
}
}
}
82 changes: 82 additions & 0 deletions tests/test_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3820,6 +3820,88 @@ export function anotherFunctionG(a, b) {
}
}

#[test]
fn function_script_origin_and_id() {
let _setup_guard = setup::parallel_test();
let isolate = &mut v8::Isolate::new(Default::default());

let scope = &mut v8::HandleScope::new(isolate);
let context = v8::Context::new(scope);
let scope = &mut v8::ContextScope::new(scope, context);

let mut num_cases = 10;
let mut prev_id = None;
while num_cases > 0 {
let resource_name = format!("google.com/{}", num_cases);
let source = mock_source(
scope,
resource_name.as_str(), // make sure each source has a different resource name
r#"export function f(a, b) {
return a;
}

export function anotherFunctionG(a, b) {
return b;
}"#,
);
let module = v8::script_compiler::compile_module(scope, source).unwrap();
let result =
module.instantiate_module(scope, unexpected_module_resolve_callback);
assert!(result.is_some());
module.evaluate(scope).unwrap();
assert_eq!(v8::ModuleStatus::Evaluated, module.get_status());

let namespace = module.get_module_namespace();
assert!(namespace.is_module_namespace_object());
let namespace_obj = namespace.to_object(scope).unwrap();

let f_str = v8::String::new(scope, "f").unwrap();
let f_function_obj: v8::Local<v8::Function> = namespace_obj
.get(scope, f_str.into())
.unwrap()
.try_into()
.unwrap();

// Modules with different resource names will have incrementing script IDs
// but the script ID of the first module is a V8 internal, so should not
// be depended on.
// See https://groups.google.com/g/v8-users/c/iEfceRohiy8 for more discussion.
let script_id = f_function_obj.get_script_id();
assert!(f_function_obj.get_script_id().is_some());
let script_id = script_id.unwrap();

if let Some(id) = prev_id {
assert_eq!(script_id, id + 1);
assert_eq!(
script_id,
f_function_obj.get_script_origin().get_script_id().unwrap()
);
}
prev_id = Some(script_id);

// Verify source map URL matches
assert_eq!(
"source_map_url",
f_function_obj
.get_script_origin()
.get_source_map_url()
.unwrap()
.to_rust_string_lossy(scope)
);

// Verify resource name matches in script origin
let resource_name_val =
f_function_obj.get_script_origin().get_resource_name();
assert!(resource_name_val.is_some());
assert_eq!(
resource_name_val.unwrap().to_rust_string_lossy(scope),
resource_name
);

num_cases -= 1;
}
}

#[test]
fn constructor() {
let _setup_guard = setup::parallel_test();
Expand Down
Loading