Skip to content

Commit e6c548d

Browse files
divybotlittledivy
andauthored
fix(ext/node): implement displayErrors for vm scripts (#33942)
## Summary - Implements the `displayErrors` option for vm scripts in `ext/node/ops/vm.rs` - When `displayErrors` is true (the default), decorates the thrown error's `stack` property with source location info — `filename:line\nsource_line\narrow\n\n` — prepended to the existing stack, matching Node.js behaviour - Adds `validateObject(options, "options")` to `vm.compileFunction` so the test's invalid-options assertions throw with the expected `ERR_INVALID_ARG_TYPE` message - Enables `parallel/test-vm-basic.js` in `tests/node_compat/config.jsonc` Closes denoland/orchid#3 Diff is fully self-contained: 51 lines in `ext/node/ops/vm.rs` + 1 line in `ext/node/polyfills/vm.js` + 1 line in `tests/node_compat/config.jsonc`. No follow-up needed; everything the enabled test requires is in this PR. Co-authored-by: Divy Srivastava <me@littledivy.com>
1 parent 6a70eb7 commit e6c548d

3 files changed

Lines changed: 52 additions & 1 deletion

File tree

ext/node/ops/vm.rs

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ impl ContextifyScript {
179179
scope: &mut v8::PinScope<'s, 'i>,
180180
context: v8::Local<'s, v8::Context>,
181181
timeout: i64,
182-
_display_errors: bool,
182+
display_errors: bool,
183183
_break_on_sigint: bool,
184184
microtask_queue: Option<&v8::MicrotaskQueue>,
185185
) -> Option<v8::Local<'s, v8::Value>> {
@@ -252,6 +252,55 @@ impl ContextifyScript {
252252
}
253253

254254
if scope.has_caught() {
255+
// When displayErrors is true (the default), decorate the error's stack
256+
// with source location info prepended, matching Node.js behaviour.
257+
if display_errors
258+
&& !scope.has_terminated()
259+
&& let Some(msg) = scope.message()
260+
&& let Some(exception) = scope.exception()
261+
&& let Ok(exc_obj) = v8::Local::<v8::Object>::try_from(exception)
262+
{
263+
let filename = if let Some(v) = msg.get_script_resource_name(scope) {
264+
if let Some(s) = v.to_string(scope) {
265+
s.to_rust_string_lossy(scope)
266+
} else {
267+
String::new()
268+
}
269+
} else {
270+
String::new()
271+
};
272+
let line_number = msg.get_line_number(scope).unwrap_or(1);
273+
let source_line = if let Some(s) = msg.get_source_line(scope) {
274+
s.to_rust_string_lossy(scope)
275+
} else {
276+
String::new()
277+
};
278+
if !filename.is_empty() {
279+
let start_col = msg.get_start_column();
280+
let arrow = format!("{}^", " ".repeat(start_col));
281+
let preamble = format!(
282+
"{}:{}\n{}\n{}\n\n",
283+
filename, line_number, source_line, arrow
284+
);
285+
286+
let stack_key =
287+
v8::String::new_external_onebyte_static(scope, b"stack").unwrap();
288+
let current_stack =
289+
if let Some(v) = exc_obj.get(scope, stack_key.into()) {
290+
if let Some(s) = v.to_string(scope) {
291+
s.to_rust_string_lossy(scope)
292+
} else {
293+
String::new()
294+
}
295+
} else {
296+
String::new()
297+
};
298+
let new_stack = format!("{}{}", preamble, current_stack);
299+
if let Some(new_stack_str) = v8::String::new(scope, &new_stack) {
300+
exc_obj.set(scope, stack_key.into(), new_stack_str.into());
301+
}
302+
}
303+
}
255304
// If there was an exception thrown during script execution, re-throw it.
256305
if !scope.has_terminated() {
257306
scope.rethrow();

ext/node/polyfills/vm.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,7 @@ function compileFunction(code, params, options = { __proto__: null }) {
356356
if (params !== undefined) {
357357
validateStringArray(params, "params");
358358
}
359+
validateObject(options, "options");
359360
const {
360361
filename = "",
361362
columnOffset = 0,

tests/node_compat/config.jsonc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3640,6 +3640,7 @@
36403640
"parallel/test-vm-access-process-env.js": {},
36413641
"parallel/test-vm-api-handles-getter-errors.js": {},
36423642
"parallel/test-vm-attributes-property-not-on-sandbox.js": {},
3643+
"parallel/test-vm-basic.js": {},
36433644
"parallel/test-vm-codegen.js": {},
36443645
"parallel/test-vm-context-async-script.js": {},
36453646
"parallel/test-vm-context-property-forwarding.js": {},

0 commit comments

Comments
 (0)