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.

Sign up for GitHub

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

Jump to bottom

Rewrite "prepareStackTrace" to use Rust API #7644

Closed
bartlomieju opened this issue Sep 23, 2020 · 1 comment
Closed

Rewrite "prepareStackTrace" to use Rust API #7644

bartlomieju opened this issue Sep 23, 2020 · 1 comment
Assignees
@bartlomieju
Labels
cli related to cli/ dir deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted)

Comments

@bartlomieju
Copy link
Member

Currently CLI uses Error.prepareStackTrace API to perform source mapping and formatting of errors.

This situation leads to overall complexity of this method as well as requirement to manually acquire information about stack frames as well as distributing the logic between JS and Rust.

We want to avoid jumping between JS and Rust as much as possible and to solve this problem in this situation we need to:

  1. Add new APIs to V8's StackFrame to acquire the same data as using CallSite:

deno/cli/rt/40_error_stack.js

Lines 172 to 191 in d68fb81

function evaluateCallSite(callSite) {
return {
this: callSite.getThis(),
typeName: callSite.getTypeName(),
function: callSite.getFunction(),
functionName: callSite.getFunctionName(),
methodName: callSite.getMethodName(),
fileName: callSite.getFileName(),
lineNumber: callSite.getLineNumber(),
columnNumber: callSite.getColumnNumber(),
evalOrigin: callSite.getEvalOrigin(),
isToplevel: callSite.isToplevel(),
isEval: callSite.isEval(),
isNative: callSite.isNative(),
isConstructor: callSite.isConstructor(),
isAsync: callSite.isAsync(),
isPromiseAll: callSite.isPromiseAll(),
promiseIndex: callSite.getPromiseIndex(),
};
}

  1. Remove prepareStackTrace method from cli/rt/40_error_stack.js and instead use Isolate::set_prepare_stack_trace_callback - needs to be added to rusty_v8 first

CC @ry @nayeemrmn
@bartlomieju bartlomieju self-assigned this Sep 23, 2020
@kitsonk kitsonk added cli related to cli/ dir deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted) labels Sep 25, 2020
@bartlomieju bartlomieju mentioned this issue Oct 10, 2020
Closed
22 tasks
@bartlomieju bartlomieju mentioned this issue Nov 19, 2020
Closed
@bartlomieju bartlomieju mentioned this issue Dec 7, 2020
Merged
@nayeemrmn nayeemrmn mentioned this issue Jan 9, 2021
Closed
@bnoordhuis bnoordhuis mentioned this issue Jan 21, 2021
Closed
bnoordhuis added a commit to bnoordhuis/rusty_v8 that referenced this issue Jan 21, 2021
@bnoordhuis
Add Isolate::set_prepare_stack_trace_callback()
dea6238 
Refs: denoland/deno#7644
@bnoordhuis bnoordhuis mentioned this issue Jan 21, 2021
Merged
bnoordhuis added a commit to bnoordhuis/rusty_v8 that referenced this issue Feb 7, 2021
@bnoordhuis
Add Isolate::set_prepare_stack_trace_callback()
8a78355 
Refs: denoland/deno#7644
bnoordhuis added a commit to denoland/rusty_v8 that referenced this issue Feb 8, 2021
@bnoordhuis
Add Isolate::set_prepare_stack_trace_callback() (#863)
64aa11e 
Refs: denoland/deno#7644
@bartlomieju bartlomieju mentioned this issue Aug 16, 2021
Closed
@bartlomieju
Copy link
Member Author

With recent improvements to error handling by @nayeemrmn done in #14378, #14394, #14302 and #14274 I feel this issue is no longer of high importance. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bartlomieju bartlomieju

Labels
cli related to cli/ dir deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kitsonk @bartlomieju